Skip to content

Commit c984506

Browse files
authored
Merge branch 'main' into main
2 parents fe71e0a + 5f6b37d commit c984506

40 files changed

Lines changed: 1074 additions & 235 deletions
Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
---
2+
category: minorAnalysis
3+
---
4+
* GitHub Actions analysis now recognizes untrusted data in `github.event.merge_group` for workflows triggered by the `merge_group` event.

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -151,7 +151,7 @@ Event getRelevantNonArtifactEventInPrivilegedContext(DataFlow::Node sink) {
151151
private module EnvVarInjectionConfig implements DataFlow::ConfigSig {
152152
predicate isSource(DataFlow::Node source) {
153153
source instanceof RemoteFlowSource and
154-
not source.(RemoteFlowSource).getSourceType() = ["branch", "username"]
154+
not source.(RemoteFlowSource).getSourceType() = ["branch", "label", "username"]
155155
}
156156

157157
predicate isSink(DataFlow::Node sink) { sink instanceof EnvVarInjectionSink }

actions/ql/lib/codeql/actions/security/OutputClobberingQuery.qll

Lines changed: 43 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -111,6 +111,43 @@ class WorkflowCommandClobberingFromEnvVarSink extends OutputClobberingSink {
111111
}
112112
}
113113

114+
private string jqSafeOptionRegexp() {
115+
result = "-[acCMeRnSs]+"
116+
or
117+
result =
118+
"--(ascii-output|color-output|compact-output|exit-status|monochrome-output|null-input|" +
119+
"raw-input|slurp|sort-keys|unbuffered)"
120+
}
121+
122+
private string jqSimpleFilterRegexp() {
123+
result = "\\."
124+
or
125+
result = "\\.[A-Za-z_][A-Za-z0-9_]*(\\.[A-Za-z_][A-Za-z0-9_]*|\\[[0-9]+\\])*"
126+
}
127+
128+
private string jqSimpleFilterArgumentRegexp() {
129+
result = jqSimpleFilterRegexp()
130+
or
131+
result = "'" + jqSimpleFilterRegexp() + "'"
132+
or
133+
result = "\"" + jqSimpleFilterRegexp() + "\""
134+
}
135+
136+
private string jqLiteralInputRegexp() {
137+
result = "[A-Za-z0-9_./][A-Za-z0-9_./-]*"
138+
or
139+
result = "\\$GITHUB_EVENT_PATH"
140+
or
141+
result = "\\$\\{GITHUB_EVENT_PATH\\}"
142+
}
143+
144+
bindingset[command]
145+
private predicate jqProducesJsonEncodedOutput(string command) {
146+
command
147+
.regexpMatch("jq(\\s+" + jqSafeOptionRegexp() + ")*\\s+" + jqSimpleFilterArgumentRegexp() +
148+
"(\\s+" + jqSafeOptionRegexp() + ")*(\\s+" + jqLiteralInputRegexp() + ")*")
149+
}
150+
114151
/**
115152
* - id: clob1
116153
* run: |
@@ -159,13 +196,17 @@ class WorkflowCommandClobberingFromFileReadSink extends OutputClobberingSink {
159196
clobbering_cmd.regexpMatch(["ls", Bash::fileReadCommand()] + "\\s.*") and
160197
(
161198
// - run: echo "foo=$(<pr-id.txt)"
162-
clobbering_stmt.regexpMatch("echo.*" + clobbering_cmd + ".*")
199+
exists(string echo, int echoOffset |
200+
echo = clobbering_stmt.regexpFind("\\becho\\s+", _, echoOffset) and
201+
clobbering_stmt.indexOf(clobbering_cmd, 0, echoOffset + echo.length()) >= 0
202+
)
163203
or
164204
// A file content is printed to stdout
165205
// - run: cat pr-id.txt
166206
clobbering_stmt.indexOf(clobbering_cmd) = 0
167207
)
168-
)
208+
) and
209+
not jqProducesJsonEncodedOutput(clobbering_cmd)
169210
)
170211
}
171212
}

actions/ql/lib/codeql/actions/security/UntrustedCheckoutQuery.qll

Lines changed: 47 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -382,3 +382,50 @@ class GhSHACheckout extends SHACheckoutStep instanceof Run {
382382

383383
override string getPath() { result = this.(Run).getWorkingDirectory() }
384384
}
385+
386+
private predicate isRunCheckoutReference(
387+
PRHeadCheckoutStep checkout, Expression reference, string variable
388+
) {
389+
reference = checkout.(Run).getInScopeEnvVarExpr(variable) and
390+
(
391+
checkout instanceof SHACheckoutStep and containsHeadSHA(reference.getExpression())
392+
or
393+
checkout instanceof MutableRefCheckoutStep and
394+
(
395+
containsHeadRef(reference.getExpression()) or
396+
containsPullRequestNumber(reference.getExpression())
397+
)
398+
) and
399+
exists(string command |
400+
checkout.(Run).getScript().getACommand() = command and
401+
exists(command.regexpFind(variable, _, _))
402+
)
403+
}
404+
405+
/** Gets the expression that controls the untrusted checkout, if one can be identified. */
406+
AstNode getCheckoutReference(PRHeadCheckoutStep checkout) {
407+
exists(UsesStep uses | uses = checkout |
408+
result = uses.getArgumentExpr("ref")
409+
or
410+
not exists(uses.getArgumentExpr("ref")) and result = uses.getArgumentExpr("repository")
411+
)
412+
or
413+
isRunCheckoutReference(checkout, result, _)
414+
or
415+
checkout instanceof Run and
416+
result = checkout and
417+
not isRunCheckoutReference(checkout, _, _)
418+
}
419+
420+
/** Gets a display label for the expression that controls the untrusted checkout. */
421+
string getCheckoutReferenceText(AstNode reference) {
422+
result = reference.(Expression).toString()
423+
or
424+
not reference instanceof Expression and result = "the checkout command"
425+
}
426+
427+
/** Adds checkout-reference provenance before the checkout step in path queries. */
428+
predicate checkoutReferenceEdge(AstNode predecessor, AstNode successor) {
429+
predecessor = getCheckoutReference(successor) and
430+
not predecessor = successor
431+
}

actions/ql/lib/ext/config/context_event_map.yml

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@ extensions:
1919
- ["gollum", "github.event.changes"]
2020
- ["pull_request_comment", "github.event.comment"]
2121
- ["pull_request_comment", "github.event.pull_request"]
22+
- ["merge_group", "github.event.merge_group"]
2223
- ["pull_request_comment", "github.head_ref"]
2324
- ["pull_request_comment", "github.event.changes"]
2425
- ["pull_request_review", "github.event.pull_request"]

actions/ql/lib/ext/config/externally_triggereable_events.yml

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -17,4 +17,4 @@ extensions:
1717
- ["workflow_run"] # depending on branch filter
1818
- ["workflow_call"] # depending on caller
1919
- ["workflow_dispatch"]
20-
- ["scheduled"]
20+
- ["schedule"]

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

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -19,9 +19,16 @@ import codeql.actions.dataflow.FlowSources
1919
import EnvVarInjectionFlow::PathGraph
2020
import codeql.actions.security.ControlChecks
2121

22+
bindingset[source, event]
23+
pragma[inline_late]
24+
private predicate hasSameEventName(RemoteFlowSource source, Event event) {
25+
source.getEventName() = event.getName()
26+
}
27+
2228
from EnvVarInjectionFlow::PathNode source, EnvVarInjectionFlow::PathNode sink, Event event
2329
where
2430
EnvVarInjectionFlow::flowPath(source, sink) and
31+
hasSameEventName(source.getNode(), event) and
2532
// exclude paths to file read sinks from non-artifact sources
2633
(
2734
// source is text

actions/ql/src/Security/CWE-349/CachePoisoningViaPoisonableStep.ql

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

21-
query predicate edges(Step a, Step b) { a.getNextStep() = b }
21+
query predicate edges(AstNode predecessor, AstNode successor) {
22+
predecessor.(Step).getNextStep() = successor
23+
or
24+
checkoutReferenceEdge(predecessor, successor)
25+
}
2226

23-
from LocalJob job, Event event, Step source, Step step, string message, string path
27+
from
28+
LocalJob job, Event event, Step source, Step step, string message, string path,
29+
AstNode untrustedInput, string untrustedInputText
2430
where
2531
// the job checkouts untrusted code from a pull request or downloads an untrusted artifact
2632
job.getAStep() = source and
2733
(
2834
source instanceof PRHeadCheckoutStep and
29-
message = "due to privilege checkout of untrusted code." and
30-
path = source.(PRHeadCheckoutStep).getPath()
35+
message = "due to privilege checkout of untrusted code from" and
36+
path = source.(PRHeadCheckoutStep).getPath() and
37+
untrustedInput = getCheckoutReference(source) and
38+
untrustedInputText = getCheckoutReferenceText(untrustedInput)
3139
or
3240
source instanceof UntrustedArtifactDownloadStep and
33-
message = "due to downloading an untrusted artifact." and
34-
path = source.(UntrustedArtifactDownloadStep).getPath()
41+
message = "due to downloading" and
42+
path = source.(UntrustedArtifactDownloadStep).getPath() and
43+
untrustedInput = source and
44+
untrustedInputText = "an untrusted artifact"
3545
) and
3646
// the checkout/download is not controlled by an access check
3747
not exists(ControlCheck check |
@@ -57,6 +67,6 @@ where
5767
step instanceof PoisonableStep and
5868
// excluding privileged workflows since they can be exploited in easier circumstances
5969
not job.isPrivileged()
60-
select step, source, step,
61-
"Potential cache poisoning in the context of the default branch " + message + " ($@).", event,
62-
event.getName()
70+
select step, untrustedInput, step,
71+
"Potential cache poisoning in the context of the default branch " + message + " $@. ($@).",
72+
untrustedInput, untrustedInputText, event, event.getName()

actions/ql/src/Security/CWE-829/UntrustedCheckoutCritical.ql

Lines changed: 13 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -18,10 +18,18 @@ import codeql.actions.security.UntrustedCheckoutQuery
1818
import codeql.actions.security.PoisonableSteps
1919
import codeql.actions.security.ControlChecks
2020

21-
query predicate edges(Step a, Step b) { a.getNextStep() = b }
21+
query predicate edges(AstNode predecessor, AstNode successor) {
22+
predecessor.(Step).getNextStep() = successor
23+
or
24+
checkoutReferenceEdge(predecessor, successor)
25+
}
2226

23-
from PRHeadCheckoutStep checkout, PoisonableStep poisonable, Event event
27+
from
28+
PRHeadCheckoutStep checkout, PoisonableStep poisonable, Event event, AstNode checkoutReference,
29+
string checkoutReferenceText
2430
where
31+
checkoutReference = getCheckoutReference(checkout) and
32+
checkoutReferenceText = getCheckoutReferenceText(checkoutReference) and
2533
// the checkout is followed by a known poisonable step
2634
checkout.getAFollowingStep() = poisonable and
2735
(
@@ -51,6 +59,6 @@ where
5159
event.getName() = checkoutTriggers() and
5260
not exists(ControlCheck check | check.protects(checkout, event, "untrusted-checkout")) and
5361
not exists(ControlCheck check | check.protects(poisonable, event, "untrusted-checkout"))
54-
select checkout, checkout, poisonable,
55-
"Checkout of untrusted code in a privileged workflow with later potential execution (event trigger: $@).",
56-
event, event.getName()
62+
select checkout, checkoutReference, poisonable,
63+
"Checkout of untrusted code from $@ in a privileged workflow with later potential execution (event trigger: $@).",
64+
checkoutReference, checkoutReferenceText, event, event.getName()
Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
---
2+
category: minorAnalysis
3+
---
4+
* The `actions/envvar-injection/critical` query now requires the untrusted source and privileged context to originate from the same trigger event. The environment variable injection queries also no longer treat pull request head labels as injection-capable because they cannot contain newlines.

0 commit comments

Comments
 (0)