Merge pull request #7836 from geoffw0/clrtxt9

C++: Fix more FPs in cpp/cleartext-transmission

Author: Robert Marsh

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

@@ -5,7 +5,7 @@
  * @kind path-problem
  * @problem.severity warning
  * @security-severity 7.5
- * @precision medium
+ * @precision high
  * @id cpp/cleartext-transmission
  * @tags security
  *       external/cwe/cwe-319
@@ -14,8 +14,8 @@
 import cpp
 import semmle.code.cpp.security.SensitiveExprs
 import semmle.code.cpp.dataflow.TaintTracking
-import semmle.code.cpp.valuenumbering.GlobalValueNumbering
 import semmle.code.cpp.models.interfaces.FlowSource
+import semmle.code.cpp.commons.File
 import DataFlow::PathGraph
 
 /**
@@ -121,24 +121,32 @@ abstract class NetworkSendRecv extends FunctionCall {
   NetworkSendRecv() {
     this.getTarget() = target and
     // exclude calls based on the socket...
-    not exists(GVN g |
-      g = globalValueNumber(target.getSocketExpr(this)) and
+    not exists(DataFlow::Node src, DataFlow::Node dest |
+      DataFlow::localFlow(src, dest) and
+      dest.asExpr() = target.getSocketExpr(this) and
       (
         // literal constant
-        globalValueNumber(any(Literal l)) = g
+        src.asExpr() instanceof Literal
         or
         // variable (such as a global) initialized to a literal constant
         exists(Variable v |
           v.getInitializer().getExpr() instanceof Literal and
-          g = globalValueNumber(v.getAnAccess())
+          src.asExpr() = v.getAnAccess()
         )
         or
         // result of a function call with literal inputs (likely constant)
+        forex(Expr arg | arg = src.asExpr().(FunctionCall).getAnArgument() | arg instanceof Literal)
+        or
+        // variable called `stdin`, `stdout` or `stderr`
+        src.asExpr().(VariableAccess).getTarget().getName() = ["stdin", "stdout", "stderr"]
+        or
+        // open of `"/dev/tty"`
         exists(FunctionCall fc |
-          forex(Expr arg | arg = fc.getAnArgument() | arg instanceof Literal) and
-          g = globalValueNumber(fc)
+          fopenCall(fc) and
+          fc.getAnArgument().getValue() = "/dev/tty" and
+          src.asExpr() = fc
         )
-        // (this is far from exhaustive)
+        // (this is not exhaustive)
       )
     )
   }

cpp/ql/src/change-notes/2022-02-22-cleartext-transmission.md

@@ -0,0 +1,4 @@
+---
+category: minorAnalysis
+---
+* The "Cleartext transmission of sensitive information" (`cpp/cleartext-transmission`) query has been further improved to reduce false positive results, and upgraded from `medium` to `high` precision.

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

@@ -91,6 +91,7 @@ edges
 | test3.cpp:398:18:398:25 | password | test3.cpp:400:16:400:23 | password |
 | test3.cpp:398:18:398:25 | password | test3.cpp:400:33:400:40 | password |
 | test3.cpp:421:21:421:28 | password | test3.cpp:421:3:421:17 | call to decrypt_inplace |
+| test3.cpp:429:7:429:14 | password | test3.cpp:431:8:431:15 | password |
 | test.cpp:41:23:41:43 | cleartext password! | test.cpp:48:21:48:27 | call to encrypt |
 | test.cpp:41:23:41:43 | cleartext password! | test.cpp:48:29:48:39 | thePassword |
 | test.cpp:66:23:66:43 | cleartext password! | test.cpp:76:21:76:27 | call to encrypt |
@@ -218,6 +219,8 @@ nodes
 | test3.cpp:421:3:421:17 | call to decrypt_inplace | semmle.label | call to decrypt_inplace |
 | test3.cpp:421:21:421:28 | password | semmle.label | password |
 | test3.cpp:421:21:421:28 | password | semmle.label | password |
+| test3.cpp:429:7:429:14 | password | semmle.label | password |
+| test3.cpp:431:8:431:15 | password | semmle.label | password |
 | test.cpp:41:23:41:43 | cleartext password! | semmle.label | cleartext password! |
 | test.cpp:48:21:48:27 | call to encrypt | semmle.label | call to encrypt |
 | test.cpp:48:29:48:39 | thePassword | semmle.label | thePassword |
@@ -250,3 +253,4 @@ subpaths
 | test3.cpp:388:3:388:6 | call to recv | test3.cpp:386:8:386:15 | password | test3.cpp:388:15:388:22 | password | This operation receives into 'password', which may put unencrypted sensitive data into $@ | test3.cpp:386:8:386:15 | password | password |
 | test3.cpp:414:3:414:6 | call to recv | test3.cpp:414:17:414:24 | password | test3.cpp:414:17:414:24 | password | This operation receives into 'password', which may put unencrypted sensitive data into $@ | test3.cpp:414:17:414:24 | password | password |
 | test3.cpp:420:3:420:6 | call to recv | test3.cpp:420:17:420:24 | password | test3.cpp:420:17:420:24 | password | This operation receives into 'password', which may put unencrypted sensitive data into $@ | test3.cpp:420:17:420:24 | password | password |
+| test3.cpp:431:2:431:6 | call to fgets | test3.cpp:429:7:429:14 | password | test3.cpp:431:8:431:15 | password | This operation receives into 'password', which may put unencrypted sensitive data into $@ | test3.cpp:429:7:429:14 | password | password |

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

@@ -421,3 +421,56 @@ void test_member_password()
 		decrypt_inplace(p.password); // proof that `password` was in fact encrypted
 	}
 }
+
+extern FILE *stdin;
+
+void test_stdin_param(FILE *stream)
+{
+	char password[128];
+
+	fgets(password, 128, stream); // GOOD: from standard input (see call below) [FALSE POSITIVE]
+}
+
+void test_stdin()
+{
+	char password[128];
+	FILE *f = stdin;
+
+	fgets(password, 128, stdin); // GOOD: from standard input
+	fgets(password, 128, f); // GOOD: from standard input
+	test_stdin_param(stdin);
+}
+
+int open(const char *filename, int b);
+
+void test_tty()
+{
+	{
+		char password[256];
+		int f;
+
+		f = open("/dev/tty", val());
+		recv(f, password, 256, val()); // GOOD: from terminal
+	}
+
+	{
+		char password[256];
+		int f;
+
+		f = STDIN_FILENO;
+		recv(f, password, 256, val()); // GOOD: from stdin
+	}
+
+	{
+		char password[256];
+		int f;
+
+		f = open("/dev/tty", val());
+		if (f == -1)
+		{
+			f = STDIN_FILENO;
+		}
+
+		recv(f, password, 256, val()); // GOOD: from terminal or stdin
+	}
+}