Merge pull request #2402 from geoffw0/nospace

CPP: Make NoSpaceForZeroTerminator.ql more conservative.

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

@@ -16,28 +16,28 @@
 
 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") }
 
-  Expr getAllocatedSize() {
-    if this.getArgument(0) instanceof VariableAccess
-    then
-      exists(LocalScopeVariable v, ControlFlowNode def |
-        definitionUsePair(v, def, this.getArgument(0)) and
-        exprDefinition(v, def, result)
-      )
-    else result = this.getArgument(0)
-  }
+  Expr getAllocatedSize() { result = this.getArgument(0) }
 }
 
 predicate terminationProblem(MallocCall malloc, string msg) {
-  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)
+  // malloc(strlen(...))
+  exists(StrlenCall strlen | DataFlow::localExprFlow(strlen, malloc.getAllocatedSize())) and
+  // 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,4 +1,6 @@
-| test.c:15:20:15:25 | call to malloc | This allocation does not include space to null-terminate the string. |
-| test.c:29:20:29:25 | call to malloc | This allocation does not include space to null-terminate the string. |
-| test.c:44:20:44:25 | call to malloc | This allocation does not include space to null-terminate the string. |
-| test.cpp:18:35:18:40 | 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: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. |

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

@@ -7,18 +7,21 @@
 typedef unsigned long size_t;
 void *malloc(size_t size);
 void free(void *ptr);
+char *strcpy(char *s1, const char *s2);
 
 //// Test code /////
 
 void bad0(char *str) {
     // BAD -- Not allocating space for '\0' terminator
     char *buffer = malloc(strlen(str));
+    strcpy(buffer, str);
     free(buffer);
 }
 
 void good0(char *str) {
     // GOOD -- Allocating extra byte for terminator
     char *buffer = malloc(strlen(str)+1);
+    strcpy(buffer, str);
     free(buffer);
 }
 
@@ -27,6 +30,7 @@ void bad1(char *str) {
     int len = strlen(str);
     // BAD -- Not allocating space for '\0' terminator
     char *buffer = malloc(len);
+    strcpy(buffer, str);
     free(buffer);
 }
 
@@ -34,6 +38,7 @@ void good1(char *str) {
     int len = strlen(str);
     // GOOD -- Allocating extra byte for terminator
     char *buffer = malloc(len+1);
+    strcpy(buffer, str);
     free(buffer);
 }
 
@@ -42,6 +47,7 @@ void bad2(char *str) {
     int len = strlen(str);
     // BAD -- Not allocating space for '\0' terminator
     char *buffer = malloc(len);
+    strcpy(buffer, str);
     free(buffer);
 }
 
@@ -49,18 +55,21 @@ void good2(char *str) {
     int len = strlen(str)+1;
     // GOOD -- Allocating extra byte for terminator
     char *buffer = malloc(len);
+    strcpy(buffer, str);
     free(buffer);
 }
 
 void bad3(char *str) {
     // BAD -- Not allocating space for '\0' terminator [NOT DETECTED]
     char *buffer = malloc(strlen(str) * sizeof(char));
+    strcpy(buffer, str);
     free(buffer);
 }
 
 void good3(char *str) {
     // GOOD -- Allocating extra byte for terminator
     char *buffer = malloc((strlen(str) + 1) * sizeof(char));
+    strcpy(buffer, str);
     free(buffer);
 }
 

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

@@ -10,23 +10,100 @@ typedef unsigned long size_t;
 void *malloc(size_t size);
 void free(void *ptr);
 size_t wcslen(const wchar_t *s);
+wchar_t* wcscpy(wchar_t* s1, const wchar_t* s2);
+int sprintf(char *s, const char *format, ...);
+int wprintf(const wchar_t *format, ...);
+char *strcat(char *s1, const char *s2);
+size_t strlen(const char *s);
+int strcmp(const char *s1, const char *s2);
 
 //// Test code /////
 
 void bad1(wchar_t *wstr) {
     // BAD -- Not allocating space for '\0' terminator
     wchar_t *wbuffer = (wchar_t *)malloc(wcslen(wstr));
+    wcscpy(wbuffer, wstr);
     free(wbuffer);
 }
 
 void bad2(wchar_t *wstr) {
     // BAD -- Not allocating space for '\0' terminator [NOT DETECTED]
     wchar_t *wbuffer = (wchar_t *)malloc(wcslen(wstr) * sizeof(wchar_t));
+    wcscpy(wbuffer, wstr);
     free(wbuffer);
 }
 
 void good1(wchar_t *wstr) {
     // GOOD -- Allocating extra character for terminator
     wchar_t *wbuffer = (wchar_t *)malloc((wcslen(wstr) + 1) * sizeof(wchar_t));
+    wcscpy(wbuffer, wstr);
     free(wbuffer);
 }
+
+void bad3(char *str) {
+    // BAD -- zero-termination proved by sprintf (as destination) [NOT DETECTED]
+    char *buffer = (char *)malloc(strlen(str));
+    sprintf(buffer, "%s", str);
+    free(buffer);
+}
+
+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]
+    char *buffer = (char *)malloc(strlen(str));
+    decode(buffer, str);
+    wprintf(L"%s", buffer);
+    free(buffer);
+}
+
+void bad5(char *str) {
+    // BAD -- zero-termination proved by strcat (as destination)
+    char *buffer = (char *)malloc(strlen(str));
+    buffer[0] = 0;
+    strcat(buffer, str);
+    free(buffer);
+}
+
+void bad6(char *str, char *dest) {
+    // BAD -- zero-termination proved by strcat (as source)
+    char *buffer = (char *)malloc(strlen(str));
+    decode(buffer, str);
+    strcat(dest, buffer);
+    free(buffer);
+}
+
+void bad7(char *str, char *str2) {
+    // BAD -- zero-termination proved by strcmp [NOT DETECTED]
+    char *buffer = (char *)malloc(strlen(str));
+    decode(buffer, str);
+    if (strcmp(buffer, str2) == 0) {
+        // ...
+    }
+    free(buffer);
+}
+
+void bad8(wchar_t *str) {
+    // BAD -- zero-termination proved by wcslen [NOT DETECTED]
+    wchar_t *wbuffer = (wchar_t *)malloc(wcslen(str));
+    wdecode(wbuffer, str);
+    if (wcslen(wbuffer) == 0) {
+        // ...
+    }
+    free(wbuffer);
+}
+
+void good2(char *str, char *dest) {
+    // GOOD -- zero-termination not proven
+    char *buffer = (char *)malloc(strlen(str));
+    decode(buffer, str);
+    free(buffer);
+}
+
+void bad9(wchar_t *wstr) {
+    // BAD -- using new [NOT DETECTED]
+    wchar_t *wbuffer = new wchar_t[wcslen(wstr)];
+    wcscpy(wbuffer, wstr);
+    delete wbuffer;
+}

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

@@ -0,0 +1,50 @@
+
+///// Library functions //////
+
+typedef unsigned long size_t;
+
+void *malloc(size_t size);
+void free(void *ptr);
+size_t strlen(const char *s);
+
+namespace std
+{
+	template<class charT> struct char_traits;
+
+	template <class T> class allocator {
+	public:
+		allocator() throw();
+		typedef size_t size_type;
+	};
+	
+	template<class charT, class traits = char_traits<charT>, class Allocator = allocator<charT> >
+	class basic_string {
+	public:
+		typedef typename Allocator::size_type size_type;
+		explicit basic_string(const Allocator& a = Allocator());
+		basic_string(const charT* s, const Allocator& a = Allocator());
+		basic_string(const charT* s, size_type n, const Allocator& a = Allocator()); 
+
+		const charT* c_str() const;
+	};
+
+	typedef basic_string<char> string;
+}
+
+//// Test code /////
+
+void bad1(char *str) {
+    // BAD -- Not allocating space for '\0' terminator [NOT DETECTED]
+    char *buffer = (char *)malloc(strlen(str));
+    std::string str2(buffer);
+    free(buffer);
+}
+
+void good1(char *str) {
+    // GOOD --- copy does not overrun due to size limit
+    char *buffer = (char *)malloc(strlen(str));
+    std::string str2(buffer, strlen(str));
+    free(buffer);
+}
+
+