Merge pull request github#6794 from geoffw0/impropnullfp

C++: Improvements to cpp/improper-null-termination

Author: MathiasVP

cpp/change-notes/2021-10-01-improper-null-termination.md

@@ -0,0 +1,2 @@
+lgtm,codescanning
+* Several improvements made to the `NullTermination.qll` library and the 'Potential improper null termination' (cpp/improper-null-termination). These changes reduce the number of false positive results for this query and related query 'User-controlled data may not be null terminated' (cpp/user-controlled-null-termination-tainted).

cpp/ql/lib/semmle/code/cpp/commons/NullTermination.qll

@@ -1,6 +1,7 @@
 import cpp
 private import semmle.code.cpp.models.interfaces.ArrayFunction
 private import semmle.code.cpp.models.implementations.Strcat
+import semmle.code.cpp.dataflow.DataFlow
 
 private predicate mayAddNullTerminatorHelper(Expr e, VariableAccess va, Expr e0) {
   exists(StackVariable v0, Expr val |
@@ -45,22 +46,28 @@ predicate mayAddNullTerminator(Expr e, VariableAccess va) {
     ae.getRValue().getAChild*() = va
   )
   or
-  // Function call: library function, varargs function, function
-  // containing assembler code, or function where the relevant
-  // parameter is potentially added a null terminator.
+  // Function calls...
   exists(Call c, Function f, int i |
     e = c and
     f = c.getTarget() and
     not functionArgumentMustBeNullTerminated(f, i) and
     c.getAnArgumentSubExpr(i) = va
   |
-    not f.hasEntryPoint() and not functionArgumentMustBeNullTerminated(f, i)
+    // library function
+    not f.hasEntryPoint()
     or
+    // function where the relevant parameter is potentially added a null terminator
     mayAddNullTerminator(_, f.getParameter(i).getAnAccess())
     or
+    // varargs function
     f.isVarargs() and i >= f.getNumberOfParameters()
     or
+    // function containing assembler code
     exists(AsmStmt s | s.getEnclosingFunction() = f)
+    or
+    // function where the relevant parameter is returned (leaking it to be potentially null terminated elsewhere)
+    DataFlow::localFlow(DataFlow::parameterNode(f.getParameter(i)),
+      DataFlow::exprNode(any(ReturnStmt rs).getExpr()))
   )
   or
   // Call without target (e.g., function pointer call)

cpp/ql/src/Likely Bugs/Memory Management/ImproperNullTermination.ql

@@ -5,6 +5,7 @@
  * @kind problem
  * @id cpp/improper-null-termination
  * @problem.severity warning
+ * @precision medium
  * @security-severity 7.8
  * @tags security
  *       external/cwe/cwe-170
@@ -53,6 +54,7 @@ class ImproperNullTerminationReachability extends StackVariableReachabilityWithR
   override predicate isBarrier(ControlFlowNode node, StackVariable v) {
     exprDefinition(v, node, _) or
     mayAddNullTerminator(node, v.getAnAccess()) or
+    node.(AddressOfExpr).getOperand() = v.getAnAccess() or // address taken
     isSinkActual(node, v) // only report first use
   }
 }

cpp/ql/src/Security/CWE/CWE-170/ImproperNullTerminationTainted.ql

@@ -5,6 +5,7 @@
  * @kind problem
  * @id cpp/user-controlled-null-termination-tainted
  * @problem.severity warning
+ * @precision medium
  * @security-severity 10.0
  * @tags security
  *       external/cwe/cwe-170

cpp/ql/test/query-tests/Likely Bugs/Memory Management/ImproperNullTermination/ImproperNullTermination.expected

@@ -1,27 +1,29 @@
-| test.cpp:25:10:25:16 | buffer1 | Variable $@ may not be null terminated. | test.cpp:22:8:22:14 | buffer1 | buffer1 |
-| test.cpp:26:10:26:16 | buffer2 | Variable $@ may not be null terminated. | test.cpp:23:8:23:14 | buffer2 | buffer2 |
-| test.cpp:39:10:39:16 | buffer2 | Variable $@ may not be null terminated. | test.cpp:35:8:35:14 | buffer2 | buffer2 |
-| test.cpp:59:10:59:13 | ptr1 | Variable $@ may not be null terminated. | test.cpp:56:9:56:12 | ptr1 | ptr1 |
-| test.cpp:69:10:69:16 | buffer1 | Variable $@ may not be null terminated. | test.cpp:64:8:64:14 | buffer1 | buffer1 |
-| test.cpp:70:10:70:12 | ptr | Variable $@ may not be null terminated. | test.cpp:64:8:64:14 | buffer1 | buffer1 |
-| test.cpp:81:10:81:16 | buffer2 | Variable $@ may not be null terminated. | test.cpp:65:8:65:14 | buffer2 | buffer2 |
-| test.cpp:82:10:82:12 | ptr | Variable $@ may not be null terminated. | test.cpp:65:8:65:14 | buffer2 | buffer2 |
-| test.cpp:93:10:93:15 | buffer | Variable $@ may not be null terminated. | test.cpp:86:8:86:13 | buffer | buffer |
-| test.cpp:116:10:116:15 | buffer | Variable $@ may not be null terminated. | test.cpp:109:8:109:13 | buffer | buffer |
-| test.cpp:130:14:130:19 | buffer | Variable $@ may not be null terminated. | test.cpp:127:7:127:12 | buffer | buffer |
-| test.cpp:139:10:139:15 | buffer | Variable $@ may not be null terminated. | test.cpp:136:8:136:13 | buffer | buffer |
-| test.cpp:147:14:147:19 | buffer | Variable $@ may not be null terminated. | test.cpp:143:8:143:13 | buffer | buffer |
-| test.cpp:182:10:182:15 | buffer | Variable $@ may not be null terminated. | test.cpp:178:8:178:13 | buffer | buffer |
-| test.cpp:234:10:234:15 | buffer | Variable $@ may not be null terminated. | test.cpp:232:8:232:13 | buffer | buffer |
-| test.cpp:262:10:262:15 | buffer | Variable $@ may not be null terminated. | test.cpp:259:8:259:13 | buffer | buffer |
-| test.cpp:283:10:283:15 | buffer | Variable $@ may not be null terminated. | test.cpp:280:8:280:13 | buffer | buffer |
-| test.cpp:300:10:300:16 | buffer2 | Variable $@ may not be null terminated. | test.cpp:295:8:295:14 | buffer2 | buffer2 |
-| test.cpp:312:10:312:15 | buffer | Variable $@ may not be null terminated. | test.cpp:308:8:308:13 | buffer | buffer |
-| test.cpp:327:18:327:23 | buffer | Variable $@ may not be null terminated. | test.cpp:326:8:326:13 | buffer | buffer |
-| test.cpp:346:11:346:16 | buffer | Variable $@ may not be null terminated. | test.cpp:341:8:341:13 | buffer | buffer |
+| test.cpp:26:10:26:16 | buffer1 | Variable $@ may not be null terminated. | test.cpp:23:8:23:14 | buffer1 | buffer1 |
+| test.cpp:27:10:27:16 | buffer2 | Variable $@ may not be null terminated. | test.cpp:24:8:24:14 | buffer2 | buffer2 |
+| test.cpp:40:10:40:16 | buffer2 | Variable $@ may not be null terminated. | test.cpp:36:8:36:14 | buffer2 | buffer2 |
+| test.cpp:60:10:60:13 | ptr1 | Variable $@ may not be null terminated. | test.cpp:57:9:57:12 | ptr1 | ptr1 |
+| test.cpp:70:10:70:16 | buffer1 | Variable $@ may not be null terminated. | test.cpp:65:8:65:14 | buffer1 | buffer1 |
+| test.cpp:71:10:71:12 | ptr | Variable $@ may not be null terminated. | test.cpp:65:8:65:14 | buffer1 | buffer1 |
+| test.cpp:82:10:82:16 | buffer2 | Variable $@ may not be null terminated. | test.cpp:66:8:66:14 | buffer2 | buffer2 |
+| test.cpp:83:10:83:12 | ptr | Variable $@ may not be null terminated. | test.cpp:66:8:66:14 | buffer2 | buffer2 |
+| test.cpp:94:10:94:15 | buffer | Variable $@ may not be null terminated. | test.cpp:87:8:87:13 | buffer | buffer |
+| test.cpp:117:10:117:15 | buffer | Variable $@ may not be null terminated. | test.cpp:110:8:110:13 | buffer | buffer |
+| test.cpp:131:14:131:19 | buffer | Variable $@ may not be null terminated. | test.cpp:128:7:128:12 | buffer | buffer |
+| test.cpp:140:10:140:15 | buffer | Variable $@ may not be null terminated. | test.cpp:137:8:137:13 | buffer | buffer |
+| test.cpp:148:14:148:19 | buffer | Variable $@ may not be null terminated. | test.cpp:144:8:144:13 | buffer | buffer |
+| test.cpp:183:10:183:15 | buffer | Variable $@ may not be null terminated. | test.cpp:179:8:179:13 | buffer | buffer |
+| test.cpp:236:10:236:15 | buffer | Variable $@ may not be null terminated. | test.cpp:234:8:234:13 | buffer | buffer |
+| test.cpp:264:10:264:15 | buffer | Variable $@ may not be null terminated. | test.cpp:261:8:261:13 | buffer | buffer |
+| test.cpp:285:10:285:15 | buffer | Variable $@ may not be null terminated. | test.cpp:282:8:282:13 | buffer | buffer |
+| test.cpp:302:10:302:16 | buffer2 | Variable $@ may not be null terminated. | test.cpp:297:8:297:14 | buffer2 | buffer2 |
+| test.cpp:314:10:314:15 | buffer | Variable $@ may not be null terminated. | test.cpp:310:8:310:13 | buffer | buffer |
+| test.cpp:336:18:336:23 | buffer | Variable $@ may not be null terminated. | test.cpp:335:8:335:13 | buffer | buffer |
 | test.cpp:355:11:355:16 | buffer | Variable $@ may not be null terminated. | test.cpp:350:8:350:13 | buffer | buffer |
-| test.cpp:365:19:365:25 | buffer2 | Variable $@ may not be null terminated. | test.cpp:363:8:363:14 | buffer2 | buffer2 |
-| test.cpp:392:17:392:22 | buffer | Variable $@ may not be null terminated. | test.cpp:390:8:390:13 | buffer | buffer |
-| test.cpp:398:18:398:23 | buffer | Variable $@ may not be null terminated. | test.cpp:396:8:396:13 | buffer | buffer |
-| test.cpp:444:10:444:15 | buffer | Variable $@ may not be null terminated. | test.cpp:442:8:442:13 | buffer | buffer |
-| test.cpp:450:16:450:21 | buffer | Variable $@ may not be null terminated. | test.cpp:448:8:448:13 | buffer | buffer |
+| test.cpp:364:11:364:16 | buffer | Variable $@ may not be null terminated. | test.cpp:359:8:359:13 | buffer | buffer |
+| test.cpp:392:11:392:16 | buffer | Variable $@ may not be null terminated. | test.cpp:381:8:381:13 | buffer | buffer |
+| test.cpp:410:11:410:16 | buffer | Variable $@ may not be null terminated. | test.cpp:397:8:397:13 | buffer | buffer |
+| test.cpp:421:19:421:25 | buffer2 | Variable $@ may not be null terminated. | test.cpp:419:8:419:14 | buffer2 | buffer2 |
+| test.cpp:448:17:448:22 | buffer | Variable $@ may not be null terminated. | test.cpp:446:8:446:13 | buffer | buffer |
+| test.cpp:454:18:454:23 | buffer | Variable $@ may not be null terminated. | test.cpp:452:8:452:13 | buffer | buffer |
+| test.cpp:513:10:513:15 | buffer | Variable $@ may not be null terminated. | test.cpp:511:8:511:13 | buffer | buffer |
+| test.cpp:519:16:519:21 | buffer | Variable $@ may not be null terminated. | test.cpp:517:8:517:13 | buffer | buffer |

cpp/ql/test/query-tests/Likely Bugs/Memory Management/ImproperNullTermination/ImproperNullTerminationTainted.expected

@@ -1,2 +1,2 @@
-| test.cpp:410:10:410:15 | buffer | $@ flows to here and may not be null terminated. | test.cpp:409:18:409:23 | buffer | User-provided value |
-| test.cpp:425:10:425:15 | buffer | $@ flows to here and may not be null terminated. | test.cpp:424:9:424:14 | buffer | User-provided value |
+| test.cpp:466:10:466:15 | buffer | $@ flows to here and may not be null terminated. | test.cpp:465:18:465:23 | buffer | User-provided value |
+| test.cpp:481:10:481:15 | buffer | $@ flows to here and may not be null terminated. | test.cpp:480:9:480:14 | buffer | User-provided value |

cpp/ql/test/query-tests/Likely Bugs/Memory Management/ImproperNullTermination/test.cpp

@@ -6,6 +6,7 @@ size_t strlen(const char *s);
 char *strcpy(char *s1, const char *s2);
 char *strcat(char *s1, const char *s2);
 char *strdup(const char *s1);
+long int strtol(const char* nptr, char** endptr, int base);
 void *malloc(size_t size);
 void *memset(void *s, int c, size_t n);
 void *memcpy(void *s1, const void *s2, size_t n);
@@ -225,6 +226,7 @@ void test_readlink(int fd, const char *path, size_t sz)
 void doNothing(char *data) { };
 void doNothing2(const char *data);
 void clearBuffer(char *data, size_t size);
+char *id(char *data) { return data; }
 
 void test_strcat()
 {
@@ -318,6 +320,13 @@ void test_strcat()
 		clearBuffer(buffer, 1024);
 		strcat(buffer, "content"); // GOOD
 	}
+
+	{
+		char buffer[1024];
+
+		clearBuffer(id(buffer), 1024);
+		strcat(buffer, "content"); // GOOD
+	}
 }
 
 void test_strlen(bool cond1, bool cond2)
@@ -354,6 +363,53 @@ void test_strlen(bool cond1, bool cond2)
 		if (cond2)
 			strlen(buffer); // BAD
 	}
+
+	{
+		char buffer[1024];
+
+		if (cond1)
+		{
+			buffer[0] = 0;
+		} else {
+			buffer[0] = 0;
+		}
+
+		strlen(buffer); // GOOD
+	}
+
+	{
+		char buffer[1024];
+		int init = 0;
+
+		if (cond1)
+		{
+			buffer[0] = 0;
+			init = 1;
+		}
+
+		if (init != 0)
+		{
+			strlen(buffer); // GOOD [FALSE POSITIVE]
+		}
+	}
+
+	{
+		char buffer[1024];
+		int init = 0;
+
+		if (cond1)
+		{
+			buffer[0] = 0;
+			init = 1;
+		}
+
+		if (init == 0)
+		{
+			// ...
+		} else {
+			strlen(buffer); // GOOD [FALSE POSITIVE]
+		}
+	}
 }
 
 void test_strcpy()
@@ -434,6 +490,19 @@ void test_read_fread(int read_src, FILE *s)
 	}
 }
 
+void test_strtol()
+{
+	{
+		char buffer[100];
+		char *after_ptr;
+		long int num;
+
+		strcpy(buffer, "123abc");
+		num = strtol("123abc", &after_ptr, 10);
+		strlen(after_ptr); // GOOD
+	}
+}
+
 int printf(const char *format, ...);
 
 void test_printf(char *str)
@@ -466,3 +535,4 @@ void test_printf(char *str)
 		printf("%s", copied_str); // GOOD
 	}
 }
+