CPP: Add query for CWE-377 Insecure Temporary File

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
+...

cpp/ql/src/experimental/Security/CWE/CWE-377/InsecureTemporaryFile.qhelp

@@ -0,0 +1,23 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+<overview>
+<p>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.</p>
+
+</overview>
+
+<example>
+<p>The following example demonstrates erroneous and corrected work with file.</p>
+<sample src="InsecureTemporaryFile.cpp" />
+
+</example>
+<references>
+
+<li>
+  CERT C Coding Standard:
+  <a href="https://wiki.sei.cmu.edu/confluence/display/c/CON33-C.+Avoid+race+conditions+when+using+library+functions">CON33-C. Avoid race conditions when using library functions</a>.
+</li>
+
+</references>
+</qhelp>

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

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. |

cpp/ql/test/experimental/query-tests/Security/CWE/CWE-377/semmle/tests/InsecureTemporaryFile.qlref

@@ -0,0 +1 @@
+experimental/Security/CWE/CWE-377/InsecureTemporaryFile.ql

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;
+}