diff --git a/java/ql/test/library-tests/dataflow/deduplicate-path-graph/A.java b/java/ql/test/library-tests/dataflow/deduplicate-path-graph/A.java new file mode 100644 index 000000000000..8757165a07ff --- /dev/null +++ b/java/ql/test/library-tests/dataflow/deduplicate-path-graph/A.java @@ -0,0 +1,34 @@ +import java.util.function.Function; + +class A { + String source() { return ""; } + + void sink(String s) { } + + String propagateState(String s, String state) { + return ""; + } + + void foo() { + Function lambda = Math.random() > 0.5 + ? x -> propagateState(x, "A") + : x -> propagateState(x, "B"); + + String step0 = source(); + String step1 = lambda.apply(step0); + String step2 = lambda.apply(step1); + + sink(step2); + } + + void bar() { + Function lambda = + (x -> Math.random() > 0.5 ? propagateState(x, "A") : propagateState(x, "B")); + + String step0 = source(); + String step1 = lambda.apply(step0); + String step2 = lambda.apply(step1); + + sink(step2); + } +} diff --git a/java/ql/test/library-tests/dataflow/deduplicate-path-graph/test.expected b/java/ql/test/library-tests/dataflow/deduplicate-path-graph/test.expected new file mode 100644 index 000000000000..f41b92b2a3da --- /dev/null +++ b/java/ql/test/library-tests/dataflow/deduplicate-path-graph/test.expected @@ -0,0 +1,79 @@ +nodes +| A.java:14:9:14:9 | x : String | semmle.label | x : String | +| A.java:14:14:14:35 | propagateState(...) : String | semmle.label | propagateState(...) : String | +| A.java:14:29:14:29 | x : String | semmle.label | x : String | +| A.java:15:9:15:9 | x : String | semmle.label | x : String | +| A.java:15:14:15:35 | propagateState(...) : String | semmle.label | propagateState(...) : String | +| A.java:15:29:15:29 | x : String | semmle.label | x : String | +| A.java:17:20:17:27 | source(...) : String | semmle.label | source(...) : String | +| A.java:18:20:18:38 | apply(...) : String | semmle.label | apply(...) : String | +| A.java:18:20:18:38 | apply(...) : String | semmle.label | apply(...) : String | +| A.java:18:33:18:37 | step0 : String | semmle.label | step0 : String | +| A.java:19:20:19:38 | apply(...) : String | semmle.label | apply(...) : String | +| A.java:19:33:19:37 | step1 : String | semmle.label | step1 : String | +| A.java:19:33:19:37 | step1 : String | semmle.label | step1 : String | +| A.java:21:10:21:14 | step2 | semmle.label | step2 | +| A.java:26:8:26:8 | x : String | semmle.label | x : String | +| A.java:26:8:26:8 | x : String | semmle.label | x : String | +| A.java:26:13:26:81 | ...?...:... : String | semmle.label | ...?...:... : String | +| A.java:26:13:26:81 | ...?...:... : String | semmle.label | ...?...:... : String | +| A.java:26:35:26:56 | propagateState(...) : String | semmle.label | propagateState(...) : String | +| A.java:26:50:26:50 | x : String | semmle.label | x : String | +| A.java:26:60:26:81 | propagateState(...) : String | semmle.label | propagateState(...) : String | +| A.java:26:75:26:75 | x : String | semmle.label | x : String | +| A.java:28:20:28:27 | source(...) : String | semmle.label | source(...) : String | +| A.java:29:20:29:38 | apply(...) : String | semmle.label | apply(...) : String | +| A.java:29:20:29:38 | apply(...) : String | semmle.label | apply(...) : String | +| A.java:29:33:29:37 | step0 : String | semmle.label | step0 : String | +| A.java:30:20:30:38 | apply(...) : String | semmle.label | apply(...) : String | +| A.java:30:33:30:37 | step1 : String | semmle.label | step1 : String | +| A.java:30:33:30:37 | step1 : String | semmle.label | step1 : String | +| A.java:32:10:32:14 | step2 | semmle.label | step2 | +edges +| A.java:14:9:14:9 | x : String | A.java:14:29:14:29 | x : String | provenance | | +| A.java:14:29:14:29 | x : String | A.java:14:14:14:35 | propagateState(...) : String | provenance | Config | +| A.java:15:9:15:9 | x : String | A.java:15:29:15:29 | x : String | provenance | | +| A.java:15:29:15:29 | x : String | A.java:15:14:15:35 | propagateState(...) : String | provenance | Config | +| A.java:17:20:17:27 | source(...) : String | A.java:18:33:18:37 | step0 : String | provenance | | +| A.java:18:20:18:38 | apply(...) : String | A.java:19:33:19:37 | step1 : String | provenance | | +| A.java:18:20:18:38 | apply(...) : String | A.java:19:33:19:37 | step1 : String | provenance | | +| A.java:18:33:18:37 | step0 : String | A.java:14:9:14:9 | x : String | provenance | | +| A.java:18:33:18:37 | step0 : String | A.java:15:9:15:9 | x : String | provenance | | +| A.java:18:33:18:37 | step0 : String | A.java:18:20:18:38 | apply(...) : String | provenance | Config | +| A.java:18:33:18:37 | step0 : String | A.java:18:20:18:38 | apply(...) : String | provenance | Config | +| A.java:19:20:19:38 | apply(...) : String | A.java:21:10:21:14 | step2 | provenance | | +| A.java:19:33:19:37 | step1 : String | A.java:14:9:14:9 | x : String | provenance | | +| A.java:19:33:19:37 | step1 : String | A.java:15:9:15:9 | x : String | provenance | | +| A.java:19:33:19:37 | step1 : String | A.java:19:20:19:38 | apply(...) : String | provenance | Config | +| A.java:19:33:19:37 | step1 : String | A.java:19:20:19:38 | apply(...) : String | provenance | Config | +| A.java:26:8:26:8 | x : String | A.java:26:50:26:50 | x : String | provenance | | +| A.java:26:8:26:8 | x : String | A.java:26:75:26:75 | x : String | provenance | | +| A.java:26:35:26:56 | propagateState(...) : String | A.java:26:13:26:81 | ...?...:... : String | provenance | | +| A.java:26:50:26:50 | x : String | A.java:26:35:26:56 | propagateState(...) : String | provenance | Config | +| A.java:26:60:26:81 | propagateState(...) : String | A.java:26:13:26:81 | ...?...:... : String | provenance | | +| A.java:26:75:26:75 | x : String | A.java:26:60:26:81 | propagateState(...) : String | provenance | Config | +| A.java:28:20:28:27 | source(...) : String | A.java:29:33:29:37 | step0 : String | provenance | | +| A.java:29:20:29:38 | apply(...) : String | A.java:30:33:30:37 | step1 : String | provenance | | +| A.java:29:20:29:38 | apply(...) : String | A.java:30:33:30:37 | step1 : String | provenance | | +| A.java:29:33:29:37 | step0 : String | A.java:26:8:26:8 | x : String | provenance | | +| A.java:29:33:29:37 | step0 : String | A.java:26:8:26:8 | x : String | provenance | | +| A.java:29:33:29:37 | step0 : String | A.java:29:20:29:38 | apply(...) : String | provenance | Config | +| A.java:29:33:29:37 | step0 : String | A.java:29:20:29:38 | apply(...) : String | provenance | Config | +| A.java:30:20:30:38 | apply(...) : String | A.java:32:10:32:14 | step2 | provenance | | +| A.java:30:33:30:37 | step1 : String | A.java:26:8:26:8 | x : String | provenance | | +| A.java:30:33:30:37 | step1 : String | A.java:26:8:26:8 | x : String | provenance | | +| A.java:30:33:30:37 | step1 : String | A.java:30:20:30:38 | apply(...) : String | provenance | Config | +| A.java:30:33:30:37 | step1 : String | A.java:30:20:30:38 | apply(...) : String | provenance | Config | +subpaths +| A.java:18:33:18:37 | step0 : String | A.java:14:9:14:9 | x : String | A.java:14:14:14:35 | propagateState(...) : String | A.java:18:20:18:38 | apply(...) : String | +| A.java:18:33:18:37 | step0 : String | A.java:15:9:15:9 | x : String | A.java:15:14:15:35 | propagateState(...) : String | A.java:18:20:18:38 | apply(...) : String | +| A.java:19:33:19:37 | step1 : String | A.java:14:9:14:9 | x : String | A.java:14:14:14:35 | propagateState(...) : String | A.java:19:20:19:38 | apply(...) : String | +| A.java:19:33:19:37 | step1 : String | A.java:15:9:15:9 | x : String | A.java:15:14:15:35 | propagateState(...) : String | A.java:19:20:19:38 | apply(...) : String | +| A.java:29:33:29:37 | step0 : String | A.java:26:8:26:8 | x : String | A.java:26:13:26:81 | ...?...:... : String | A.java:29:20:29:38 | apply(...) : String | +| A.java:29:33:29:37 | step0 : String | A.java:26:8:26:8 | x : String | A.java:26:13:26:81 | ...?...:... : String | A.java:29:20:29:38 | apply(...) : String | +| A.java:30:33:30:37 | step1 : String | A.java:26:8:26:8 | x : String | A.java:26:13:26:81 | ...?...:... : String | A.java:30:20:30:38 | apply(...) : String | +| A.java:30:33:30:37 | step1 : String | A.java:26:8:26:8 | x : String | A.java:26:13:26:81 | ...?...:... : String | A.java:30:20:30:38 | apply(...) : String | +spuriousFlow +#select +| A.java:17:20:17:27 | source(...) : String | A.java:17:20:17:27 | source(...) : String | A.java:21:10:21:14 | step2 | Flow | +| A.java:28:20:28:27 | source(...) : String | A.java:28:20:28:27 | source(...) : String | A.java:32:10:32:14 | step2 | Flow | diff --git a/java/ql/test/library-tests/dataflow/deduplicate-path-graph/test.ql b/java/ql/test/library-tests/dataflow/deduplicate-path-graph/test.ql new file mode 100644 index 000000000000..6d7e7c7b95d4 --- /dev/null +++ b/java/ql/test/library-tests/dataflow/deduplicate-path-graph/test.ql @@ -0,0 +1,84 @@ +/** + * @kind path-problem + */ + +import java +import semmle.code.java.dataflow.DataFlow +import DataFlow + +MethodCall propagateCall(string state) { + result.getMethod().getName() = "propagateState" and + state = result.getArgument(1).(StringLiteral).getValue() +} + +module TestConfig implements StateConfigSig { + class FlowState = string; + + predicate isSource(Node n, FlowState state) { + n.asExpr().(MethodCall).getMethod().getName() = "source" and state = ["A", "B"] + } + + predicate isSink(Node n, FlowState state) { + n.asExpr() = any(MethodCall acc | acc.getMethod().getName() = "sink").getAnArgument() and + state = ["A", "B"] + } + + predicate isAdditionalFlowStep(Node node1, FlowState state1, Node node2, FlowState state2) { + exists(MethodCall call | + call = propagateCall(state1) and + state2 = state1 and + node1.asExpr() = call.getArgument(0) and + node2.asExpr() = call + ) + } +} + +module TestFlow = DataFlow::GlobalWithState; + +module Graph = DataFlow::DeduplicatePathGraph; + +/** + * Holds if `node` is reachable from a call to `propagateState` with the given `state` argument. + * `call` indicates if a call step was taken (i.e. into a subpath). + * + * We use this to check if one `propagateState` can flow out of another, which is not allowed. + */ +predicate reachableFromPropagate(Graph::PathNode node, string state, boolean call) { + node.getNode().asExpr() = propagateCall(state) and call = false + or + exists(Graph::PathNode prev | reachableFromPropagate(prev, state, call) | + Graph::edges(prev, node, _, _) and + not Graph::subpaths(prev, node, _, _) // argument-passing edges are handled separately + or + Graph::subpaths(prev, _, _, node) // arg -> out (should be included in 'edges' but handle the case here for clarity) + ) + or + exists(Graph::PathNode prev | + reachableFromPropagate(prev, state, _) and + Graph::subpaths(prev, node, _, _) and // arg -> parameter + call = true + ) + or + exists(Graph::PathNode prev | + reachableFromPropagate(prev, state, call) and + Graph::subpaths(_, _, prev, node) and // return -> out + call = false + ) +} + +/** + * Holds if `node` is the return value of a `propagateState` call that appears to be reachable + * with a different state than the one propagated by the call, indicating spurious flow resulting from + * merging path nodes. + */ +query predicate spuriousFlow(Graph::PathNode node, string state1, string state2) { + reachableFromPropagate(node, state1, _) and + node.getNode().asExpr() = propagateCall(state2) and + state1 != state2 +} + +import Graph::PathGraph + +from Graph::PathNode source, Graph::PathNode sink +where TestFlow::flowPath(source.getAnOriginalPathNode(), sink.getAnOriginalPathNode()) +select source, source, sink, "Flow" diff --git a/ruby/ql/src/queries/security/cwe-094/CodeInjection.ql b/ruby/ql/src/queries/security/cwe-094/CodeInjection.ql index 79ae0d9c6727..65a130566955 100644 --- a/ruby/ql/src/queries/security/cwe-094/CodeInjection.ql +++ b/ruby/ql/src/queries/security/cwe-094/CodeInjection.ql @@ -16,20 +16,9 @@ private import codeql.ruby.AST private import codeql.ruby.security.CodeInjectionQuery -import CodeInjectionFlow::PathGraph +import DataFlow::DeduplicatePathGraph -from CodeInjectionFlow::PathNode source, CodeInjectionFlow::PathNode sink, Source sourceNode -where - CodeInjectionFlow::flowPath(source, sink) and - sourceNode = source.getNode() and - // removing duplications of the same path, but different flow-labels. - sink = - min(CodeInjectionFlow::PathNode otherSink | - CodeInjectionFlow::flowPath(any(CodeInjectionFlow::PathNode s | s.getNode() = sourceNode), - otherSink) and - otherSink.getNode() = sink.getNode() - | - otherSink order by otherSink.getState().getStringRepresentation() - ) -select sink.getNode(), source, sink, "This code execution depends on a $@.", sourceNode, +from PathNode source, PathNode sink +where CodeInjectionFlow::flowPath(source.getAnOriginalPathNode(), sink.getAnOriginalPathNode()) +select sink.getNode(), source, sink, "This code execution depends on a $@.", source.getNode(), "user-provided value" diff --git a/ruby/ql/test/query-tests/security/cwe-094/CodeInjection/CodeInjection.expected b/ruby/ql/test/query-tests/security/cwe-094/CodeInjection/CodeInjection.expected index bc709a0e469d..3ce4b8ebe033 100644 --- a/ruby/ql/test/query-tests/security/cwe-094/CodeInjection/CodeInjection.expected +++ b/ruby/ql/test/query-tests/security/cwe-094/CodeInjection/CodeInjection.expected @@ -1,98 +1,60 @@ -edges -| CodeInjection.rb:5:5:5:8 | code | CodeInjection.rb:8:10:8:13 | code | provenance | | -| CodeInjection.rb:5:5:5:8 | code | CodeInjection.rb:8:10:8:13 | code | provenance | | -| CodeInjection.rb:5:5:5:8 | code | CodeInjection.rb:20:20:20:23 | code | provenance | | -| CodeInjection.rb:5:5:5:8 | code | CodeInjection.rb:20:20:20:23 | code | provenance | | -| CodeInjection.rb:5:5:5:8 | code | CodeInjection.rb:23:21:23:24 | code | provenance | | -| CodeInjection.rb:5:5:5:8 | code | CodeInjection.rb:23:21:23:24 | code | provenance | | -| CodeInjection.rb:5:5:5:8 | code | CodeInjection.rb:29:15:29:18 | code | provenance | | -| CodeInjection.rb:5:5:5:8 | code | CodeInjection.rb:32:19:32:22 | code | provenance | | -| CodeInjection.rb:5:5:5:8 | code | CodeInjection.rb:38:24:38:27 | code | provenance | | -| CodeInjection.rb:5:5:5:8 | code | CodeInjection.rb:38:24:38:27 | code | provenance | | -| CodeInjection.rb:5:5:5:8 | code | CodeInjection.rb:41:40:41:43 | code | provenance | | -| CodeInjection.rb:5:12:5:17 | call to params | CodeInjection.rb:5:12:5:24 | ...[...] | provenance | | -| CodeInjection.rb:5:12:5:17 | call to params | CodeInjection.rb:5:12:5:24 | ...[...] | provenance | | -| CodeInjection.rb:5:12:5:24 | ...[...] | CodeInjection.rb:5:5:5:8 | code | provenance | | -| CodeInjection.rb:5:12:5:24 | ...[...] | CodeInjection.rb:5:5:5:8 | code | provenance | | -| CodeInjection.rb:38:24:38:27 | code | CodeInjection.rb:38:10:38:28 | call to escape | provenance | MaD:21 | -| CodeInjection.rb:38:24:38:27 | code | CodeInjection.rb:38:10:38:28 | call to escape | provenance | MaD:21 | -| CodeInjection.rb:78:5:78:8 | code | CodeInjection.rb:80:16:80:19 | code | provenance | | -| CodeInjection.rb:78:5:78:8 | code | CodeInjection.rb:86:10:86:37 | ... + ... | provenance | | -| CodeInjection.rb:78:5:78:8 | code | CodeInjection.rb:86:22:86:25 | code | provenance | | -| CodeInjection.rb:78:5:78:8 | code | CodeInjection.rb:88:10:88:32 | "prefix_#{...}_suffix" | provenance | AdditionalTaintStep | -| CodeInjection.rb:78:5:78:8 | code | CodeInjection.rb:90:10:90:13 | code | provenance | | -| CodeInjection.rb:78:5:78:8 | code | CodeInjection.rb:90:10:90:13 | code | provenance | | -| CodeInjection.rb:78:12:78:17 | call to params | CodeInjection.rb:78:12:78:24 | ...[...] | provenance | | -| CodeInjection.rb:78:12:78:17 | call to params | CodeInjection.rb:78:12:78:24 | ...[...] | provenance | | -| CodeInjection.rb:78:12:78:24 | ...[...] | CodeInjection.rb:78:5:78:8 | code | provenance | | -| CodeInjection.rb:78:12:78:24 | ...[...] | CodeInjection.rb:78:5:78:8 | code | provenance | | -| CodeInjection.rb:86:10:86:25 | ... + ... [element] | CodeInjection.rb:86:10:86:37 | ... + ... | provenance | | -| CodeInjection.rb:86:22:86:25 | code | CodeInjection.rb:86:10:86:25 | ... + ... [element] | provenance | | -| CodeInjection.rb:101:3:102:5 | self in index [@foo] | CodeInjection.rb:111:3:113:5 | self in baz [@foo] | provenance | | -| CodeInjection.rb:101:3:102:5 | self in index [@foo] | CodeInjection.rb:111:3:113:5 | self in baz [@foo] | provenance | | -| CodeInjection.rb:105:5:105:8 | [post] self [@foo] | CodeInjection.rb:108:3:109:5 | self in bar [@foo] | provenance | | -| CodeInjection.rb:105:5:105:8 | [post] self [@foo] | CodeInjection.rb:108:3:109:5 | self in bar [@foo] | provenance | | -| CodeInjection.rb:105:12:105:17 | call to params | CodeInjection.rb:105:12:105:23 | ...[...] | provenance | | -| CodeInjection.rb:105:12:105:17 | call to params | CodeInjection.rb:105:12:105:23 | ...[...] | provenance | | -| CodeInjection.rb:105:12:105:23 | ...[...] | CodeInjection.rb:105:5:105:8 | [post] self [@foo] | provenance | | -| CodeInjection.rb:105:12:105:23 | ...[...] | CodeInjection.rb:105:5:105:8 | [post] self [@foo] | provenance | | -| CodeInjection.rb:108:3:109:5 | self in bar [@foo] | CodeInjection.rb:101:3:102:5 | self in index [@foo] | provenance | | -| CodeInjection.rb:108:3:109:5 | self in bar [@foo] | CodeInjection.rb:101:3:102:5 | self in index [@foo] | provenance | | -| CodeInjection.rb:111:3:113:5 | self in baz [@foo] | CodeInjection.rb:112:10:112:13 | self [@foo] | provenance | | -| CodeInjection.rb:111:3:113:5 | self in baz [@foo] | CodeInjection.rb:112:10:112:13 | self [@foo] | provenance | | -| CodeInjection.rb:112:10:112:13 | self [@foo] | CodeInjection.rb:112:10:112:13 | @foo | provenance | | -| CodeInjection.rb:112:10:112:13 | self [@foo] | CodeInjection.rb:112:10:112:13 | @foo | provenance | | nodes | CodeInjection.rb:5:5:5:8 | code | semmle.label | code | -| CodeInjection.rb:5:5:5:8 | code | semmle.label | code | -| CodeInjection.rb:5:12:5:17 | call to params | semmle.label | call to params | | CodeInjection.rb:5:12:5:17 | call to params | semmle.label | call to params | | CodeInjection.rb:5:12:5:24 | ...[...] | semmle.label | ...[...] | -| CodeInjection.rb:5:12:5:24 | ...[...] | semmle.label | ...[...] | -| CodeInjection.rb:8:10:8:13 | code | semmle.label | code | | CodeInjection.rb:8:10:8:13 | code | semmle.label | code | | CodeInjection.rb:11:10:11:15 | call to params | semmle.label | call to params | -| CodeInjection.rb:11:10:11:15 | call to params | semmle.label | call to params | | CodeInjection.rb:20:20:20:23 | code | semmle.label | code | -| CodeInjection.rb:20:20:20:23 | code | semmle.label | code | -| CodeInjection.rb:23:21:23:24 | code | semmle.label | code | | CodeInjection.rb:23:21:23:24 | code | semmle.label | code | | CodeInjection.rb:29:15:29:18 | code | semmle.label | code | | CodeInjection.rb:32:19:32:22 | code | semmle.label | code | | CodeInjection.rb:38:10:38:28 | call to escape | semmle.label | call to escape | -| CodeInjection.rb:38:10:38:28 | call to escape | semmle.label | call to escape | -| CodeInjection.rb:38:24:38:27 | code | semmle.label | code | | CodeInjection.rb:38:24:38:27 | code | semmle.label | code | | CodeInjection.rb:41:40:41:43 | code | semmle.label | code | | CodeInjection.rb:78:5:78:8 | code | semmle.label | code | -| CodeInjection.rb:78:5:78:8 | code | semmle.label | code | -| CodeInjection.rb:78:12:78:17 | call to params | semmle.label | call to params | | CodeInjection.rb:78:12:78:17 | call to params | semmle.label | call to params | | CodeInjection.rb:78:12:78:24 | ...[...] | semmle.label | ...[...] | -| CodeInjection.rb:78:12:78:24 | ...[...] | semmle.label | ...[...] | | CodeInjection.rb:80:16:80:19 | code | semmle.label | code | | CodeInjection.rb:86:10:86:25 | ... + ... [element] | semmle.label | ... + ... [element] | | CodeInjection.rb:86:10:86:37 | ... + ... | semmle.label | ... + ... | | CodeInjection.rb:86:22:86:25 | code | semmle.label | code | | CodeInjection.rb:88:10:88:32 | "prefix_#{...}_suffix" | semmle.label | "prefix_#{...}_suffix" | | CodeInjection.rb:90:10:90:13 | code | semmle.label | code | -| CodeInjection.rb:90:10:90:13 | code | semmle.label | code | -| CodeInjection.rb:101:3:102:5 | self in index [@foo] | semmle.label | self in index [@foo] | | CodeInjection.rb:101:3:102:5 | self in index [@foo] | semmle.label | self in index [@foo] | | CodeInjection.rb:105:5:105:8 | [post] self [@foo] | semmle.label | [post] self [@foo] | -| CodeInjection.rb:105:5:105:8 | [post] self [@foo] | semmle.label | [post] self [@foo] | -| CodeInjection.rb:105:12:105:17 | call to params | semmle.label | call to params | | CodeInjection.rb:105:12:105:17 | call to params | semmle.label | call to params | | CodeInjection.rb:105:12:105:23 | ...[...] | semmle.label | ...[...] | -| CodeInjection.rb:105:12:105:23 | ...[...] | semmle.label | ...[...] | | CodeInjection.rb:108:3:109:5 | self in bar [@foo] | semmle.label | self in bar [@foo] | -| CodeInjection.rb:108:3:109:5 | self in bar [@foo] | semmle.label | self in bar [@foo] | -| CodeInjection.rb:111:3:113:5 | self in baz [@foo] | semmle.label | self in baz [@foo] | | CodeInjection.rb:111:3:113:5 | self in baz [@foo] | semmle.label | self in baz [@foo] | | CodeInjection.rb:112:10:112:13 | @foo | semmle.label | @foo | -| CodeInjection.rb:112:10:112:13 | @foo | semmle.label | @foo | -| CodeInjection.rb:112:10:112:13 | self [@foo] | semmle.label | self [@foo] | | CodeInjection.rb:112:10:112:13 | self [@foo] | semmle.label | self [@foo] | +edges +| CodeInjection.rb:5:5:5:8 | code | CodeInjection.rb:8:10:8:13 | code | provenance | | +| CodeInjection.rb:5:5:5:8 | code | CodeInjection.rb:20:20:20:23 | code | provenance | | +| CodeInjection.rb:5:5:5:8 | code | CodeInjection.rb:23:21:23:24 | code | provenance | | +| CodeInjection.rb:5:5:5:8 | code | CodeInjection.rb:29:15:29:18 | code | provenance | | +| CodeInjection.rb:5:5:5:8 | code | CodeInjection.rb:32:19:32:22 | code | provenance | | +| CodeInjection.rb:5:5:5:8 | code | CodeInjection.rb:38:24:38:27 | code | provenance | | +| CodeInjection.rb:5:5:5:8 | code | CodeInjection.rb:41:40:41:43 | code | provenance | | +| CodeInjection.rb:5:12:5:17 | call to params | CodeInjection.rb:5:12:5:24 | ...[...] | provenance | | +| CodeInjection.rb:5:12:5:24 | ...[...] | CodeInjection.rb:5:5:5:8 | code | provenance | | +| CodeInjection.rb:38:24:38:27 | code | CodeInjection.rb:38:10:38:28 | call to escape | provenance | MaD:21 | +| CodeInjection.rb:78:5:78:8 | code | CodeInjection.rb:80:16:80:19 | code | provenance | | +| CodeInjection.rb:78:5:78:8 | code | CodeInjection.rb:86:10:86:37 | ... + ... | provenance | | +| CodeInjection.rb:78:5:78:8 | code | CodeInjection.rb:86:22:86:25 | code | provenance | | +| CodeInjection.rb:78:5:78:8 | code | CodeInjection.rb:88:10:88:32 | "prefix_#{...}_suffix" | provenance | AdditionalTaintStep | +| CodeInjection.rb:78:5:78:8 | code | CodeInjection.rb:90:10:90:13 | code | provenance | | +| CodeInjection.rb:78:12:78:17 | call to params | CodeInjection.rb:78:12:78:24 | ...[...] | provenance | | +| CodeInjection.rb:78:12:78:24 | ...[...] | CodeInjection.rb:78:5:78:8 | code | provenance | | +| CodeInjection.rb:86:10:86:25 | ... + ... [element] | CodeInjection.rb:86:10:86:37 | ... + ... | provenance | | +| CodeInjection.rb:86:22:86:25 | code | CodeInjection.rb:86:10:86:25 | ... + ... [element] | provenance | | +| CodeInjection.rb:101:3:102:5 | self in index [@foo] | CodeInjection.rb:111:3:113:5 | self in baz [@foo] | provenance | | +| CodeInjection.rb:105:5:105:8 | [post] self [@foo] | CodeInjection.rb:108:3:109:5 | self in bar [@foo] | provenance | | +| CodeInjection.rb:105:12:105:17 | call to params | CodeInjection.rb:105:12:105:23 | ...[...] | provenance | | +| CodeInjection.rb:105:12:105:23 | ...[...] | CodeInjection.rb:105:5:105:8 | [post] self [@foo] | provenance | | +| CodeInjection.rb:108:3:109:5 | self in bar [@foo] | CodeInjection.rb:101:3:102:5 | self in index [@foo] | provenance | | +| CodeInjection.rb:111:3:113:5 | self in baz [@foo] | CodeInjection.rb:112:10:112:13 | self [@foo] | provenance | | +| CodeInjection.rb:112:10:112:13 | self [@foo] | CodeInjection.rb:112:10:112:13 | @foo | provenance | | subpaths #select | CodeInjection.rb:8:10:8:13 | code | CodeInjection.rb:5:12:5:17 | call to params | CodeInjection.rb:8:10:8:13 | code | This code execution depends on a $@. | CodeInjection.rb:5:12:5:17 | call to params | user-provided value | diff --git a/shared/dataflow/change-notes/2023-10-02-deduplicate-path-graph.md b/shared/dataflow/change-notes/2023-10-02-deduplicate-path-graph.md new file mode 100644 index 000000000000..30e71ade6afd --- /dev/null +++ b/shared/dataflow/change-notes/2023-10-02-deduplicate-path-graph.md @@ -0,0 +1,4 @@ +--- +category: minorAnalysis +--- +* Added a module `DataFlow::DeduplicatePathGraph` that can be used to avoid generating duplicate path explanations in queries that use flow state. diff --git a/shared/dataflow/codeql/dataflow/DataFlow.qll b/shared/dataflow/codeql/dataflow/DataFlow.qll index 7a2f78089778..400ed5746ebd 100644 --- a/shared/dataflow/codeql/dataflow/DataFlow.qll +++ b/shared/dataflow/codeql/dataflow/DataFlow.qll @@ -851,4 +851,241 @@ module DataFlowMake Lang> { } } } + + /** + * Generates a `PathGraph` in which equivalent path nodes are merged, in order to avoid duplicate paths. + */ + module DeduplicatePathGraph Graph> { + // NOTE: there is a known limitation in that this module cannot see which nodes are sources or sinks. + // This only matters in the rare case where a sink PathNode has a non-empty set of succesors, and there is a + // non-sink PathNode with the same `(node, toString)` value and the same successors, but is transitively + // reachable from a different set of PathNodes. (And conversely for sources). + // + pragma[nomagic] + private InputPathNode getAPathNode(Node node, string toString) { + result.getNode() = node and + Graph::nodes(result, _, toString) + } + + private signature predicate collapseCandidateSig(Node node, string toString); + + private signature predicate stepSig( + InputPathNode node1, InputPathNode node2, string key, string val + ); + + private signature predicate subpathStepSig( + InputPathNode arg, InputPathNode param, InputPathNode ret, InputPathNode out + ); + + /** + * Performs a forward or backward pass computing which `(node, toString)` pairs can subsume their corresponding + * path nodes. + * + * This is similar to automaton minimization, but for an NFA. Since minimizing an NFA is NP-hard (and does not have + * a unique minimal NFA), we operate with the simpler model: for a given `(node, toString)` pair, either all + * corresponding path nodes are merged, or none are merged. + * + * Comments are written as if this checks for outgoing edges and propagates backward, though the module is also + * used to perform the opposite direction. + */ + private module MakeDiscriminatorPass< + collapseCandidateSig/2 collapseCandidate, stepSig/4 step, subpathStepSig/4 subpathStep> + { + /** + * Gets the number of `(key, val, node, toString)` tuples reachable in one step from `pathNode`. + * + * That is, two edges are counted as one if their target nodes are the same after projection, and the edges have the + * same `(key, val)`. + */ + private int getOutDegreeFromPathNode(InputPathNode pathNode) { + result = + count(Node node, string toString, string key, string val | + step(pathNode, getAPathNode(node, toString), key, val) + ) + } + + /** + * Gets the number of `(key, val, node2, toString2)` pairs reachable in one step from path nodes corresponding to `(node, toString)`. + */ + private int getOutDegreeFromNode(Node node, string toString) { + result = + strictcount(Node node2, string toString2, string key, string val | + step(getAPathNode(node, toString), getAPathNode(node2, toString2), key, val) + ) + } + + /** + * Like `getOutDegreeFromPathNode` except counts `subpath` tuples. + */ + private int getSubpathOutDegreeFromPathNode(InputPathNode pathNode) { + result = + count(Node n1, string s1, Node n2, string s2, Node n3, string s3 | + subpathStep(pathNode, getAPathNode(n1, s1), getAPathNode(n2, s2), getAPathNode(n3, s3)) + ) + } + + /** + * Like `getOutDegreeFromNode` except counts `subpath` tuples. + */ + private int getSubpathOutDegreeFromNode(Node node, string toString) { + result = + strictcount(Node n1, string s1, Node n2, string s2, Node n3, string s3 | + subpathStep(getAPathNode(node, toString), getAPathNode(n1, s1), getAPathNode(n2, s2), + getAPathNode(n3, s3)) + ) + } + + /** Gets a successor of `node`, including subpath flow-through, but not enter or exit subpath steps. */ + InputPathNode stepEx(InputPathNode node) { + step(node, result, _, _) and + not result = enterSubpathStep(node) and + not result = exitSubpathStep(node) + or + // Assuming the input is pruned properly, all subpaths have flow-through. + // This step should be in 'step' as well, but include it here for clarity as we rely on it. + subpathStep(node, _, _, result) + } + + InputPathNode enterSubpathStep(InputPathNode node) { subpathStep(node, result, _, _) } + + InputPathNode exitSubpathStep(InputPathNode node) { subpathStep(_, _, node, result) } + + /** Holds if `(node, toString)` cannot be collapsed (but was a candidate for being collapsed). */ + predicate discriminatedPair(Node node, string toString, boolean hasEnter) { + collapseCandidate(node, toString) and + hasEnter = false and + ( + // Check if all corresponding PathNodes have the same successor sets when projected to `(node, toString)`. + // To do this, we check that each successor set has the same size as the union of the succesor sets. + // - If the successor sets are equal, then they are also equal to their union, and so have the correct size. + // - Conversely, if two successor sets are not equal, one of them must be missing an element that is present + // in the union, but must still be a subset of the union, and thus be strictly smaller than the union. + getOutDegreeFromPathNode(getAPathNode(node, toString)) < + getOutDegreeFromNode(node, toString) + or + // Same as above but counting associated subpath triples instead + getSubpathOutDegreeFromPathNode(getAPathNode(node, toString)) < + getSubpathOutDegreeFromNode(node, toString) + ) + or + collapseCandidate(node, toString) and + ( + // Retain flow state if one of the successors requires it to be retained + discriminatedPathNode(stepEx(getAPathNode(node, toString)), hasEnter) + or + // Propagate backwards from parameter to argument + discriminatedPathNode(enterSubpathStep(getAPathNode(node, toString)), false) and + hasEnter = false + or + // Propagate backwards from out to return + discriminatedPathNode(exitSubpathStep(getAPathNode(node, toString)), _) and + hasEnter = true + ) + } + + /** Holds if `pathNode` cannot be collapsed. */ + private predicate discriminatedPathNode(InputPathNode pathNode, boolean hasEnter) { + exists(Node node, string toString | + discriminatedPair(node, toString, hasEnter) and + getAPathNode(node, toString) = pathNode + ) + } + + /** Holds if `(node, toString)` cannot be collapsed (but was a candidate for being collapsed). */ + predicate discriminatedPair(Node node, string toString) { + discriminatedPair(node, toString, _) + } + + /** Holds if `pathNode` cannot be collapsed. */ + predicate discriminatedPathNode(InputPathNode pathNode) { discriminatedPathNode(pathNode, _) } + } + + private InputPathNode getUniqPathNode(Node node, string toString) { + result = unique(InputPathNode pathNode | pathNode = getAPathNode(node, toString)) + } + + private predicate initialCandidate(Node node, string toString) { + exists(getAPathNode(node, toString)) and not exists(getUniqPathNode(node, toString)) + } + + private module Pass1 = + MakeDiscriminatorPass; + + private predicate edgesRev(InputPathNode node1, InputPathNode node2, string key, string val) { + Graph::edges(node2, node1, key, val) + } + + private predicate subpathsRev( + InputPathNode n1, InputPathNode n2, InputPathNode n3, InputPathNode n4 + ) { + Graph::subpaths(n4, n3, n2, n1) + } + + private module Pass2 = + MakeDiscriminatorPass; + + private newtype TPathNode = + TPreservedPathNode(InputPathNode node) { + Pass2::discriminatedPathNode(node) or node = getUniqPathNode(_, _) + } or + TCollapsedPathNode(Node node, string toString) { + initialCandidate(node, toString) and + not Pass2::discriminatedPair(node, toString) + } + + /** A node in the path graph after equivalent nodes have been collapsed. */ + class PathNode extends TPathNode { + private Node asCollapsedNode() { this = TCollapsedPathNode(result, _) } + + private InputPathNode asPreservedNode() { this = TPreservedPathNode(result) } + + /** Gets a correspondng node in the original graph. */ + InputPathNode getAnOriginalPathNode() { + exists(Node node, string toString | + this = TCollapsedPathNode(node, toString) and + result = getAPathNode(node, toString) + ) + or + result = this.asPreservedNode() + } + + /** Gets a string representation of this node. */ + string toString() { + result = this.asPreservedNode().toString() or this = TCollapsedPathNode(_, result) + } + + /** Gets the location of this node. */ + Location getLocation() { result = this.getAnOriginalPathNode().getLocation() } + + /** Gets the corresponding data-flow node. */ + Node getNode() { + result = this.asCollapsedNode() + or + result = this.asPreservedNode().getNode() + } + } + + /** + * Provides the query predicates needed to include a graph in a path-problem query. + */ + module PathGraph implements PathGraphSig { + query predicate nodes(PathNode node, string key, string val) { + Graph::nodes(node.getAnOriginalPathNode(), key, val) + } + + query predicate edges(PathNode node1, PathNode node2, string key, string val) { + Graph::edges(node1.getAnOriginalPathNode(), node2.getAnOriginalPathNode(), key, val) + } + + query predicate subpaths(PathNode arg, PathNode par, PathNode ret, PathNode out) { + // Note: this may look suspiciously simple, but it's not an oversight. Even if the caller needs to retain state, + // it is entirely possible to step through a subpath in which state has been projected away. + Graph::subpaths(arg.getAnOriginalPathNode(), par.getAnOriginalPathNode(), + ret.getAnOriginalPathNode(), out.getAnOriginalPathNode()) + } + } + + // Re-export the PathGraph so the user can import a single module and get both PathNode and the query predicates + import PathGraph + } }