Skip to content

Commit ddb7e3f

Browse files
aschackmullaschackmull
authored
Merge pull request #22243 from JarLob/improve/actions-untrusted-checkout-source-link
Actions: Improve `actions/cache-poisoning/poisonable-step` and `actions/untrusted-checkout/critical` path UI
2 parents 0fb9774 + a44c25c commit ddb7e3f

8 files changed

Lines changed: 269 additions & 65 deletions

File tree

‎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-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()

‎actions/ql/src/change-notes/2026-07-28-checkout-provenance.md‎

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.

‎actions/ql/test/query-tests/Security/CWE-349/.github/workflows/cache_write_capable_workflow_dispatch.yml‎

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,23 @@
1+
on: workflow_dispatch
2+
3+
jobs:
4+
cache:
5+
permissions: {}
6+
runs-on: ubuntu-latest
7+
steps:
8+
- id: pr
9+
env:
10+
HEAD_SHA: ${{ github.event.inputs.head_sha }}
11+
run: |
12+
jq -cn --arg sha "$HEAD_SHA" '{head: {sha: $sha}}' |
13+
sed 's/^/json=/' >> "$GITHUB_OUTPUT"
14+
- env:
15+
HEAD_SHA: ${{ fromJSON(steps.pr.outputs.json).head.sha }}
16+
run: |
17+
git fetch origin "$HEAD_SHA"
18+
git checkout "$HEAD_SHA"
19+
- run: npm install
20+
- uses: actions/cache@v4
21+
with:
22+
path: .npm
23+
key: workflow-dispatch

‎actions/ql/test/query-tests/Security/CWE-349/CachePoisoningViaDirectCache.expected‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,7 @@
11
edges
2+
| .github/workflows/cache_write_capable_workflow_dispatch.yml:8:9:14:6 | Run Step: pr | .github/workflows/cache_write_capable_workflow_dispatch.yml:14:9:19:6 | Run Step |
3+
| .github/workflows/cache_write_capable_workflow_dispatch.yml:14:9:19:6 | Run Step | .github/workflows/cache_write_capable_workflow_dispatch.yml:19:9:20:6 | Run Step |
4+
| .github/workflows/cache_write_capable_workflow_dispatch.yml:19:9:20:6 | Run Step | .github/workflows/cache_write_capable_workflow_dispatch.yml:20:9:23:33 | Uses Step |
25
| .github/workflows/code_injection2.yml:12:9:16:6 | Uses Step: modified_files | .github/workflows/code_injection2.yml:16:9:16:71 | Run Step |
36
| .github/workflows/direct_cache1.yml:10:9:13:6 | Uses Step: comment-branch | .github/workflows/direct_cache1.yml:13:9:18:6 | Uses Step |
47
| .github/workflows/direct_cache1.yml:13:9:18:6 | Uses Step | .github/workflows/direct_cache1.yml:18:9:22:6 | Uses Step |

0 commit comments

Comments
 (0)