Skip to content

Commit 4b8cb3f

Browse files
redsun82Copilot
redsun82
and
Copilot
committed
Fix false negative for branching nested reusable workflows
The previous fix required all outermost callers of a reusable workflow to be protected, which collapsed distinct safe/unsafe inner paths that share the same outermost caller. Track protection per caller chain instead: a node inside a reusable workflow is only considered protected if there is no unprotected caller path up to an outer workflow. Adds a branching nested regression test where one inner job is protected by a permission check and a sibling inner job is not. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1 parent 31f6e71 commit 4b8cb3f

4 files changed

Lines changed: 140 additions & 31 deletions

File tree

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

Lines changed: 97 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -67,19 +67,15 @@ abstract class ControlCheck extends AstNode {
6767
this.protectsCategoryAndEvent(category, event.getName()) and
6868
// The check can be triggered by the event
6969
this.getATriggerEvent() = event and
70-
// For reusable workflows, ALL callers for this event must be protected by SOME check
70+
// For reusable workflows, there must be no unprotected caller chain for this event.
7171
(
7272
not node.getEnclosingWorkflow() instanceof ReusableWorkflow
7373
or
74-
forall(ExternalJob directCaller |
74+
this.dominatesSameWorkflow(node, event)
75+
or
76+
not exists(ExternalJob directCaller |
7577
directCaller = node.getEnclosingWorkflow().(ReusableWorkflow).getACaller() and
76-
directCaller.getATriggerEvent() = event
77-
|
78-
exists(ControlCheck check |
79-
check.protectsCategoryAndEvent(category, event.getName()) and
80-
check.getATriggerEvent() = event and
81-
check.dominatesViaCaller(node, event, directCaller)
82-
)
78+
unprotectedCallerChain(directCaller, event, category)
8379
)
8480
)
8581
}
@@ -88,7 +84,17 @@ abstract class ControlCheck extends AstNode {
8884
* Holds if this control check must execute and pass before `node` can run.
8985
*/
9086
predicate dominates(AstNode node, Event event) {
91-
// Same-workflow dominance: bind event to this check's trigger 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) {
9298
this.getATriggerEvent() = event and
9399
(
94100
// Step-level: the check is an `if:` on the step containing `node`,
@@ -121,10 +127,6 @@ abstract class ControlCheck extends AstNode {
121127
node.getEnclosingJob().getANeededJob().(LocalJob).getAStep() = this
122128
)
123129
)
124-
or
125-
// When the node is inside a reusable workflow,
126-
// this check dominates via at least one caller chain.
127-
this.dominatesViaCaller(node, event, _)
128130
}
129131

130132
/**
@@ -136,29 +138,93 @@ abstract class ControlCheck extends AstNode {
136138
directCaller.getATriggerEvent() = event and
137139
exists(ExternalJob caller |
138140
caller = getAnOuterCaller*(directCaller) and
139-
(
140-
this instanceof If and
141-
(
142-
caller.getIf() = this or
143-
caller.getANeededJob().(LocalJob).getIf() = this or
144-
caller.getANeededJob().(LocalJob).getAStep().getIf() = this
145-
)
146-
or
147-
this instanceof Environment and
148-
(
149-
caller.getEnvironment() = this or
150-
caller.getANeededJob().getEnvironment() = this
151-
)
152-
or
153-
(this instanceof Run or this instanceof UsesStep) and
154-
caller.getANeededJob().(LocalJob).getAStep() = this
155-
)
141+
this.dominatesCaller(caller)
142+
)
143+
}
144+
145+
/**
146+
* Holds if this control check directly dominates `caller`.
147+
*/
148+
predicate dominatesCaller(ExternalJob caller) {
149+
this instanceof If and
150+
(
151+
caller.getIf() = this or
152+
caller.getANeededJob().(LocalJob).getIf() = this or
153+
caller.getANeededJob().(LocalJob).getAStep().getIf() = this
154+
)
155+
or
156+
this instanceof Environment and
157+
(
158+
caller.getEnvironment() = this or
159+
caller.getANeededJob().getEnvironment() = this
156160
)
161+
or
162+
(this instanceof Run or this instanceof UsesStep) and
163+
caller.getANeededJob().(LocalJob).getAStep() = this
157164
}
158165

159166
abstract predicate protectsCategoryAndEvent(string category, string event);
160167
}
161168

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 = MkCallerState(ExternalJob caller, Event event, string category) {
183+
caller.getATriggerEvent() = event and
184+
category = any_category()
185+
}
186+
187+
private class CallerState extends TCallerState, MkCallerState {
188+
ExternalJob caller;
189+
Event event;
190+
string category;
191+
192+
CallerState() { this = MkCallerState(caller, event, category) }
193+
194+
ExternalJob getCaller() { result = caller }
195+
196+
Event getEvent() { result = event }
197+
198+
string getCategory() { result = category }
199+
200+
/**
201+
* Gets an outer caller state if this caller is not protected.
202+
*/
203+
CallerState getUnprotectedOuterState() {
204+
not protectedCaller(this.getCaller(), this.getEvent(), this.getCategory()) and
205+
result = MkCallerState(getAnOuterCaller(this.getCaller()), this.getEvent(), this.getCategory())
206+
}
207+
208+
predicate isUnprotectedOutermost() {
209+
not protectedCaller(this.getCaller(), this.getEvent(), this.getCategory()) and
210+
not exists(getAnOuterCaller(this.getCaller()))
211+
}
212+
213+
string toString() { result = caller + " / " + event + " / " + category }
214+
}
215+
216+
/**
217+
* Holds if there is a caller path from `caller` to an outer workflow that has no protection.
218+
*/
219+
bindingset[caller, event, category]
220+
private predicate unprotectedCallerChain(ExternalJob caller, Event event, string category) {
221+
exists(CallerState start, CallerState outermost |
222+
start = MkCallerState(caller, event, category) and
223+
outermost = start.getUnprotectedOuterState*() and
224+
outermost.isUnprotectedOutermost()
225+
)
226+
}
227+
162228
abstract class AssociationCheck extends ControlCheck {
163229
// Checks if the actor is a MEMBER/OWNER the repo
164230
// - 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/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_permission_check_reusable_branching_nested.yml‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,8 @@
1+
on:
2+
pull_request_target:
3+
4+
jobs:
5+
build:
6+
uses: TestOrg/TestRepo/.github/workflows/build_nested_branching.yml@main
7+
with:
8+
COMMIT_SHA: ${{ github.event.pull_request.head.sha }}

‎actions/ql/test/query-tests/Security/CWE-829/UntrustedCheckoutCritical.expected‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -94,6 +94,7 @@ edges
9494
| .github/workflows/dependabot3.yml:20:9:25:6 | Uses Step | .github/workflows/dependabot3.yml:25:9:48:6 | Run Step: set-milestone |
9595
| .github/workflows/dependabot3.yml:25:9:48:6 | Run Step: set-milestone | .github/workflows/dependabot3.yml:48:9:52:57 | Run Step |
9696
| .github/workflows/external/TestOrg/TestRepo/.github/workflows/build.yml:11:9:14:6 | Uses Step | .github/workflows/external/TestOrg/TestRepo/.github/workflows/build.yml:14:9:17:7 | Run Step |
97+
| .github/workflows/external/TestOrg/TestRepo/.github/workflows/build_nested_branching.yml:11:9:19:6 | Uses Step: checkAccess | .github/workflows/external/TestOrg/TestRepo/.github/workflows/build_nested_branching.yml:19:9:25:2 | Run Step |
9798
| .github/workflows/external/TestOrg/TestRepo/.github/workflows/formal.yml:14:9:19:6 | Uses Step | .github/workflows/external/TestOrg/TestRepo/.github/workflows/formal.yml:19:9:25:6 | Run Step |
9899
| .github/workflows/external/TestOrg/TestRepo/.github/workflows/formal.yml:19:9:25:6 | Run Step | .github/workflows/external/TestOrg/TestRepo/.github/workflows/formal.yml:25:9:70:20 | Run Step |
99100
| .github/workflows/external/TestOrg/TestRepo/.github/workflows/reusable.yml:23:9:26:6 | Uses Step | .github/workflows/external/TestOrg/TestRepo/.github/workflows/reusable.yml:26:9:29:7 | Run Step |
@@ -357,6 +358,7 @@ edges
357358
| .github/workflows/auto_ci.yml:67:9:74:6 | Uses Step | .github/workflows/auto_ci.yml:67:9:74:6 | Uses Step | .github/workflows/auto_ci.yml:84:9:93:6 | Run Step | Checkout of untrusted code in a privileged workflow with later potential execution (event trigger: $@). | .github/workflows/auto_ci.yml:6:3:6:21 | pull_request_target | pull_request_target |
358359
| .github/workflows/dependabot3.yml:15:9:20:6 | Uses Step | .github/workflows/dependabot3.yml:15:9:20:6 | Uses Step | .github/workflows/dependabot3.yml:25:9:48:6 | Run Step: set-milestone | Checkout of untrusted code in a privileged workflow with later potential execution (event trigger: $@). | .github/workflows/dependabot3.yml:3:5:3:23 | pull_request_target | pull_request_target |
359360
| .github/workflows/external/TestOrg/TestRepo/.github/workflows/build.yml:11:9:14:6 | Uses Step | .github/workflows/external/TestOrg/TestRepo/.github/workflows/build.yml:11:9:14:6 | Uses Step | .github/workflows/external/TestOrg/TestRepo/.github/workflows/build.yml:14:9:17:7 | Run Step | Checkout of untrusted code in a privileged workflow with later potential execution (event trigger: $@). | .github/workflows/untrusted_checkout_permission_check_reusable2.yml:2:3:2:21 | pull_request_target | pull_request_target |
361+
| .github/workflows/external/TestOrg/TestRepo/.github/workflows/build.yml:11:9:14:6 | Uses Step | .github/workflows/external/TestOrg/TestRepo/.github/workflows/build.yml:11:9:14:6 | Uses Step | .github/workflows/external/TestOrg/TestRepo/.github/workflows/build.yml:14:9:17:7 | Run Step | Checkout of untrusted code in a privileged workflow with later potential execution (event trigger: $@). | .github/workflows/untrusted_checkout_permission_check_reusable_branching_nested.yml:2:3:2:21 | pull_request_target | pull_request_target |
360362
| .github/workflows/external/TestOrg/TestRepo/.github/workflows/build.yml:11:9:14:6 | Uses Step | .github/workflows/external/TestOrg/TestRepo/.github/workflows/build.yml:11:9:14:6 | Uses Step | .github/workflows/external/TestOrg/TestRepo/.github/workflows/build.yml:14:9:17:7 | Run Step | Checkout of untrusted code in a privileged workflow with later potential execution (event trigger: $@). | .github/workflows/untrusted_checkout_permission_check_reusable_no_needs.yml:2:3:2:21 | pull_request_target | pull_request_target |
361363
| .github/workflows/external/TestOrg/TestRepo/.github/workflows/reusable.yml:23:9:26:6 | Uses Step | .github/workflows/external/TestOrg/TestRepo/.github/workflows/reusable.yml:23:9:26:6 | Uses Step | .github/workflows/external/TestOrg/TestRepo/.github/workflows/reusable.yml:26:9:29:7 | Run Step | Checkout of untrusted code in a privileged workflow with later potential execution (event trigger: $@). | .github/workflows/reusable_caller1.yaml:4:3:4:21 | pull_request_target | pull_request_target |
362364
| .github/workflows/gitcheckout.yml:10:11:18:8 | Run Step | .github/workflows/gitcheckout.yml:10:11:18:8 | Run Step | .github/workflows/gitcheckout.yml:21:11:23:22 | Run Step | Checkout of untrusted code in a privileged workflow with later potential execution (event trigger: $@). | .github/workflows/gitcheckout.yml:2:3:2:21 | pull_request_target | pull_request_target |

0 commit comments

Comments
 (0)