Skip to content

Commit

Permalink
Merge pull request #14350 from asgerf/shared/deduplicate-path-graph
Browse files Browse the repository at this point in the history 
Shared: Add DataFlow::DeduplicatePathGraph
  • Loading branch information
@asgerf
asgerf authored Dec 18, 2024
2 parents 00688eb + e34fbc8 commit be939dc
Show file tree
Hide file tree
Showing 7 changed files with 469 additions and 80 deletions.
34 changes: 34 additions & 0 deletions java/ql/test/library-tests/dataflow/deduplicate-path-graph/A.java
View file
Original file line number Diff line number Diff line change
@@ -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<String, String> 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<String, String> 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);
}
}
79 changes: 79 additions & 0 deletions java/ql/test/library-tests/dataflow/deduplicate-path-graph/test.expected
View file
Original file line number Diff line number Diff line change
@@ -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 |
84 changes: 84 additions & 0 deletions java/ql/test/library-tests/dataflow/deduplicate-path-graph/test.ql
View file
Original file line number Diff line number Diff line change
@@ -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<TestConfig>;

module Graph = DataFlow::DeduplicatePathGraph<TestFlow::PathNode, TestFlow::PathGraph>;

/**
* 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"
19 changes: 4 additions & 15 deletions ruby/ql/src/queries/security/cwe-094/CodeInjection.ql
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,20 +16,9 @@

private import codeql.ruby.AST
private import codeql.ruby.security.CodeInjectionQuery
import CodeInjectionFlow::PathGraph
import DataFlow::DeduplicatePathGraph<CodeInjectionFlow::PathNode, CodeInjectionFlow::PathGraph>

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"
Loading

0 comments on commit be939dc

Please sign in to comment.