Skip to content

Commit 42d4bb5

Browse files
author
Alvaro Muñoz
committed
Better identification of checkout of untrusted code depending on the triggering events
1 parent 8f350d9 commit 42d4bb5

11 files changed

Lines changed: 250 additions & 144 deletions

File tree

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

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -90,7 +90,8 @@ class GitCommandSource extends RemoteFlowSource, CommandSource {
9090
checkout = uses and
9191
uses.getCallee() = "actions/checkout" and
9292
exists(uses.getArgument("ref")) and
93-
not uses.getArgument("ref").matches("%base%")
93+
not uses.getArgument("ref").matches("%base%") and
94+
uses.getEnclosingJob().getATriggerEvent().getName() = checkoutTriggers()
9495
)
9596
or
9697
checkout instanceof GitMutableRefCheckout
@@ -237,7 +238,8 @@ private class CheckoutSource extends RemoteFlowSource, FileSource {
237238
this.asExpr() = uses and
238239
uses.getCallee() = "actions/checkout" and
239240
exists(uses.getArgument("ref")) and
240-
not uses.getArgument("ref").matches("%base%")
241+
not uses.getArgument("ref").matches("%base%") and
242+
uses.getEnclosingJob().getATriggerEvent().getName() = checkoutTriggers()
241243
)
242244
or
243245
this.asExpr() instanceof GitMutableRefCheckout

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

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,8 @@ class OutputClobberingFromFileReadSink extends OutputClobberingSink {
2929
step = uses and
3030
uses.getCallee() = "actions/checkout" and
3131
exists(uses.getArgument("ref")) and
32-
not uses.getArgument("ref").matches("%base%")
32+
not uses.getArgument("ref").matches("%base%") and
33+
uses.getEnclosingJob().getATriggerEvent().getName() = checkoutTriggers()
3334
)
3435
or
3536
step instanceof GitMutableRefCheckout

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

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -17,12 +17,13 @@ class PoisonableCommandStep extends PoisonableStep, Run {
1717

1818
class JavascriptImportUsesStep extends PoisonableStep, UsesStep {
1919
JavascriptImportUsesStep() {
20-
exists(string script, string line, string import_stmt |
20+
exists(string script, string line |
2121
this.getCallee() = "actions/github-script" and
2222
script = this.getArgument("script") and
2323
line = script.splitAt("\n").trim() and
24-
import_stmt = line.regexpCapture(".*await\\s+import\\((.*)\\).*", 1) and
25-
import_stmt.regexpMatch(".*\\bgithub.workspace\\b.*")
24+
// const script = require('${{ github.workspace }}/scripts/test.js');
25+
// await script({ github, context, core });
26+
line.regexpMatch(".*(import|require)\\b.*github.workspace\\b.*")
2627
)
2728
}
2829
}

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

Lines changed: 100 additions & 127 deletions
Original file line numberDiff line numberDiff line change
@@ -3,49 +3,57 @@ private import codeql.actions.DataFlow
33
private import codeql.actions.dataflow.FlowSources
44
private import codeql.actions.TaintTracking
55

6+
string checkoutTriggers() {
7+
result = ["pull_request_target", "workflow_run", "workflow_call", "issue_comment"]
8+
}
9+
610
/**
711
* A taint-tracking configuration for PR HEAD references flowing
812
* into actions/checkout's ref argument.
913
*/
1014
private module ActionsMutableRefCheckoutConfig implements DataFlow::ConfigSig {
1115
predicate isSource(DataFlow::Node source) {
12-
// remote flow sources
13-
source instanceof ArtifactSource
14-
or
15-
source instanceof GitHubCtxSource
16-
or
17-
source instanceof GitHubEventCtxSource
18-
or
19-
source instanceof GitHubEventJsonSource
20-
or
21-
source instanceof MaDSource
22-
or
23-
// `ref` argument contains the PR id/number or head ref
24-
exists(Expression e |
25-
source.asExpr() = e and
26-
(
27-
containsHeadRef(e.getExpression()) or
28-
containsPullRequestNumber(e.getExpression())
16+
source.asExpr().getEnclosingJob().getATriggerEvent().getName() = checkoutTriggers() and
17+
(
18+
// remote flow sources
19+
source instanceof ArtifactSource
20+
or
21+
source instanceof GitHubCtxSource
22+
or
23+
source instanceof GitHubEventCtxSource
24+
or
25+
source instanceof GitHubEventJsonSource
26+
or
27+
source instanceof MaDSource
28+
or
29+
// `ref` argument contains the PR id/number or head ref
30+
exists(Expression e |
31+
source.asExpr() = e and
32+
(
33+
containsHeadRef(e.getExpression()) or
34+
containsPullRequestNumber(e.getExpression())
35+
)
2936
)
30-
)
31-
or
32-
// 3rd party actions returning the PR head ref
33-
exists(StepsExpression e, UsesStep step |
34-
source.asExpr() = e and
35-
e.getStepId() = step.getId() and
36-
(
37-
step.getCallee() = "eficode/resolve-pr-refs" and e.getFieldName() = "head_ref"
38-
or
39-
step.getCallee() = "xt0rted/pull-request-comment-branch" and e.getFieldName() = "head_ref"
40-
or
41-
step.getCallee() = "alessbell/pull-request-comment-branch" and e.getFieldName() = "head_ref"
42-
or
43-
step.getCallee() = "gotson/pull-request-comment-branch" and e.getFieldName() = "head_ref"
44-
or
45-
step.getCallee() = "potiuk/get-workflow-origin" and
46-
e.getFieldName() = ["sourceHeadBranch", "pullRequestNumber"]
47-
or
48-
step.getCallee() = "github/branch-deploy" and e.getFieldName() = ["ref", "fork_ref"]
37+
or
38+
// 3rd party actions returning the PR head ref
39+
exists(StepsExpression e, UsesStep step |
40+
source.asExpr() = e and
41+
e.getStepId() = step.getId() and
42+
(
43+
step.getCallee() = "eficode/resolve-pr-refs" and e.getFieldName() = "head_ref"
44+
or
45+
step.getCallee() = "xt0rted/pull-request-comment-branch" and e.getFieldName() = "head_ref"
46+
or
47+
step.getCallee() = "alessbell/pull-request-comment-branch" and
48+
e.getFieldName() = "head_ref"
49+
or
50+
step.getCallee() = "gotson/pull-request-comment-branch" and e.getFieldName() = "head_ref"
51+
or
52+
step.getCallee() = "potiuk/get-workflow-origin" and
53+
e.getFieldName() = ["sourceHeadBranch", "pullRequestNumber"]
54+
or
55+
step.getCallee() = "github/branch-deploy" and e.getFieldName() = ["ref", "fork_ref"]
56+
)
4957
)
5058
)
5159
}
@@ -71,27 +79,32 @@ module ActionsMutableRefCheckoutFlow = TaintTracking::Global<ActionsMutableRefCh
7179

7280
private module ActionsSHACheckoutConfig implements DataFlow::ConfigSig {
7381
predicate isSource(DataFlow::Node source) {
74-
// `ref` argument contains the PR head/merge commit sha
75-
exists(Expression e |
76-
source.asExpr() = e and
77-
containsHeadSHA(e.getExpression())
78-
)
79-
or
80-
// 3rd party actions returning the PR head sha
81-
exists(StepsExpression e, UsesStep step |
82-
source.asExpr() = e and
83-
e.getStepId() = step.getId() and
84-
(
85-
step.getCallee() = "eficode/resolve-pr-refs" and e.getFieldName() = "head_sha"
86-
or
87-
step.getCallee() = "xt0rted/pull-request-comment-branch" and e.getFieldName() = "head_sha"
88-
or
89-
step.getCallee() = "alessbell/pull-request-comment-branch" and e.getFieldName() = "head_sha"
90-
or
91-
step.getCallee() = "gotson/pull-request-comment-branch" and e.getFieldName() = "head_sha"
92-
or
93-
step.getCallee() = "potiuk/get-workflow-origin" and
94-
e.getFieldName() = ["sourceHeadSha", "mergeCommitSha"]
82+
source.asExpr().getEnclosingJob().getATriggerEvent().getName() =
83+
["pull_request_target", "workflow_run", "workflow_call", "issue_comment"] and
84+
(
85+
// `ref` argument contains the PR head/merge commit sha
86+
exists(Expression e |
87+
source.asExpr() = e and
88+
containsHeadSHA(e.getExpression())
89+
)
90+
or
91+
// 3rd party actions returning the PR head sha
92+
exists(StepsExpression e, UsesStep step |
93+
source.asExpr() = e and
94+
e.getStepId() = step.getId() and
95+
(
96+
step.getCallee() = "eficode/resolve-pr-refs" and e.getFieldName() = "head_sha"
97+
or
98+
step.getCallee() = "xt0rted/pull-request-comment-branch" and e.getFieldName() = "head_sha"
99+
or
100+
step.getCallee() = "alessbell/pull-request-comment-branch" and
101+
e.getFieldName() = "head_sha"
102+
or
103+
step.getCallee() = "gotson/pull-request-comment-branch" and e.getFieldName() = "head_sha"
104+
or
105+
step.getCallee() = "potiuk/get-workflow-origin" and
106+
e.getFieldName() = ["sourceHeadSha", "mergeCommitSha"]
107+
)
95108
)
96109
)
97110
}
@@ -201,44 +214,24 @@ class ActionsMutableRefCheckout extends MutableRefCheckoutStep instanceof UsesSt
201214
ActionsMutableRefCheckoutFlow::PathNode source, ActionsMutableRefCheckoutFlow::PathNode sink
202215
|
203216
ActionsMutableRefCheckoutFlow::flowPath(source, sink) and
204-
sink.getNode().asExpr() = this.getArgumentExpr(["ref", "repository"]) and
205-
(
206-
not source.getNode() instanceof GitHubEventCtxSource
207-
or
208-
source.getNode() instanceof GitHubEventCtxSource and
209-
// the context is available for the job trigger events
210-
exists(string context, string context_prefix |
211-
contextTriggerDataModel(this.getEnclosingWorkflow().getATriggerEvent().getName(),
212-
context_prefix) and
213-
context = source.getNode().(GitHubEventCtxSource).getContext() and
214-
normalizeExpr(context).matches("%" + context_prefix + "%")
215-
)
216-
)
217+
sink.getNode().asExpr() = this.getArgumentExpr(["ref", "repository"])
217218
)
218219
or
219220
// heuristic base on the step id and field name
220-
exists(StepsExpression e |
221-
this.getArgumentExpr("ref") = e and
222-
(
223-
e.getStepId().matches("%" + ["head", "branch", "ref"] + "%") or
224-
e.getFieldName().matches("%" + ["head", "branch", "ref"] + "%")
225-
)
226-
)
227-
or
228-
exists(NeedsExpression e |
229-
this.getArgumentExpr("ref") = e and
230-
(
231-
e.getNeededJobId().matches("%" + ["head", "branch", "ref"] + "%") or
232-
e.getFieldName().matches("%" + ["head", "branch", "ref"] + "%")
233-
)
234-
)
235-
or
236-
exists(JsonReferenceExpression e |
237-
this.getArgumentExpr("ref") = e and
238-
(
239-
e.getAccessPath().matches("%." + ["head", "branch", "ref"] + "%") or
240-
e.getInnerExpression().matches("%." + ["head", "branch", "ref"] + "%")
241-
)
221+
exists(string value |
222+
this.getArgumentExpr("ref")
223+
.(SimpleReferenceExpression)
224+
.getEnclosingJob()
225+
.getATriggerEvent()
226+
.getName() = checkoutTriggers() and
227+
value.regexpMatch(".*(head|branch|ref).*")
228+
|
229+
this.getArgumentExpr("ref").(StepsExpression).getStepId() = value or
230+
this.getArgumentExpr("ref").(StepsExpression).getFieldName() = value or
231+
this.getArgumentExpr("ref").(NeedsExpression).getNeededJobId() = value or
232+
this.getArgumentExpr("ref").(NeedsExpression).getFieldName() = value or
233+
this.getArgumentExpr("ref").(JsonReferenceExpression).getAccessPath() = value or
234+
this.getArgumentExpr("ref").(JsonReferenceExpression).getInnerExpression() = value
242235
)
243236
)
244237
}
@@ -257,44 +250,24 @@ class ActionsSHACheckout extends SHACheckoutStep instanceof UsesStep {
257250
(
258251
exists(ActionsSHACheckoutFlow::PathNode source, ActionsSHACheckoutFlow::PathNode sink |
259252
ActionsSHACheckoutFlow::flowPath(source, sink) and
260-
sink.getNode().asExpr() = this.getArgumentExpr(["ref", "repository"]) and
261-
(
262-
not source.getNode() instanceof GitHubEventCtxSource
263-
or
264-
source.getNode() instanceof GitHubEventCtxSource and
265-
// the context is available for the job trigger events
266-
exists(string context, string context_prefix |
267-
contextTriggerDataModel(this.getEnclosingWorkflow().getATriggerEvent().getName(),
268-
context_prefix) and
269-
context = source.getNode().(GitHubEventCtxSource).getContext() and
270-
normalizeExpr(context).matches("%" + context_prefix + "%")
271-
)
272-
)
253+
sink.getNode().asExpr() = this.getArgumentExpr(["ref", "repository"])
273254
)
274255
or
275256
// heuristic base on the step id and field name
276-
exists(StepsExpression e |
277-
this.getArgumentExpr("ref") = e and
278-
(
279-
e.getStepId().matches("%" + ["head", "sha", "commit"] + "%") or
280-
e.getFieldName().matches("%" + ["head", "sha", "commit"] + "%")
281-
)
282-
)
283-
or
284-
exists(NeedsExpression e |
285-
this.getArgumentExpr("ref") = e and
286-
(
287-
e.getNeededJobId().matches("%" + ["head", "sha", "commit"] + "%") or
288-
e.getFieldName().matches("%" + ["head", "sha", "commit"] + "%")
289-
)
290-
)
291-
or
292-
exists(JsonReferenceExpression e |
293-
this.getArgumentExpr("ref") = e and
294-
(
295-
e.getAccessPath().matches("%." + ["head", "sha", "commit"] + "%") or
296-
e.getInnerExpression().matches("%." + ["head", "sha", "commit"] + "%")
297-
)
257+
exists(string value |
258+
this.getArgumentExpr("ref")
259+
.(SimpleReferenceExpression)
260+
.getEnclosingJob()
261+
.getATriggerEvent()
262+
.getName() = checkoutTriggers() and
263+
value.regexpMatch(".*(head|sha|commit).*")
264+
|
265+
this.getArgumentExpr("ref").(StepsExpression).getStepId() = value or
266+
this.getArgumentExpr("ref").(StepsExpression).getFieldName() = value or
267+
this.getArgumentExpr("ref").(NeedsExpression).getNeededJobId() = value or
268+
this.getArgumentExpr("ref").(NeedsExpression).getFieldName() = value or
269+
this.getArgumentExpr("ref").(JsonReferenceExpression).getAccessPath() = value or
270+
this.getArgumentExpr("ref").(JsonReferenceExpression).getInnerExpression() = value
298271
)
299272
)
300273
}

‎ql/src/Security/CWE-829/UntrustedCheckoutCritical.ql‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -47,6 +47,7 @@ where
4747
) and
4848
// the checkout occurs in a privileged context
4949
inPrivilegedContext(poisonable, event) and
50+
inPrivilegedContext(checkout, event) and
5051
not exists(ControlCheck check | check.protects(checkout, event, "untrusted-checkout")) and
5152
not exists(ControlCheck check | check.protects(poisonable, event, "untrusted-checkout"))
5253
select poisonable, checkout, poisonable,

‎ql/test/query-tests/Security/CWE-829/.github/workflows/test21.yml‎

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,27 @@
1+
on:
2+
push:
3+
branches: [main]
4+
pull_request:
5+
branches: [main]
6+
workflow_dispatch:
7+
inputs:
8+
publish_docs:
9+
description: "pub"
10+
default: true
11+
type: boolean
12+
13+
jobs:
14+
Docs:
15+
if: github.repository == 'test/test'
16+
runs-on: macos-14
17+
steps:
18+
- name: Checkout Repository
19+
uses: actions/checkout@v4
20+
with:
21+
repository: ${{ github.event.pull_request.head.repo.full_name || github.repository }}
22+
token: ${{ secrets.GITHUB_TOKEN }}
23+
ref: ${{ github.head_ref || github.ref }}
24+
fetch-depth: 0
25+
- run: |
26+
# NOT VULNERABLE
27+
python docs/build_docs.py

0 commit comments

Comments
 (0)