diff --git a/cpp/ql/src/experimental/Security/CWE/CWE-377/InsecureTemporaryFile.cpp b/cpp/ql/src/experimental/Security/CWE/CWE-377/InsecureTemporaryFile.cpp new file mode 100644 index 000000000000..a45b3fd3dfef --- /dev/null +++ b/cpp/ql/src/experimental/Security/CWE/CWE-377/InsecureTemporaryFile.cpp @@ -0,0 +1,14 @@ +... + fp = fopen("/tmp/name.tmp","w"); // BAD +... + char filename = tmpnam(NULL); + fp = fopen(filename,"w"); // BAD +... + + strcat (filename, "/tmp/name.XXXXXX"); + fd = mkstemp(filename); + if ( fd < 0 ) { + return error; + } + fp = fdopen(fd,"w") // GOOD +... diff --git a/cpp/ql/src/experimental/Security/CWE/CWE-377/InsecureTemporaryFile.qhelp b/cpp/ql/src/experimental/Security/CWE/CWE-377/InsecureTemporaryFile.qhelp new file mode 100644 index 000000000000..248394525d80 --- /dev/null +++ b/cpp/ql/src/experimental/Security/CWE/CWE-377/InsecureTemporaryFile.qhelp @@ -0,0 +1,23 @@ + + + +

Working with a file, without checking its existence and its rights, as well as working with names that can be predicted, may not be safe. Requires the attention of developers.

+ +
+ + +

The following example demonstrates erroneous and corrected work with file.

+ + +
+ + +
  • + CERT C Coding Standard: + CON33-C. Avoid race conditions when using library functions. +
  • + +
    +
    diff --git a/cpp/ql/src/experimental/Security/CWE/CWE-377/InsecureTemporaryFile.ql b/cpp/ql/src/experimental/Security/CWE/CWE-377/InsecureTemporaryFile.ql new file mode 100644 index 000000000000..f08f7ef61f55 --- /dev/null +++ b/cpp/ql/src/experimental/Security/CWE/CWE-377/InsecureTemporaryFile.ql @@ -0,0 +1,112 @@ +/** + * @name Insecure generation of filenames. + * @description Using a predictable filename when creating a temporary file can lead to an attacker-controlled input. + * @kind problem + * @id cpp/insecure-generation-of-filename + * @problem.severity warning + * @precision medium + * @tags correctness + * security + * external/cwe/cwe-377 + */ + +import cpp +import semmle.code.cpp.valuenumbering.GlobalValueNumbering + +/** Holds for a function `f` that has an argument at index `apos` used to read the file. */ +predicate numberArgumentRead(Function f, int apos) { + f.hasGlobalOrStdName("fgets") and apos = 2 + or + f.hasGlobalOrStdName("fread") and apos = 3 + or + f.hasGlobalOrStdName("read") and apos = 0 + or + f.hasGlobalOrStdName("fscanf") and apos = 0 +} + +/** Holds for a function `f` that has an argument at index `apos` used to write to file */ +predicate numberArgumentWrite(Function f, int apos) { + f.hasGlobalOrStdName("fprintf") and apos = 0 + or + f.hasGlobalOrStdName("fputs") and apos = 1 + or + f.hasGlobalOrStdName("write") and apos = 0 + or + f.hasGlobalOrStdName("fwrite") and apos = 3 + or + f.hasGlobalOrStdName("fflush") and apos = 0 +} + +from FunctionCall fc, string msg +where + // search for functions for generating a name, without a guarantee of the absence of a file during the period of work with it. + ( + fc.getTarget().hasGlobalOrStdName("tmpnam") or + fc.getTarget().hasGlobalOrStdName("tmpnam_s") or + fc.getTarget().hasGlobalOrStdName("tmpnam_r") + ) and + not exists(FunctionCall fctmp | + ( + fctmp.getTarget().hasGlobalOrStdName("mktemp") or + fctmp.getTarget().hasGlobalOrStdName("mkstemp") or + fctmp.getTarget().hasGlobalOrStdName("mkstemps") or + fctmp.getTarget().hasGlobalOrStdName("mkdtemp") + ) and + ( + fc.getBasicBlock().getASuccessor*() = fctmp.getBasicBlock() or + fctmp.getBasicBlock().getASuccessor*() = fc.getBasicBlock() + ) + ) and + msg = + "Finding the name of a file that does not exist does not mean that it will not be exist at the next operation." + or + // finding places to work with a file without setting permissions, but with predictable names. + ( + fc.getTarget().hasGlobalOrStdName("fopen") or + fc.getTarget().hasGlobalOrStdName("open") + ) and + fc.getNumberOfArguments() = 2 and + exists(FunctionCall fctmp, int i | + numberArgumentWrite(fctmp.getTarget(), i) and + globalValueNumber(fc) = globalValueNumber(fctmp.getArgument(i)) + ) and + not exists(FunctionCall fctmp, int i | + numberArgumentRead(fctmp.getTarget(), i) and + globalValueNumber(fc) = globalValueNumber(fctmp.getArgument(i)) + ) and + exists(FunctionCall fctmp | + ( + fctmp.getTarget().hasGlobalOrStdName("strcat") or + fctmp.getTarget().hasGlobalOrStdName("strcpy") + ) and + globalValueNumber(fc.getArgument(0)) = globalValueNumber(fctmp.getAnArgument()) + or + fctmp.getTarget().hasGlobalOrStdName("getenv") and + globalValueNumber(fc.getArgument(0)) = globalValueNumber(fctmp) + or + ( + fctmp.getTarget().hasGlobalOrStdName("asprintf") or + fctmp.getTarget().hasGlobalOrStdName("vasprintf") or + fctmp.getTarget().hasGlobalOrStdName("xasprintf") or + fctmp.getTarget().hasGlobalOrStdName("xvasprintf ") + ) and + exists(Variable vrtmp | + vrtmp = fc.getArgument(0).(VariableAccess).getTarget() and + vrtmp = fctmp.getArgument(0).(AddressOfExpr).getAddressable().(Variable) and + not vrtmp instanceof Field + ) + ) and + not exists(FunctionCall fctmp | + ( + fctmp.getTarget().hasGlobalOrStdName("umask") or + fctmp.getTarget().hasGlobalOrStdName("fchmod") or + fctmp.getTarget().hasGlobalOrStdName("chmod") + ) and + ( + fc.getBasicBlock().getASuccessor*() = fctmp.getBasicBlock() or + fctmp.getBasicBlock().getASuccessor*() = fc.getBasicBlock() + ) + ) and + msg = + "Creating a file for writing without evaluating its existence and setting permissions can be unsafe." +select fc, msg diff --git a/cpp/ql/test/experimental/query-tests/Security/CWE/CWE-377/semmle/tests/InsecureTemporaryFile.expected b/cpp/ql/test/experimental/query-tests/Security/CWE/CWE-377/semmle/tests/InsecureTemporaryFile.expected new file mode 100644 index 000000000000..b0632edcd2a2 --- /dev/null +++ b/cpp/ql/test/experimental/query-tests/Security/CWE/CWE-377/semmle/tests/InsecureTemporaryFile.expected @@ -0,0 +1,2 @@ +| test.cpp:16:20:16:25 | call to tmpnam | Finding the name of a file that does not exist does not mean that it will not be exist at the next operation. | +| test.cpp:42:8:42:12 | call to fopen | Creating a file for writing without evaluating its existence and setting permissions can be unsafe. | diff --git a/cpp/ql/test/experimental/query-tests/Security/CWE/CWE-377/semmle/tests/InsecureTemporaryFile.qlref b/cpp/ql/test/experimental/query-tests/Security/CWE/CWE-377/semmle/tests/InsecureTemporaryFile.qlref new file mode 100644 index 000000000000..beec38ab5dc6 --- /dev/null +++ b/cpp/ql/test/experimental/query-tests/Security/CWE/CWE-377/semmle/tests/InsecureTemporaryFile.qlref @@ -0,0 +1 @@ +experimental/Security/CWE/CWE-377/InsecureTemporaryFile.ql diff --git a/cpp/ql/test/experimental/query-tests/Security/CWE/CWE-377/semmle/tests/test.cpp b/cpp/ql/test/experimental/query-tests/Security/CWE/CWE-377/semmle/tests/test.cpp new file mode 100644 index 000000000000..ceffdf11fa9e --- /dev/null +++ b/cpp/ql/test/experimental/query-tests/Security/CWE/CWE-377/semmle/tests/test.cpp @@ -0,0 +1,68 @@ +typedef int FILE; +#define NULL (0) +FILE *fopen(char *filename, const char *mode); +FILE *fdopen(int handle, char *mode); +char * tmpnam(char * name); +int mkstemp(char * name); +char * strcat(char *str1, const char *str2); +int umask(int pmode); +int chmod(char * filename,int pmode); +int fprintf(FILE *fp,const char *fmt, ...); +int fclose(FILE *stream); + +int funcTest1() +{ + FILE *fp; + char *filename = tmpnam(NULL); // BAD + fp = fopen(filename,"w"); + fprintf(fp,"%s\n","data to file"); + fclose(fp); + return 0; +} + +int funcTest2() +{ + FILE *fp; + int fd; + char filename[80]; + strcat (filename, "/tmp/name.XXXXXX"); + fd = mkstemp(filename); + if ( fd < 0 ) { + return 1; + } + fp = fdopen(fd,"w"); // GOOD + return 0; +} + +int funcTest3() +{ + FILE *fp; + char filename[80]; + strcat(filename, "/tmp/tmp.name"); + fp = fopen(filename,"w"); // BAD + fprintf(fp,"%s\n","data to file"); + fclose(fp); + return 0; +} + +int funcTest4() +{ + FILE *fp; + char filename[80]; + umask(0022); + strcat(filename, "/tmp/tmp.name"); + fp = fopen(filename,"w"); // GOOD + chmod(filename,0666); + fprintf(fp,"%s\n","data to file"); + fclose(fp); + return 0; +} + +int main(int argc, char *argv[]) +{ + funcTest1(); + funcTest2(); + funcTest3(); + funcTest4(); + return 0; +}