Skip to content

Commit bb9daa0

Browse files
d10cd10c
authored
Merge pull request #20072 from d10c/d10c/diff-informed-phase-3-actions
Actions: Diff-informed queries: phase 3 (non-trivial locations)
2 parents 84119ba + 126d24a commit bb9daa0

13 files changed

+195
-61
lines changed

‎actions/ql/lib/codeql/actions/security/ArgumentInjectionQuery.qll

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
private import actions
22
private import codeql.actions.TaintTracking
33
private import codeql.actions.dataflow.ExternalFlow
4+
private import codeql.actions.security.ControlChecks
45
import codeql.actions.dataflow.FlowSources
56
import codeql.actions.DataFlow
67

@@ -65,6 +66,16 @@ class ArgumentInjectionFromMaDSink extends ArgumentInjectionSink {
6566
override string getCommand() { result = "unknown" }
6667
}
6768

69+
/**
70+
* Gets the event that is relevant for the given node in the context of argument injection.
71+
*
72+
* This is used to highlight the event in the query results when an alert is raised.
73+
*/
74+
Event getRelevantEventInPrivilegedContext(DataFlow::Node node) {
75+
inPrivilegedContext(node.asExpr(), result) and
76+
not exists(ControlCheck check | check.protects(node.asExpr(), result, "argument-injection"))
77+
}
78+
6879
/**
6980
* A taint-tracking configuration for unsafe user input
7081
* that is used to construct and evaluate a code script.
@@ -88,6 +99,16 @@ private module ArgumentInjectionConfig implements DataFlow::ConfigSig {
8899
run.getScript().getAnEnvReachingArgumentInjectionSink(var, _, _)
89100
)
90101
}
102+
103+
predicate observeDiffInformedIncrementalMode() { any() }
104+
105+
Location getASelectedSourceLocation(DataFlow::Node source) { none() }
106+
107+
Location getASelectedSinkLocation(DataFlow::Node sink) {
108+
result = sink.getLocation()
109+
or
110+
result = getRelevantEventInPrivilegedContext(sink).getLocation()
111+
}
91112
}
92113

93114
/** Tracks flow of unsafe user input that is used to construct and evaluate a code script. */

‎actions/ql/lib/codeql/actions/security/ArtifactPoisoningQuery.qll

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ import codeql.actions.DataFlow
44
import codeql.actions.dataflow.FlowSources
55
import codeql.actions.security.PoisonableSteps
66
import codeql.actions.security.UntrustedCheckoutQuery
7+
import codeql.actions.security.ControlChecks
78

89
string unzipRegexp() { result = "(unzip|tar)\\s+.*" }
910

@@ -292,6 +293,16 @@ class ArtifactPoisoningSink extends DataFlow::Node {
292293
string getPath() { result = download.getPath() }
293294
}
294295

296+
/**
297+
* Gets the event that is relevant for the given node in the context of artifact poisoning.
298+
*
299+
* This is used to highlight the event in the query results when an alert is raised.
300+
*/
301+
Event getRelevantEventInPrivilegedContext(DataFlow::Node node) {
302+
inPrivilegedContext(node.asExpr(), result) and
303+
not exists(ControlCheck check | check.protects(node.asExpr(), result, "artifact-poisoning"))
304+
}
305+
295306
/**
296307
* A taint-tracking configuration for unsafe artifacts
297308
* that is used may lead to artifact poisoning
@@ -318,6 +329,16 @@ private module ArtifactPoisoningConfig implements DataFlow::ConfigSig {
318329
exists(run.getScript().getAFileReadCommand())
319330
)
320331
}
332+
333+
predicate observeDiffInformedIncrementalMode() { any() }
334+
335+
Location getASelectedSourceLocation(DataFlow::Node source) { none() }
336+
337+
Location getASelectedSinkLocation(DataFlow::Node sink) {
338+
result = sink.getLocation()
339+
or
340+
result = getRelevantEventInPrivilegedContext(sink).getLocation()
341+
}
321342
}
322343

323344
/** Tracks flow of unsafe artifacts that is used in an insecure way. */

‎actions/ql/lib/codeql/actions/security/CodeInjectionQuery.qll

Lines changed: 54 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,8 @@ private import codeql.actions.TaintTracking
33
private import codeql.actions.dataflow.ExternalFlow
44
import codeql.actions.dataflow.FlowSources
55
import codeql.actions.DataFlow
6+
import codeql.actions.security.ControlChecks
7+
import codeql.actions.security.CachePoisoningQuery
68

79
class CodeInjectionSink extends DataFlow::Node {
810
CodeInjectionSink() {
@@ -11,6 +13,46 @@ class CodeInjectionSink extends DataFlow::Node {
1113
}
1214
}
1315

16+
/**
17+
* Get the relevant event for the sink in CodeInjectionCritical.ql.
18+
*/
19+
Event getRelevantCriticalEventForSink(DataFlow::Node sink) {
20+
inPrivilegedContext(sink.asExpr(), result) and
21+
not exists(ControlCheck check | check.protects(sink.asExpr(), result, "code-injection")) and
22+
// exclude cases where the sink is a JS script and the expression uses toJson
23+
not exists(UsesStep script |
24+
script.getCallee() = "actions/github-script" and
25+
script.getArgumentExpr("script") = sink.asExpr() and
26+
exists(getAToJsonReferenceExpression(sink.asExpr().(Expression).getExpression(), _))
27+
)
28+
}
29+
30+
/**
31+
* Get the relevant event for the sink in CachePoisoningViaCodeInjection.ql.
32+
*/
33+
Event getRelevantCachePoisoningEventForSink(DataFlow::Node sink) {
34+
exists(LocalJob job |
35+
job = sink.asExpr().getEnclosingJob() and
36+
job.getATriggerEvent() = result and
37+
// job can be triggered by an external user
38+
result.isExternallyTriggerable() and
39+
// excluding privileged workflows since they can be exploited in easier circumstances
40+
// which is covered by `actions/code-injection/critical`
41+
not job.isPrivilegedExternallyTriggerable(result) and
42+
(
43+
// the workflow runs in the context of the default branch
44+
runsOnDefaultBranch(result)
45+
or
46+
// the workflow caller runs in the context of the default branch
47+
result.getName() = "workflow_call" and
48+
exists(ExternalJob caller |
49+
caller.getCallee() = job.getLocation().getFile().getRelativePath() and
50+
runsOnDefaultBranch(caller.getATriggerEvent())
51+
)
52+
)
53+
)
54+
}
55+
1456
/**
1557
* A taint-tracking configuration for unsafe user input
1658
* that is used to construct and evaluate a code script.
@@ -35,6 +77,18 @@ private module CodeInjectionConfig implements DataFlow::ConfigSig {
3577
exists(run.getScript().getAFileReadCommand())
3678
)
3779
}
80+
81+
predicate observeDiffInformedIncrementalMode() { any() }
82+
83+
Location getASelectedSourceLocation(DataFlow::Node source) { none() }
84+
85+
Location getASelectedSinkLocation(DataFlow::Node sink) {
86+
result = sink.getLocation()
87+
or
88+
result = getRelevantCriticalEventForSink(sink).getLocation()
89+
or
90+
result = getRelevantCachePoisoningEventForSink(sink).getLocation()
91+
}
3892
}
3993

4094
/** Tracks flow of unsafe user input that is used to construct and evaluate a code script. */

‎actions/ql/lib/codeql/actions/security/CommandInjectionQuery.qll

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,11 +3,20 @@ private import codeql.actions.TaintTracking
33
private import codeql.actions.dataflow.ExternalFlow
44
import codeql.actions.dataflow.FlowSources
55
import codeql.actions.DataFlow
6+
import codeql.actions.security.ControlChecks
67

78
private class CommandInjectionSink extends DataFlow::Node {
89
CommandInjectionSink() { madSink(this, "command-injection") }
910
}
1011

12+
/** Get the relevant event for the sink in CommandInjectionCritical.ql. */
13+
Event getRelevantEventInPrivilegedContext(DataFlow::Node sink) {
14+
inPrivilegedContext(sink.asExpr(), result) and
15+
not exists(ControlCheck check |
16+
check.protects(sink.asExpr(), result, ["command-injection", "code-injection"])
17+
)
18+
}
19+
1120
/**
1221
* A taint-tracking configuration for unsafe user input
1322
* that is used to construct and evaluate a system command.
@@ -16,6 +25,16 @@ private module CommandInjectionConfig implements DataFlow::ConfigSig {
1625
predicate isSource(DataFlow::Node source) { source instanceof RemoteFlowSource }
1726

1827
predicate isSink(DataFlow::Node sink) { sink instanceof CommandInjectionSink }
28+
29+
predicate observeDiffInformedIncrementalMode() { any() }
30+
31+
Location getASelectedSourceLocation(DataFlow::Node source) { none() }
32+
33+
Location getASelectedSinkLocation(DataFlow::Node sink) {
34+
result = sink.getLocation()
35+
or
36+
result = getRelevantEventInPrivilegedContext(sink).getLocation()
37+
}
1938
}
2039

2140
/** Tracks flow of unsafe user input that is used to construct and evaluate a system command. */

‎actions/ql/lib/codeql/actions/security/EnvPathInjectionQuery.qll

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -72,6 +72,25 @@ class EnvPathInjectionFromMaDSink extends EnvPathInjectionSink {
7272
EnvPathInjectionFromMaDSink() { madSink(this, "envpath-injection") }
7373
}
7474

75+
/**
76+
* Get the relevant event for a sink in EnvPathInjectionCritical.ql where the source type is "artifact".
77+
*/
78+
Event getRelevantArtifactEventInPrivilegedContext(DataFlow::Node sink) {
79+
inPrivilegedContext(sink.asExpr(), result) and
80+
not exists(ControlCheck check |
81+
check.protects(sink.asExpr(), result, ["untrusted-checkout", "artifact-poisoning"])
82+
) and
83+
sink instanceof EnvPathInjectionFromFileReadSink
84+
}
85+
86+
/**
87+
* Get the relevant event for a sink in EnvPathInjectionCritical.ql where the source type is not "artifact".
88+
*/
89+
Event getRelevantNonArtifactEventInPrivilegedContext(DataFlow::Node sink) {
90+
inPrivilegedContext(sink.asExpr(), result) and
91+
not exists(ControlCheck check | check.protects(sink.asExpr(), result, "code-injection"))
92+
}
93+
7594
/**
7695
* A taint-tracking configuration for unsafe user input
7796
* that is used to construct and evaluate an environment variable.
@@ -108,6 +127,18 @@ private module EnvPathInjectionConfig implements DataFlow::ConfigSig {
108127
exists(run.getScript().getAFileReadCommand())
109128
)
110129
}
130+
131+
predicate observeDiffInformedIncrementalMode() { any() }
132+
133+
Location getASelectedSourceLocation(DataFlow::Node source) { none() }
134+
135+
Location getASelectedSinkLocation(DataFlow::Node sink) {
136+
result = sink.getLocation()
137+
or
138+
result = getRelevantArtifactEventInPrivilegedContext(sink).getLocation()
139+
or
140+
result = getRelevantNonArtifactEventInPrivilegedContext(sink).getLocation()
141+
}
111142
}
112143

113144
/** Tracks flow of unsafe user input that is used to construct and evaluate the PATH environment variable. */

‎actions/ql/lib/codeql/actions/security/EnvVarInjectionQuery.qll

Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -126,6 +126,32 @@ class EnvVarInjectionFromMaDSink extends EnvVarInjectionSink {
126126
EnvVarInjectionFromMaDSink() { madSink(this, "envvar-injection") }
127127
}
128128

129+
/**
130+
* Get the relevant event for a sink in EnvVarInjectionCritical.ql where the source type is "artifact".
131+
*/
132+
Event getRelevantArtifactEventInPrivilegedContext(DataFlow::Node sink) {
133+
inPrivilegedContext(sink.asExpr(), result) and
134+
not exists(ControlCheck check |
135+
check
136+
.protects(sink.asExpr(), result,
137+
["envvar-injection", "untrusted-checkout", "artifact-poisoning"])
138+
) and
139+
(
140+
sink instanceof EnvVarInjectionFromFileReadSink or
141+
madSink(sink, "envvar-injection")
142+
)
143+
}
144+
145+
/**
146+
* Get the relevant event for a sink in EnvVarInjectionCritical.ql where the source type is not "artifact".
147+
*/
148+
Event getRelevantNonArtifactEventInPrivilegedContext(DataFlow::Node sink) {
149+
inPrivilegedContext(sink.asExpr(), result) and
150+
not exists(ControlCheck check |
151+
check.protects(sink.asExpr(), result, ["envvar-injection", "code-injection"])
152+
)
153+
}
154+
129155
/**
130156
* A taint-tracking configuration for unsafe user input
131157
* that is used to construct and evaluate an environment variable.
@@ -163,6 +189,18 @@ private module EnvVarInjectionConfig implements DataFlow::ConfigSig {
163189
exists(run.getScript().getAFileReadCommand())
164190
)
165191
}
192+
193+
predicate observeDiffInformedIncrementalMode() { any() }
194+
195+
Location getASelectedSourceLocation(DataFlow::Node source) { none() }
196+
197+
Location getASelectedSinkLocation(DataFlow::Node sink) {
198+
result = sink.getLocation()
199+
or
200+
result = getRelevantArtifactEventInPrivilegedContext(sink).getLocation()
201+
or
202+
result = getRelevantNonArtifactEventInPrivilegedContext(sink).getLocation()
203+
}
166204
}
167205

168206
/** Tracks flow of unsafe user input that is used to construct and evaluate an environment variable. */

‎actions/ql/src/Security/CWE-077/EnvPathInjectionCritical.ql

Lines changed: 2 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -21,18 +21,12 @@ import codeql.actions.security.ControlChecks
2121
from EnvPathInjectionFlow::PathNode source, EnvPathInjectionFlow::PathNode sink, Event event
2222
where
2323
EnvPathInjectionFlow::flowPath(source, sink) and
24-
inPrivilegedContext(sink.getNode().asExpr(), event) and
2524
(
2625
not source.getNode().(RemoteFlowSource).getSourceType() = "artifact" and
27-
not exists(ControlCheck check |
28-
check.protects(sink.getNode().asExpr(), event, "code-injection")
29-
)
26+
event = getRelevantNonArtifactEventInPrivilegedContext(sink.getNode())
3027
or
3128
source.getNode().(RemoteFlowSource).getSourceType() = "artifact" and
32-
not exists(ControlCheck check |
33-
check.protects(sink.getNode().asExpr(), event, ["untrusted-checkout", "artifact-poisoning"])
34-
) and
35-
sink.getNode() instanceof EnvPathInjectionFromFileReadSink
29+
event = getRelevantArtifactEventInPrivilegedContext(sink.getNode())
3630
)
3731
select sink.getNode(), source, sink,
3832
"Potential PATH environment variable injection in $@, which may be controlled by an external user ($@).",

‎actions/ql/src/Security/CWE-077/EnvVarInjectionCritical.ql

Lines changed: 2 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -22,26 +22,15 @@ import codeql.actions.security.ControlChecks
2222
from EnvVarInjectionFlow::PathNode source, EnvVarInjectionFlow::PathNode sink, Event event
2323
where
2424
EnvVarInjectionFlow::flowPath(source, sink) and
25-
inPrivilegedContext(sink.getNode().asExpr(), event) and
2625
// exclude paths to file read sinks from non-artifact sources
2726
(
2827
// source is text
2928
not source.getNode().(RemoteFlowSource).getSourceType() = "artifact" and
30-
not exists(ControlCheck check |
31-
check.protects(sink.getNode().asExpr(), event, ["envvar-injection", "code-injection"])
32-
)
29+
event = getRelevantNonArtifactEventInPrivilegedContext(sink.getNode())
3330
or
3431
// source is an artifact or a file from an untrusted checkout
3532
source.getNode().(RemoteFlowSource).getSourceType() = "artifact" and
36-
not exists(ControlCheck check |
37-
check
38-
.protects(sink.getNode().asExpr(), event,
39-
["envvar-injection", "untrusted-checkout", "artifact-poisoning"])
40-
) and
41-
(
42-
sink.getNode() instanceof EnvVarInjectionFromFileReadSink or
43-
madSink(sink.getNode(), "envvar-injection")
44-
)
33+
event = getRelevantArtifactEventInPrivilegedContext(sink.getNode())
4534
)
4635
select sink.getNode(), source, sink,
4736
"Potential environment variable injection in $@, which may be controlled by an external user ($@).",

‎actions/ql/src/Security/CWE-094/CodeInjectionCritical.ql

Lines changed: 2 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -22,15 +22,8 @@ import codeql.actions.security.ControlChecks
2222
from CodeInjectionFlow::PathNode source, CodeInjectionFlow::PathNode sink, Event event
2323
where
2424
CodeInjectionFlow::flowPath(source, sink) and
25-
inPrivilegedContext(sink.getNode().asExpr(), event) and
26-
source.getNode().(RemoteFlowSource).getEventName() = event.getName() and
27-
not exists(ControlCheck check | check.protects(sink.getNode().asExpr(), event, "code-injection")) and
28-
// exclude cases where the sink is a JS script and the expression uses toJson
29-
not exists(UsesStep script |
30-
script.getCallee() = "actions/github-script" and
31-
script.getArgumentExpr("script") = sink.getNode().asExpr() and
32-
exists(getAToJsonReferenceExpression(sink.getNode().asExpr().(Expression).getExpression(), _))
33-
)
25+
event = getRelevantCriticalEventForSink(sink.getNode()) and
26+
source.getNode().(RemoteFlowSource).getEventName() = event.getName()
3427
select sink.getNode(), source, sink,
3528
"Potential code injection in $@, which may be controlled by an external user ($@).", sink,
3629
sink.getNode().asExpr().(Expression).getRawExpression(), event, event.getName()

‎actions/ql/src/Security/CWE-349/CachePoisoningViaCodeInjection.ql

Lines changed: 2 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -18,30 +18,13 @@ import codeql.actions.security.CachePoisoningQuery
1818
import CodeInjectionFlow::PathGraph
1919
import codeql.actions.security.ControlChecks
2020

21-
from CodeInjectionFlow::PathNode source, CodeInjectionFlow::PathNode sink, LocalJob job, Event event
21+
from CodeInjectionFlow::PathNode source, CodeInjectionFlow::PathNode sink, Event event
2222
where
2323
CodeInjectionFlow::flowPath(source, sink) and
24-
job = sink.getNode().asExpr().getEnclosingJob() and
25-
job.getATriggerEvent() = event and
26-
// job can be triggered by an external user
27-
event.isExternallyTriggerable() and
24+
event = getRelevantCachePoisoningEventForSink(sink.getNode()) and
2825
// the checkout is not controlled by an access check
2926
not exists(ControlCheck check |
3027
check.protects(source.getNode().asExpr(), event, "code-injection")
31-
) and
32-
// excluding privileged workflows since they can be exploited in easier circumstances
33-
// which is covered by `actions/code-injection/critical`
34-
not job.isPrivilegedExternallyTriggerable(event) and
35-
(
36-
// the workflow runs in the context of the default branch
37-
runsOnDefaultBranch(event)
38-
or
39-
// the workflow caller runs in the context of the default branch
40-
event.getName() = "workflow_call" and
41-
exists(ExternalJob caller |
42-
caller.getCallee() = job.getLocation().getFile().getRelativePath() and
43-
runsOnDefaultBranch(caller.getATriggerEvent())
44-
)
4528
)
4629
select sink.getNode(), source, sink,
4730
"Unprivileged code injection in $@, which may lead to cache poisoning ($@).", sink,

0 commit comments

Comments
 (0)