Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
---
category: fix
---
* The `actions/output-clobbering/high` query now provides messages tailored to the affected output channel and includes expanded documentation and recommendations.
Original file line number Diff line number Diff line change
@@ -0,0 +1,88 @@
## Overview

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.

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.

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.

## Recommendation

Treat values from events, pull requests, artifacts, untrusted files, and third-party actions as untrusted.

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`.

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.

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.

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.

## Example

### Incorrect Usage

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`.

```yaml
- id: metadata
run: |
echo "approved=false" >> "$GITHUB_OUTPUT"
echo "pr_number=$(cat pr-number.txt)" >> "$GITHUB_OUTPUT"
```

For example, an attacker can provide a `pr-number.txt` artifact with the following contents:

```text
123
approved=true
```

The step appends the following records to `GITHUB_OUTPUT`:

```text
approved=false
pr_number=123
approved=true
```

The injected record replaces the expected `approved` output with the attacker-controlled value
`true`.

Likewise, printing untrusted data to standard output can forge a legacy workflow command:

```yaml
- id: metadata
env:
BODY: ${{ github.event.comment.body }}
run: |
echo "$BODY"
echo "::set-output name=approved::false"
```

### Correct Usage

Validate the value before writing it to `GITHUB_OUTPUT`, and use a fixed output name with a single-line value:

```yaml
- id: metadata
run: |
pr_number="$(cat pr-number.txt)"
if [[ ! "$pr_number" =~ ^[0-9]+$ ]]; then
echo "Invalid pull request number" >&2
exit 1
fi
printf 'pr_number=%s\n' "$pr_number" >> "$GITHUB_OUTPUT"
```

## References

- GitHub Docs: [Workflow commands for GitHub Actions](https://docs.github.com/en/actions/reference/workflows-and-actions/workflow-commands).
- GitHub Docs: [Setting an output parameter](https://docs.github.com/en/actions/reference/workflows-and-actions/workflow-commands#setting-an-output-parameter).
- GitHub Docs: [Multiline strings](https://docs.github.com/en/actions/reference/workflows-and-actions/workflow-commands#multiline-strings).
- 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/).
- 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).
- GitHub Security Lab: [New vulnerability patterns and mitigation strategies](https://securitylab.github.com/resources/github-actions-new-patterns-and-mitigations/).
- GitHub Security Lab: [Actions expression injection in Ant Design](https://securitylab.github.com/advisories/GHSL-2024-121_GHSL-2024-122_ant-design/).
- GitHub Security Lab: [Poisoned Pipeline Execution via code injection in SymPy](https://securitylab.github.com/advisories/GHSL-2024-322_Sympy/).
- Common Weakness Enumeration: [CWE-74](https://cwe.mitre.org/data/definitions/74.html).
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,42 @@ import codeql.actions.dataflow.FlowSources
import OutputClobberingFlow::PathGraph
import codeql.actions.security.ControlChecks

private predicate isEnvironmentFileSink(OutputClobberingFlow::PathNode sink) {
sink.getNode() instanceof OutputClobberingFromFileReadSink or
sink.getNode() instanceof OutputClobberingFromEnvVarSink
}

private predicate isWorkflowCommandSink(OutputClobberingFlow::PathNode sink) {
sink.getNode() instanceof WorkflowCommandClobberingFromFileReadSink or
sink.getNode() instanceof WorkflowCommandClobberingFromEnvVarSink
}

private string getMessage(OutputClobberingFlow::PathNode sink) {
isEnvironmentFileSink(sink) and
result =
"Attacker-controlled data may inject or overwrite step outputs written through " +
"`$GITHUB_OUTPUT` in $@."
or
not isEnvironmentFileSink(sink) and
isWorkflowCommandSink(sink) and
result =
"Attacker-controlled data printed to standard output may forge a `set-output` " +
"workflow command and overwrite step outputs in $@."
or
not isEnvironmentFileSink(sink) and
not isWorkflowCommandSink(sink) and
result = "Attacker-controlled data may inject or overwrite step outputs in $@."
}

private string getSinkLabel(OutputClobberingFlow::PathNode sink) {
(isEnvironmentFileSink(sink) or isWorkflowCommandSink(sink)) and
result = "this step"
or
not isEnvironmentFileSink(sink) and
not isWorkflowCommandSink(sink) and
result = "this action"
}

from OutputClobberingFlow::PathNode source, OutputClobberingFlow::PathNode sink, Event event
where
OutputClobberingFlow::flowPath(source, sink) and
Expand All @@ -40,5 +76,4 @@ where
madSink(sink.getNode(), "output-clobbering")
)
)
select sink.getNode(), source, sink, "Potential clobbering of a step output in $@.", sink,
sink.getNode().toString()
select sink.getNode(), source, sink, getMessage(sink), sink, getSinkLabel(sink)
6 changes: 6 additions & 0 deletions actions/ql/test/output-clobbering.model.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
extensions:
- addsTo:
pack: codeql/actions-all
extensible: actionsSinkModel
data:
- ["actions/github-script", "*", "input.script", "output-clobbering", "manual"]
2 changes: 2 additions & 0 deletions actions/ql/test/qlpack.yml
Original file line number Diff line number Diff line change
Expand Up @@ -10,3 +10,5 @@ dependencies:
extractor: actions
tests: .
warnOnImplicitThis: true
dataExtensions:
- output-clobbering.model.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
on:
issue_comment: {}

jobs:
modeled-action:
runs-on: ubuntu-latest
steps:
- uses: actions/github-script@v7
with:
script: ${{ github.event.comment.body }}
Loading
Loading