Merge pull request #854 from geoffw0/taintedmalloc

CPP: Improve TaintedAllocationSize.ql

Author: jbj

change-notes/1.21/analysis-cpp.md

@@ -12,6 +12,7 @@
 | **Query**                  | **Expected impact**    | **Change**                                                       |
 |----------------------------|------------------------|------------------------------------------------------------------|
 | 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`. |
+| Overflow in uncontrolled allocation size (`cpp/uncontrolled-allocation-size`) | More correct results | This query has been reworked so that it can find a wider variety of results. |
 | Memory may not be freed (`cpp/memory-may-not-be-freed`) | More correct results | Support added for more Microsoft-specific allocation functions, including `LocalAlloc`, `GlobalAlloc`, `HeapAlloc` and `CoTaskMemAlloc`. |
 | Memory is never freed (`cpp/memory-never-freed`) | More correct results | Support added for more Microsoft-specific allocation functions, including `LocalAlloc`, `GlobalAlloc`, `HeapAlloc` and `CoTaskMemAlloc`. |
 | Resource not released in destructor (`cpp/resource-not-released-in-destructor`) | Fewer false positive results | Resource allocation and deallocation functions are now determined more accurately. |

cpp/ql/src/Security/CWE/CWE-190/TaintedAllocationSize.ql

@@ -14,15 +14,20 @@
 import cpp
 import semmle.code.cpp.security.TaintTracking
 
-from Expr source, Expr tainted, BinaryArithmeticOperation oper,
-     SizeofOperator sizeof, string taintCause
-where tainted(source, tainted)
-  and oper.getAnOperand() = tainted
-  and oper.getOperator() = "*"
-  and oper.getAnOperand() = sizeof
-  and oper != tainted
-  and sizeof.getValue().toInt() > 1
-  and isUserInput(source, taintCause)
-select
-  oper, "This allocation size is derived from $@ and might overflow",
-  source, "user input (" + taintCause + ")"
+predicate taintedAllocSize(Expr e, Expr source, string taintCause) {
+  (
+    isAllocationExpr(e) or
+    any(MulExpr me | me.getAChild() instanceof SizeofOperator) = e
+  ) and
+  exists(Expr tainted |
+    tainted = e.getAChild() and
+    tainted.getType().getUnspecifiedType() instanceof IntegralType and
+    isUserInput(source, taintCause) and
+    tainted(source, tainted)
+  )
+}
+
+from Expr e, Expr source, string taintCause
+where taintedAllocSize(e, source, taintCause)
+select e, "This allocation size is derived from $@ and might overflow", source,
+  "user input (" + taintCause + ")"

cpp/ql/src/semmle/code/cpp/security/TaintTracking.qll

@@ -245,9 +245,14 @@ predicate insideFunctionValueMoveTo(Element src, Element dest)
       and format.getConversionChar(arg - formattingSend.getTarget().getNumberOfParameters()) = argFormat
       and (argFormat = "s" or argFormat = "S" or argFormat = "@"))
     // Expressions computed from tainted data are also tainted
-    or (exists (FunctionCall call | dest = call and isPureFunction(call.getTarget().getName()) |
-      call.getAnArgument() = src
-      and forall(Expr arg | arg = call.getAnArgument() | arg = src or predictable(arg))))
+    or exists(FunctionCall call | dest = call and isPureFunction(call.getTarget().getName()) |
+      call.getAnArgument() = src and
+      forall(Expr arg | arg = call.getAnArgument() | arg = src or predictable(arg)) and
+
+      // flow through `strlen` tends to cause dubious results, if the length is
+      // bounded.
+      not call.getTarget().getName() = "strlen"
+    )
     or exists(Element a, Element b |
       moveToDependingOnSide(a, b) and
       if insideValueSource(a) then

cpp/ql/test/query-tests/Security/CWE/CWE-190/semmle/TaintedAllocationSize/TaintedAllocationSize.expected

@@ -0,0 +1,6 @@
+| test.cpp:42:31:42:36 | call to malloc | This allocation size is derived from $@ and might overflow | test.cpp:39:21:39:24 | argv | user input (argv) |
+| test.cpp:43:38:43:63 | ... * ... | This allocation size is derived from $@ and might overflow | test.cpp:39:21:39:24 | argv | user input (argv) |
+| test.cpp:48:25:48:30 | call to malloc | This allocation size is derived from $@ and might overflow | test.cpp:39:21:39:24 | argv | user input (argv) |
+| test.cpp:49:17:49:30 | new[] | This allocation size is derived from $@ and might overflow | test.cpp:39:21:39:24 | argv | user input (argv) |
+| test.cpp:52:35:52:60 | ... * ... | This allocation size is derived from $@ and might overflow | test.cpp:39:21:39:24 | argv | user input (argv) |
+| test.cpp:55:11:55:24 | new[] | This allocation size is derived from $@ and might overflow | test.cpp:39:21:39:24 | argv | user input (argv) |

cpp/ql/test/query-tests/Security/CWE/CWE-190/semmle/TaintedAllocationSize/TaintedAllocationSize.qlref

@@ -0,0 +1 @@
+Security/CWE/CWE-190/TaintedAllocationSize.ql

cpp/ql/test/query-tests/Security/CWE/CWE-190/semmle/TaintedAllocationSize/test.cpp

@@ -0,0 +1,58 @@
+// Associated with CWE-190: Integer Overflow or Wraparound. http://cwe.mitre.org/data/definitions/190.html
+
+typedef unsigned long size_t;
+typedef struct {} FILE;
+
+void *malloc(size_t size);
+void *realloc(void *ptr, size_t size);
+int atoi(const char *nptr);
+
+struct MyStruct
+{
+	char data[256];
+};
+
+namespace std
+{
+	template<class charT> struct char_traits;
+
+	template <class charT, class traits = char_traits<charT> >
+	class basic_istream /*: virtual public basic_ios<charT,traits> - not needed for this test */ {
+	public:
+		basic_istream<charT,traits>& operator>>(int& n);
+	};
+
+	typedef basic_istream<char> istream;
+
+	extern istream cin;
+}
+
+int getTainted() {
+	int i;
+	
+	std::cin >> i;
+
+	return i;
+}
+
+int main(int argc, char **argv) {
+	int tainted = atoi(argv[1]);
+
+	MyStruct *arr1 = (MyStruct *)malloc(sizeof(MyStruct)); // GOOD
+	MyStruct *arr2 = (MyStruct *)malloc(tainted); // BAD
+	MyStruct *arr3 = (MyStruct *)malloc(tainted * sizeof(MyStruct)); // BAD
+	MyStruct *arr4 = (MyStruct *)malloc(getTainted() * sizeof(MyStruct)); // BAD [NOT DETECTED]
+	MyStruct *arr5 = (MyStruct *)malloc(sizeof(MyStruct) + tainted); // BAD [NOT DETECTED]
+
+	int size = tainted * 8;
+	char *chars1 = (char *)malloc(size); // BAD
+	char *chars2 = new char[size]; // BAD
+	char *chars3 = new char[8]; // GOOD
+
+	arr1 = (MyStruct *)realloc(arr1, sizeof(MyStruct) * tainted); // BAD
+
+	size = 8;
+	chars3 = new char[size]; // GOOD [FALSE POSITIVE]
+
+	return 0;
+}

cpp/ql/test/query-tests/Security/CWE/CWE-190/semmle/tainted/ArithmeticTainted.expected

@@ -8,6 +8,3 @@
 | test.c:14:15:14:28 | maxConnections | $@ flows to here and is used in arithmetic, potentially causing an underflow. | test.c:11:29:11:32 | argv | User-provided value |
 | test.c:44:7:44:10 | len2 | $@ flows to here and is used in arithmetic, potentially causing an underflow. | test.c:41:17:41:20 | argv | User-provided value |
 | test.c:54:7:54:10 | len3 | $@ flows to here and is used in arithmetic, potentially causing an underflow. | test.c:51:17:51:20 | argv | User-provided value |
-| test.c:74:7:74:10 | len5 | $@ flows to here and is used in arithmetic, potentially causing an underflow. | test.c:71:19:71:22 | argv | User-provided value |
-| test.c:84:7:84:10 | len6 | $@ flows to here and is used in arithmetic, potentially causing an underflow. | test.c:81:19:81:22 | argv | User-provided value |
-| test.c:94:7:94:10 | len7 | $@ flows to here and is used in arithmetic, potentially causing an underflow. | test.c:91:19:91:22 | argv | User-provided value |

cpp/ql/test/query-tests/Security/CWE/CWE-190/semmle/tainted/test.c

@@ -71,7 +71,7 @@ int main(int argc, char** argv) {
     len5 = strlen(argv[1]);
     while (len5)
     {
-      len5--; // GOOD: can't underflow [FALSE POSITIVE]
+      len5--; // GOOD: can't underflow
     }
   }
 
@@ -81,7 +81,7 @@ int main(int argc, char** argv) {
     len6 = strlen(argv[1]);
     while (len6 != 0)
     {
-      len6--; // GOOD: can't underflow [FALSE POSITIVE]
+      len6--; // GOOD: can't underflow
     }
   }
 
@@ -91,7 +91,7 @@ int main(int argc, char** argv) {
     len7 = strlen(argv[1]);
     while ((len7) && (1))
     {
-      len7--; // GOOD: can't underflow [FALSE POSITIVE]
+      len7--; // GOOD: can't underflow
     }
   }