From cba7b98f7a399166c3562c283b5c58e94b61ad8a Mon Sep 17 00:00:00 2001 From: Asger F Date: Mon, 2 Oct 2023 10:50:43 +0200 Subject: [PATCH 01/14] Shared: Add DataFlow::DeduplicatePathGraph --- shared/dataflow/codeql/dataflow/DataFlow.qll | 172 +++++++++++++++++++ 1 file changed, 172 insertions(+) diff --git a/shared/dataflow/codeql/dataflow/DataFlow.qll b/shared/dataflow/codeql/dataflow/DataFlow.qll index 7a2f78089778..999aaa8dcf8d 100644 --- a/shared/dataflow/codeql/dataflow/DataFlow.qll +++ b/shared/dataflow/codeql/dataflow/DataFlow.qll @@ -851,4 +851,176 @@ 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). + // + /** + * Gets a successor of `node`, taking `subpaths` into account. + */ + private InputPathNode getASuccessorLike(InputPathNode node) { + Graph::edges(node, result) + or + Graph::subpaths(node, _, _, result) // arg -> out + // + // Note that there is no case for `arg -> param` or `ret -> out` for subpaths. + // It is OK to collapse nodes inside a subpath while calls to that subpaths aren't collapsed and vice versa. + } + + private InputPathNode getAPredecessorLike(InputPathNode node) { + node = getASuccessorLike(result) + } + + 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 InputPathNode stepSig(InputPathNode node); + + /** + * 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 { + /** + * Gets the number of `(node, toString)` pairs reachable in one step from `pathNode`. + */ + private int getOutDegreeFromPathNode(InputPathNode pathNode) { + result = count(Node node, string toString | step(pathNode) = getAPathNode(node, toString)) + } + + /** + * Gets the number of `(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 | + step(getAPathNode(node, toString)) = getAPathNode(node2, toString2) + ) + } + + /** Holds if `(node, toString)` cannot be collapsed (but was a candidate for being collapsed). */ + predicate discriminatedPair(Node node, string toString) { + collapseCandidate(node, toString) 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 + // Retain flow state if one of the successors requires it to be retained + discriminatedPathNode(step(getAPathNode(node, toString))) + ) + } + + /** Holds if `pathNode` cannot be collapsed. */ + predicate discriminatedPathNode(InputPathNode pathNode) { + exists(Node node, string toString | + discriminatedPair(node, toString) and + getAPathNode(node, toString) = pathNode + ) + } + } + + private predicate initialCandidate(Node node, string toString) { + exists(getAPathNode(node, toString)) + } + + private module Pass1 = MakeDiscriminatorPass; + + private module Pass2 = MakeDiscriminatorPass; + + private newtype TPathNode = + TPreservedPathNode(InputPathNode node) { Pass2::discriminatedPathNode(node) } 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) + } + + /** + * Holds if this element is at the specified location. + * The location spans column `startcolumn` of line `startline` to + * column `endcolumn` of line `endline` in file `filepath`. + * For more information, see + * [Locations](https://codeql.github.com/docs/writing-codeql-queries/providing-locations-in-codeql-queries/). + */ + predicate hasLocationInfo( + string filepath, int startline, int startcolumn, int endline, int endcolumn + ) { + this.getAnOriginalPathNode() + .hasLocationInfo(filepath, startline, startcolumn, endline, endcolumn) + } + + /** 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) { + Graph::edges(node1.getAnOriginalPathNode(), node2.getAnOriginalPathNode()) + } + + 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 + } } From 8efdc2df7be29efa5110e4b8f8d4fdbed61279ab Mon Sep 17 00:00:00 2001 From: Asger F Date: Mon, 2 Oct 2023 11:21:54 +0200 Subject: [PATCH 02/14] Shared: change note --- .../change-notes/2023-10-02-deduplicate-path-graph.md | 4 ++++ 1 file changed, 4 insertions(+) create mode 100644 shared/dataflow/change-notes/2023-10-02-deduplicate-path-graph.md 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. From 0eb543e0a9b252f00f56930dceb8c15c1ab1352d Mon Sep 17 00:00:00 2001 From: Asger F Date: Wed, 11 Oct 2023 14:06:32 +0200 Subject: [PATCH 03/14] Java: add test for spurious flow from path graph deduplication --- .../dataflow/deduplicate-path-graph/A.java | 34 ++++++++ .../deduplicate-path-graph/test.expected | 67 +++++++++++++++ .../dataflow/deduplicate-path-graph/test.ql | 83 +++++++++++++++++++ 3 files changed, 184 insertions(+) create mode 100644 java/ql/test/library-tests/dataflow/deduplicate-path-graph/A.java create mode 100644 java/ql/test/library-tests/dataflow/deduplicate-path-graph/test.expected create mode 100644 java/ql/test/library-tests/dataflow/deduplicate-path-graph/test.ql 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..6d4b923d2a6b --- /dev/null +++ b/java/ql/test/library-tests/dataflow/deduplicate-path-graph/test.expected @@ -0,0 +1,67 @@ +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: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:21:10:21:14 | step2 | semmle.label | step2 | +| 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: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: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: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 | +| A.java:14:29:14:29 | x : String | A.java:14:14:14:35 | propagateState(...) : String | +| A.java:15:9:15:9 | x : String | A.java:15:29:15:29 | x : String | +| A.java:15:29:15:29 | x : String | A.java:15:14:15:35 | propagateState(...) : String | +| A.java:17:20:17:27 | source(...) : String | A.java:18:33:18:37 | step0 : String | +| A.java:18:20:18:38 | apply(...) : String | A.java:19:33:19:37 | step1 : String | +| A.java:18:33:18:37 | step0 : String | A.java:14:9:14:9 | x : String | +| A.java:18:33:18:37 | step0 : String | A.java:15:9:15:9 | x : String | +| A.java:18:33:18:37 | step0 : String | A.java:18:20:18:38 | apply(...) : String | +| A.java:19:20:19:38 | apply(...) : String | A.java:21:10:21:14 | step2 | +| A.java:19:33:19:37 | step1 : String | A.java:14:9:14:9 | x : String | +| A.java:19:33:19:37 | step1 : String | A.java:15:9:15:9 | x : String | +| A.java:19:33:19:37 | step1 : String | A.java:19:20:19:38 | apply(...) : String | +| A.java:26:8:26:8 | x : String | A.java:26:50:26:50 | x : String | +| A.java:26:8:26:8 | x : String | A.java:26:75:26:75 | x : String | +| A.java:26:35:26:56 | propagateState(...) : String | A.java:26:13:26:81 | ...?...:... : String | +| A.java:26:50:26:50 | x : String | A.java:26:35:26:56 | propagateState(...) : String | +| A.java:26:60:26:81 | propagateState(...) : String | A.java:26:13:26:81 | ...?...:... : String | +| A.java:26:75:26:75 | x : String | A.java:26:60:26:81 | propagateState(...) : String | +| A.java:28:20:28:27 | source(...) : String | A.java:29:33:29:37 | step0 : String | +| A.java:29:20:29:38 | apply(...) : String | A.java:30:33:30:37 | step1 : String | +| A.java:29:33:29:37 | step0 : String | A.java:26:8:26:8 | x : String | +| A.java:29:33:29:37 | step0 : String | A.java:29:20:29:38 | apply(...) : String | +| A.java:30:20:30:38 | apply(...) : String | A.java:32:10:32:14 | step2 | +| A.java:30:33:30:37 | step1 : String | A.java:26:8:26:8 | x : String | +| A.java:30:33:30:37 | step1 : String | A.java:30:20:30:38 | apply(...) : String | +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: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 +| A.java:14:14:14:35 | propagateState(...) : String | B | A | +| A.java:15:14:15:35 | propagateState(...) : String | A | B | +| A.java:26:35:26:56 | propagateState(...) : String | B | A | +| A.java:26:60:26:81 | propagateState(...) : String | A | B | +#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..feda614c112a --- /dev/null +++ b/java/ql/test/library-tests/dataflow/deduplicate-path-graph/test.ql @@ -0,0 +1,83 @@ +/** + * @kind path-problem + */ + +import java +import semmle.code.java.dataflow.DataFlow +import DataFlow + +MethodAccess 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().(MethodAccess).getMethod().getName() = "source" and state = ["A", "B"] + } + + predicate isSink(Node n, FlowState state) { + n.asExpr() = any(MethodAccess acc | acc.getMethod().getName() = "sink").getAnArgument() and + state = ["A", "B"] + } + + predicate isAdditionalFlowStep(Node node1, FlowState state1, Node node2, FlowState state2) { + exists(MethodAccess 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) + or + Graph::subpaths(prev, _, _, node) // arg -> out + ) + 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" From 5aa1242117a80fe8eb9760e784cc9bd105f660e9 Mon Sep 17 00:00:00 2001 From: Asger F Date: Wed, 11 Oct 2023 15:47:39 +0200 Subject: [PATCH 04/14] Shared: use a call bit when tracking reachability to/from a discriminator --- .../deduplicate-path-graph/test.expected | 20 +++- shared/dataflow/codeql/dataflow/DataFlow.qll | 110 +++++++++++++----- 2 files changed, 100 insertions(+), 30 deletions(-) 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 index 6d4b923d2a6b..b36b6c70665b 100644 --- a/java/ql/test/library-tests/dataflow/deduplicate-path-graph/test.expected +++ b/java/ql/test/library-tests/dataflow/deduplicate-path-graph/test.expected @@ -7,11 +7,15 @@ nodes | 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 | @@ -19,9 +23,11 @@ nodes | 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 | @@ -30,13 +36,16 @@ edges | A.java:15:29:15:29 | x : String | A.java:15:14:15:35 | propagateState(...) : String | | A.java:17:20:17:27 | source(...) : String | A.java:18:33:18:37 | step0 : String | | A.java:18:20:18:38 | apply(...) : String | A.java:19:33:19:37 | step1 : String | +| A.java:18:20:18:38 | apply(...) : String | A.java:19:33:19:37 | step1 : String | | A.java:18:33:18:37 | step0 : String | A.java:14:9:14:9 | x : String | | A.java:18:33:18:37 | step0 : String | A.java:15:9:15:9 | x : String | | A.java:18:33:18:37 | step0 : String | A.java:18:20:18:38 | apply(...) : String | +| A.java:18:33:18:37 | step0 : String | A.java:18:20:18:38 | apply(...) : String | | A.java:19:20:19:38 | apply(...) : String | A.java:21:10:21:14 | step2 | | A.java:19:33:19:37 | step1 : String | A.java:14:9:14:9 | x : String | | A.java:19:33:19:37 | step1 : String | A.java:15:9:15:9 | x : String | | A.java:19:33:19:37 | step1 : String | A.java:19:20:19:38 | apply(...) : String | +| A.java:19:33:19:37 | step1 : String | A.java:19:20:19:38 | apply(...) : String | | A.java:26:8:26:8 | x : String | A.java:26:50:26:50 | x : String | | A.java:26:8:26:8 | x : String | A.java:26:75:26:75 | x : String | | A.java:26:35:26:56 | propagateState(...) : String | A.java:26:13:26:81 | ...?...:... : String | @@ -45,10 +54,15 @@ edges | A.java:26:75:26:75 | x : String | A.java:26:60:26:81 | propagateState(...) : String | | A.java:28:20:28:27 | source(...) : String | A.java:29:33:29:37 | step0 : String | | A.java:29:20:29:38 | apply(...) : String | A.java:30:33:30:37 | step1 : String | +| A.java:29:20:29:38 | apply(...) : String | A.java:30:33:30:37 | step1 : String | | A.java:29:33:29:37 | step0 : String | A.java:26:8:26:8 | x : String | +| A.java:29:33:29:37 | step0 : String | A.java:26:8:26:8 | x : String | +| A.java:29:33:29:37 | step0 : String | A.java:29:20:29:38 | apply(...) : String | | A.java:29:33:29:37 | step0 : String | A.java:29:20:29:38 | apply(...) : String | | A.java:30:20:30:38 | apply(...) : String | A.java:32:10:32:14 | step2 | | A.java:30:33:30:37 | step1 : String | A.java:26:8:26:8 | x : String | +| A.java:30:33:30:37 | step1 : String | A.java:26:8:26:8 | x : String | +| A.java:30:33:30:37 | step1 : String | A.java:30:20:30:38 | apply(...) : String | | A.java:30:33:30:37 | step1 : String | A.java:30:20:30:38 | apply(...) : String | 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 | @@ -56,12 +70,10 @@ subpaths | 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 -| A.java:14:14:14:35 | propagateState(...) : String | B | A | -| A.java:15:14:15:35 | propagateState(...) : String | A | B | -| A.java:26:35:26:56 | propagateState(...) : String | B | A | -| A.java:26:60:26:81 | propagateState(...) : String | A | B | #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/shared/dataflow/codeql/dataflow/DataFlow.qll b/shared/dataflow/codeql/dataflow/DataFlow.qll index 999aaa8dcf8d..883c67962cb5 100644 --- a/shared/dataflow/codeql/dataflow/DataFlow.qll +++ b/shared/dataflow/codeql/dataflow/DataFlow.qll @@ -861,22 +861,6 @@ module DataFlowMake Lang> { // 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). // - /** - * Gets a successor of `node`, taking `subpaths` into account. - */ - private InputPathNode getASuccessorLike(InputPathNode node) { - Graph::edges(node, result) - or - Graph::subpaths(node, _, _, result) // arg -> out - // - // Note that there is no case for `arg -> param` or `ret -> out` for subpaths. - // It is OK to collapse nodes inside a subpath while calls to that subpaths aren't collapsed and vice versa. - } - - private InputPathNode getAPredecessorLike(InputPathNode node) { - node = getASuccessorLike(result) - } - pragma[nomagic] private InputPathNode getAPathNode(Node node, string toString) { result.getNode() = node and @@ -885,7 +869,11 @@ module DataFlowMake Lang> { private signature predicate collapseCandidateSig(Node node, string toString); - private signature InputPathNode stepSig(InputPathNode node); + private signature predicate stepSig(InputPathNode node1, InputPathNode node2); + + 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 @@ -898,12 +886,14 @@ module DataFlowMake Lang> { * 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 { + private module MakeDiscriminatorPass< + collapseCandidateSig/2 collapseCandidate, stepSig/2 step, subpathStepSig/4 subpathStep> + { /** * Gets the number of `(node, toString)` pairs reachable in one step from `pathNode`. */ private int getOutDegreeFromPathNode(InputPathNode pathNode) { - result = count(Node node, string toString | step(pathNode) = getAPathNode(node, toString)) + result = count(Node node, string toString | step(pathNode, getAPathNode(node, toString))) } /** @@ -912,13 +902,46 @@ module DataFlowMake Lang> { private int getOutDegreeFromNode(Node node, string toString) { result = strictcount(Node node2, string toString2 | - step(getAPathNode(node, toString)) = getAPathNode(node2, toString2) + step(getAPathNode(node, toString), getAPathNode(node2, toString2)) ) } + /** + * 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. */ + InputPathNode stepEx(InputPathNode node) { + step(node, result) + or + subpathStep(node, _, _, result) // assuming the input is pruned properly, all subpaths have flow-through + } + + 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) { + 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. @@ -928,27 +951,62 @@ module DataFlowMake Lang> { 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(step(getAPathNode(node, toString))) + discriminatedPathNode(stepEx(getAPathNode(node, toString)), hasEnter) + or + // Enter a subpath + discriminatedPathNode(enterSubpathStep(getAPathNode(node, toString)), _) and + hasEnter = true + or + // Exit a subpath + discriminatedPathNode(exitSubpathStep(getAPathNode(node, toString)), false) and + hasEnter = false ) } /** Holds if `pathNode` cannot be collapsed. */ - predicate discriminatedPathNode(InputPathNode pathNode) { + private predicate discriminatedPathNode(InputPathNode pathNode, boolean hasEnter) { exists(Node node, string toString | - discriminatedPair(node, toString) and + 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 predicate initialCandidate(Node node, string toString) { exists(getAPathNode(node, toString)) } - private module Pass1 = MakeDiscriminatorPass; + private module Pass1 = + MakeDiscriminatorPass; + + private predicate edgesRev(InputPathNode node1, InputPathNode node2) { + Graph::edges(node2, node1) + } + + private predicate subpathsRev( + InputPathNode n1, InputPathNode n2, InputPathNode n3, InputPathNode n4 + ) { + Graph::subpaths(n4, n3, n2, n1) + } - private module Pass2 = MakeDiscriminatorPass; + private module Pass2 = + MakeDiscriminatorPass; private newtype TPathNode = TPreservedPathNode(InputPathNode node) { Pass2::discriminatedPathNode(node) } or From 815581dc11f78f4afb91d4180457ee5f6c15f7e8 Mon Sep 17 00:00:00 2001 From: Asger F Date: Wed, 11 Dec 2024 11:47:48 +0100 Subject: [PATCH 05/14] JS: Update to account for key,val pairs on edges --- shared/dataflow/codeql/dataflow/DataFlow.qll | 51 +++++++++----------- 1 file changed, 24 insertions(+), 27 deletions(-) diff --git a/shared/dataflow/codeql/dataflow/DataFlow.qll b/shared/dataflow/codeql/dataflow/DataFlow.qll index 883c67962cb5..65fa3df0fff7 100644 --- a/shared/dataflow/codeql/dataflow/DataFlow.qll +++ b/shared/dataflow/codeql/dataflow/DataFlow.qll @@ -869,7 +869,9 @@ module DataFlowMake Lang> { private signature predicate collapseCandidateSig(Node node, string toString); - private signature predicate stepSig(InputPathNode node1, InputPathNode node2); + private signature predicate stepSig( + InputPathNode node1, InputPathNode node2, string key, string val + ); private signature predicate subpathStepSig( InputPathNode arg, InputPathNode param, InputPathNode ret, InputPathNode out @@ -887,22 +889,28 @@ module DataFlowMake Lang> { * used to perform the opposite direction. */ private module MakeDiscriminatorPass< - collapseCandidateSig/2 collapseCandidate, stepSig/2 step, subpathStepSig/4 subpathStep> + collapseCandidateSig/2 collapseCandidate, stepSig/4 step, subpathStepSig/4 subpathStep> { /** - * Gets the number of `(node, toString)` pairs reachable in one step from `pathNode`. + * 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 | step(pathNode, getAPathNode(node, toString))) + result = + count(Node node, string toString, string key, string val | + step(pathNode, getAPathNode(node, toString), key, val) + ) } /** - * Gets the number of `(node2, toString2)` pairs reachable in one step from path nodes corresponding to `(node, toString)`. + * 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 | - step(getAPathNode(node, toString), getAPathNode(node2, toString2)) + strictcount(Node node2, string toString2, string key, string val | + step(getAPathNode(node, toString), getAPathNode(node2, toString2), key, val) ) } @@ -929,7 +937,7 @@ module DataFlowMake Lang> { /** Gets a successor of `node` including subpath flow-through. */ InputPathNode stepEx(InputPathNode node) { - step(node, result) + step(node, result, _, _) or subpathStep(node, _, _, result) // assuming the input is pruned properly, all subpaths have flow-through } @@ -993,10 +1001,10 @@ module DataFlowMake Lang> { } private module Pass1 = - MakeDiscriminatorPass; + MakeDiscriminatorPass; - private predicate edgesRev(InputPathNode node1, InputPathNode node2) { - Graph::edges(node2, node1) + private predicate edgesRev(InputPathNode node1, InputPathNode node2, string key, string val) { + Graph::edges(node2, node1, key, val) } private predicate subpathsRev( @@ -1006,7 +1014,7 @@ module DataFlowMake Lang> { } private module Pass2 = - MakeDiscriminatorPass; + MakeDiscriminatorPass; private newtype TPathNode = TPreservedPathNode(InputPathNode node) { Pass2::discriminatedPathNode(node) } or @@ -1036,19 +1044,8 @@ module DataFlowMake Lang> { result = this.asPreservedNode().toString() or this = TCollapsedPathNode(_, result) } - /** - * Holds if this element is at the specified location. - * The location spans column `startcolumn` of line `startline` to - * column `endcolumn` of line `endline` in file `filepath`. - * For more information, see - * [Locations](https://codeql.github.com/docs/writing-codeql-queries/providing-locations-in-codeql-queries/). - */ - predicate hasLocationInfo( - string filepath, int startline, int startcolumn, int endline, int endcolumn - ) { - this.getAnOriginalPathNode() - .hasLocationInfo(filepath, startline, startcolumn, endline, endcolumn) - } + /** Gets the location of this node. */ + Location getLocation() { result = this.getAnOriginalPathNode().getLocation() } /** Gets the corresponding data-flow node. */ Node getNode() { @@ -1066,8 +1063,8 @@ module DataFlowMake Lang> { Graph::nodes(node.getAnOriginalPathNode(), key, val) } - query predicate edges(PathNode node1, PathNode node2) { - Graph::edges(node1.getAnOriginalPathNode(), node2.getAnOriginalPathNode()) + 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) { From f9c0ba38267462cfbdaf341b82822d94eca32f18 Mon Sep 17 00:00:00 2001 From: Asger F Date: Mon, 2 Oct 2023 11:14:28 +0200 Subject: [PATCH 06/14] Ruby: use DeduplicatePathGraph in CodeInjection query --- .../queries/security/cwe-094/CodeInjection.ql | 19 +--- .../CodeInjection/CodeInjection.expected | 92 ++++++------------- 2 files changed, 31 insertions(+), 80 deletions(-) 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 | From 736388809d0c1cc3e17d344f80675e8344ef751a Mon Sep 17 00:00:00 2001 From: Asger F Date: Wed, 11 Dec 2024 13:19:25 +0100 Subject: [PATCH 07/14] Java: MethodAccess -> MethodCall --- .../library-tests/dataflow/deduplicate-path-graph/test.ql | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) 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 index feda614c112a..0c0c4af3db73 100644 --- a/java/ql/test/library-tests/dataflow/deduplicate-path-graph/test.ql +++ b/java/ql/test/library-tests/dataflow/deduplicate-path-graph/test.ql @@ -6,7 +6,7 @@ import java import semmle.code.java.dataflow.DataFlow import DataFlow -MethodAccess propagateCall(string state) { +MethodCall propagateCall(string state) { result.getMethod().getName() = "propagateState" and state = result.getArgument(1).(StringLiteral).getValue() } @@ -15,16 +15,16 @@ module TestConfig implements StateConfigSig { class FlowState = string; predicate isSource(Node n, FlowState state) { - n.asExpr().(MethodAccess).getMethod().getName() = "source" and state = ["A", "B"] + n.asExpr().(MethodCall).getMethod().getName() = "source" and state = ["A", "B"] } predicate isSink(Node n, FlowState state) { - n.asExpr() = any(MethodAccess acc | acc.getMethod().getName() = "sink").getAnArgument() and + 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(MethodAccess call | + exists(MethodCall call | call = propagateCall(state1) and state2 = state1 and node1.asExpr() = call.getArgument(0) and From afdbf2c3c6a2fac01630fe18fdf7fce3b10c9cc3 Mon Sep 17 00:00:00 2001 From: Asger F Date: Wed, 11 Dec 2024 13:19:36 +0100 Subject: [PATCH 08/14] Java: update test to account for key,val --- .../test/library-tests/dataflow/deduplicate-path-graph/test.ql | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 index 0c0c4af3db73..efe8dabc0246 100644 --- a/java/ql/test/library-tests/dataflow/deduplicate-path-graph/test.ql +++ b/java/ql/test/library-tests/dataflow/deduplicate-path-graph/test.ql @@ -47,7 +47,7 @@ predicate reachableFromPropagate(Graph::PathNode node, string state, boolean cal node.getNode().asExpr() = propagateCall(state) and call = false or exists(Graph::PathNode prev | reachableFromPropagate(prev, state, call) | - Graph::edges(prev, node) + Graph::edges(prev, node, _, _) or Graph::subpaths(prev, _, _, node) // arg -> out ) From 889100a2433f5c8243efa2b574c4f26a0fb8cb7e Mon Sep 17 00:00:00 2001 From: Asger F Date: Wed, 11 Dec 2024 13:19:47 +0100 Subject: [PATCH 09/14] Java: update test output with provenance --- .../deduplicate-path-graph/test.expected | 68 +++++++++---------- 1 file changed, 34 insertions(+), 34 deletions(-) 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 index b36b6c70665b..f41b92b2a3da 100644 --- a/java/ql/test/library-tests/dataflow/deduplicate-path-graph/test.expected +++ b/java/ql/test/library-tests/dataflow/deduplicate-path-graph/test.expected @@ -30,40 +30,40 @@ nodes | 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 | -| A.java:14:29:14:29 | x : String | A.java:14:14:14:35 | propagateState(...) : String | -| A.java:15:9:15:9 | x : String | A.java:15:29:15:29 | x : String | -| A.java:15:29:15:29 | x : String | A.java:15:14:15:35 | propagateState(...) : String | -| A.java:17:20:17:27 | source(...) : String | A.java:18:33:18:37 | step0 : String | -| A.java:18:20:18:38 | apply(...) : String | A.java:19:33:19:37 | step1 : String | -| A.java:18:20:18:38 | apply(...) : String | A.java:19:33:19:37 | step1 : String | -| A.java:18:33:18:37 | step0 : String | A.java:14:9:14:9 | x : String | -| A.java:18:33:18:37 | step0 : String | A.java:15:9:15:9 | x : String | -| A.java:18:33:18:37 | step0 : String | A.java:18:20:18:38 | apply(...) : String | -| A.java:18:33:18:37 | step0 : String | A.java:18:20:18:38 | apply(...) : String | -| A.java:19:20:19:38 | apply(...) : String | A.java:21:10:21:14 | step2 | -| A.java:19:33:19:37 | step1 : String | A.java:14:9:14:9 | x : String | -| A.java:19:33:19:37 | step1 : String | A.java:15:9:15:9 | x : String | -| A.java:19:33:19:37 | step1 : String | A.java:19:20:19:38 | apply(...) : String | -| A.java:19:33:19:37 | step1 : String | A.java:19:20:19:38 | apply(...) : String | -| A.java:26:8:26:8 | x : String | A.java:26:50:26:50 | x : String | -| A.java:26:8:26:8 | x : String | A.java:26:75:26:75 | x : String | -| A.java:26:35:26:56 | propagateState(...) : String | A.java:26:13:26:81 | ...?...:... : String | -| A.java:26:50:26:50 | x : String | A.java:26:35:26:56 | propagateState(...) : String | -| A.java:26:60:26:81 | propagateState(...) : String | A.java:26:13:26:81 | ...?...:... : String | -| A.java:26:75:26:75 | x : String | A.java:26:60:26:81 | propagateState(...) : String | -| A.java:28:20:28:27 | source(...) : String | A.java:29:33:29:37 | step0 : String | -| A.java:29:20:29:38 | apply(...) : String | A.java:30:33:30:37 | step1 : String | -| A.java:29:20:29:38 | apply(...) : String | A.java:30:33:30:37 | step1 : String | -| A.java:29:33:29:37 | step0 : String | A.java:26:8:26:8 | x : String | -| A.java:29:33:29:37 | step0 : String | A.java:26:8:26:8 | x : String | -| A.java:29:33:29:37 | step0 : String | A.java:29:20:29:38 | apply(...) : String | -| A.java:29:33:29:37 | step0 : String | A.java:29:20:29:38 | apply(...) : String | -| A.java:30:20:30:38 | apply(...) : String | A.java:32:10:32:14 | step2 | -| A.java:30:33:30:37 | step1 : String | A.java:26:8:26:8 | x : String | -| A.java:30:33:30:37 | step1 : String | A.java:26:8:26:8 | x : String | -| A.java:30:33:30:37 | step1 : String | A.java:30:20:30:38 | apply(...) : String | -| A.java:30:33:30:37 | step1 : String | A.java:30:20:30:38 | apply(...) : String | +| 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 | From 0edb30638adcf4227ae537b529cfdb8ef8374a83 Mon Sep 17 00:00:00 2001 From: Asger F Date: Mon, 16 Dec 2024 13:14:27 +0100 Subject: [PATCH 10/14] Apply suggestions from code review Co-authored-by: Anders Schack-Mulligen --- shared/dataflow/codeql/dataflow/DataFlow.qll | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/shared/dataflow/codeql/dataflow/DataFlow.qll b/shared/dataflow/codeql/dataflow/DataFlow.qll index 65fa3df0fff7..0c39f8873c1d 100644 --- a/shared/dataflow/codeql/dataflow/DataFlow.qll +++ b/shared/dataflow/codeql/dataflow/DataFlow.qll @@ -996,8 +996,12 @@ module DataFlowMake Lang> { 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)) + exists(getAPathNode(node, toString)) and not exists(getUniqPathNode(node, toString)) } private module Pass1 = @@ -1017,7 +1021,7 @@ module DataFlowMake Lang> { MakeDiscriminatorPass; private newtype TPathNode = - TPreservedPathNode(InputPathNode node) { Pass2::discriminatedPathNode(node) } or + TPreservedPathNode(InputPathNode node) { Pass2::discriminatedPathNode(node) or node = getUniqPathNode(_, _) } or TCollapsedPathNode(Node node, string toString) { initialCandidate(node, toString) and not Pass2::discriminatedPair(node, toString) From f2968f4e1401d677957cce380ab7ce6dc554d550 Mon Sep 17 00:00:00 2001 From: Asger F Date: Mon, 16 Dec 2024 13:21:43 +0100 Subject: [PATCH 11/14] Shared: Ensure subpath-induced edges are handled properly Argument-passing and flow-through edges are present in 'edges' in addition to 'subpaths', but the implementation didn't take this into account. --- .../dataflow/deduplicate-path-graph/test.ql | 5 +++-- shared/dataflow/codeql/dataflow/DataFlow.qll | 10 +++++++--- 2 files changed, 10 insertions(+), 5 deletions(-) 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 index efe8dabc0246..6d7e7c7b95d4 100644 --- a/java/ql/test/library-tests/dataflow/deduplicate-path-graph/test.ql +++ b/java/ql/test/library-tests/dataflow/deduplicate-path-graph/test.ql @@ -47,9 +47,10 @@ predicate reachableFromPropagate(Graph::PathNode node, string state, boolean cal node.getNode().asExpr() = propagateCall(state) and call = false or exists(Graph::PathNode prev | reachableFromPropagate(prev, state, call) | - Graph::edges(prev, node, _, _) + Graph::edges(prev, node, _, _) and + not Graph::subpaths(prev, node, _, _) // argument-passing edges are handled separately or - Graph::subpaths(prev, _, _, node) // arg -> out + Graph::subpaths(prev, _, _, node) // arg -> out (should be included in 'edges' but handle the case here for clarity) ) or exists(Graph::PathNode prev | diff --git a/shared/dataflow/codeql/dataflow/DataFlow.qll b/shared/dataflow/codeql/dataflow/DataFlow.qll index 0c39f8873c1d..701b50a4ec13 100644 --- a/shared/dataflow/codeql/dataflow/DataFlow.qll +++ b/shared/dataflow/codeql/dataflow/DataFlow.qll @@ -935,11 +935,15 @@ module DataFlowMake Lang> { ) } - /** Gets a successor of `node` including subpath flow-through. */ + /** Gets a successor of `node`, including subpath flow-through, but not enter or exit subpath steps. */ InputPathNode stepEx(InputPathNode node) { - step(node, result, _, _) + step(node, result, _, _) and + not result = enterSubpathStep(node) and + not result = exitSubpathStep(node) or - subpathStep(node, _, _, result) // assuming the input is pruned properly, all subpaths have flow-through + // 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, _, _) } From 950ae44d03a2752ecb267fc8802fe9a2b54242f5 Mon Sep 17 00:00:00 2001 From: Asger F Date: Tue, 17 Dec 2024 10:30:07 +0100 Subject: [PATCH 12/14] Shared: Show test failures --- .../dataflow/deduplicate-path-graph/test.expected | 13 ++----------- 1 file changed, 2 insertions(+), 11 deletions(-) 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 index f41b92b2a3da..8e08ce4f4f2c 100644 --- a/java/ql/test/library-tests/dataflow/deduplicate-path-graph/test.expected +++ b/java/ql/test/library-tests/dataflow/deduplicate-path-graph/test.expected @@ -14,8 +14,6 @@ nodes | 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 | @@ -23,11 +21,9 @@ nodes | 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 | | @@ -54,15 +50,10 @@ edges | 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 | @@ -70,10 +61,10 @@ subpaths | 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 +| A.java:26:35:26:56 | propagateState(...) : String | B | A | +| A.java:26:60:26:81 | propagateState(...) : String | A | B | #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 | From 8340841d545dc8602db586db0f5101de9280e3f9 Mon Sep 17 00:00:00 2001 From: Asger F Date: Tue, 17 Dec 2024 11:14:00 +0100 Subject: [PATCH 13/14] Shared: Fix propagation of call bit --- .../dataflow/deduplicate-path-graph/test.expected | 13 +++++++++++-- shared/dataflow/codeql/dataflow/DataFlow.qll | 12 ++++++------ 2 files changed, 17 insertions(+), 8 deletions(-) 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 index 8e08ce4f4f2c..f41b92b2a3da 100644 --- a/java/ql/test/library-tests/dataflow/deduplicate-path-graph/test.expected +++ b/java/ql/test/library-tests/dataflow/deduplicate-path-graph/test.expected @@ -14,6 +14,8 @@ nodes | 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 | @@ -21,9 +23,11 @@ nodes | 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 | | @@ -50,10 +54,15 @@ edges | 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 | @@ -61,10 +70,10 @@ subpaths | 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 -| A.java:26:35:26:56 | propagateState(...) : String | B | A | -| A.java:26:60:26:81 | propagateState(...) : String | A | B | #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/shared/dataflow/codeql/dataflow/DataFlow.qll b/shared/dataflow/codeql/dataflow/DataFlow.qll index 701b50a4ec13..01ccfaebf4a7 100644 --- a/shared/dataflow/codeql/dataflow/DataFlow.qll +++ b/shared/dataflow/codeql/dataflow/DataFlow.qll @@ -973,13 +973,13 @@ module DataFlowMake Lang> { // Retain flow state if one of the successors requires it to be retained discriminatedPathNode(stepEx(getAPathNode(node, toString)), hasEnter) or - // Enter a subpath - discriminatedPathNode(enterSubpathStep(getAPathNode(node, toString)), _) and - hasEnter = true - or - // Exit a subpath - discriminatedPathNode(exitSubpathStep(getAPathNode(node, toString)), false) and + // 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 ) } From e34fbc8bd1f858870bd5d79ac0268111f3ec09d1 Mon Sep 17 00:00:00 2001 From: Asger F Date: Tue, 17 Dec 2024 11:26:56 +0100 Subject: [PATCH 14/14] Shared: autoformat --- shared/dataflow/codeql/dataflow/DataFlow.qll | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/shared/dataflow/codeql/dataflow/DataFlow.qll b/shared/dataflow/codeql/dataflow/DataFlow.qll index 01ccfaebf4a7..400ed5746ebd 100644 --- a/shared/dataflow/codeql/dataflow/DataFlow.qll +++ b/shared/dataflow/codeql/dataflow/DataFlow.qll @@ -1025,7 +1025,9 @@ module DataFlowMake Lang> { MakeDiscriminatorPass; private newtype TPathNode = - TPreservedPathNode(InputPathNode node) { Pass2::discriminatedPathNode(node) or node = getUniqPathNode(_, _) } or + TPreservedPathNode(InputPathNode node) { + Pass2::discriminatedPathNode(node) or node = getUniqPathNode(_, _) + } or TCollapsedPathNode(Node node, string toString) { initialCandidate(node, toString) and not Pass2::discriminatedPair(node, toString)