Skip to content

Commit 5d5b0c7

Browse files
authored
Merge pull request #22260 from JarLob/fix/actions-cache-poisoning-cache-write-access
Actions: Fix actions cache poisoning queries to account for the latest changes in cache write access
2 parents bd36dea + 6a2d7be commit 5d5b0c7

21 files changed

Lines changed: 280 additions & 139 deletions

‎actions/ql/lib/codeql/actions/dataflow/FlowSources.qll‎

Lines changed: 45 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -52,29 +52,43 @@ class GitHubCtxSource extends RemoteFlowSource {
5252
override string getEventName() { result = event }
5353
}
5454

55+
bindingset[expression]
56+
pragma[inline_late]
57+
private predicate untrustedEventProperty(Expression expression, string kind) {
58+
exists(string regexp |
59+
untrustedEventPropertiesDataModel(regexp, kind) and
60+
kind != "json" and
61+
normalizeExpr(expression.getExpression()).regexpMatch("(?i)\\s*" + wrapRegexp(regexp) + ".*")
62+
)
63+
}
64+
65+
bindingset[expression, event]
66+
pragma[inline_late]
67+
private predicate expressionContainsEventContext(Expression expression, string event) {
68+
exists(string contextPrefix |
69+
contextTriggerDataModel(event, contextPrefix) and
70+
normalizeExpr(expression.getExpression()).matches("%" + contextPrefix + "%")
71+
)
72+
}
73+
5574
class GitHubEventCtxSource extends RemoteFlowSource {
5675
string flag;
5776
string context;
5877
string event;
5978

6079
GitHubEventCtxSource() {
61-
exists(Expression e, string regexp |
80+
exists(Expression e |
6281
this.asExpr() = e and
6382
context = e.getExpression() and
6483
(
6584
// the context is available for the job trigger events
6685
event = e.getATriggerEvent().getName() and
67-
exists(string context_prefix |
68-
contextTriggerDataModel(event, context_prefix) and
69-
normalizeExpr(context).matches("%" + context_prefix + "%")
70-
)
86+
expressionContainsEventContext(e, event)
7187
or
7288
not exists(e.getATriggerEvent()) and
7389
event = "unknown"
7490
) and
75-
untrustedEventPropertiesDataModel(regexp, flag) and
76-
not flag = "json" and
77-
normalizeExpr(context).regexpMatch("(?i)\\s*" + wrapRegexp(regexp) + ".*")
91+
untrustedEventProperty(e, flag)
7892
)
7993
}
8094

@@ -177,32 +191,41 @@ class GitHubEventPathSource extends RemoteFlowSource, CommandSource {
177191
override Run getEnclosingRun() { result = run }
178192
}
179193

194+
bindingset[expression, event]
195+
pragma[inline_late]
196+
private predicate jsonSourceForEvent(Expression expression, string event) {
197+
exists(string context, string regexp, string contextPrefix |
198+
context = expression.getExpression() and
199+
untrustedEventPropertiesDataModel(regexp, _) and
200+
contextTriggerDataModel(event, contextPrefix) and
201+
normalizeExpr(context).matches("%" + contextPrefix + "%") and
202+
normalizeExpr(context).regexpMatch("(?i).*" + wrapJsonRegexp(regexp) + ".*")
203+
)
204+
or
205+
exists(string context |
206+
context = expression.getExpression() and
207+
untrustedEventPropertiesDataModel(_, _) and
208+
contextTriggerDataModel(event, _) and
209+
normalizeExpr(context).regexpMatch("(?i).*" + wrapJsonRegexp("\\bgithub.event\\b") + ".*")
210+
)
211+
}
212+
180213
class GitHubEventJsonSource extends RemoteFlowSource {
181214
string flag;
182215
string event;
183216

184217
GitHubEventJsonSource() {
185-
exists(Expression e, string context, string regexp |
218+
exists(Expression e |
186219
this.asExpr() = e and
187-
context = e.getExpression() and
188-
untrustedEventPropertiesDataModel(regexp, _) and
189220
(
190221
// only contexts for the triggering events are considered tainted.
191222
// eg: for `pull_request`, we only consider `github.event.pull_request`
192223
event = e.getEnclosingWorkflow().getATriggerEvent().getName() and
193-
exists(string context_prefix |
194-
contextTriggerDataModel(event, context_prefix) and
195-
normalizeExpr(context).matches("%" + context_prefix + "%")
196-
) and
197-
normalizeExpr(context).regexpMatch("(?i).*" + wrapJsonRegexp(regexp) + ".*")
198-
or
199-
// github.event is tainted for all triggers
200-
event = e.getEnclosingWorkflow().getATriggerEvent().getName() and
201-
contextTriggerDataModel(e.getEnclosingWorkflow().getATriggerEvent().getName(), _) and
202-
normalizeExpr(context).regexpMatch("(?i).*" + wrapJsonRegexp("\\bgithub.event\\b") + ".*")
224+
jsonSourceForEvent(e, event)
203225
or
204226
not exists(e.getATriggerEvent()) and
205-
event = "unknown"
227+
event = "unknown" and
228+
untrustedEventPropertiesDataModel(_, _)
206229
) and
207230
flag = "json"
208231
)

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

Lines changed: 23 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -5,8 +5,8 @@ string defaultBranchTriggerEvent() {
55
[
66
"check_run", "check_suite", "delete", "discussion", "discussion_comment", "fork", "gollum",
77
"issue_comment", "issues", "label", "milestone", "project", "project_card", "project_column",
8-
"public", "pull_request_comment", "pull_request_target", "repository_dispatch", "schedule",
9-
"watch", "workflow_run"
8+
"public", "pull_request_comment", "pull_request_target", "repository_dispatch",
9+
"registry_package", "page_build", "schedule", "watch", "workflow_dispatch", "workflow_run"
1010
]
1111
}
1212

@@ -42,6 +42,27 @@ predicate runsOnDefaultBranch(Event e) {
4242
)
4343
}
4444

45+
private string defaultBranchCacheWriteEvent() {
46+
result =
47+
[
48+
"push", "workflow_dispatch", "repository_dispatch", "delete", "registry_package",
49+
"page_build", "schedule"
50+
]
51+
}
52+
53+
private predicate eventHasDefaultBranchCacheWriteAccess(Event event) {
54+
runsOnDefaultBranch(event) and event.getName() = defaultBranchCacheWriteEvent()
55+
}
56+
57+
/**
58+
* Holds if `job` can write to the cache scope of the default branch for `event`.
59+
* Reusable workflow jobs inherit their caller's trigger event.
60+
*/
61+
predicate hasDefaultBranchCacheWriteAccess(LocalJob job, Event event) {
62+
job.getATriggerEvent() = event and
63+
eventHasDefaultBranchCacheWriteAccess(event)
64+
}
65+
4566
abstract class CacheWritingStep extends Step {
4667
abstract string getPath();
4768
}

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

Lines changed: 1 addition & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -29,22 +29,10 @@ Event getRelevantCachePoisoningEventForSink(DataFlow::Node sink) {
2929
exists(LocalJob job |
3030
job = sink.asExpr().getEnclosingJob() and
3131
job.getATriggerEvent() = result and
32-
// job can be triggered by an external user
33-
result.isExternallyTriggerable() and
3432
// excluding privileged workflows since they can be exploited in easier circumstances
3533
// which is covered by `actions/code-injection/critical`
3634
not job.isPrivilegedExternallyTriggerable(result) and
37-
(
38-
// the workflow runs in the context of the default branch
39-
runsOnDefaultBranch(result)
40-
or
41-
// the workflow caller runs in the context of the default branch
42-
result.getName() = "workflow_call" and
43-
exists(ExternalJob caller |
44-
caller.getCallee() = job.getLocation().getFile().getRelativePath() and
45-
runsOnDefaultBranch(caller.getATriggerEvent())
46-
)
47-
)
35+
hasDefaultBranchCacheWriteAccess(job, result)
4836
)
4937
}
5038

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

Lines changed: 19 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -34,48 +34,55 @@ Due to the above design, if something is cached in the context of the default br
3434

3535
## Example
3636

37+
GitHub gives workflows triggered by low-trust events, such as `issue_comment`,
38+
`pull_request_target`, and `workflow_run`, read-only access to the default branch cache scope.
39+
This query therefore reports only workflows whose trigger can write to that scope.
40+
3741
### Incorrect Usage
3842

39-
The following workflow is vulnerable to code injection in a non-privileged job but in the context of the default branch.
43+
The following workflow interpolates a commit message directly into a script on a push to the
44+
default branch. A commit message originating from a merged contribution may contain shell syntax,
45+
which can expose the cache write token and allow the default branch cache to be poisoned.
4046

4147
```yaml
4248
name: Vulnerable Workflow
4349
on:
44-
issue_comment:
45-
types: [created]
50+
push:
51+
branches: [main]
4652

4753
jobs:
48-
pr-comment:
54+
build:
4955
permissions: {}
5056
runs-on: ubuntu-latest
5157
steps:
5258
- run: |
53-
echo ${{ github.event.comment.body }}
59+
echo ${{ github.event.head_commit.message }}
5460
```
5561
5662
### Correct Usage
5763
58-
The following workflow is not vulnerable to code injections even if it runs in the context of the default branch.
64+
The following workflow passes the commit message through an environment variable, so the shell
65+
does not interpret its contents as code.
5966
6067
```yaml
6168
name: Secure Workflow
6269
on:
63-
issue_comment:
64-
types: [created]
70+
push:
71+
branches: [main]
6572

6673
jobs:
67-
pr-comment:
74+
build:
6875
permissions: {}
6976
runs-on: ubuntu-latest
7077
steps:
7178
- env:
72-
BODY: ${{ github.event.comment.body }}
79+
MESSAGE: ${{ github.event.head_commit.message }}
7380
run: |
74-
echo "$BODY"
81+
echo "$MESSAGE"
7582
```
7683
7784
## References
7885
7986
- Adnan Khan's Blog: [The Monsters in Your Build Cache – GitHub Actions Cache Poisoning](https://adnanthekhan.com/2024/05/06/the-monsters-in-your-build-cache-github-actions-cache-poisoning/).
80-
- GitHub Docs: [GitHub Actions Caching Documentation](https://docs.github.com/en/actions/using-workflows/caching-dependencies-to-speed-up-workflows).
87+
- GitHub Docs: [Cache access for low-trust workflow triggers](https://docs.github.com/actions/reference/workflows-and-actions/dependency-caching#cache-access-for-low-trust-workflow-triggers).
8188
- Scribe Security Blog: [Cache Poisoning in GitHub Actions](https://scribesecurity.com/blog/github-cache-poisoning/).

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

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@ from CodeInjectionFlow::PathNode source, CodeInjectionFlow::PathNode sink, Event
2222
where
2323
CodeInjectionFlow::flowPath(source, sink) and
2424
event = getRelevantCachePoisoningEventForSink(sink.getNode()) and
25+
source.getNode().(RemoteFlowSource).getEventName() = event.getName() and
2526
// the checkout is not controlled by an access check
2627
not exists(ControlCheck check |
2728
check.protects(source.getNode().asExpr(), event, "code-injection")

‎actions/ql/src/Security/CWE-349/CachePoisoningViaDirectCache.md‎

Lines changed: 21 additions & 45 deletions
Original file line numberDiff line numberDiff line change
@@ -34,35 +34,39 @@ Due to the above design, if something is cached in the context of the default br
3434

3535
## Example
3636

37+
GitHub gives workflows triggered by low-trust events, such as `issue_comment`,
38+
`pull_request_target`, and `workflow_run`, read-only access to the default branch cache scope.
39+
This query therefore reports only workflows whose trigger can write to that scope.
40+
3741
### Incorrect Usage
3842

39-
The following workflow is caching an attacker-controlled file (`large_file`) in the context of the default branch.
43+
The following write-capable manually dispatched workflow accepts a revision without validation,
44+
fetches files from it, and saves those files in the default branch cache. This is unsafe if an
45+
untrusted integration or automation can influence the dispatch input.
4046

4147
```yaml
4248
name: Vulnerable Workflow
4349
on:
44-
issue_comment:
45-
types: [created]
50+
workflow_dispatch:
51+
inputs:
52+
head_sha:
53+
required: true
4654

4755
jobs:
48-
pr-comment:
56+
cache:
4957
permissions: read-all
5058
runs-on: ubuntu-latest
5159
steps:
52-
- uses: xt0rted/pull-request-comment-branch@v2
53-
id: comment-branch
54-
- uses: actions/checkout@v3
55-
with:
56-
ref: ${{ steps.comment-branch.outputs.head_sha }}
57-
- name: Set up Python 3.10
58-
uses: actions/setup-python@v5
59-
- name: Cache pip dependencies
60+
- env:
61+
HEAD_SHA: ${{ github.event.inputs.head_sha }}
62+
run: |
63+
git fetch origin "$HEAD_SHA"
64+
git checkout "$HEAD_SHA"
65+
- name: Cache fetched files
6066
uses: actions/cache@v4
61-
id: cache-pip
6267
with:
63-
path: ~/.cache/pip
64-
key: ${{ runner.os }}-pip-${{ hashFiles('**/pyproject.toml') }}
65-
restore-keys: ${{ runner.os }}-pip-
68+
path: .
69+
key: dispatched-${{ github.event.inputs.head_sha }}
6670
```
6771
6872
### Correct Usage
@@ -91,36 +95,8 @@ jobs:
9195
restore-keys: ${{ runner.os }}-pip-
9296
```
9397
94-
Note, that the example above doesn't allow using secrets if the Pull Request originates from a fork. In case secrets are needed, `pull_request_target` with labels as `safe to test` can be used, but the code in Pull Request must be manually reviewed before applying the label.
95-
96-
```yaml
97-
name: Secure Workflow
98-
on:
99-
pull_request_target:
100-
types: [labeled]
101-
102-
jobs:
103-
pr-comment:
104-
if: contains(github.event.pull_request.labels.*.name, 'safe to test')
105-
permissions: read-all
106-
runs-on: ubuntu-latest
107-
steps:
108-
- uses: actions/checkout@v3
109-
with:
110-
ref: ${{ github.event.pull_request.head.sha}}
111-
- name: Set up Python 3.10
112-
uses: actions/setup-python@v5
113-
- name: Cache pip dependencies
114-
uses: actions/cache@v4
115-
id: cache-pip
116-
with:
117-
path: ~/.cache/pip
118-
key: ${{ runner.os }}-pip-${{ hashFiles('**/pyproject.toml') }}
119-
restore-keys: ${{ runner.os }}-pip-
120-
```
121-
12298
## References
12399
124100
- Adnan Khan's Blog: [The Monsters in Your Build Cache – GitHub Actions Cache Poisoning](https://adnanthekhan.com/2024/05/06/the-monsters-in-your-build-cache-github-actions-cache-poisoning/).
125-
- GitHub Docs: [GitHub Actions Caching Documentation](https://docs.github.com/en/actions/using-workflows/caching-dependencies-to-speed-up-workflows).
101+
- GitHub Docs: [Cache access for low-trust workflow triggers](https://docs.github.com/actions/reference/workflows-and-actions/dependency-caching#cache-access-for-low-trust-workflow-triggers).
126102
- Scribe Security Blog: [Cache Poisoning in GitHub Actions](https://scribesecurity.com/blog/github-cache-poisoning/).

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

Lines changed: 1 addition & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -40,17 +40,7 @@ where
4040
job.getATriggerEvent() = event and
4141
// job can be triggered by an external user
4242
event.isExternallyTriggerable() and
43-
(
44-
// the workflow runs in the context of the default branch
45-
runsOnDefaultBranch(event)
46-
or
47-
// the workflow's caller runs in the context of the default branch
48-
event.getName() = "workflow_call" and
49-
exists(ExternalJob caller |
50-
caller.getCallee() = job.getLocation().getFile().getRelativePath() and
51-
runsOnDefaultBranch(caller.getATriggerEvent())
52-
)
53-
) and
43+
hasDefaultBranchCacheWriteAccess(job, event) and
5444
// the job writes to the cache
5545
// (No need to follow the checkout/download step since the cache is normally write after the job completes)
5646
job.getAStep() = step and

0 commit comments

Comments
 (0)