Skip to content

Commit c9727af

Browse files
committed
Merge remote-tracking branch 'github/main' into fix/actions-output-clobbering-regex
# Conflicts: # actions/ql/test/query-tests/Security/CWE-074/OutputClobberingHigh.expected
2 parents 9c83964 + ddb7e3f commit c9727af

22 files changed

Lines changed: 465 additions & 87 deletions

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/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/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.
Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
---
2+
category: fix
3+
---
4+
* The `actions/cache-poisoning/poisonable-step` and `actions/untrusted-checkout/critical` queries now start paths at the expressions that control untrusted checkouts and link their alert messages to those expressions.
Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
---
2+
category: fix
3+
---
4+
* The `actions/output-clobbering/high` query now provides messages tailored to the affected output channel and includes expanded documentation and recommendations.
Lines changed: 88 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,88 @@
1+
## Overview
2+
3+
GitHub Actions steps communicate output values to the runner through a line-oriented command format. A step normally sets an output by appending a `name=value` record to the file referenced by `GITHUB_OUTPUT`. Multiline values use a delimiter-based form. Older workflows may instead emit `set-output` workflow commands to standard output.
4+
5+
If attacker-controlled data is written to one of these command channels without validation, the data may be interpreted as command syntax rather than as a single value. An attacker can use newline characters, a matching multiline delimiter, or a forged workflow command to create additional outputs or overwrite output values that later steps expect to be trusted.
6+
7+
The attacker-controlled data may come directly from an event, or indirectly from an untrusted checkout, downloaded artifact, file, or action output. Clobbered outputs can alter conditions and arguments in later steps. If a later step interpolates an injected output into a script, this issue may contribute to arbitrary code execution.
8+
9+
## Recommendation
10+
11+
Treat values from events, pull requests, artifacts, untrusted files, and third-party actions as untrusted.
12+
13+
Before writing an untrusted value to `GITHUB_OUTPUT`, validate it against the narrow format required by the workflow. For example, require a pull request number to contain only decimal digits. For a single-line output, reject carriage-return and newline characters. Do not append an untrusted file directly to `GITHUB_OUTPUT`.
14+
15+
Do not use the deprecated `set-output` workflow command. Migrate to `GITHUB_OUTPUT`, and avoid printing untrusted data while legacy workflow-command processing is enabled.
16+
17+
For multiline values, use a random delimiter that cannot occur on a line by itself in the value. If the value is arbitrary, store it in a normal file instead of using the multiline command format, and pass only the validated file path as an output.
18+
19+
Review the documentation and implementation of actions that consume untrusted inputs. Use only inputs that the action handles as data rather than as output-command syntax.
20+
21+
## Example
22+
23+
### Incorrect Usage
24+
25+
The following step reads an attacker-controlled artifact file and writes its contents directly to `GITHUB_OUTPUT`. A newline in `pr-number.txt` can add another output record and overwrite `approved`.
26+
27+
```yaml
28+
- id: metadata
29+
run: |
30+
echo "approved=false" >> "$GITHUB_OUTPUT"
31+
echo "pr_number=$(cat pr-number.txt)" >> "$GITHUB_OUTPUT"
32+
```
33+
34+
For example, an attacker can provide a `pr-number.txt` artifact with the following contents:
35+
36+
```text
37+
123
38+
approved=true
39+
```
40+
41+
The step appends the following records to `GITHUB_OUTPUT`:
42+
43+
```text
44+
approved=false
45+
pr_number=123
46+
approved=true
47+
```
48+
49+
The injected record replaces the expected `approved` output with the attacker-controlled value
50+
`true`.
51+
52+
Likewise, printing untrusted data to standard output can forge a legacy workflow command:
53+
54+
```yaml
55+
- id: metadata
56+
env:
57+
BODY: ${{ github.event.comment.body }}
58+
run: |
59+
echo "$BODY"
60+
echo "::set-output name=approved::false"
61+
```
62+
63+
### Correct Usage
64+
65+
Validate the value before writing it to `GITHUB_OUTPUT`, and use a fixed output name with a single-line value:
66+
67+
```yaml
68+
- id: metadata
69+
run: |
70+
pr_number="$(cat pr-number.txt)"
71+
if [[ ! "$pr_number" =~ ^[0-9]+$ ]]; then
72+
echo "Invalid pull request number" >&2
73+
exit 1
74+
fi
75+
printf 'pr_number=%s\n' "$pr_number" >> "$GITHUB_OUTPUT"
76+
```
77+
78+
## References
79+
80+
- GitHub Docs: [Workflow commands for GitHub Actions](https://docs.github.com/en/actions/reference/workflows-and-actions/workflow-commands).
81+
- GitHub Docs: [Setting an output parameter](https://docs.github.com/en/actions/reference/workflows-and-actions/workflow-commands#setting-an-output-parameter).
82+
- GitHub Docs: [Multiline strings](https://docs.github.com/en/actions/reference/workflows-and-actions/workflow-commands#multiline-strings).
83+
- GitHub Changelog: [Deprecating `save-state` and `set-output` commands](https://github.blog/changelog/2022-10-10-github-actions-deprecating-save-state-and-set-output-commands/).
84+
- GitHub Actions Toolkit: [`add-path` and `set-env` runner commands are processed via stdout](https://github.com/actions/toolkit/security/advisories/GHSA-mfwh-5m23-j46w).
85+
- GitHub Security Lab: [New vulnerability patterns and mitigation strategies](https://securitylab.github.com/resources/github-actions-new-patterns-and-mitigations/).
86+
- GitHub Security Lab: [Actions expression injection in Ant Design](https://securitylab.github.com/advisories/GHSL-2024-121_GHSL-2024-122_ant-design/).
87+
- GitHub Security Lab: [Poisoned Pipeline Execution via code injection in SymPy](https://securitylab.github.com/advisories/GHSL-2024-322_Sympy/).
88+
- Common Weakness Enumeration: [CWE-74](https://cwe.mitre.org/data/definitions/74.html).

actions/ql/src/experimental/Security/CWE-074/OutputClobberingHigh.ql

Lines changed: 37 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,42 @@ import codeql.actions.dataflow.FlowSources
1919
import OutputClobberingFlow::PathGraph
2020
import codeql.actions.security.ControlChecks
2121

22+
private predicate isEnvironmentFileSink(OutputClobberingFlow::PathNode sink) {
23+
sink.getNode() instanceof OutputClobberingFromFileReadSink or
24+
sink.getNode() instanceof OutputClobberingFromEnvVarSink
25+
}
26+
27+
private predicate isWorkflowCommandSink(OutputClobberingFlow::PathNode sink) {
28+
sink.getNode() instanceof WorkflowCommandClobberingFromFileReadSink or
29+
sink.getNode() instanceof WorkflowCommandClobberingFromEnvVarSink
30+
}
31+
32+
private string getMessage(OutputClobberingFlow::PathNode sink) {
33+
isEnvironmentFileSink(sink) and
34+
result =
35+
"Attacker-controlled data may inject or overwrite step outputs written through " +
36+
"`$GITHUB_OUTPUT` in $@."
37+
or
38+
not isEnvironmentFileSink(sink) and
39+
isWorkflowCommandSink(sink) and
40+
result =
41+
"Attacker-controlled data printed to standard output may forge a `set-output` " +
42+
"workflow command and overwrite step outputs in $@."
43+
or
44+
not isEnvironmentFileSink(sink) and
45+
not isWorkflowCommandSink(sink) and
46+
result = "Attacker-controlled data may inject or overwrite step outputs in $@."
47+
}
48+
49+
private string getSinkLabel(OutputClobberingFlow::PathNode sink) {
50+
(isEnvironmentFileSink(sink) or isWorkflowCommandSink(sink)) and
51+
result = "this step"
52+
or
53+
not isEnvironmentFileSink(sink) and
54+
not isWorkflowCommandSink(sink) and
55+
result = "this action"
56+
}
57+
2258
from OutputClobberingFlow::PathNode source, OutputClobberingFlow::PathNode sink, Event event
2359
where
2460
OutputClobberingFlow::flowPath(source, sink) and
@@ -40,5 +76,4 @@ where
4076
madSink(sink.getNode(), "output-clobbering")
4177
)
4278
)
43-
select sink.getNode(), source, sink, "Potential clobbering of a step output in $@.", sink,
44-
sink.getNode().toString()
79+
select sink.getNode(), source, sink, getMessage(sink), sink, getSinkLabel(sink)

0 commit comments

Comments
 (0)