diff --git a/java/ql/test/experimental/query-tests/security/CWE-078/CommandInjectionRuntimeExecLocal.expected b/java/ql/test/experimental/query-tests/security/CWE-078/CommandInjectionRuntimeExecLocal.expected index 010ce03e9fc3f..34655e6c0216e 100644 --- a/java/ql/test/experimental/query-tests/security/CWE-078/CommandInjectionRuntimeExecLocal.expected +++ b/java/ql/test/experimental/query-tests/security/CWE-078/CommandInjectionRuntimeExecLocal.expected @@ -8,11 +8,11 @@ edges | RuntimeExecTest.java:17:25:17:51 | getenv(...) : String | RuntimeExecTest.java:25:66:25:71 | script : String | provenance | Src:MaD:2 | | RuntimeExecTest.java:17:25:17:51 | getenv(...) : String | RuntimeExecTest.java:31:36:31:41 | script : String | provenance | Src:MaD:2 | | RuntimeExecTest.java:17:25:17:51 | getenv(...) : String | RuntimeExecTest.java:38:52:38:57 | script : String | provenance | Src:MaD:2 | -| RuntimeExecTest.java:22:43:22:73 | {...} : String[] [[]] : String | RuntimeExecTest.java:22:43:22:73 | new String[] | provenance | Sink:MaD:1 | +| RuntimeExecTest.java:22:43:22:73 | {...} : String[] [[]] : String | RuntimeExecTest.java:22:43:22:73 | new String[] | provenance | Sink: MaD:1 | | RuntimeExecTest.java:22:67:22:72 | script : String | RuntimeExecTest.java:22:43:22:73 | {...} : String[] [[]] : String | provenance | | -| RuntimeExecTest.java:25:42:25:72 | {...} : String[] [[]] : String | RuntimeExecTest.java:26:43:26:55 | commandArray1 | provenance | Sink:MaD:1 | +| RuntimeExecTest.java:25:42:25:72 | {...} : String[] [[]] : String | RuntimeExecTest.java:26:43:26:55 | commandArray1 | provenance | Sink: MaD:1 | | RuntimeExecTest.java:25:66:25:71 | script : String | RuntimeExecTest.java:25:42:25:72 | {...} : String[] [[]] : String | provenance | | -| RuntimeExecTest.java:31:17:31:29 | commandArray2 [post update] : String[] [[]] : String | RuntimeExecTest.java:32:43:32:55 | commandArray2 | provenance | Sink:MaD:1 | +| RuntimeExecTest.java:31:17:31:29 | commandArray2 [post update] : String[] [[]] : String | RuntimeExecTest.java:32:43:32:55 | commandArray2 | provenance | Sink: MaD:1 | | RuntimeExecTest.java:31:36:31:41 | script : String | RuntimeExecTest.java:31:17:31:29 | commandArray2 [post update] : String[] [[]] : String | provenance | | | RuntimeExecTest.java:36:21:39:21 | concat(...) : Stream [] : String | RuntimeExecTest.java:36:21:39:44 | toArray(...) | provenance | MaD:5 | | RuntimeExecTest.java:38:25:38:59 | stream(...) : Stream [] : String | RuntimeExecTest.java:36:21:39:21 | concat(...) : Stream [] : String | provenance | MaD:4 | diff --git a/shared/dataflow/codeql/dataflow/internal/DataFlowImpl.qll b/shared/dataflow/codeql/dataflow/internal/DataFlowImpl.qll index 23576fdc5cb54..c0fe3f4550c71 100644 --- a/shared/dataflow/codeql/dataflow/internal/DataFlowImpl.qll +++ b/shared/dataflow/codeql/dataflow/internal/DataFlowImpl.qll @@ -153,20 +153,6 @@ module MakeImpl Lang> { module Impl { private class FlowState = Config::FlowState; - private class NodeEx extends NodeExImpl { - NodeEx() { - Config::allowImplicitRead(any(Node n | this.isImplicitReadNode(n)), _) - or - not this.isImplicitReadNode(_) - } - } - - private class ArgNodeEx extends NodeEx, ArgNodeExImpl { } - - private class ParamNodeEx extends NodeEx, ParamNodeExImpl { } - - private class RetNodeEx extends NodeEx, RetNodeExImpl { } - private predicate inBarrier(NodeEx node) { exists(Node n | node.asNode() = n and @@ -968,10 +954,15 @@ module MakeImpl Lang> { bindingset[label1, label2] pragma[inline_late] private string mergeLabels(string label1, string label2) { - // Big-step, hidden nodes, and summaries all may need to merge labels. - // These cases are expected to involve at most one non-empty label, so - // we'll just discard the 2nd+ label for now. - if label1 = "" then result = label2 else result = label1 + if label2.matches("Sink:") + then if label1 = "" then result = label2 else result = label1 + " " + label2 + else + // Big-step, hidden nodes, and summaries all may need to merge labels. + // These cases are expected to involve at most one non-empty label, so + // we'll just discard the 2nd+ label for now. + if label1 = "" + then result = label2 + else result = label1 } pragma[nomagic] @@ -2742,18 +2733,17 @@ module MakeImpl Lang> { abstract PathNodeImpl getASuccessorImpl(string label); pragma[nomagic] - PathNodeImpl getAnImplicitReadSuccessorAtSink(string label) { + PathNodeSink getAnImplicitReadSink(string label) { exists(PathNodeMid readTarget | - result = this.getASuccessorImpl(label) and + result = this.getASuccessorImpl(_) and localStep(this, readTarget, _) and readTarget.getNodeEx().isImplicitReadNode(_) | // last implicit read, leaving the access path empty - result = readTarget.projectToSink(_) + result = readTarget.projectToSink(label) or // implicit read, leaving the access path non-empty - exists(result.getAnImplicitReadSuccessorAtSink(_)) and - result = readTarget + result = readTarget.getAnImplicitReadSink(label) ) } @@ -2761,7 +2751,7 @@ module MakeImpl Lang> { this.isHidden() and result = this.getASuccessorImpl(label) or - result = this.getAnImplicitReadSuccessorAtSink(label) + result = this.getAnImplicitReadSink(label) } private PathNodeImpl getASuccessorFromNonHidden(string label) { @@ -2784,7 +2774,7 @@ module MakeImpl Lang> { // `x [Field]` -> `x` // // which the restriction below ensures. - not result = this.getAnImplicitReadSuccessorAtSink(label) + not result = this.getAnImplicitReadSink(_) or exists(string l1, string l2 | result = this.getASuccessorFromNonHidden(l1).getASuccessorIfHidden(l2) and @@ -2905,17 +2895,15 @@ module MakeImpl Lang> { ) or // a final step to a sink - exists(string l2, string sinkmodel | - result = this.getSuccMid(l2).projectToSink(sinkmodel) + exists(string l2, string sinkLabel | + result = this.getSuccMid(l2).projectToSink(sinkLabel) | not this.isSourceWithLabel(_) and - if sinkmodel != "" then label = l2 + " Sink:" + sinkmodel else label = l2 + label = mergeLabels(l2, sinkLabel) or exists(string l1 | this.isSourceWithLabel(l1) and - if sinkmodel != "" - then label = l1 + l2 + " Sink:" + sinkmodel - else label = l1 + l2 + label = l1 + mergeLabels(l2, sinkLabel) ) ) } @@ -2981,11 +2969,14 @@ module MakeImpl Lang> { else any() } - PathNodeSink projectToSink(string model) { - this.isAtSink() and - sinkModel(node, model) and - result.getNodeEx() = this.toNormalSinkNodeEx() and - result.getState() = state + PathNodeSink projectToSink(string label) { + exists(string model | + this.isAtSink() and + sinkModel(node, model) and + result.getNodeEx() = this.toNormalSinkNodeEx() and + result.getState() = state and + if model != "" then label = "Sink: " + model else label = "" + ) } } @@ -3258,7 +3249,7 @@ module MakeImpl Lang> { n2 = localStep(n1) and n1.isHidden() or - n2 = n1.getAnImplicitReadSuccessorAtSink(_) + n2 = n1.getAnImplicitReadSink(_) } bindingset[par, ret] diff --git a/shared/dataflow/codeql/dataflow/internal/DataFlowImplCommon.qll b/shared/dataflow/codeql/dataflow/internal/DataFlowImplCommon.qll index fffe28d2994b9..81f9946126db0 100644 --- a/shared/dataflow/codeql/dataflow/internal/DataFlowImplCommon.qll +++ b/shared/dataflow/codeql/dataflow/internal/DataFlowImplCommon.qll @@ -850,7 +850,7 @@ module MakeImplCommon Lang> { class SndLevelScopeOption = SndLevelScopeOption::Option; - final class NodeExImpl extends TNodeEx { + final class NodeEx extends TNodeEx { string toString() { result = this.asNode().toString() or @@ -899,14 +899,14 @@ module MakeImplCommon Lang> { Location getLocation() { result = this.projectToNode().getLocation() } } - final class ArgNodeExImpl extends NodeExImpl { - ArgNodeExImpl() { this.asNode() instanceof ArgNode } + final class ArgNodeEx extends NodeEx { + ArgNodeEx() { this.asNode() instanceof ArgNode } DataFlowCall getCall() { this.asNode().(ArgNode).argumentOf(result, _) } } - final class ParamNodeExImpl extends NodeExImpl { - ParamNodeExImpl() { this.asNode() instanceof ParamNode } + final class ParamNodeEx extends NodeEx { + ParamNodeEx() { this.asNode() instanceof ParamNode } predicate isParameterOf(DataFlowCallable c, ParameterPosition pos) { this.asNode().(ParamNode).isParameterOf(c, pos) @@ -919,10 +919,10 @@ module MakeImplCommon Lang> { * A node from which flow can return to the caller. This is either a regular * `ReturnNode` or a synthesized node for flow out via a parameter. */ - final class RetNodeExImpl extends NodeExImpl { + final class RetNodeEx extends NodeEx { private ReturnPosition pos; - RetNodeExImpl() { pos = getReturnPositionEx(this) } + RetNodeEx() { pos = getReturnPositionEx(this) } ReturnPosition getReturnPosition() { result = pos } @@ -1713,7 +1713,7 @@ module MakeImplCommon Lang> { * Holds if data can flow in one local step from `node1` to `node2`. */ cached - predicate localFlowStepExImpl(NodeExImpl node1, NodeExImpl node2, string model) { + predicate localFlowStepExImpl(NodeEx node1, NodeEx node2, string model) { exists(Node n1, Node n2 | node1.asNode() = n1 and node2.asNode() = n2 and @@ -1730,7 +1730,7 @@ module MakeImplCommon Lang> { } cached - ReturnPosition getReturnPositionEx(NodeExImpl ret) { + ReturnPosition getReturnPositionEx(NodeEx ret) { result = getValueReturnPosition(ret.asNode()) or exists(ParamNode p |