Skip to content

Commit

Permalink
Address review comments
Browse files Browse the repository at this point in the history
  • Loading branch information
@hvitved
hvitved committed Sep 18, 2024
1 parent 8372866 commit 7218ff7
Show file tree
Hide file tree
Showing 3 changed files with 40 additions and 49 deletions.
6 changes: 3 additions & 3 deletions .../test/experimental/query-tests/security/CWE-078/CommandInjectionRuntimeExecLocal.expected
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 [<element>] : String | RuntimeExecTest.java:36:21:39:44 | toArray(...) | provenance | MaD:5 |
| RuntimeExecTest.java:38:25:38:59 | stream(...) : Stream [<element>] : String | RuntimeExecTest.java:36:21:39:21 | concat(...) : Stream [<element>] : String | provenance | MaD:4 |
Expand Down
65 changes: 28 additions & 37 deletions shared/dataflow/codeql/dataflow/internal/DataFlowImpl.qll
View file
Original file line number Diff line number Diff line change
Expand Up @@ -153,20 +153,6 @@ module MakeImpl<LocationSig Location, InputSig<Location> Lang> {
module Impl<FullStateConfigSig Config> {
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
Expand Down Expand Up @@ -968,10 +954,15 @@ module MakeImpl<LocationSig Location, InputSig<Location> 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:")

Check notice

Code scanning / CodeQL

Use of regexp to match a set of constant string Note

Use string comparison instead of regexp to compare against a constant set of string.
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]
Expand Down Expand Up @@ -2742,26 +2733,25 @@ module MakeImpl<LocationSig Location, InputSig<Location> 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)
)
}

private PathNodeImpl getASuccessorIfHidden(string label) {
this.isHidden() and
result = this.getASuccessorImpl(label)
or
result = this.getAnImplicitReadSuccessorAtSink(label)
result = this.getAnImplicitReadSink(label)
}

private PathNodeImpl getASuccessorFromNonHidden(string label) {
Expand All @@ -2784,7 +2774,7 @@ module MakeImpl<LocationSig Location, InputSig<Location> 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
Expand Down Expand Up @@ -2905,17 +2895,15 @@ module MakeImpl<LocationSig Location, InputSig<Location> 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)
)
)
}
Expand Down Expand Up @@ -2981,11 +2969,14 @@ module MakeImpl<LocationSig Location, InputSig<Location> 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 = ""
)
}
}

Expand Down Expand Up @@ -3258,7 +3249,7 @@ module MakeImpl<LocationSig Location, InputSig<Location> Lang> {
n2 = localStep(n1) and
n1.isHidden()
or
n2 = n1.getAnImplicitReadSuccessorAtSink(_)
n2 = n1.getAnImplicitReadSink(_)
}

bindingset[par, ret]
Expand Down
18 changes: 9 additions & 9 deletions shared/dataflow/codeql/dataflow/internal/DataFlowImplCommon.qll
View file
Original file line number Diff line number Diff line change
Expand Up @@ -850,7 +850,7 @@ module MakeImplCommon<LocationSig Location, InputSig<Location> Lang> {

class SndLevelScopeOption = SndLevelScopeOption::Option;

final class NodeExImpl extends TNodeEx {
final class NodeEx extends TNodeEx {
string toString() {
result = this.asNode().toString()
or
Expand Down Expand Up @@ -899,14 +899,14 @@ module MakeImplCommon<LocationSig Location, InputSig<Location> 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)
Expand All @@ -919,10 +919,10 @@ module MakeImplCommon<LocationSig Location, InputSig<Location> 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 }

Expand Down Expand Up @@ -1713,7 +1713,7 @@ module MakeImplCommon<LocationSig Location, InputSig<Location> 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
Expand All @@ -1730,7 +1730,7 @@ module MakeImplCommon<LocationSig Location, InputSig<Location> Lang> {
}

cached
ReturnPosition getReturnPositionEx(NodeExImpl ret) {
ReturnPosition getReturnPositionEx(NodeEx ret) {
result = getValueReturnPosition(ret.asNode())
or
exists(ParamNode p |
Expand Down

0 comments on commit 7218ff7

Please sign in to comment.