Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Aug 16, 2024
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
5e8ac5e
C++: Update documentation for cpp/uncontrolled-allocation-size to cla…
paldepind Aug 13, 2024
5548304
C++: Grammar improvements to query help text
paldepind Aug 16, 2024
07800ea
Merge branch 'main' into uncontrolled-allocation-size-docs
paldepind Aug 16, 2024
5504799
Merge branch 'main' into uncontrolled-allocation-size-docs
paldepind Aug 16, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 6 additions & 8 deletions cpp/ql/src/Security/CWE/CWE-190/TaintedAllocationSize.c
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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 *));
}
16 changes: 10 additions & 6 deletions cpp/ql/src/Security/CWE/CWE-190/TaintedAllocationSize.qhelp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Expand Down
9 changes: 5 additions & 4 deletions cpp/ql/src/Security/CWE/CWE-190/TaintedAllocationSize.ql
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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
Expand Down Expand Up @@ -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 + ")"
36 changes: 18 additions & 18 deletions ...ry-tests/Security/CWE/CWE-190/semmle/TaintedAllocationSize/TaintedAllocationSize.expected
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) |
4 changes: 2 additions & 2 deletions cpp/ql/test/query-tests/Security/CWE/CWE-190/semmle/TaintedAllocationSize/test.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed this comment as this unbounded allocation size is bad per the updated description of the query.

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
Expand Down
Loading