Merge pull request #6273 from geoffw0/cleartext-storage-file

C++: Improve the CleartextFileWrite query

cpp/change-notes/2021-07-13-cleartext-storage-file.md

@@ -0,0 +1,3 @@
+lgtm,codescanning
+* The "Cleartext storage of sensitive information in file" (cpp/cleartext-storage-file) query now uses dataflow to produce additional results.
+* Heuristics in the SensitiveExprs.qll library have been improved, making the "Cleartext storage of sensitive information in file" (cpp/cleartext-storage-file), "Cleartext storage of sensitive information in buffer" (cpp/cleartext-storage-buffer) and "Cleartext storage of sensitive information in an SQLite" (cpp/cleartext-storage-database) queries more accurate.

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

@@ -5,7 +5,7 @@
  * @kind problem
  * @problem.severity warning
  * @security-severity 7.5
- * @precision medium
+ * @precision high
  * @id cpp/cleartext-storage-file
  * @tags security
  *       external/cwe/cwe-313
@@ -14,10 +14,40 @@
 import cpp
 import semmle.code.cpp.security.SensitiveExprs
 import semmle.code.cpp.security.FileWrite
+import semmle.code.cpp.dataflow.DataFlow
+import semmle.code.cpp.valuenumbering.GlobalValueNumbering
 
-from FileWrite w, SensitiveExpr source, Expr dest
+/**
+ * An operation on a filename.
+ */
+predicate filenameOperation(FunctionCall op, Expr path) {
+  exists(string name | name = op.getTarget().getName() |
+    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"
+      ] and
+    path = op.getArgument(0)
+    or
+    name = ["fopen_s", "wfopen_s", "rename"] and
+    path = op.getArgument(1)
+  )
+}
+
+predicate isFileName(GVN gvn) {
+  exists(FunctionCall op, Expr path |
+    filenameOperation(op, path) and
+    gvn = globalValueNumber(path)
+  )
+}
+
+from FileWrite w, SensitiveExpr source, Expr mid, Expr dest
 where
-  source = w.getASource() and
-  dest = w.getDest()
+  DataFlow::localFlow(DataFlow::exprNode(source), DataFlow::exprNode(mid)) and
+  mid = w.getASource() and
+  dest = w.getDest() and
+  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,20 @@ 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/src/semmle/code/cpp/security/SensitiveExprs.qll

@@ -14,14 +14,13 @@ private predicate suspicious(string s) {
   (
     s.matches("%password%") or
     s.matches("%passwd%") or
-    s.matches("%account%") or
-    s.matches("%accnt%") or
     s.matches("%trusted%")
   ) and
   not (
-    s.matches("%hashed%") or
-    s.matches("%encrypted%") or
-    s.matches("%crypt%")
+    s.matches("%hash%") or
+    s.matches("%crypt%") or
+    s.matches("%file%") or
+    s.matches("%path%")
   )
 }
 
@@ -29,14 +28,20 @@ private predicate suspicious(string s) {
  * A variable that might contain a password or other sensitive information.
  */
 class SensitiveVariable extends Variable {
-  SensitiveVariable() { suspicious(getName().toLowerCase()) }
+  SensitiveVariable() {
+    suspicious(getName().toLowerCase()) and
+    not this.getUnspecifiedType() instanceof IntegralType
+  }
 }
 
 /**
  * A function that might return a password or other sensitive information.
  */
 class SensitiveFunction extends Function {
-  SensitiveFunction() { suspicious(getName().toLowerCase()) }
+  SensitiveFunction() {
+    suspicious(getName().toLowerCase()) and
+    not this.getUnspecifiedType() instanceof IntegralType
+  }
 }
 
 /**

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

@@ -1,3 +1,10 @@
+| test2.cpp:43:2:43:8 | call to fprintf | This write into file 'log' may contain unencrypted data from $@ | test2.cpp:43:36:43:43 | password | this source. |
+| test2.cpp:44:2:44:8 | call to fprintf | This write into file 'log' may contain unencrypted data from $@ | test2.cpp:44:37:44:45 | thepasswd | this source. |
+| test2.cpp:50:2:50:8 | call to fprintf | This write into file 'log' may contain unencrypted data from $@ | test2.cpp:50:41:50:53 | passwd_config | this source. |
+| test2.cpp:54:2:54:8 | call to fprintf | This write into file 'log' may contain unencrypted data from $@ | test2.cpp:54:41:54:52 | widepassword | this source. |
+| 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. |
 | 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

@@ -0,0 +1,94 @@
+
+#define FILE int
+
+typedef unsigned long size_t;
+
+FILE *fopen(const char *filename, const char *mode);
+int snprintf(char *s, size_t n, const char *format, ...);
+int fprintf(FILE *stream, const char *format, ...);
+char *strcpy(char *s1, const char *s2);
+
+char *crypt(char *input);
+
+struct myStruct
+{
+	// sensitive
+	char *password;
+	char *thepasswd;
+	char *accountkey;
+	wchar_t *widepassword;
+
+	// encrypted
+	char password_hash[64];
+	char *encrypted_passwd;
+
+	// not sensitive
+	char *password_file;
+	char *password_path;
+	int num_passwords;
+	int *password_tries;
+	bool have_passwd;
+
+	// dubious
+	char *passwd_config;
+	char *passwd_config2;
+};
+
+char *getPassword();
+char *getPasswordHash();
+int getPasswordMaxChars();
+
+void tests(FILE *log, myStruct &s)
+{
+	fprintf(log, "password = %s\n", s.password); // BAD
+	fprintf(log, "thepasswd = %s\n", s.thepasswd); // BAD
+	fprintf(log, "accountkey = %s\n", s.accountkey); // DUBIOUS [NOT REPORTED]
+	fprintf(log, "password_hash = %s\n", s.password_hash); // GOOD
+	fprintf(log, "encrypted_passwd = %s\n", s.encrypted_passwd); // GOOD
+	fprintf(log, "password_file = %s\n", s.password_file); // GOOD
+	fprintf(log, "password_path = %s\n", s.password_path); // GOOD
+	fprintf(log, "passwd_config = %s\n", s.passwd_config); // DUBIOUS [REPORTED]
+	fprintf(log, "num_passwords = %i\n", s.num_passwords); // GOOD
+	fprintf(log, "password_tries = %i\n", *(s.password_tries)); // GOOD
+	fprintf(log, "have_passwd = %i\n", s.have_passwd); // GOOD
+	fprintf(log, "widepassword = %ls\n", s.widepassword); // BAD
+	fprintf(log, "widepassword = %S\n", s.widepassword); // BAD
+
+	fprintf(log, "getPassword() = %s\n", getPassword()); // BAD
+	fprintf(log, "getPasswordHash() = %s\n", getPasswordHash()); // GOOD
+	fprintf(log, "getPasswordMaxChars() = %i\n", getPasswordMaxChars()); // GOOD
+
+	{
+		char *cpy1 = s.password;
+		char *cpy2 = crypt(s.password);
+
+		fprintf(log, "cpy1 = %s\n", cpy1); // BAD
+		fprintf(log, "cpy2 = %s\n", cpy2); // GOOD
+	}
+
+	{
+		char buf[1024];
+
+		strcpy(buf, s.password);
+		fprintf(log, "buf = %s\n", buf); // BAD [NOT DETECTED]
+		
+		strcpy(buf, s.password_hash);
+		fprintf(log, "buf = %s\n", buf); // GOOD
+	}
+
+	fprintf(log, "password = %p\n", s.password); // GOOD
+
+	{
+		if (fopen(s.passwd_config2, "rt") == 0)
+		{
+			fprintf(log, "could not open file '%s'.\n", s.passwd_config2); // GOOD
+		}
+	}
+
+	{
+		char buffer[1024];
+
+		snprintf(buffer, 1024, "password = %s", s.password);
+		fprintf(log, "log: %s", buffer); // BAD [NOT DETECTED]
+	}
+}