Skip to content

C++: Remove field conflation caused by IR field flow #3532

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 5 commits into from
May 27, 2020
Merged

C++: Remove field conflation caused by IR field flow #3532

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
218a3cf
C++: Remove field conflation
MathiasVP May 20, 2020
3c16712
C++: Accept test output
MathiasVP May 20, 2020
617ef32
C++: Remove [FALSE POSITIVE] annotations
MathiasVP May 21, 2020
b205d36
C++: Remove chi -> load rule from simpleLocalFlowStep and accept tests
MathiasVP May 26, 2020
bd97fe6
Merge branch 'master' into remove-field-conflation-from-ir-fieldflow
MathiasVP May 27, 2020
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
1 change: 1 addition & 0 deletions cpp/ql/src/semmle/code/cpp/ir/dataflow/DefaultTaintTracking.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import cpp
import semmle.code.cpp.security.Security
private import semmle.code.cpp.ir.dataflow.DataFlow
private import semmle.code.cpp.ir.dataflow.internal.DataFlowUtil
private import semmle.code.cpp.ir.dataflow.DataFlow2
private import semmle.code.cpp.ir.dataflow.DataFlow3
private import semmle.code.cpp.ir.IR
Expand Down
17 changes: 6 additions & 11 deletions cpp/ql/src/semmle/code/cpp/ir/dataflow/internal/DataFlowUtil.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -458,9 +458,9 @@ private predicate simpleInstructionLocalFlowStep(Instruction iFrom, Instruction
// for now.
iTo.getAnOperand().(ChiTotalOperand).getDef() = iFrom
or
// The next two rules allow flow from partial definitions in setters to succeeding loads in the caller.
// First, we add flow from write side-effects to non-conflated chi instructions through their
// partial operands. Consider the following example:
// Add flow from write side-effects to non-conflated chi instructions through their
// partial operands. From there, a `readStep` will find subsequent reads of that field.
// Consider the following example:
// ```
// void setX(Point* p, int new_x) {
// p->x = new_x;
Expand All @@ -470,14 +470,9 @@ private predicate simpleInstructionLocalFlowStep(Instruction iFrom, Instruction
// ```
// Here, a `WriteSideEffectInstruction` will provide a new definition for `p->x` after the call to
// `setX`, which will be melded into `p` through a chi instruction.
iTo.getAnOperand().(ChiPartialOperand).getDef() = iFrom.(WriteSideEffectInstruction) and
not iTo.isResultConflated()
or
// Next, we add flow from non-conflated chi instructions to loads (even when they are not precise).
// This ensures that loads of `p->x` gets data flow from the `WriteSideEffectInstruction` above.
exists(ChiInstruction chi | iFrom = chi |
not chi.isResultConflated() and
iTo.(LoadInstruction).getSourceValueOperand().getAnyDef() = chi
exists(ChiInstruction chi | chi = iTo |
chi.getPartialOperand().getDef() = iFrom.(WriteSideEffectInstruction) and
not chi.isResultConflated()
)
or
// Flow from stores to structs with a single field to a load of that field.
Expand Down
2 changes: 1 addition & 1 deletion cpp/ql/test/library-tests/dataflow/DefaultTaintTracking/defaulttainttracking.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,5 +115,5 @@ void test_conflated_fields3() {
XY xy;
xy.x = 0;
taint_y(&xy);
sink(xy.x); // not tainted [FALSE POSITIVE]
sink(xy.x); // not tainted
}
2 changes: 0 additions & 2 deletions cpp/ql/test/library-tests/dataflow/DefaultTaintTracking/tainted.expected
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,8 +103,6 @@
| defaulttainttracking.cpp:110:17:110:22 | call to getenv | defaulttainttracking.cpp:110:17:110:32 | (int)... |
| defaulttainttracking.cpp:110:17:110:22 | call to getenv | defaulttainttracking.cpp:110:17:110:32 | access to array |
| defaulttainttracking.cpp:110:17:110:22 | call to getenv | defaulttainttracking.cpp:111:12:111:18 | tainted |
| defaulttainttracking.cpp:110:17:110:22 | call to getenv | defaulttainttracking.cpp:118:11:118:11 | x |
| defaulttainttracking.cpp:110:17:110:22 | call to getenv | shared.h:6:15:6:23 | sinkparam |
| dispatch.cpp:28:29:28:34 | call to getenv | dispatch.cpp:28:24:28:27 | call to atoi |
| dispatch.cpp:28:29:28:34 | call to getenv | dispatch.cpp:28:29:28:34 | call to getenv |
| dispatch.cpp:28:29:28:34 | call to getenv | dispatch.cpp:28:29:28:45 | (const char *)... |
Expand Down
2 changes: 0 additions & 2 deletions cpp/ql/test/library-tests/dataflow/DefaultTaintTracking/test_diff.expected
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,8 +21,6 @@
| defaulttainttracking.cpp:97:27:97:32 | call to getenv | defaulttainttracking.cpp:98:10:98:11 | p2 | IR only |
| defaulttainttracking.cpp:97:27:97:32 | call to getenv | shared.h:5:23:5:31 | sinkparam | IR only |
| defaulttainttracking.cpp:110:17:110:22 | call to getenv | defaulttainttracking.cpp:111:8:111:8 | y | AST only |
| defaulttainttracking.cpp:110:17:110:22 | call to getenv | defaulttainttracking.cpp:118:11:118:11 | x | IR only |
| defaulttainttracking.cpp:110:17:110:22 | call to getenv | shared.h:6:15:6:23 | sinkparam | IR only |
| globals.cpp:13:15:13:20 | call to getenv | globals.cpp:13:5:13:11 | global1 | AST only |
| globals.cpp:23:15:23:20 | call to getenv | globals.cpp:23:5:23:11 | global2 | AST only |
| test_diff.cpp:104:12:104:15 | argv | test_diff.cpp:104:11:104:20 | (...) | IR only |
Expand Down
19 changes: 0 additions & 19 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
@@ -1,13 +1,4 @@
edges
| field_conflation.c:12:22:12:27 | call to getenv | field_conflation.c:13:3:13:18 | Chi |
| field_conflation.c:12:22:12:34 | (const char *)... | field_conflation.c:13:3:13:18 | Chi |
| field_conflation.c:13:3:13:18 | Chi | field_conflation.c:19:15:19:17 | taint_array output argument |
| field_conflation.c:19:15:19:17 | taint_array output argument | field_conflation.c:20:10:20:13 | (unsigned long)... |
| field_conflation.c:19:15:19:17 | taint_array output argument | field_conflation.c:20:13:20:13 | x |
| field_conflation.c:19:15:19:17 | taint_array output argument | field_conflation.c:20:13:20:13 | x |
| field_conflation.c:19:15:19:17 | taint_array output argument | field_conflation.c:20:13:20:13 | x |
| field_conflation.c:20:13:20:13 | x | field_conflation.c:20:10:20:13 | (unsigned long)... |
| field_conflation.c:20:13:20:13 | x | field_conflation.c:20:13:20:13 | x |
| test.cpp:39:21:39:24 | argv | test.cpp:42:38:42:44 | (size_t)... |
| test.cpp:39:21:39:24 | argv | test.cpp:42:38:42:44 | (size_t)... |
| test.cpp:39:21:39:24 | argv | test.cpp:42:38:42:44 | tainted |
Expand Down Expand Up @@ -89,15 +80,6 @@ edges
| test.cpp:309:19:309:32 | (const char *)... | test.cpp:314:10:314:27 | ... * ... |
| test.cpp:309:19:309:32 | (const char *)... | test.cpp:314:10:314:27 | ... * ... |
nodes
| field_conflation.c:12:22:12:27 | call to getenv | semmle.label | call to getenv |
| field_conflation.c:12:22:12:34 | (const char *)... | semmle.label | (const char *)... |
| field_conflation.c:13:3:13:18 | Chi | semmle.label | Chi |
| field_conflation.c:19:15:19:17 | taint_array output argument | semmle.label | taint_array output argument |
| field_conflation.c:20:10:20:13 | (unsigned long)... | semmle.label | (unsigned long)... |
| field_conflation.c:20:10:20:13 | (unsigned long)... | semmle.label | (unsigned long)... |
| field_conflation.c:20:13:20:13 | x | semmle.label | x |
| field_conflation.c:20:13:20:13 | x | semmle.label | x |
| field_conflation.c:20:13:20:13 | x | semmle.label | x |
| test.cpp:39:21:39:24 | argv | semmle.label | argv |
| test.cpp:39:21:39:24 | argv | semmle.label | argv |
| test.cpp:42:38:42:44 | (size_t)... | semmle.label | (size_t)... |
Expand Down Expand Up @@ -187,7 +169,6 @@ nodes
| test.cpp:314:10:314:27 | ... * ... | semmle.label | ... * ... |
| test.cpp:314:10:314:27 | ... * ... | semmle.label | ... * ... |
#select
| field_conflation.c:20:3:20:8 | call to malloc | field_conflation.c:12:22:12:27 | call to getenv | field_conflation.c:20:13:20:13 | x | This allocation size is derived from $@ and might overflow | field_conflation.c:12:22:12:27 | call to getenv | user input (getenv) |
| test.cpp:42:31:42:36 | call to malloc | test.cpp:39:21:39:24 | argv | test.cpp:42:38:42:44 | tainted | This allocation size is derived from $@ and might overflow | test.cpp:39:21:39:24 | argv | user input (argv) |
| test.cpp:43:31:43:36 | call to malloc | test.cpp:39:21:39:24 | 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:45:31:45:36 | call to malloc | test.cpp:39:21:39:24 | argv | test.cpp:45:38:45:63 | ... + ... | This allocation size is derived from $@ and might overflow | test.cpp:39:21:39:24 | argv | user input (argv) |
Expand Down
2 changes: 1 addition & 1 deletion cpp/ql/test/query-tests/Security/CWE/CWE-190/semmle/TaintedAllocationSize/field_conflation.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,5 +17,5 @@ void test_conflated_fields3(void) {
struct XY xy;
xy.x = 4;
taint_array(&xy);
malloc(xy.x); // not tainted [FALSE POSITIVE]
malloc(xy.x); // not tainted
}