Merge pull request #17211 from paldepind/uncontrolled-allocation-size-docs

C++: Update documentation for cpp/uncontrolled-allocation-size to clarify its scope

Author: geoffw0

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

@@ -1,11 +1,9 @@
 int factor = atoi(getenv("BRANCHING_FACTOR"));
 
-// GOOD: Prevent overflow by checking the input
-if (factor < 0 || factor > 1000) {
-    log("Factor out of range (%d)\n", factor);
-    return -1;
-}
-
-// This line can allocate too little memory if factor
-// is very large.
+// BAD: This can allocate too little memory if factor is very large due to overflow.
 char **root_node = (char **) malloc(factor * sizeof(char *));
+
+// GOOD: Prevent overflow and unbounded allocation size by checking the input.
+if (factor > 0 && factor <= 1000) {
+    char **root_node = (char **) malloc(factor * sizeof(char *));
+}

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

@@ -3,12 +3,16 @@
   "qhelp.dtd">
 <qhelp>
 <overview>
-<p>This code calculates an allocation size by multiplying a user input
-by a <code>sizeof</code> expression. Since the user input has no
-apparent guard on its magnitude, this multiplication can
-overflow. When an integer multiply overflows in C, the result can wrap
-around and be much smaller than intended. A later attempt to put data
-into the allocated buffer can then overflow.</p>
+
+<p>This code allocates memory using a size value based on user input,
+with no apparent bound on its magnitude being established. This allows
+for arbitrary amounts of memory to be allocated.</p>
+
+<p>If the allocation size is calculated by multiplying user input by a
+<code>sizeof</code> expression, the multiplication can overflow. When
+an integer multiplication overflows in C, the result wraps around and
+can be much smaller than intended. A later attempt to write data into
+the allocated memory can then be out of bounds.</p>
 
 </overview>
 <recommendation>

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

@@ -1,7 +1,7 @@
 /**
- * @name Overflow in uncontrolled allocation size
- * @description Allocating memory with a size controlled by an external
- *              user can result in integer overflow.
+ * @name Uncontrolled allocation size
+ * @description Allocating memory with a size controlled by an external user can result in
+ *              arbitrary amounts of memory being allocated.
  * @kind path-problem
  * @problem.severity error
  * @security-severity 8.1
@@ -104,5 +104,6 @@ where
   isFlowSource(source.getNode(), taintCause) and
   TaintedAllocationSize::flowPath(source, sink) and
   allocSink(alloc, sink.getNode())
-select alloc, source, sink, "This allocation size is derived from $@ and might overflow.",
+select alloc, source, sink,
+  "This allocation size is derived from $@ and could allocate arbitrary amounts of memory.",
   source.getNode(), "user input (" + taintCause + ")"

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

@@ -79,21 +79,21 @@ nodes
 | test.cpp:356:35:356:38 | size | semmle.label | size |
 subpaths
 #select
-| test.cpp:43:31:43:36 | call to malloc | test.cpp:39:27:39:30 | **argv | test.cpp:43:38:43:44 | tainted | This allocation size is derived from $@ and might overflow. | test.cpp:39:27:39:30 | **argv | user input (a command-line argument) |
-| test.cpp:44:31:44:36 | call to malloc | test.cpp:39:27:39:30 | **argv | test.cpp:44:38:44:63 | ... * ... | This allocation size is derived from $@ and might overflow. | test.cpp:39:27:39:30 | **argv | user input (a command-line argument) |
-| test.cpp:46:31:46:36 | call to malloc | test.cpp:39:27:39:30 | **argv | test.cpp:46:38:46:63 | ... + ... | This allocation size is derived from $@ and might overflow. | test.cpp:39:27:39:30 | **argv | user input (a command-line argument) |
-| test.cpp:49:25:49:30 | call to malloc | test.cpp:39:27:39:30 | **argv | test.cpp:49:32:49:35 | size | This allocation size is derived from $@ and might overflow. | test.cpp:39:27:39:30 | **argv | user input (a command-line argument) |
-| test.cpp:50:17:50:30 | new[] | test.cpp:39:27:39:30 | **argv | test.cpp:50:17:50:30 | size | This allocation size is derived from $@ and might overflow. | test.cpp:39:27:39:30 | **argv | user input (a command-line argument) |
-| test.cpp:53:21:53:27 | call to realloc | test.cpp:39:27:39:30 | **argv | test.cpp:53:35:53:60 | ... * ... | This allocation size is derived from $@ and might overflow. | test.cpp:39:27:39:30 | **argv | user input (a command-line argument) |
-| test.cpp:128:17:128:22 | call to malloc | test.cpp:124:18:124:31 | *call to getenv | test.cpp:128:24:128:41 | ... * ... | This allocation size is derived from $@ and might overflow. | test.cpp:124:18:124:31 | *call to getenv | user input (an environment variable) |
-| test.cpp:135:3:135:8 | call to malloc | test.cpp:133:19:133:32 | *call to getenv | test.cpp:135:10:135:27 | ... * ... | This allocation size is derived from $@ and might overflow. | test.cpp:133:19:133:32 | *call to getenv | user input (an environment variable) |
-| test.cpp:152:4:152:9 | call to malloc | test.cpp:148:20:148:33 | *call to getenv | test.cpp:152:11:152:28 | ... * ... | This allocation size is derived from $@ and might overflow. | test.cpp:148:20:148:33 | *call to getenv | user input (an environment variable) |
-| test.cpp:231:14:231:19 | call to malloc | test.cpp:237:24:237:37 | *call to getenv | test.cpp:231:21:231:21 | s | This allocation size is derived from $@ and might overflow. | test.cpp:237:24:237:37 | *call to getenv | user input (an environment variable) |
-| test.cpp:239:2:239:7 | call to malloc | test.cpp:237:24:237:37 | *call to getenv | test.cpp:239:9:239:18 | local_size | This allocation size is derived from $@ and might overflow. | test.cpp:237:24:237:37 | *call to getenv | user input (an environment variable) |
-| test.cpp:241:2:241:7 | call to malloc | test.cpp:211:14:211:27 | *call to getenv | test.cpp:241:9:241:24 | call to get_tainted_size | This allocation size is derived from $@ and might overflow. | test.cpp:211:14:211:27 | *call to getenv | user input (an environment variable) |
-| test.cpp:245:2:245:9 | call to my_alloc | test.cpp:237:24:237:37 | *call to getenv | test.cpp:245:11:245:20 | local_size | This allocation size is derived from $@ and might overflow. | test.cpp:237:24:237:37 | *call to getenv | user input (an environment variable) |
-| test.cpp:263:4:263:9 | call to malloc | test.cpp:259:20:259:33 | *call to getenv | test.cpp:263:11:263:29 | ... * ... | This allocation size is derived from $@ and might overflow. | test.cpp:259:20:259:33 | *call to getenv | user input (an environment variable) |
-| test.cpp:291:4:291:9 | call to malloc | test.cpp:251:18:251:31 | *call to getenv | test.cpp:291:11:291:28 | ... * ... | This allocation size is derived from $@ and might overflow. | test.cpp:251:18:251:31 | *call to getenv | user input (an environment variable) |
-| test.cpp:308:3:308:8 | call to malloc | test.cpp:251:18:251:31 | *call to getenv | test.cpp:308:10:308:27 | ... * ... | This allocation size is derived from $@ and might overflow. | test.cpp:251:18:251:31 | *call to getenv | user input (an environment variable) |
-| test.cpp:355:25:355:33 | call to MyMalloc1 | test.cpp:353:18:353:31 | *call to getenv | test.cpp:355:35:355:38 | size | This allocation size is derived from $@ and might overflow. | test.cpp:353:18:353:31 | *call to getenv | user input (an environment variable) |
-| test.cpp:356:25:356:33 | call to MyMalloc2 | test.cpp:353:18:353:31 | *call to getenv | test.cpp:356:35:356:38 | size | This allocation size is derived from $@ and might overflow. | test.cpp:353:18:353:31 | *call to getenv | user input (an environment variable) |
+| test.cpp:43:31:43:36 | call to malloc | test.cpp:39:27:39:30 | **argv | test.cpp:43:38:43:44 | tainted | This allocation size is derived from $@ and could allocate arbitrary amounts of memory. | test.cpp:39:27:39:30 | **argv | user input (a command-line argument) |
+| test.cpp:44:31:44:36 | call to malloc | test.cpp:39:27:39:30 | **argv | test.cpp:44:38:44:63 | ... * ... | This allocation size is derived from $@ and could allocate arbitrary amounts of memory. | test.cpp:39:27:39:30 | **argv | user input (a command-line argument) |
+| test.cpp:46:31:46:36 | call to malloc | test.cpp:39:27:39:30 | **argv | test.cpp:46:38:46:63 | ... + ... | This allocation size is derived from $@ and could allocate arbitrary amounts of memory. | test.cpp:39:27:39:30 | **argv | user input (a command-line argument) |
+| test.cpp:49:25:49:30 | call to malloc | test.cpp:39:27:39:30 | **argv | test.cpp:49:32:49:35 | size | This allocation size is derived from $@ and could allocate arbitrary amounts of memory. | test.cpp:39:27:39:30 | **argv | user input (a command-line argument) |
+| test.cpp:50:17:50:30 | new[] | test.cpp:39:27:39:30 | **argv | test.cpp:50:17:50:30 | size | This allocation size is derived from $@ and could allocate arbitrary amounts of memory. | test.cpp:39:27:39:30 | **argv | user input (a command-line argument) |
+| test.cpp:53:21:53:27 | call to realloc | test.cpp:39:27:39:30 | **argv | test.cpp:53:35:53:60 | ... * ... | This allocation size is derived from $@ and could allocate arbitrary amounts of memory. | test.cpp:39:27:39:30 | **argv | user input (a command-line argument) |
+| test.cpp:128:17:128:22 | call to malloc | test.cpp:124:18:124:31 | *call to getenv | test.cpp:128:24:128:41 | ... * ... | This allocation size is derived from $@ and could allocate arbitrary amounts of memory. | test.cpp:124:18:124:31 | *call to getenv | user input (an environment variable) |
+| test.cpp:135:3:135:8 | call to malloc | test.cpp:133:19:133:32 | *call to getenv | test.cpp:135:10:135:27 | ... * ... | This allocation size is derived from $@ and could allocate arbitrary amounts of memory. | test.cpp:133:19:133:32 | *call to getenv | user input (an environment variable) |
+| test.cpp:152:4:152:9 | call to malloc | test.cpp:148:20:148:33 | *call to getenv | test.cpp:152:11:152:28 | ... * ... | This allocation size is derived from $@ and could allocate arbitrary amounts of memory. | test.cpp:148:20:148:33 | *call to getenv | user input (an environment variable) |
+| test.cpp:231:14:231:19 | call to malloc | test.cpp:237:24:237:37 | *call to getenv | test.cpp:231:21:231:21 | s | This allocation size is derived from $@ and could allocate arbitrary amounts of memory. | test.cpp:237:24:237:37 | *call to getenv | user input (an environment variable) |
+| test.cpp:239:2:239:7 | call to malloc | test.cpp:237:24:237:37 | *call to getenv | test.cpp:239:9:239:18 | local_size | This allocation size is derived from $@ and could allocate arbitrary amounts of memory. | test.cpp:237:24:237:37 | *call to getenv | user input (an environment variable) |
+| test.cpp:241:2:241:7 | call to malloc | test.cpp:211:14:211:27 | *call to getenv | test.cpp:241:9:241:24 | call to get_tainted_size | This allocation size is derived from $@ and could allocate arbitrary amounts of memory. | test.cpp:211:14:211:27 | *call to getenv | user input (an environment variable) |
+| test.cpp:245:2:245:9 | call to my_alloc | test.cpp:237:24:237:37 | *call to getenv | test.cpp:245:11:245:20 | local_size | This allocation size is derived from $@ and could allocate arbitrary amounts of memory. | test.cpp:237:24:237:37 | *call to getenv | user input (an environment variable) |
+| test.cpp:263:4:263:9 | call to malloc | test.cpp:259:20:259:33 | *call to getenv | test.cpp:263:11:263:29 | ... * ... | This allocation size is derived from $@ and could allocate arbitrary amounts of memory. | test.cpp:259:20:259:33 | *call to getenv | user input (an environment variable) |
+| test.cpp:291:4:291:9 | call to malloc | test.cpp:251:18:251:31 | *call to getenv | test.cpp:291:11:291:28 | ... * ... | This allocation size is derived from $@ and could allocate arbitrary amounts of memory. | test.cpp:251:18:251:31 | *call to getenv | user input (an environment variable) |
+| test.cpp:308:3:308:8 | call to malloc | test.cpp:251:18:251:31 | *call to getenv | test.cpp:308:10:308:27 | ... * ... | This allocation size is derived from $@ and could allocate arbitrary amounts of memory. | test.cpp:251:18:251:31 | *call to getenv | user input (an environment variable) |
+| test.cpp:355:25:355:33 | call to MyMalloc1 | test.cpp:353:18:353:31 | *call to getenv | test.cpp:355:35:355:38 | size | This allocation size is derived from $@ and could allocate arbitrary amounts of memory. | test.cpp:353:18:353:31 | *call to getenv | user input (an environment variable) |
+| test.cpp:356:25:356:33 | call to MyMalloc2 | test.cpp:353:18:353:31 | *call to getenv | test.cpp:356:35:356:38 | size | This allocation size is derived from $@ and could allocate arbitrary amounts of memory. | test.cpp:353:18:353:31 | *call to getenv | user input (an environment variable) |

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

@@ -40,10 +40,10 @@ int main(int argc, char **argv) {
 	int tainted = atoi(argv[1]);
 
 	MyStruct *arr1 = (MyStruct *)malloc(sizeof(MyStruct)); // GOOD
-	MyStruct *arr2 = (MyStruct *)malloc(tainted); // DUBIOUS (not multiplied by anything)
+	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); // DUBIOUS (not multiplied by anything)
+	MyStruct *arr5 = (MyStruct *)malloc(sizeof(MyStruct) + tainted); // BAD
 
 	int size = tainted * 8;
 	char *chars1 = (char *)malloc(size); // BAD