C++: Exclude results formatted with a character other than %s.

Author: geoffw0

cpp/ql/src/Security/CWE/CWE-311/CleartextFileWrite.ql

@@ -25,27 +25,29 @@ predicate filenameOperation(FunctionCall op, Expr path) {
     name =
       [
         "remove", "unlink", "rmdir", "rename", "fopen", "open", "freopen", "_open", "_wopen",
-        "_wfopen", "_fsopen", "_wfsopen", "chmod", "chown", "stat", "lstat", "fstat", "access", "_access", "_waccess", "_access_s", "_waccess_s"
+        "_wfopen", "_fsopen", "_wfsopen", "chmod", "chown", "stat", "lstat", "fstat", "access",
+        "_access", "_waccess", "_access_s", "_waccess_s"
       ] and
-      path = op.getArgument(0)
+    path = op.getArgument(0)
     or
     name = ["fopen_s", "wfopen_s", "rename"] and
     path = op.getArgument(1)
   )
 }
 
-predicate isFileName(GVN gvn)
-{
+predicate isFileName(GVN gvn) {
   exists(FunctionCall op, Expr path |
     filenameOperation(op, path) and
     gvn = globalValueNumber(path)
   )
 }
 
-from FileWrite w, SensitiveExpr source, Expr dest
+from FileWrite w, SensitiveExpr source, Expr mid, Expr dest
 where
-  DataFlow::localFlow(DataFlow::exprNode(source), DataFlow::exprNode(w.getASource())) and
+  DataFlow::localFlow(DataFlow::exprNode(source), DataFlow::exprNode(mid)) and
+  mid = w.getASource() and
   dest = w.getDest() and
-  not isFileName(globalValueNumber(source)) // file names are not passwords
+  not isFileName(globalValueNumber(source)) and // file names are not passwords
+  not exists(string convChar | convChar = w.getSourceConvChar(mid) | not convChar = ["s", "S"]) // ignore things written with other conversion characters
 select w, "This write into file '" + dest.toString() + "' may contain unencrypted data from $@",
   source, "this source."

cpp/ql/src/semmle/code/cpp/security/FileWrite.qll

@@ -19,6 +19,15 @@ class FileWrite extends Expr {
    * Gets the expression for the object being written to.
    */
   Expr getDest() { fileWrite(this, _, result) }
+
+  /**
+   * Gets the conversion character for this write, if it exists and is known. For example in the following code the write of `value1` has conversion character `"s"`, whereas the write of `value2` has no conversion specifier.
+   * ```
+   * fprintf(file, "%s", value1);
+   * stream << value2;
+   * ```
+   */
+  string getSourceConvChar(Expr source) { fileWriteWithConvChar(this, source, result) }
 }
 
 /**
@@ -150,3 +159,22 @@ private predicate fileWrite(Call write, Expr source, Expr dest) {
   // file stream using '<<', 'put' or 'write'
   fileStreamChain(write, source, dest)
 }
+
+/**
+ * Whether the function call is a write to a file from 'source' with
+ * conversion character 'conv'. Does not hold if there isn't a conversion
+ * character, or if it is unknown (for example the format string is not a
+ * constant).
+ */
+private predicate fileWriteWithConvChar(
+  FormattingFunctionCall ffc, Expr source, string conv
+) {
+  // fprintf
+  exists(FormattingFunction f, int n |
+    f = ffc.getTarget() and
+    source = ffc.getFormatArgument(n)
+  |
+    exists(f.getOutputParameterIndex(true)) and
+    conv = ffc.(FormattingFunctionCall).getFormat().(FormatLiteral).getConversionChar(n)
+  )
+}

cpp/ql/test/query-tests/Security/CWE/CWE-311/semmle/tests/CleartextFileWrite.expected

@@ -5,7 +5,6 @@
 | test2.cpp:55:2:55:8 | call to fprintf | This write into file 'log' may contain unencrypted data from $@ | test2.cpp:55:40:55:51 | widepassword | this source. |
 | test2.cpp:57:2:57:8 | call to fprintf | This write into file 'log' may contain unencrypted data from $@ | test2.cpp:57:39:57:49 | call to getPassword | this source. |
 | test2.cpp:65:3:65:9 | call to fprintf | This write into file 'log' may contain unencrypted data from $@ | test2.cpp:62:18:62:25 | password | this source. |
-| test2.cpp:79:2:79:8 | call to fprintf | This write into file 'log' may contain unencrypted data from $@ | test2.cpp:79:36:79:43 | password | this source. |
 | test.cpp:45:3:45:7 | call to fputs | This write into file 'file' may contain unencrypted data from $@ | test.cpp:45:9:45:19 | thePassword | this source. |
 | test.cpp:70:35:70:35 | call to operator<< | This write into file 'mystream' may contain unencrypted data from $@ | test.cpp:70:38:70:48 | thePassword | this source. |
 | test.cpp:73:37:73:41 | call to write | This write into file 'mystream' may contain unencrypted data from $@ | test.cpp:73:43:73:53 | thePassword | this source. |

cpp/ql/test/query-tests/Security/CWE/CWE-311/semmle/tests/test2.cpp

@@ -76,7 +76,7 @@ void tests(FILE *log, myStruct &s)
 		fprintf(log, "buf = %s\n", buf); // GOOD
 	}
 
-	fprintf(log, "password = %p\n", s.password); // GOOD [FALSE POSITIVE]
+	fprintf(log, "password = %p\n", s.password); // GOOD
 
 	{
 		if (fopen(s.passwd_config2, "rt") == 0)