Skip to content

C++: Use TaintTracking::Configuration in TaintedAllocationSize #3519

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.

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

Open
wants to merge 12 commits into
base: main
Choose a base branch
from
Open
Changes from 3 commits
Commits
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
55 changes: 49 additions & 6 deletions cpp/ql/src/Security/CWE/CWE-190/TaintedAllocationSize.ql
Original file line number Diff line number Diff line change
@@ -39,16 +39,59 @@ class TaintedAllocationSizeConfiguration extends TaintTracking::Configuration {
override predicate isSanitizerGuard(DataFlow::BarrierGuard guard) {
guard instanceof UpperBoundGuard
}

override predicate isSanitizer(DataFlow::Node node) {
node.asInstruction() instanceof BoundedVariableRead
}
}

class UpperBoundGuard extends DataFlow::BarrierGuard {
UpperBoundGuard() {
this.comparesLt(_, _, _, _, _)
}
UpperBoundGuard() { this.comparesLt(_, _, _, _, _) }

override predicate checks(Instruction i, boolean b) {
this.comparesLt(i.getAUse(), _, _, _, b)
}
override predicate checks(Instruction i, boolean b) { this.comparesLt(i.getAUse(), _, _, _, b) }
}

private predicate readsVariable(LoadInstruction load, Variable var) {
load.getSourceAddress().(VariableAddressInstruction).getASTVariable() = var
}

/**
* A variable that has any kind of upper-bound check anywhere in the program. This is
* biased towards being inclusive because there are a lot of valid ways of doing an
* upper bounds checks if we don't consider where it occurs, for example:
* ```
* if (x < 10) { sink(x); }
*
* if (10 > y) { sink(y); }
*
* if (z > 10) { z = 10; }
* sink(z);
* ```
*/
// TODO: This coarse overapproximation, ported from the old taint tracking
// library, could be replaced with an actual semantic check that a particular
// variable _access_ is guarded by an upper-bound check. We probably don't want
// to do this right away since it could expose a lot of FPs that were
// previously suppressed by this predicate by coincidence.
private predicate hasUpperBoundsCheck(Variable var) {
exists(RelationalOperation oper, VariableAccess access |
oper.getAnOperand() = access and
access.getTarget() = var and
// Comparing to 0 is not an upper bound check
not oper.getAnOperand().getValue() = "0"
)
}

/**
* A read of a variable that has an upper-bound check somewhere in the
* program.
*/
class BoundedVariableRead extends LoadInstruction {
BoundedVariableRead() {
exists(Variable checkedVar |
readsVariable(this, checkedVar) and
hasUpperBoundsCheck(checkedVar)
)
}

/*
Original file line number Diff line number Diff line change
@@ -23,16 +23,10 @@ edges
| test.cpp:100:17:100:22 | processData1 output argument | test.cpp:101:25:101:39 | ... + ... |
| test.cpp:101:17:101:22 | array to pointer conversion | test.cpp:75:25:75:29 | start |
| test.cpp:101:25:101:39 | ... + ... | test.cpp:75:38:75:40 | end |
| test.cpp:123:18:123:23 | call to getenv | test.cpp:124:29:124:32 | size |
| test.cpp:123:18:123:23 | call to getenv | test.cpp:127:24:127:41 | ... * ... |
| test.cpp:124:21:124:27 | call to bounded | test.cpp:126:24:126:49 | ... * ... |
| test.cpp:124:29:124:32 | size | test.cpp:124:21:124:27 | call to bounded |
| test.cpp:132:19:132:24 | call to getenv | test.cpp:134:10:134:27 | ... * ... |
| test.cpp:174:19:174:24 | call to getenv | test.cpp:176:10:176:27 | ... * ... |
| test.cpp:201:9:201:42 | Store | test.cpp:231:9:231:24 | call to get_tainted_size |
| test.cpp:201:14:201:19 | call to getenv | test.cpp:201:9:201:42 | Store |
| test.cpp:206:18:206:23 | call to getenv | test.cpp:211:9:211:9 | Store |
| test.cpp:211:9:211:9 | Store | test.cpp:232:9:232:24 | call to get_bounded_size |
| test.cpp:214:23:214:23 | s | test.cpp:215:21:215:21 | s |
| test.cpp:220:21:220:21 | s | test.cpp:221:21:221:21 | s |
| test.cpp:227:24:227:29 | call to getenv | test.cpp:229:9:229:18 | (size_t)... |
@@ -66,18 +60,11 @@ nodes
| test.cpp:101:17:101:22 | array to pointer conversion | semmle.label | array to pointer conversion |
| test.cpp:101:25:101:39 | ... + ... | semmle.label | ... + ... |
| test.cpp:123:18:123:23 | call to getenv | semmle.label | call to getenv |
| test.cpp:124:21:124:27 | call to bounded | semmle.label | call to bounded |
| test.cpp:124:29:124:32 | size | semmle.label | size |
| test.cpp:126:24:126:49 | ... * ... | semmle.label | ... * ... |
| test.cpp:127:24:127:41 | ... * ... | semmle.label | ... * ... |
| test.cpp:132:19:132:24 | call to getenv | semmle.label | call to getenv |
| test.cpp:134:10:134:27 | ... * ... | semmle.label | ... * ... |
| test.cpp:174:19:174:24 | call to getenv | semmle.label | call to getenv |
| test.cpp:176:10:176:27 | ... * ... | semmle.label | ... * ... |
| test.cpp:201:9:201:42 | Store | semmle.label | Store |
| test.cpp:201:14:201:19 | call to getenv | semmle.label | call to getenv |
| test.cpp:206:18:206:23 | call to getenv | semmle.label | call to getenv |
| test.cpp:211:9:211:9 | Store | semmle.label | Store |
| test.cpp:214:23:214:23 | s | semmle.label | s |
| test.cpp:215:21:215:21 | s | semmle.label | s |
| test.cpp:220:21:220:21 | s | semmle.label | s |
@@ -86,7 +73,6 @@ nodes
| test.cpp:229:9:229:18 | (size_t)... | semmle.label | (size_t)... |
| test.cpp:229:9:229:18 | local_size | semmle.label | local_size |
| test.cpp:231:9:231:24 | call to get_tainted_size | semmle.label | call to get_tainted_size |
| test.cpp:232:9:232:24 | call to get_bounded_size | semmle.label | call to get_bounded_size |
| test.cpp:235:11:235:20 | (size_t)... | semmle.label | (size_t)... |
| test.cpp:237:10:237:19 | (size_t)... | semmle.label | (size_t)... |
#select
@@ -98,12 +84,9 @@ nodes
| test.cpp:49:17:49:30 | new[] | test.cpp:39:21:39:24 | argv | test.cpp:49:26:49:29 | size | This allocation size is derived from $@ and might overflow | test.cpp:39:21:39:24 | argv | user input (a command line argument) |
| test.cpp:52:21:52:27 | call to realloc | test.cpp:39:21:39:24 | 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 (a command line argument) |
| test.cpp:79:9:79:29 | new[] | test.cpp:97:18:97:23 | fread output argument | test.cpp:79:18:79:28 | ... - ... | This allocation size is derived from $@ and might overflow | test.cpp:97:18:97:23 | buffer | user input (String read by fread) |
| test.cpp:126:17:126:22 | call to malloc | test.cpp:123:18:123:23 | call to getenv | test.cpp:126:24:126:49 | ... * ... | This allocation size is derived from $@ and might overflow | test.cpp:123:18:123:23 | call to getenv | user input (an environment variable) |
| test.cpp:127:17:127:22 | call to malloc | test.cpp:123:18:123:23 | call to getenv | test.cpp:127:24:127:41 | ... * ... | This allocation size is derived from $@ and might overflow | test.cpp:123:18:123:23 | call to getenv | user input (an environment variable) |
| test.cpp:134:3:134:8 | call to malloc | test.cpp:132:19:132:24 | call to getenv | test.cpp:134:10:134:27 | ... * ... | This allocation size is derived from $@ and might overflow | test.cpp:132:19:132:24 | call to getenv | user input (an environment variable) |
| test.cpp:176:3:176:8 | call to malloc | test.cpp:174:19:174:24 | call to getenv | test.cpp:176:10:176:27 | ... * ... | This allocation size is derived from $@ and might overflow | test.cpp:174:19:174:24 | call to getenv | user input (an environment variable) |
| test.cpp:215:14:215:19 | call to malloc | test.cpp:227:24:227:29 | call to getenv | test.cpp:215:21:215:21 | s | This allocation size is derived from $@ and might overflow | test.cpp:227:24:227:29 | call to getenv | user input (an environment variable) |
| test.cpp:221:14:221:19 | call to malloc | test.cpp:227:24:227:29 | call to getenv | test.cpp:221:21:221:21 | s | This allocation size is derived from $@ and might overflow | test.cpp:227:24:227:29 | call to getenv | user input (an environment variable) |
| test.cpp:229:2:229:7 | call to malloc | test.cpp:227:24:227:29 | call to getenv | test.cpp:229:9:229:18 | local_size | This allocation size is derived from $@ and might overflow | test.cpp:227:24:227:29 | call to getenv | user input (an environment variable) |
| test.cpp:231:2:231:7 | call to malloc | test.cpp:201:14:201:19 | call to getenv | test.cpp:231:9:231:24 | call to get_tainted_size | This allocation size is derived from $@ and might overflow | test.cpp:201:14:201:19 | call to getenv | user input (an environment variable) |
| test.cpp:232:2:232:7 | call to malloc | test.cpp:206:18:206:23 | call to getenv | test.cpp:232:9:232:24 | call to get_bounded_size | This allocation size is derived from $@ and might overflow | test.cpp:206:18:206:23 | call to getenv | user input (an environment variable) |
Original file line number Diff line number Diff line change
@@ -123,7 +123,7 @@ void open_file_bounded () {
int size = atoi(getenv("USER"));
int bounded_size = bounded(size, MAX_SIZE);

int* a = (int*)malloc(bounded_size * sizeof(int)); // GOOD [FALSE POSITIVE]
int* a = (int*)malloc(bounded_size * sizeof(int)); // GOOD
int* b = (int*)malloc(size * sizeof(int)); // BAD
}

@@ -229,7 +229,7 @@ void more_cases() {
malloc(local_size); // BAD
malloc(get_untainted_size()); // GOOD
malloc(get_tainted_size()); // BAD
malloc(get_bounded_size()); // GOOD [FALSE POSITIVE]
malloc(get_bounded_size()); // GOOD

my_alloc(100); // GOOD
my_alloc(local_size); // BAD [NOT DETECTED IN CORRECT LOCATION]