diff --git a/cpp/ql/src/semmle/code/cpp/ir/dataflow/DefaultTaintTracking.qll b/cpp/ql/src/semmle/code/cpp/ir/dataflow/DefaultTaintTracking.qll index 8186ac9268fc..d13a6b58d830 100644 --- a/cpp/ql/src/semmle/code/cpp/ir/dataflow/DefaultTaintTracking.qll +++ b/cpp/ql/src/semmle/code/cpp/ir/dataflow/DefaultTaintTracking.qll @@ -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 diff --git a/cpp/ql/src/semmle/code/cpp/ir/dataflow/internal/DataFlowUtil.qll b/cpp/ql/src/semmle/code/cpp/ir/dataflow/internal/DataFlowUtil.qll index 9f9659a506e3..f9e1aad4e660 100644 --- a/cpp/ql/src/semmle/code/cpp/ir/dataflow/internal/DataFlowUtil.qll +++ b/cpp/ql/src/semmle/code/cpp/ir/dataflow/internal/DataFlowUtil.qll @@ -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; @@ -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. diff --git a/cpp/ql/test/library-tests/dataflow/DefaultTaintTracking/defaulttainttracking.cpp b/cpp/ql/test/library-tests/dataflow/DefaultTaintTracking/defaulttainttracking.cpp index a456bc856ffe..4dfba0aebf92 100644 --- a/cpp/ql/test/library-tests/dataflow/DefaultTaintTracking/defaulttainttracking.cpp +++ b/cpp/ql/test/library-tests/dataflow/DefaultTaintTracking/defaulttainttracking.cpp @@ -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 } diff --git a/cpp/ql/test/library-tests/dataflow/DefaultTaintTracking/tainted.expected b/cpp/ql/test/library-tests/dataflow/DefaultTaintTracking/tainted.expected index a2acc1a7dbf4..e1620e55f659 100644 --- a/cpp/ql/test/library-tests/dataflow/DefaultTaintTracking/tainted.expected +++ b/cpp/ql/test/library-tests/dataflow/DefaultTaintTracking/tainted.expected @@ -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 *)... | diff --git a/cpp/ql/test/library-tests/dataflow/DefaultTaintTracking/test_diff.expected b/cpp/ql/test/library-tests/dataflow/DefaultTaintTracking/test_diff.expected index 26c83f57cb77..b02339f160cc 100644 --- a/cpp/ql/test/library-tests/dataflow/DefaultTaintTracking/test_diff.expected +++ b/cpp/ql/test/library-tests/dataflow/DefaultTaintTracking/test_diff.expected @@ -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 | 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 2f9b7e626b40..8aa79b5bb6d9 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 @@ -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 | @@ -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)... | @@ -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) | diff --git a/cpp/ql/test/query-tests/Security/CWE/CWE-190/semmle/TaintedAllocationSize/field_conflation.c b/cpp/ql/test/query-tests/Security/CWE/CWE-190/semmle/TaintedAllocationSize/field_conflation.c index 5f6cb730e804..9a69a420a796 100644 --- a/cpp/ql/test/query-tests/Security/CWE/CWE-190/semmle/TaintedAllocationSize/field_conflation.c +++ b/cpp/ql/test/query-tests/Security/CWE/CWE-190/semmle/TaintedAllocationSize/field_conflation.c @@ -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 }