Merge pull request #2812 from geoffw0/nospacezero

C++: Improve NoSpaceForZeroTerminator.ql

Author: Robert Marsh

change-notes/1.24/analysis-cpp.md

@@ -19,6 +19,7 @@ The following changes in version 1.24 affect C/C++ analysis in all applications.
 | Memory is never freed (`cpp/memory-never-freed`) | More true positive results | This query now identifies a wider variety of buffer allocations using the `semmle.code.cpp.models.interfaces.Allocation` library. |
 | Memory may not be freed (`cpp/memory-may-not-be-freed`) | More true positive results | This query now identifies a wider variety of buffer allocations using the `semmle.code.cpp.models.interfaces.Allocation` library. |
 | Missing return statement (`cpp/missing-return`) | Fewer false positive results | Functions containing `asm` statements are no longer highlighted by this query. |
+| No space for zero terminator (`cpp/no-space-for-terminator`) | More correct results | String arguments to formatting functions are now (usually) expected to be null terminated strings. |
 | Hard-coded Japanese era start date (`cpp/japanese-era/exact-era-date`) |  | This query is no longer run on LGTM. |
 | No space for zero terminator (`cpp/no-space-for-terminator`) | Fewer false positive results | This query has been modified to be more conservative when identifying which pointers point to null-terminated strings.  This approach produces fewer, more accurate results. |
 | Overloaded assignment does not return 'this' (`cpp/assignment-does-not-return-this`) | Fewer false positive results | This query no longer reports incorrect results in template classes. |

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

@@ -22,16 +22,25 @@ import semmle.code.cpp.models.interfaces.Allocation
 predicate terminationProblem(AllocationExpr malloc, string msg) {
   // malloc(strlen(...))
   exists(StrlenCall strlen | DataFlow::localExprFlow(strlen, malloc.getSizeExpr())) and
-  // flows into a null-terminated string function
+  // flows to a call that implies this is a null-terminated string
   exists(ArrayFunction af, FunctionCall fc, int arg |
     DataFlow::localExprFlow(malloc, fc.getArgument(arg)) and
     fc.getTarget() = af and
     (
-      // null terminated string
+      // flows into null terminated string argument
       af.hasArrayWithNullTerminator(arg)
       or
-      // likely a null terminated string (such as `strcpy`, `strcat`)
+      // flows into likely null terminated string argument (such as `strcpy`, `strcat`)
       af.hasArrayWithUnknownSize(arg)
+      or
+      // flows into string argument to a formatting function (such as `printf`)
+      exists(int n, FormatLiteral fl |
+        fc.getArgument(arg) = fc.(FormattingFunctionCall).getConversionArgument(n) and
+        fl = fc.(FormattingFunctionCall).getFormat() and
+        fl.getConversionType(n) instanceof PointerType and // `%s`, `%ws` etc
+        not fl.getConversionType(n) instanceof VoidPointerType and // exclude: `%p`
+        not fl.hasPrecision(n) // exclude: `%.*s`
+      )
     )
   ) 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

@@ -5,6 +5,7 @@
 | 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. |

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

@@ -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) [NOT DETECTED]
+    // BAD -- zero-termination proved by wprintf (as parameter)
     char *buffer = (char *)malloc(strlen(str));
     decode(buffer, str);
     wprintf(L"%s", buffer);
@@ -107,3 +107,19 @@ void bad9(wchar_t *wstr) {
     wcscpy(wbuffer, wstr);
     delete wbuffer;
 }
+
+void good3(char *str) {
+    // GOOD -- zero-termination not required for this printf
+    char *buffer = (char *)malloc(strlen(str));
+    decode(buffer, str);
+    wprintf(L"%p", buffer);
+    free(buffer);
+}
+
+void good4(char *str) {
+    // GOOD -- zero-termination not required for this printf
+    char *buffer = (char *)malloc(strlen(str));
+    decode(buffer, str);
+    wprintf(L"%.*s", strlen(str), buffer);
+    free(buffer);
+}