Merge pull request #1263 from geoffw0/bufferoverflowqueries

CPP: Resolve overlap between OverflowCalculated.ql and NoSpaceForZeroTerminator.ql

Author: rdmarsh2

change-notes/1.21/analysis-cpp.md

@@ -11,6 +11,8 @@
 
 | **Query**                  | **Expected impact**    | **Change**                                                       |
 |----------------------------|------------------------|------------------------------------------------------------------|
+| Buffer not sufficient for string (`cpp/overflow-calculated`) | Fewer results | This query no longer reports results that would be found by the 'No space for zero terminator' (`cpp/no-space-for-terminator`) query. |
+| No space for zero terminator (`cpp/no-space-for-terminator`) | More correct results | This query now detects calls to `std::malloc`. |
 | Commented-out code (`cpp/commented-out-code`) | More correct results | Commented out preprocessor code is now detected by this query. |
 | Dead code due to goto or break statement (`cpp/dead-code-goto`) | Fewer false positive results | Functions containing preprocessor logic are now excluded from this analysis. |
 | Mismatching new/free or malloc/delete (`cpp/new-free-mismatch`) | Fewer false positive results | Fixed an issue where functions were being identified as allocation functions inappropriately.  Also affects `cpp/new-array-delete-mismatch` and `cpp/new-delete-array-mismatch`. |

cpp/ql/src/Critical/OverflowCalculated.ql

@@ -13,7 +13,10 @@ import cpp
 
 class MallocCall extends FunctionCall
 {
-  MallocCall() { this.getTarget().hasQualifiedName("malloc") }
+  MallocCall() {
+  	this.getTarget().hasQualifiedName("malloc") or
+  	this.getTarget().hasQualifiedName("std::malloc")
+  }
 
   Expr getAllocatedSize() {
     if this.getArgument(0) instanceof VariableAccess then
@@ -25,12 +28,6 @@ class MallocCall extends FunctionCall
   }
 }
 
-predicate terminationProblem(MallocCall malloc, string msg)
-{
-  malloc.getAllocatedSize() instanceof StrlenCall and
-  msg = "This allocation does not include space to null-terminate the string."
-}
-
 predicate spaceProblem(FunctionCall append, string msg)
 {
   exists(MallocCall malloc, StrlenCall strlen, AddExpr add, FunctionCall insert, Variable buffer |
@@ -48,5 +45,5 @@ predicate spaceProblem(FunctionCall append, string msg)
 }
 
 from Expr problem, string msg
-where terminationProblem(problem, msg) or spaceProblem(problem, msg)
+where spaceProblem(problem, msg)
 select problem, msg

cpp/ql/src/Security/CWE/CWE-131/NoSpaceForZeroTerminator.ql

@@ -15,8 +15,12 @@
  */
 import cpp
 
-class MallocCall extends FunctionCall {
-  MallocCall() { this.getTarget().hasGlobalName("malloc") }
+class MallocCall extends FunctionCall
+{
+  MallocCall() {
+  	this.getTarget().hasQualifiedName("malloc") or
+  	this.getTarget().hasQualifiedName("std::malloc")
+  }
 
   Expr getAllocatedSize() {
     if this.getArgument(0) instanceof VariableAccess then

cpp/ql/test/query-tests/Critical/OverflowCalculated/NoSpaceForZeroTerminator.expected

@@ -0,0 +1,5 @@
+| tests1.cpp:26:21:26:26 | call to malloc | This allocation does not include space to null-terminate the string. |
+| tests1.cpp:67:21:67:26 | call to malloc | This allocation does not include space to null-terminate the string. |
+| tests1.cpp:89:25:89:30 | call to malloc | This allocation does not include space to null-terminate the string. |
+| tests3.cpp:25:21:25:31 | call to malloc | This allocation does not include space to null-terminate the string. |
+| tests3.cpp:30:21:30:31 | call to malloc | This allocation does not include space to null-terminate the string. |

…mmle/NoSpaceForZeroTerminator/test.qlref …alculated/NoSpaceForZeroTerminator.qlrefcpp/ql/test/query-tests/Security/CWE/CWE-131/semmle/NoSpaceForZeroTerminator/test.qlref renamed to cpp/ql/test/query-tests/Critical/OverflowCalculated/NoSpaceForZeroTerminator.qlref cpp/ql/test/query-tests/Security/CWE/CWE-131/semmle/NoSpaceForZeroTerminator/test.qlref renamed to cpp/ql/test/query-tests/Critical/OverflowCalculated/NoSpaceForZeroTerminator.qlref

@@ -1,4 +1 @@
-| tests1.cpp:26:21:26:26 | call to malloc | This allocation does not include space to null-terminate the string. |
-| tests1.cpp:67:21:67:26 | call to malloc | This allocation does not include space to null-terminate the string. |
-| tests1.cpp:89:25:89:30 | call to malloc | This allocation does not include space to null-terminate the string. |
 | tests2.cpp:34:4:34:9 | call to strcat | This buffer only contains enough room for 'str1' (copied on line 33) |

cpp/ql/test/query-tests/Critical/OverflowCalculated/OverflowCalculated.expected

@@ -0,0 +1,68 @@
+// tests1.cpp
+
+typedef unsigned int size_t;
+
+char *strcpy(char *destination, const char *source);
+char *strcat(char *destination, const char *source);
+size_t strlen(const char *str);
+
+namespace std
+{
+	void *malloc(size_t size);
+	void free(void *ptr);
+}
+
+const char *str3global = "123";
+
+void tests3(int case_num)
+{
+	const char *str3local = "123";
+	char *buffer = 0;
+
+	switch (case_num)
+	{
+		case 1:
+			buffer = (char *)std::malloc(strlen(str3global)); // BAD
+			strcpy(buffer, str3global);
+			break;
+
+		case 2:
+			buffer = (char *)std::malloc(strlen(str3local)); // BAD
+			strcpy(buffer, str3local);
+			break;
+
+		case 3:
+			buffer = (char *)std::malloc(strlen(str3global) + 1); // GOOD
+			strcpy(buffer, str3global);
+			break;
+
+		case 4:
+			buffer = (char *)std::malloc(strlen(str3local) + 1); // GOOD
+			strcpy(buffer, str3local);
+			break;
+	}
+
+	if (buffer != 0)
+	{
+		std::free(buffer);
+	}
+}
+
+void test3b()
+{
+	char *buffer = new char[strlen(str3global)]; // BAD [NOT DETECTED]
+
+	strcpy(buffer, str3global);
+
+	delete buffer;
+}
+
+void test3c()
+{
+	char *buffer = new char[10]; // BAD [NOT DETECTED]
+
+	strcpy(buffer, "123456");
+	strcat(buffer, "123456");
+
+	delete buffer;
+}

cpp/ql/test/query-tests/Critical/OverflowCalculated/tests3.cpp

@@ -0,0 +1 @@
+Security/CWE/CWE-131/NoSpaceForZeroTerminator.ql