Skip to content

Commit e2347a5

Browse files
JarLobJarLob
committed
Fix for independent checks
1 parent 7f16853 commit e2347a5

3 files changed

Lines changed: 94 additions & 28 deletions

File tree

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

Lines changed: 44 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -66,7 +66,22 @@ abstract class ControlCheck extends AstNode {
6666
// The check is effective against the event and category
6767
this.protectsCategoryAndEvent(category, event.getName()) and
6868
// The check can be triggered by the event
69-
this.getATriggerEvent() = event
69+
this.getATriggerEvent() = event and
70+
// For reusable workflows, ALL callers for this event must be protected by SOME check
71+
(
72+
not node.getEnclosingWorkflow() instanceof ReusableWorkflow
73+
or
74+
forall(ExternalJob directCaller |
75+
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+
)
83+
)
84+
)
7085
}
7186

7287
/**
@@ -103,35 +118,36 @@ abstract class ControlCheck extends AstNode {
103118
node.getEnclosingJob().getANeededJob().(LocalJob).getAStep() = this
104119
)
105120
or
106-
// When the node is inside a (possibly nested) reusable workflow,
107-
// all direct callers for this event must be protected along their caller chain.
108-
exists(ExternalJob directCaller |
109-
directCaller = node.getEnclosingWorkflow().(ReusableWorkflow).getACaller() and
110-
directCaller.getATriggerEvent() = event
111-
) and
112-
forall(ExternalJob directCaller |
113-
directCaller = node.getEnclosingWorkflow().(ReusableWorkflow).getACaller() and
114-
directCaller.getATriggerEvent() = event
115-
|
116-
exists(ExternalJob caller |
117-
caller = getAnOuterCaller*(directCaller) and
121+
// When the node is inside a reusable workflow,
122+
// this check dominates via at least one caller chain.
123+
this.dominatesViaCaller(node, event, _)
124+
}
125+
126+
/**
127+
* Holds if this control check dominates `node` in a reusable workflow
128+
* via the caller chain starting at `directCaller`.
129+
*/
130+
predicate dominatesViaCaller(AstNode node, Event event, ExternalJob directCaller) {
131+
directCaller = node.getEnclosingWorkflow().(ReusableWorkflow).getACaller() and
132+
directCaller.getATriggerEvent() = event and
133+
exists(ExternalJob caller |
134+
caller = getAnOuterCaller*(directCaller) and
135+
(
136+
this instanceof If and
137+
(
138+
caller.getIf() = this or
139+
caller.getANeededJob().(LocalJob).getIf() = this or
140+
caller.getANeededJob().(LocalJob).getAStep().getIf() = this
141+
)
142+
or
143+
this instanceof Environment and
118144
(
119-
this instanceof If and
120-
(
121-
caller.getIf() = this or
122-
caller.getANeededJob().(LocalJob).getIf() = this or
123-
caller.getANeededJob().(LocalJob).getAStep().getIf() = this
124-
)
125-
or
126-
this instanceof Environment and
127-
(
128-
caller.getEnvironment() = this or
129-
caller.getANeededJob().getEnvironment() = this
130-
)
131-
or
132-
(this instanceof Run or this instanceof UsesStep) and
133-
caller.getANeededJob().(LocalJob).getAStep() = this
145+
caller.getEnvironment() = this or
146+
caller.getANeededJob().getEnvironment() = this
134147
)
148+
or
149+
(this instanceof Run or this instanceof UsesStep) and
150+
caller.getANeededJob().(LocalJob).getAStep() = this
135151
)
136152
)
137153
}

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

Lines changed: 48 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,48 @@
1+
on:
2+
pull_request_target:
3+
4+
jobs:
5+
is-collaborator-a:
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+
caller-a:
23+
needs: is-collaborator-a
24+
uses: TestOrg/TestRepo/.github/workflows/build.yml@main
25+
with:
26+
COMMIT_SHA: ${{ github.event.pull_request.head.sha }}
27+
is-collaborator-b:
28+
runs-on: ubuntu-latest
29+
steps:
30+
- name: Get User Permission
31+
id: checkAccess
32+
uses: actions-cool/check-user-permission@cd622002ff25c2311d2e7fb82107c0d24be83f9b
33+
with:
34+
require: write
35+
username: ${{ github.actor }}
36+
env:
37+
GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
38+
- name: Check User Permission
39+
if: steps.checkAccess.outputs.require-result == 'false'
40+
run: |
41+
echo "${{ github.actor }} does not have permissions on this repo."
42+
echo "Current permission level is ${{ steps.checkAccess.outputs.user-permission }}"
43+
exit 1
44+
caller-b:
45+
needs: is-collaborator-b
46+
uses: TestOrg/TestRepo/.github/workflows/build.yml@main
47+
with:
48+
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
@@ -344,6 +344,8 @@ edges
344344
| .github/workflows/untrusted_checkout_permissions_check.yml:8:9:16:6 | Uses Step: checkAccess | .github/workflows/untrusted_checkout_permissions_check.yml:16:9:22:2 | Run Step |
345345
| .github/workflows/untrusted_checkout_permissions_check.yml:26:9:31:6 | Uses Step | .github/workflows/untrusted_checkout_permissions_check.yml:31:9:32:2 | Run Step |
346346
| .github/workflows/untrusted_checkout_permissions_check.yml:36:9:41:6 | Uses Step | .github/workflows/untrusted_checkout_permissions_check.yml:41:9:41:22 | Run Step |
347+
| .github/workflows/untrusted_checkout_two_callers_both_protected.yml:8:9:16:6 | Uses Step: checkAccess | .github/workflows/untrusted_checkout_two_callers_both_protected.yml:16:9:22:2 | Run Step |
348+
| .github/workflows/untrusted_checkout_two_callers_both_protected.yml:30:9:38:6 | Uses Step: checkAccess | .github/workflows/untrusted_checkout_two_callers_both_protected.yml:38:9:44:2 | Run Step |
347349
| .github/workflows/workflow_run_untrusted_checkout.yml:13:9:16:6 | Uses Step | .github/workflows/workflow_run_untrusted_checkout.yml:16:9:18:31 | Uses Step |
348350
| .github/workflows/workflow_run_untrusted_checkout_2.yml:13:9:16:6 | Uses Step | .github/workflows/workflow_run_untrusted_checkout_2.yml:16:9:18:31 | Uses Step |
349351
| .github/workflows/workflow_run_untrusted_checkout_3.yml:13:9:16:6 | Uses Step | .github/workflows/workflow_run_untrusted_checkout_3.yml:16:9:18:31 | Uses Step |

0 commit comments

Comments
 (0)