CPP: Switch from a blacklist to whitelist approach for determining null termination.

Author: geoffw0

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

@@ -16,7 +16,7 @@
 
 import cpp
 import semmle.code.cpp.dataflow.DataFlow
-import semmle.code.cpp.models.implementations.Memcpy
+import semmle.code.cpp.models.interfaces.ArrayFunction
 
 class MallocCall extends FunctionCall {
   MallocCall() { this.getTarget().hasGlobalOrStdName("malloc") }
@@ -33,11 +33,19 @@ class MallocCall extends FunctionCall {
 }
 
 predicate terminationProblem(MallocCall malloc, string msg) {
+  // malloc(strlen(...))
   malloc.getAllocatedSize() instanceof StrlenCall and
-  not exists(FunctionCall fc, MemcpyFunction memcpy, int ix |
-    DataFlow::localExprFlow(malloc, fc.getArgument(ix)) and
-    fc.getTarget() = memcpy and
-    memcpy.hasArrayOutput(ix)
+  // flows into a null-terminated string function
+  exists(ArrayFunction af, FunctionCall fc, int arg |
+    DataFlow::localExprFlow(malloc, fc.getArgument(arg)) and
+    fc.getTarget() = af and
+    (
+      // null terminated string
+      af.hasArrayWithNullTerminator(arg)
+      or
+      // likely a null terminated string (such as `strcpy`, `strcat`)
+      af.hasArrayWithUnknownSize(arg)
+    )
   ) and
   msg = "This allocation does not include space to null-terminate the string."
 }

cpp/ql/test/query-tests/Security/CWE/CWE-131/semmle/NoSpaceForZeroTerminator/NoSpaceForZeroTerminator.expected

@@ -1,12 +1,6 @@
-| test2.cpp:35:28:35:33 | call to malloc | This allocation does not include space to null-terminate the string. |
 | test.c:16:20:16:25 | call to malloc | This allocation does not include space to null-terminate the string. |
 | test.c:32:20:32:25 | call to malloc | This allocation does not include space to null-terminate the string. |
 | test.c:49:20:49:25 | call to malloc | This allocation does not include space to null-terminate the string. |
 | test.cpp:24:35:24:40 | call to malloc | This allocation does not include space to null-terminate the string. |
-| test.cpp:45:28:45:33 | call to malloc | This allocation does not include space to null-terminate the string. |
-| test.cpp:55:28:55:33 | call to malloc | This allocation does not include space to null-terminate the string. |
 | test.cpp:63:28:63:33 | call to malloc | This allocation does not include space to null-terminate the string. |
 | test.cpp:71:28:71:33 | call to malloc | This allocation does not include space to null-terminate the string. |
-| test.cpp:79:28:79:33 | call to malloc | This allocation does not include space to null-terminate the string. |
-| test.cpp:89:35:89:40 | call to malloc | This allocation does not include space to null-terminate the string. |
-| test.cpp:99:28:99:33 | call to malloc | This allocation does not include space to null-terminate the string. |

cpp/ql/test/query-tests/Security/CWE/CWE-131/semmle/NoSpaceForZeroTerminator/test.cpp

@@ -41,7 +41,7 @@ void good1(wchar_t *wstr) {
 }
 
 void bad3(char *str) {
-    // BAD -- zero-termination proved by sprintf (as destination)
+    // BAD -- zero-termination proved by sprintf (as destination) [NOT DETECTED]
     char *buffer = (char *)malloc(strlen(str));
     sprintf(buffer, "%s", str);
     free(buffer);
@@ -51,7 +51,7 @@ void decode(char *dest, char *src);
 void wdecode(wchar_t *dest, wchar_t *src);
 
 void bad4(char *str) {
-    // BAD -- zero-termination proved by wprintf (as parameter)
+    // BAD -- zero-termination proved by wprintf (as parameter) [NOT DETECTED]
     char *buffer = (char *)malloc(strlen(str));
     decode(buffer, str);
     wprintf(L"%s", buffer);
@@ -75,7 +75,7 @@ void bad6(char *str, char *dest) {
 }
 
 void bad7(char *str, char *str2) {
-    // BAD -- zero-termination proved by strcmp
+    // BAD -- zero-termination proved by strcmp [NOT DETECTED]
     char *buffer = (char *)malloc(strlen(str));
     decode(buffer, str);
     if (strcmp(buffer, str2) == 0) {
@@ -85,7 +85,7 @@ void bad7(char *str, char *str2) {
 }
 
 void bad8(wchar_t *str) {
-    // BAD -- zero-termination proved by wcslen
+    // BAD -- zero-termination proved by wcslen [NOT DETECTED]
     wchar_t *wbuffer = (wchar_t *)malloc(wcslen(str));
     wdecode(wbuffer, str);
     if (wcslen(wbuffer) == 0) {
@@ -95,7 +95,7 @@ void bad8(wchar_t *str) {
 }
 
 void good2(char *str, char *dest) {
-    // GOOD -- zero-termination not proven [FALSE POSITIVE]
+    // GOOD -- zero-termination not proven
     char *buffer = (char *)malloc(strlen(str));
     decode(buffer, str);
     free(buffer);

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

@@ -31,7 +31,7 @@ namespace std
 //// Test code /////
 
 void bad1(char *str) {
-    // BAD -- Not allocating space for '\0' terminator
+    // BAD -- Not allocating space for '\0' terminator [NOT DETECTED]
     char *buffer = (char *)malloc(strlen(str));
     std::string str2(buffer);
     free(buffer);