From 5e8ac5ef0dc2f45d58e6fc0a6cedd58cb389c4f0 Mon Sep 17 00:00:00 2001 From: Simon Friis Vindum Date: Tue, 13 Aug 2024 13:10:20 +0200 Subject: [PATCH 1/2] C++: Update documentation for cpp/uncontrolled-allocation-size to clarify its scope --- .../CWE/CWE-190/TaintedAllocationSize.c | 14 ++++---- .../CWE/CWE-190/TaintedAllocationSize.qhelp | 16 +++++---- .../CWE/CWE-190/TaintedAllocationSize.ql | 9 ++--- .../TaintedAllocationSize.expected | 36 +++++++++---------- .../semmle/TaintedAllocationSize/test.cpp | 4 +-- 5 files changed, 41 insertions(+), 38 deletions(-) diff --git a/cpp/ql/src/Security/CWE/CWE-190/TaintedAllocationSize.c b/cpp/ql/src/Security/CWE/CWE-190/TaintedAllocationSize.c index 7a84a1aad13c..60a1db04f633 100644 --- a/cpp/ql/src/Security/CWE/CWE-190/TaintedAllocationSize.c +++ b/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 *)); +} diff --git a/cpp/ql/src/Security/CWE/CWE-190/TaintedAllocationSize.qhelp b/cpp/ql/src/Security/CWE/CWE-190/TaintedAllocationSize.qhelp index 3b8524608871..1097922ca0fc 100644 --- a/cpp/ql/src/Security/CWE/CWE-190/TaintedAllocationSize.qhelp +++ b/cpp/ql/src/Security/CWE/CWE-190/TaintedAllocationSize.qhelp @@ -3,12 +3,16 @@ "qhelp.dtd"> -

This code calculates an allocation size by multiplying a user input -by a sizeof 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.

+ +

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 being allocated.

+ +

If the allocation size is calculated by multiplying user input by a +sizeof 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.

 diff --git a/cpp/ql/src/Security/CWE/CWE-190/TaintedAllocationSize.ql b/cpp/ql/src/Security/CWE/CWE-190/TaintedAllocationSize.ql index a10ee006c470..4c869b06b83f 100644 --- a/cpp/ql/src/Security/CWE/CWE-190/TaintedAllocationSize.ql +++ b/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 + ")" diff --git a/cpp/ql/test/query-tests/Security/CWE/CWE-190/semmle/TaintedAllocationSize/TaintedAllocationSize.expected b/cpp/ql/test/query-tests/Security/CWE/CWE-190/semmle/TaintedAllocationSize/TaintedAllocationSize.expected index a10ef491282c..ff79d37c54f5 100644 --- a/cpp/ql/test/query-tests/Security/CWE/CWE-190/semmle/TaintedAllocationSize/TaintedAllocationSize.expected +++ b/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) | diff --git a/cpp/ql/test/query-tests/Security/CWE/CWE-190/semmle/TaintedAllocationSize/test.cpp b/cpp/ql/test/query-tests/Security/CWE/CWE-190/semmle/TaintedAllocationSize/test.cpp index 69cb2411b9fc..7bc166a92928 100644 --- a/cpp/ql/test/query-tests/Security/CWE/CWE-190/semmle/TaintedAllocationSize/test.cpp +++ b/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 From 5548304432942d566aeccfe1161ce8e81132868f Mon Sep 17 00:00:00 2001 From: Simon Friis Vindum Date: Fri, 16 Aug 2024 13:08:34 +0200 Subject: [PATCH 2/2] C++: Grammar improvements to query help text --- .../src/Security/CWE/CWE-190/TaintedAllocationSize.qhelp | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/cpp/ql/src/Security/CWE/CWE-190/TaintedAllocationSize.qhelp b/cpp/ql/src/Security/CWE/CWE-190/TaintedAllocationSize.qhelp index 1097922ca0fc..153a089bc12c 100644 --- a/cpp/ql/src/Security/CWE/CWE-190/TaintedAllocationSize.qhelp +++ b/cpp/ql/src/Security/CWE/CWE-190/TaintedAllocationSize.qhelp @@ -4,15 +4,15 @@ -

This code allocates memory using a size value based on user input +

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 being allocated.

+for arbitrary amounts of memory to be allocated.

If the allocation size is calculated by multiplying user input by a -sizeof expression the multiplication can overflow. When +sizeof 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.

+the allocated memory can then be out of bounds.