From d5442ec9c5990f307428c2c68c9549d7b7b88204 Mon Sep 17 00:00:00 2001 From: Mathias Vorreiter Pedersen Date: Fri, 8 Sep 2023 12:58:22 +0100 Subject: [PATCH 1/3] C++: Add regression test. --- .../test/library-tests/dataflow/dataflow-tests/test.cpp | 8 ++++++++ .../dataflow/dataflow-tests/uninitialized.expected | 3 +++ 2 files changed, 11 insertions(+) diff --git a/cpp/ql/test/library-tests/dataflow/dataflow-tests/test.cpp b/cpp/ql/test/library-tests/dataflow/dataflow-tests/test.cpp index c49d9092cd7..b2e2609829a 100644 --- a/cpp/ql/test/library-tests/dataflow/dataflow-tests/test.cpp +++ b/cpp/ql/test/library-tests/dataflow/dataflow-tests/test.cpp @@ -788,4 +788,12 @@ void test_sometimes_calls_sink_switch() { sometimes_calls_sink_switch(source(), 1); sometimes_calls_sink_switch(0, 0); sometimes_calls_sink_switch(source(), 0); +} + +void intPointerSource(int *ref_source, const int* another_arg); + +void test() { + MyStruct a; + intPointerSource(a.content, a.content); + indirect_sink(a.content); // $ ast MISSING: ir } \ No newline at end of file diff --git a/cpp/ql/test/library-tests/dataflow/dataflow-tests/uninitialized.expected b/cpp/ql/test/library-tests/dataflow/dataflow-tests/uninitialized.expected index dc5ea865b94..72290967857 100644 --- a/cpp/ql/test/library-tests/dataflow/dataflow-tests/uninitialized.expected +++ b/cpp/ql/test/library-tests/dataflow/dataflow-tests/uninitialized.expected @@ -46,3 +46,6 @@ | test.cpp:595:8:595:9 | xs | test.cpp:597:9:597:10 | xs | | test.cpp:733:7:733:7 | x | test.cpp:734:41:734:41 | x | | test.cpp:733:7:733:7 | x | test.cpp:735:8:735:8 | x | +| test.cpp:796:12:796:12 | a | test.cpp:797:20:797:20 | a | +| test.cpp:796:12:796:12 | a | test.cpp:797:31:797:31 | a | +| test.cpp:796:12:796:12 | a | test.cpp:798:17:798:17 | a | From 0be61be07ab2738d3526129ebeafd5d65fc17e50 Mon Sep 17 00:00:00 2001 From: Mathias Vorreiter Pedersen Date: Fri, 8 Sep 2023 12:25:00 +0100 Subject: [PATCH 2/3] C++: Handle flow out of post-update nodes when there's another use of the variable in the call that we need to skip. --- .../cpp/ir/dataflow/internal/SsaInternals.qll | 79 ++++++++++++++++--- 1 file changed, 69 insertions(+), 10 deletions(-) diff --git a/cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/SsaInternals.qll b/cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/SsaInternals.qll index 11c77ef9613..844633ad8d7 100644 --- a/cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/SsaInternals.qll +++ b/cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/SsaInternals.qll @@ -638,12 +638,24 @@ private predicate adjustForPointerArith(PostUpdateNode pun, UseOrPhi use) { ) } +/** + * Holds if `nodeFrom` flows to `nodeTo` because there is `def-use` or + * `use-use` flow from `defOrUse` to `use`. + * + * `uncertain` is `true` if the `defOrUse` is an uncertain definition. + */ +private predicate localSsaFlow( + SsaDefOrUse defOrUse, Node nodeFrom, UseOrPhi use, Node nodeTo, boolean uncertain +) { + nodeToDefOrUse(nodeFrom, defOrUse, uncertain) and + adjacentDefRead(defOrUse, use) and + useToNode(use, nodeTo) and + nodeFrom != nodeTo +} + private predicate ssaFlowImpl(SsaDefOrUse defOrUse, Node nodeFrom, Node nodeTo, boolean uncertain) { exists(UseOrPhi use | - nodeToDefOrUse(nodeFrom, defOrUse, uncertain) and - adjacentDefRead(defOrUse, use) and - useToNode(use, nodeTo) and - nodeFrom != nodeTo + localSsaFlow(defOrUse, nodeFrom, use, nodeTo, uncertain) or // Initial global variable value to a first use nodeFrom.(InitialGlobalValue).getGlobalDef() = defOrUse and @@ -721,15 +733,62 @@ private predicate isArgumentOfCallable(DataFlowCall call, Node n) { ) } -/** Holds if there is def-use or use-use flow from `pun` to `nodeTo`. */ -predicate postUpdateFlow(PostUpdateNode pun, Node nodeTo) { - exists(UseOrPhi use, Node preUpdate | +/** + * Holds if there is use-use flow from `pun`'s pre-update node to `n`. + */ +private predicate postUpdateNodeToFirstUse(PostUpdateNode pun, Node n) { + exists(UseOrPhi use | adjustForPointerArith(pun, use) and - useToNode(use, nodeTo) and + useToNode(use, n) + ) +} + +private predicate stepUntilNotInCall(DataFlowCall call, Node n1, Node n2) { + isArgumentOfCallable(call, n1) and + exists(Node mid | localSsaFlow(_, n1, _, mid, _) | + isArgumentOfCallable(call, mid) and + stepUntilNotInCall(call, mid, n2) + or + not isArgumentOfCallable(call, mid) and + mid = n2 + ) +} + +bindingset[n1, n2] +pragma[inline_late] +private predicate isArgumentOfSameCall(DataFlowCall call, Node n1, Node n2) { + isArgumentOfCallable(call, n1) and isArgumentOfCallable(call, n2) +} + +/** + * Holds if there is def-use or use-use flow from `pun` to `nodeTo`. + * + * Note: This is more complex than it sounds. Consider a call such as: + * ```cpp + * write_first_argument(x, x); + * sink(x); + * ``` + * Assume flow comes out of the first argument to `write_first_argument`. We + * don't want flow to go to the `x` that's also an argument to + * `write_first_argument` (because we just flowed out of that function, and we + * don't want to flow back into it again). + * + * We do, however, want flow from the output argument to `x` on the next line, and + * similarly we want flow from the second argument of `write_first_argument` to `x` + * on the next line. + */ +predicate postUpdateFlow(PostUpdateNode pun, Node nodeTo) { + exists(Node preUpdate, Node mid | preUpdate = pun.getPreUpdateNode() and - not exists(DataFlowCall call | - isArgumentOfCallable(call, preUpdate) and isArgumentOfCallable(call, nodeTo) + postUpdateNodeToFirstUse(pun, mid) + | + exists(DataFlowCall call | + isArgumentOfSameCall(call, preUpdate, mid) and + stepUntilNotInCall(call, mid, nodeTo) ) + or + not isArgumentOfSameCall(_, preUpdate, mid) and + nodeTo = mid ) } From 9f89c63771c65b400838b7aac467ec916c9b28d3 Mon Sep 17 00:00:00 2001 From: Mathias Vorreiter Pedersen Date: Fri, 8 Sep 2023 13:03:38 +0100 Subject: [PATCH 3/3] C++: Accept test changes. --- .../DefaultTaintTracking/annotate_sinks_only/tainted.expected | 2 +- cpp/ql/test/library-tests/dataflow/dataflow-tests/test.cpp | 2 +- cpp/ql/test/library-tests/dataflow/dataflow-tests/test.expected | 2 +- cpp/ql/test/library-tests/dataflow/fields/flow.expected | 2 +- .../CWE/CWE-119/semmle/tests/OverflowDestination.expected | 1 + 5 files changed, 5 insertions(+), 4 deletions(-) diff --git a/cpp/ql/test/library-tests/dataflow/DefaultTaintTracking/annotate_sinks_only/tainted.expected b/cpp/ql/test/library-tests/dataflow/DefaultTaintTracking/annotate_sinks_only/tainted.expected index 4cac8898022..fe5eed1b916 100644 --- a/cpp/ql/test/library-tests/dataflow/DefaultTaintTracking/annotate_sinks_only/tainted.expected +++ b/cpp/ql/test/library-tests/dataflow/DefaultTaintTracking/annotate_sinks_only/tainted.expected @@ -1,4 +1,4 @@ WARNING: Module TaintedWithPath has been deprecated and may be removed in future (tainted.ql:10,8-47) WARNING: Predicate tainted has been deprecated and may be removed in future (tainted.ql:21,3-28) -failures testFailures +failures diff --git a/cpp/ql/test/library-tests/dataflow/dataflow-tests/test.cpp b/cpp/ql/test/library-tests/dataflow/dataflow-tests/test.cpp index b2e2609829a..c5f7ffcf160 100644 --- a/cpp/ql/test/library-tests/dataflow/dataflow-tests/test.cpp +++ b/cpp/ql/test/library-tests/dataflow/dataflow-tests/test.cpp @@ -795,5 +795,5 @@ void intPointerSource(int *ref_source, const int* another_arg); void test() { MyStruct a; intPointerSource(a.content, a.content); - indirect_sink(a.content); // $ ast MISSING: ir + indirect_sink(a.content); // $ ast ir } \ No newline at end of file diff --git a/cpp/ql/test/library-tests/dataflow/dataflow-tests/test.expected b/cpp/ql/test/library-tests/dataflow/dataflow-tests/test.expected index 0260ed62b05..d4756e8d808 100644 --- a/cpp/ql/test/library-tests/dataflow/dataflow-tests/test.expected +++ b/cpp/ql/test/library-tests/dataflow/dataflow-tests/test.expected @@ -5,5 +5,5 @@ WARNING: Module DataFlow has been deprecated and may be removed in future (test. WARNING: Module DataFlow has been deprecated and may be removed in future (test.ql:40,25-33) WARNING: Module DataFlow has been deprecated and may be removed in future (test.ql:42,17-25) WARNING: Module DataFlow has been deprecated and may be removed in future (test.ql:46,20-28) -failures testFailures +failures diff --git a/cpp/ql/test/library-tests/dataflow/fields/flow.expected b/cpp/ql/test/library-tests/dataflow/fields/flow.expected index 48de9172b36..8ec8033d086 100644 --- a/cpp/ql/test/library-tests/dataflow/fields/flow.expected +++ b/cpp/ql/test/library-tests/dataflow/fields/flow.expected @@ -1,2 +1,2 @@ -failures testFailures +failures diff --git a/cpp/ql/test/query-tests/Security/CWE/CWE-119/semmle/tests/OverflowDestination.expected b/cpp/ql/test/query-tests/Security/CWE/CWE-119/semmle/tests/OverflowDestination.expected index 73f93c6ba9b..82049fc9229 100644 --- a/cpp/ql/test/query-tests/Security/CWE/CWE-119/semmle/tests/OverflowDestination.expected +++ b/cpp/ql/test/query-tests/Security/CWE/CWE-119/semmle/tests/OverflowDestination.expected @@ -7,6 +7,7 @@ edges | overflowdestination.cpp:50:52:50:54 | src indirection | overflowdestination.cpp:53:15:53:17 | src indirection | | overflowdestination.cpp:50:52:50:54 | src indirection | overflowdestination.cpp:54:9:54:12 | memcpy output argument | | overflowdestination.cpp:53:9:53:12 | memcpy output argument | overflowdestination.cpp:54:9:54:12 | memcpy output argument | +| overflowdestination.cpp:54:9:54:12 | memcpy output argument | overflowdestination.cpp:54:9:54:12 | memcpy output argument | | overflowdestination.cpp:57:52:57:54 | src indirection | overflowdestination.cpp:64:16:64:19 | src2 indirection | | overflowdestination.cpp:73:8:73:10 | fgets output argument | overflowdestination.cpp:75:30:75:32 | src indirection | | overflowdestination.cpp:73:8:73:10 | fgets output argument | overflowdestination.cpp:76:30:76:32 | src indirection |