Skip to content

Commit baabf3d

Browse files
committed
Add code injection low severity query for step outputs
1 parent 1676f45 commit baabf3d

File tree

3 files changed

+191
-13
lines changed

3 files changed

+191
-13
lines changed

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

Lines changed: 85 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -48,6 +48,22 @@ Event getRelevantCachePoisoningEventForSink(DataFlow::Node sink) {
4848
)
4949
}
5050

51+
private predicate codeInjectionAdditionalFlowStep(DataFlow::Node pred, DataFlow::Node succ) {
52+
exists(Uses step |
53+
pred instanceof FileSource and
54+
pred.asExpr().(Step).getAFollowingStep() = step and
55+
succ.asExpr() = step and
56+
madSink(succ, "code-injection")
57+
)
58+
or
59+
exists(Run run |
60+
pred instanceof FileSource and
61+
pred.asExpr().(Step).getAFollowingStep() = run and
62+
succ.asExpr() = run.getScript() and
63+
exists(run.getScript().getAFileReadCommand())
64+
)
65+
}
66+
5167
/**
5268
* A taint-tracking configuration for unsafe user input
5369
* that is used to construct and evaluate a code script.
@@ -58,19 +74,7 @@ private module CodeInjectionConfig implements DataFlow::ConfigSig {
5874
predicate isSink(DataFlow::Node sink) { sink instanceof CodeInjectionSink }
5975

6076
predicate isAdditionalFlowStep(DataFlow::Node pred, DataFlow::Node succ) {
61-
exists(Uses step |
62-
pred instanceof FileSource and
63-
pred.asExpr().(Step).getAFollowingStep() = step and
64-
succ.asExpr() = step and
65-
madSink(succ, "code-injection")
66-
)
67-
or
68-
exists(Run run |
69-
pred instanceof FileSource and
70-
pred.asExpr().(Step).getAFollowingStep() = run and
71-
succ.asExpr() = run.getScript() and
72-
exists(run.getScript().getAFileReadCommand())
73-
)
77+
codeInjectionAdditionalFlowStep(pred, succ)
7478
}
7579

7680
predicate observeDiffInformedIncrementalMode() { any() }
@@ -87,6 +91,64 @@ private module CodeInjectionConfig implements DataFlow::ConfigSig {
8791
/** Tracks flow of unsafe user input that is used to construct and evaluate a code script. */
8892
module CodeInjectionFlow = TaintTracking::Global<CodeInjectionConfig>;
8993

94+
private predicate knownSafeAction(string action) {
95+
action =
96+
[
97+
// Setup actions - version/cache outputs are deterministic
98+
"actions/setup-java",
99+
"actions/setup-python",
100+
"actions/setup-node",
101+
"actions/setup-go",
102+
"actions/setup-dotnet",
103+
"actions/cache",
104+
"actions/download-artifact",
105+
"actions/configure-pages",
106+
"actions/attest-build-provenance",
107+
"actions/create-github-app-token",
108+
"oracle-actions/setup-java",
109+
"spring-io/artifactory-deploy-action",
110+
"YunaBraska/java-info-action",
111+
// Docker actions - digest/version outputs are system-generated
112+
"docker/build-push-action",
113+
"docker/metadata-action",
114+
"docker/setup-buildx-action",
115+
// PR/repo automation - outputs are GitHub-assigned identifiers
116+
"dorny/test-reporter",
117+
"peter-evans/create-pull-request",
118+
// AWS actions - outputs are AWS-generated identifiers
119+
"aws-actions/aws-codebuild-run-build",
120+
// Security/crypto actions - outputs are cryptographic, not user-controllable
121+
"crazy-max/ghaction-import-gpg",
122+
// Hardware/system info actions - outputs are deterministic
123+
"SimonShi1994/cpu-cores"
124+
]
125+
}
126+
127+
/**
128+
* A taint-tracking configuration for step outputs
129+
* that are used to construct and evaluate a code script.
130+
*/
131+
private module CodeInjectionFromStepOutputConfig implements DataFlow::ConfigSig {
132+
predicate isSource(DataFlow::Node source) {
133+
exists(StepOutputExpression soe, UsesStep us |
134+
soe = source.asExpr() and soe.getStepId() = us.getId()
135+
|
136+
not knownSafeAction(us.getCallee())
137+
)
138+
}
139+
140+
predicate isSink(DataFlow::Node sink) { sink instanceof CodeInjectionSink }
141+
142+
predicate isAdditionalFlowStep(DataFlow::Node pred, DataFlow::Node succ) {
143+
codeInjectionAdditionalFlowStep(pred, succ)
144+
}
145+
146+
predicate observeDiffInformedIncrementalMode() { any() }
147+
}
148+
149+
/** Tracks flow of unsafe user input that is used to construct and evaluate a code script. */
150+
module CodeInjectionFromStepOutputFlow = TaintTracking::Global<CodeInjectionFromStepOutputConfig>;
151+
90152
/**
91153
* Holds if there is a code injection flow from `source` to `sink` with
92154
* critical severity, linked by `event`.
@@ -110,6 +172,16 @@ predicate mediumSeverityCodeInjection(
110172
not isGithubScriptUsingToJson(sink.getNode().asExpr())
111173
}
112174

175+
/**
176+
* Holds if there is a code injection flow from `source` to `sink` with low severity.
177+
*/
178+
predicate lowSeverityCodeInjection(
179+
CodeInjectionFromStepOutputFlow::PathNode source, CodeInjectionFromStepOutputFlow::PathNode sink
180+
) {
181+
CodeInjectionFromStepOutputFlow::flowPath(source, sink) and
182+
not isGithubScriptUsingToJson(sink.getNode().asExpr())
183+
}
184+
113185
/**
114186
* Holds if `expr` is the `script` input to `actions/github-script` and it uses
115187
* `toJson`.
Lines changed: 80 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,80 @@
1+
## Overview
2+
3+
Using user-controlled input in GitHub Actions may lead to code injection in contexts like _run:_ or _script:_.
4+
5+
Code injection in GitHub Actions may allow an attacker to exfiltrate any secrets used in the workflow and the temporary GitHub repository authorization token. The token may have write access to the repository, allowing an attacker to make changes to the repository.
6+
7+
## Recommendation
8+
9+
The best practice to avoid code injection vulnerabilities in GitHub workflows is to set the untrusted input value of the expression to an intermediate environment variable and then use the environment variable using the native syntax of the shell/script interpreter (that is, not _${{ env.VAR }}_).
10+
11+
It is also recommended to limit the permissions of any tokens used by a workflow such as the GITHUB_TOKEN.
12+
13+
## Example
14+
15+
### Incorrect Usage
16+
17+
The following example lets attackers inject an arbitrary shell command:
18+
19+
```yaml
20+
on: issue_comment
21+
22+
jobs:
23+
echo-body:
24+
runs-on: ubuntu-latest
25+
steps:
26+
- run: |
27+
echo '${{ github.event.comment.body }}'
28+
```
29+
30+
The following example uses an environment variable, but **still allows the injection** because of the use of expression syntax:
31+
32+
```yaml
33+
on: issue_comment
34+
35+
jobs:
36+
echo-body:
37+
runs-on: ubuntu-latest
38+
steps:
39+
- env:
40+
BODY: ${{ github.event.issue.body }}
41+
run: |
42+
echo '${{ env.BODY }}'
43+
```
44+
45+
### Correct Usage
46+
47+
The following example uses shell syntax to read the environment variable and will prevent the attack:
48+
49+
```yaml
50+
jobs:
51+
echo-body:
52+
runs-on: ubuntu-latest
53+
steps:
54+
- env:
55+
BODY: ${{ github.event.issue.body }}
56+
run: |
57+
echo "$BODY"
58+
```
59+
60+
The following example uses `process.env` to read environment variables within JavaScript code.
61+
62+
```yaml
63+
jobs:
64+
echo-body:
65+
runs-on: ubuntu-latest
66+
steps:
67+
- uses: uses: actions/github-script@v4
68+
env:
69+
BODY: ${{ github.event.issue.body }}
70+
with:
71+
script: |
72+
const { BODY } = process.env
73+
...
74+
```
75+
76+
## References
77+
78+
- GitHub Security Lab Research: [Keeping your GitHub Actions and workflows secure: Untrusted input](https://securitylab.github.com/research/github-actions-untrusted-input).
79+
- GitHub Docs: [Security hardening for GitHub Actions](https://docs.github.com/en/actions/security-guides/security-hardening-for-github-actions).
80+
- GitHub Docs: [Permissions for the GITHUB_TOKEN](https://docs.github.com/en/actions/security-guides/automatic-token-authentication#permissions-for-the-github_token).
Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,26 @@
1+
/**
2+
* @name Code injection
3+
* @description Interpreting unsanitized user input as code allows a malicious user to perform arbitrary
4+
* code execution.
5+
* @kind path-problem
6+
* @problem.severity warning
7+
* @security-severity 5.0
8+
* @precision low
9+
* @id actions/code-injection/low
10+
* @tags actions
11+
* security
12+
* external/cwe/cwe-094
13+
* external/cwe/cwe-095
14+
* external/cwe/cwe-116
15+
*/
16+
17+
import actions
18+
import codeql.actions.security.CodeInjectionQuery
19+
import CodeInjectionFromStepOutputFlow::PathGraph
20+
21+
from
22+
CodeInjectionFromStepOutputFlow::PathNode source, CodeInjectionFromStepOutputFlow::PathNode sink
23+
where lowSeverityCodeInjection(source, sink)
24+
select sink.getNode(), source, sink,
25+
"Potential code injection in $@, which may be controlled by an external user because it comes from a step output.",
26+
sink, sink.getNode().asExpr().(Expression).getRawExpression()

0 commit comments

Comments
 (0)