Skip to content

Commit b67644c

Browse files
redsun82redsun82
authored
Merge pull request #21986 from JarLob/userpermissions
Actions: Fix dominates() false positive in reusable workflows
2 parents b582844 + 7fc4b48 commit b67644c

16 files changed

Lines changed: 480 additions & 22 deletions

‎actions/ql/lib/change-notes/2026-06-15-permission_check.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+
* GitHub Actions queries now better account for permission checks on jobs that call reusable workflows.

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

Lines changed: 155 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -42,6 +42,15 @@ string actor_not_attacker_event() {
4242
]
4343
}
4444

45+
/**
46+
* Gets the outer caller of `ej`, i.e. the `ExternalJob` that calls the
47+
* reusable workflow containing `ej`. Used with transitive closure to
48+
* walk up nested reusable workflow chains.
49+
*/
50+
private ExternalJob getAnOuterCaller(ExternalJob ej) {
51+
result = ej.getEnclosingWorkflow().(ReusableWorkflow).getACaller()
52+
}
53+
4554
/** An If node that contains an actor, user or label check */
4655
abstract class ControlCheck extends AstNode {
4756
ControlCheck() {
@@ -53,43 +62,170 @@ abstract class ControlCheck extends AstNode {
5362

5463
predicate protects(AstNode node, Event event, string category) {
5564
// The check dominates the step it should protect
56-
this.dominates(node) and
65+
this.dominates(node, event) and
5766
// The check is effective against the event and category
5867
this.protectsCategoryAndEvent(category, event.getName()) and
5968
// The check can be triggered by the event
60-
this.getATriggerEvent() = event
69+
this.getATriggerEvent() = event and
70+
// For reusable workflows, there must be no unprotected caller chain for this event.
71+
(
72+
not node.getEnclosingWorkflow() instanceof ReusableWorkflow
73+
or
74+
this.dominatesSameWorkflow(node, event)
75+
or
76+
not exists(ExternalJob directCaller |
77+
directCaller = node.getEnclosingWorkflow().(ReusableWorkflow).getACaller() and
78+
unprotectedCallerChain(directCaller, event, category)
79+
)
80+
)
6181
}
6282

63-
predicate dominates(AstNode node) {
83+
/**
84+
* Holds if this control check must execute and pass before `node` can run.
85+
*/
86+
predicate dominates(AstNode node, Event event) {
87+
this.dominatesSameWorkflow(node, event)
88+
or
89+
// When the node is inside a reusable workflow,
90+
// this check dominates via at least one caller chain.
91+
this.dominatesViaCaller(node, event, _)
92+
}
93+
94+
/**
95+
* Holds if this control check dominates `node` within the same workflow.
96+
*/
97+
predicate dominatesSameWorkflow(AstNode node, Event event) {
98+
this.getATriggerEvent() = event and
99+
(
100+
// Step-level: the check is an `if:` on the step containing `node`,
101+
// or on the enclosing job, or on a needed job/step.
102+
this instanceof If and
103+
(
104+
node.getEnclosingStep().getIf() = this or
105+
node.getEnclosingJob().getIf() = this or
106+
node.getEnclosingJob().getANeededJob().(LocalJob).getAStep().getIf() = this or
107+
node.getEnclosingJob().getANeededJob().(LocalJob).getIf() = this
108+
)
109+
or
110+
// Job-level: the check is an environment on the enclosing job or a needed job.
111+
this instanceof Environment and
112+
(
113+
node.getEnclosingJob().getEnvironment() = this
114+
or
115+
node.getEnclosingJob().getANeededJob().getEnvironment() = this
116+
)
117+
or
118+
// Step-level: the check is a Run/UsesStep that precedes `node`'s step
119+
// in the same job, or is a step in a needed job.
120+
(
121+
this instanceof Run or
122+
this instanceof UsesStep
123+
) and
124+
(
125+
this.(Step).getAFollowingStep() = node.getEnclosingStep()
126+
or
127+
node.getEnclosingJob().getANeededJob().(LocalJob).getAStep() = this
128+
)
129+
)
130+
}
131+
132+
/**
133+
* Holds if this control check dominates `node` in a reusable workflow
134+
* via the caller chain starting at `directCaller`.
135+
*/
136+
predicate dominatesViaCaller(AstNode node, Event event, ExternalJob directCaller) {
137+
directCaller = node.getEnclosingWorkflow().(ReusableWorkflow).getACaller() and
138+
directCaller.getATriggerEvent() = event and
139+
exists(ExternalJob caller |
140+
caller = getAnOuterCaller*(directCaller) and
141+
this.dominatesCaller(caller)
142+
)
143+
}
144+
145+
/**
146+
* Holds if this control check directly dominates `caller`.
147+
*/
148+
predicate dominatesCaller(ExternalJob caller) {
64149
this instanceof If and
65150
(
66-
node.getEnclosingStep().getIf() = this or
67-
node.getEnclosingJob().getIf() = this or
68-
node.getEnclosingJob().getANeededJob().(LocalJob).getAStep().getIf() = this or
69-
node.getEnclosingJob().getANeededJob().(LocalJob).getIf() = this
151+
caller.getIf() = this or
152+
caller.getANeededJob().(LocalJob).getIf() = this or
153+
caller.getANeededJob().(LocalJob).getAStep().getIf() = this
70154
)
71155
or
72156
this instanceof Environment and
73157
(
74-
node.getEnclosingJob().getEnvironment() = this
75-
or
76-
node.getEnclosingJob().getANeededJob().getEnvironment() = this
158+
caller.getEnvironment() = this or
159+
caller.getANeededJob().getEnvironment() = this
77160
)
78161
or
79-
(
80-
this instanceof Run or
81-
this instanceof UsesStep
82-
) and
83-
(
84-
this.(Step).getAFollowingStep() = node.getEnclosingStep()
85-
or
86-
node.getEnclosingJob().getANeededJob().(LocalJob).getAStep() = this.(Step)
87-
)
162+
(this instanceof Run or this instanceof UsesStep) and
163+
caller.getANeededJob().(LocalJob).getAStep() = this
88164
}
89165

90166
abstract predicate protectsCategoryAndEvent(string category, string event);
91167
}
92168

169+
/**
170+
* Holds if this control check directly protects `caller`.
171+
*/
172+
bindingset[caller, event, category]
173+
private predicate protectedCaller(ExternalJob caller, Event event, string category) {
174+
exists(ControlCheck check |
175+
check.protectsCategoryAndEvent(category, event.getName()) and
176+
check.getATriggerEvent() = event and
177+
check.dominatesCaller(caller)
178+
)
179+
}
180+
181+
cached
182+
private newtype TCallerState =
183+
MkCallerState(ExternalJob caller, Event event, string category) {
184+
caller.getATriggerEvent() = event and
185+
category = any_category()
186+
}
187+
188+
private class CallerState extends TCallerState, MkCallerState {
189+
ExternalJob caller;
190+
Event event;
191+
string category;
192+
193+
CallerState() { this = MkCallerState(caller, event, category) }
194+
195+
ExternalJob getCaller() { result = caller }
196+
197+
Event getEvent() { result = event }
198+
199+
string getCategory() { result = category }
200+
201+
/**
202+
* Gets an outer caller state if this caller is not protected.
203+
*/
204+
CallerState getUnprotectedOuterState() {
205+
not protectedCaller(this.getCaller(), this.getEvent(), this.getCategory()) and
206+
result = MkCallerState(getAnOuterCaller(this.getCaller()), this.getEvent(), this.getCategory())
207+
}
208+
209+
predicate isUnprotectedOutermost() {
210+
not protectedCaller(this.getCaller(), this.getEvent(), this.getCategory()) and
211+
not exists(getAnOuterCaller(this.getCaller()))
212+
}
213+
214+
string toString() { result = caller + " / " + event + " / " + category }
215+
}
216+
217+
/**
218+
* Holds if there is a caller path from `caller` to an outer workflow that has no protection.
219+
*/
220+
bindingset[caller, event, category]
221+
private predicate unprotectedCallerChain(ExternalJob caller, Event event, string category) {
222+
exists(CallerState start, CallerState outermost |
223+
start = MkCallerState(caller, event, category) and
224+
outermost = start.getUnprotectedOuterState*() and
225+
outermost.isUnprotectedOutermost()
226+
)
227+
}
228+
93229
abstract class AssociationCheck extends ControlCheck {
94230
// Checks if the actor is a MEMBER/OWNER the repo
95231
// - they are effective against pull requests and workflow_run (since these are triggered by pull_requests) since they can control who is making the PR

‎actions/ql/src/Security/CWE-285/ImproperAccessControl.ql‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,7 @@ from LocalJob job, LabelCheck check, MutableRefCheckoutStep checkout, Event even
1818
where
1919
job.isPrivileged() and
2020
job.getAStep() = checkout and
21-
check.dominates(checkout) and
21+
check.dominates(checkout, event) and
2222
(
2323
job.getATriggerEvent() = event and
2424
event.getName() = "pull_request_target" and

‎actions/ql/src/Security/CWE-829/UntrustedCheckoutHigh.ql‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -34,8 +34,8 @@ where
3434
check instanceof AssociationCheck or
3535
check instanceof PermissionCheck
3636
) and
37-
check.dominates(checkout) and
38-
date_check.dominates(checkout)
37+
check.dominates(checkout, event) and
38+
date_check.dominates(checkout, event)
3939
)
4040
or
4141
// not issue_comment triggered workflows

‎actions/ql/test/query-tests/Security/CWE-829/.github/workflows/external/TestOrg/TestRepo/.github/workflows/build.yml‎

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,17 @@
1+
on:
2+
workflow_call:
3+
inputs:
4+
COMMIT_SHA:
5+
type: string
6+
7+
jobs:
8+
build:
9+
runs-on: ubuntu-latest
10+
steps:
11+
- uses: actions/checkout@v6
12+
with:
13+
ref: ${{ inputs.COMMIT_SHA }}
14+
- run: |
15+
npm install
16+
npm run lint
17+

‎actions/ql/test/query-tests/Security/CWE-829/.github/workflows/external/TestOrg/TestRepo/.github/workflows/build_nested.yml‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,13 @@
1+
on:
2+
workflow_call:
3+
inputs:
4+
COMMIT_SHA:
5+
type: string
6+
7+
jobs:
8+
build:
9+
uses: TestOrg/TestRepo/.github/workflows/build.yml@main
10+
with:
11+
COMMIT_SHA: ${{ inputs.COMMIT_SHA }}
12+
13+

‎actions/ql/test/query-tests/Security/CWE-829/.github/workflows/external/TestOrg/TestRepo/.github/workflows/build_nested_branching.yml‎

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,33 @@
1+
on:
2+
workflow_call:
3+
inputs:
4+
COMMIT_SHA:
5+
type: string
6+
7+
jobs:
8+
is-collaborator:
9+
runs-on: ubuntu-latest
10+
steps:
11+
- name: Get User Permission
12+
id: checkAccess
13+
uses: actions-cool/check-user-permission@cd622002ff25c2311d2e7fb82107c0d24be83f9b
14+
with:
15+
require: write
16+
username: ${{ github.actor }}
17+
env:
18+
GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
19+
- name: Check User Permission
20+
if: steps.checkAccess.outputs.require-result == 'false'
21+
run: |
22+
echo "${{ github.actor }} does not have permissions on this repo."
23+
echo "Current permission level is ${{ steps.checkAccess.outputs.user-permission }}"
24+
exit 1
25+
build_safe:
26+
needs: is-collaborator
27+
uses: TestOrg/TestRepo/.github/workflows/build_nested.yml@main
28+
with:
29+
COMMIT_SHA: ${{ inputs.COMMIT_SHA }}
30+
build_unsafe:
31+
uses: TestOrg/TestRepo/.github/workflows/build_nested.yml@main
32+
with:
33+
COMMIT_SHA: ${{ inputs.COMMIT_SHA }}

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

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,31 @@
1+
on:
2+
pull_request_target:
3+
4+
jobs:
5+
is-collaborator:
6+
runs-on: ubuntu-latest
7+
steps:
8+
- name: Get User Permission
9+
id: checkAccess
10+
uses: actions-cool/check-user-permission@cd622002ff25c2311d2e7fb82107c0d24be83f9b
11+
with:
12+
require: write
13+
username: ${{ github.actor }}
14+
env:
15+
GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
16+
- name: Check User Permission
17+
if: steps.checkAccess.outputs.require-result == 'false'
18+
run: |
19+
echo "${{ github.actor }} does not have permissions on this repo."
20+
echo "Current permission level is ${{ steps.checkAccess.outputs.user-permission }}"
21+
exit 1
22+
build:
23+
runs-on: ubuntu-latest
24+
#needs: is-collaborator Mistake, doesn't wait for the collaborator - no security check
25+
steps:
26+
- name: Checkout repo
27+
uses: actions/checkout@4
28+
with:
29+
ref: ${{ github.event.pull_request.head.sha }} # should alert
30+
fetch-depth: 2
31+
- run: yarn test

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

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,26 @@
1+
on:
2+
pull_request_target:
3+
4+
jobs:
5+
is-collaborator:
6+
runs-on: ubuntu-latest
7+
steps:
8+
- name: Get User Permission
9+
id: checkAccess
10+
uses: actions-cool/check-user-permission@cd622002ff25c2311d2e7fb82107c0d24be83f9b
11+
with:
12+
require: write
13+
username: ${{ github.actor }}
14+
env:
15+
GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
16+
- name: Check User Permission
17+
if: steps.checkAccess.outputs.require-result == 'false'
18+
run: |
19+
echo "${{ github.actor }} does not have permissions on this repo."
20+
echo "Current permission level is ${{ steps.checkAccess.outputs.user-permission }}"
21+
exit 1
22+
build:
23+
needs: is-collaborator
24+
uses: TestOrg/TestRepo/.github/workflows/build.yml@main
25+
with:
26+
COMMIT_SHA: ${{ github.event.pull_request.head.sha }} # shouldn't alert since permission check

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

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,31 @@
1+
on:
2+
pull_request_target:
3+
4+
jobs:
5+
is-collaborator:
6+
runs-on: ubuntu-latest
7+
steps:
8+
- name: Get User Permission
9+
id: checkAccess
10+
uses: actions-cool/check-user-permission@cd622002ff25c2311d2e7fb82107c0d24be83f9b
11+
with:
12+
require: write
13+
username: ${{ github.actor }}
14+
env:
15+
GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
16+
- name: Check User Permission
17+
if: steps.checkAccess.outputs.require-result == 'false'
18+
run: |
19+
echo "${{ github.actor }} does not have permissions on this repo."
20+
echo "Current permission level is ${{ steps.checkAccess.outputs.user-permission }}"
21+
exit 1
22+
build_unsafe:
23+
# needs: is-collaborator
24+
uses: TestOrg/TestRepo/.github/workflows/build.yml@main
25+
with:
26+
COMMIT_SHA: ${{ github.event.pull_request.head.sha }} # should alert since no permission check
27+
build_safe:
28+
needs: is-collaborator
29+
uses: TestOrg/TestRepo/.github/workflows/build.yml@main
30+
with:
31+
COMMIT_SHA: ${{ github.event.pull_request.head.sha }} # shouldn't alert since permission check

0 commit comments

Comments
 (0)