From 6a11120e505b07c5055a85e2d967e7c12e295c62 Mon Sep 17 00:00:00 2001 From: Tom Hvitved Date: Mon, 16 Sep 2024 15:01:50 +0200 Subject: [PATCH] Address review comments --- .../CommandInjectionRuntimeExecLocal.expected | 2 +- .../dataflow/summaries/Summaries.expected | 4 +- .../codeql/dataflow/internal/DataFlowImpl.qll | 59 ++++++++----------- .../dataflow/internal/DataFlowImplCommon.qll | 18 +++--- 4 files changed, 37 insertions(+), 46 deletions(-) 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 57820bb4d52..4b46d4d5cd4 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 @@ -14,7 +14,7 @@ edges | 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: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:36:21:39:21 | concat(...) : Stream [] : String | RuntimeExecTest.java:36:21:39:44 | toArray(...) | provenance | MaD:5 Sink:MaD:1 | | RuntimeExecTest.java:38:25:38:59 | stream(...) : Stream [] : String | RuntimeExecTest.java:36:21:39:21 | concat(...) : Stream [] : String | provenance | MaD:4 | | RuntimeExecTest.java:38:39:38:58 | new String[] : String[] [[]] : String | RuntimeExecTest.java:38:25:38:59 | stream(...) : Stream [] : String | provenance | MaD:3 | | RuntimeExecTest.java:38:39:38:58 | {...} : String[] [[]] : String | RuntimeExecTest.java:38:39:38:58 | new String[] : String[] [[]] : String | provenance | | diff --git a/ruby/ql/test/library-tests/dataflow/summaries/Summaries.expected b/ruby/ql/test/library-tests/dataflow/summaries/Summaries.expected index cbfa5730e5b..ce89d3f549f 100644 --- a/ruby/ql/test/library-tests/dataflow/summaries/Summaries.expected +++ b/ruby/ql/test/library-tests/dataflow/summaries/Summaries.expected @@ -292,8 +292,8 @@ edges | summaries.rb:128:1:128:1 | [post] x | summaries.rb:129:6:129:6 | x | provenance | | | summaries.rb:128:14:128:20 | tainted | summaries.rb:128:1:128:1 | [post] x | provenance | MaD:21 | | summaries.rb:131:16:131:22 | tainted | summaries.rb:131:1:131:23 | synthetic splat argument | provenance | Sink:MaD:4 | -| summaries.rb:157:14:160:3 | do ... end [captured tainted] | summaries.rb:158:15:158:21 | tainted | provenance | heuristic-callback | -| summaries.rb:157:14:160:3 | do ... end [captured tainted] | summaries.rb:158:15:158:21 | tainted | provenance | heuristic-callback | +| summaries.rb:157:14:160:3 | do ... end [captured tainted] | summaries.rb:158:15:158:21 | tainted | provenance | heuristic-callback Sink:MaD:6 | +| summaries.rb:157:14:160:3 | do ... end [captured tainted] | summaries.rb:158:15:158:21 | tainted | provenance | heuristic-callback Sink:MaD:6 | nodes | summaries.rb:1:11:1:36 | call to identity | semmle.label | call to identity | | summaries.rb:1:11:1:36 | call to identity | semmle.label | call to identity | diff --git a/shared/dataflow/codeql/dataflow/internal/DataFlowImpl.qll b/shared/dataflow/codeql/dataflow/internal/DataFlowImpl.qll index 12a583141c9..e9d6eafde8a 100644 --- a/shared/dataflow/codeql/dataflow/internal/DataFlowImpl.qll +++ b/shared/dataflow/codeql/dataflow/internal/DataFlowImpl.qll @@ -164,20 +164,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 module SourceSinkFiltering { private import codeql.util.AlertFiltering @@ -1043,10 +1029,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] @@ -2819,15 +2810,15 @@ module MakeImpl Lang> { pragma[nomagic] PathNodeImpl getAnImplicitReadSuccessorAtSink(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 + exists(result.getAnImplicitReadSuccessorAtSink(label)) and result = readTarget ) } @@ -2859,7 +2850,7 @@ module MakeImpl Lang> { // `x [Field]` -> `x` // // which the restriction below ensures. - not result = this.getAnImplicitReadSuccessorAtSink(label) + not result = this.getAnImplicitReadSuccessorAtSink(_) or exists(string l1, string l2 | result = this.getASuccessorFromNonHidden(l1).getASuccessorIfHidden(l2) and @@ -2980,18 +2971,15 @@ module MakeImpl Lang> { ) or // a final step to a sink - exists(string l2, string sinkmodel, string l2_ | - result = this.getSuccMid(l2).projectToSink(sinkmodel) and - if l2 = "" then l2_ = l2 else l2_ = l2 + " " + 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) ) ) } @@ -3057,11 +3045,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 = "" + ) } } diff --git a/shared/dataflow/codeql/dataflow/internal/DataFlowImplCommon.qll b/shared/dataflow/codeql/dataflow/internal/DataFlowImplCommon.qll index fffe28d2994..81f9946126d 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 |