Skip to content

Commit 16a7522

Browse files
author
Alvaro Muñoz
committed
Improve Untrusted checkout queries
1 parent 33ae3b1 commit 16a7522

File tree

11 files changed

+90
-17
lines changed

11 files changed

+90
-17
lines changed

‎ql/lib/codeql/actions/ast/internal/Ast.qll

Lines changed: 22 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -748,7 +748,13 @@ class JobImpl extends AstNodeImpl, TJobNode {
748748

749749
/** Holds if the job can be triggered by an external actor. */
750750
predicate isExternallyTriggerable() {
751-
externallyTriggerableEventsDataModel(this.getATriggerEvent().getName())
751+
// the job is triggered by an event that can be triggered externally
752+
externallyTriggerableEventsDataModel(this.getATriggerEvent().getName()) or
753+
// the job is triggered by a workflow_call event that can be triggered externally
754+
this.getATriggerEvent().getName() = "workflow_call" and
755+
(exists(ExpressionImpl e, string external_trigger | e.getEnclosingJob() = this and e.getExpression().matches("%github.event" + external_trigger + "%") and externallyTriggerableEventsDataModel(external_trigger))
756+
or
757+
this.getEnclosingWorkflow().(ReusableWorkflowImpl).getACaller().isExternallyTriggerable())
752758
}
753759

754760
/** Holds if the job is privileged. */
@@ -775,7 +781,9 @@ class JobImpl extends AstNodeImpl, TJobNode {
775781
private predicate hasExplicitSecretAccess() {
776782
// the job accesses a secret other than GITHUB_TOKEN
777783
exists(SecretsExpressionImpl expr |
778-
expr.getEnclosingJob() = this and not expr.getFieldName() = "GITHUB_TOKEN"
784+
(expr.getEnclosingJob() = this or not exists(expr.getEnclosingJob())) and
785+
expr.getEnclosingWorkflow() = this.getEnclosingWorkflow() and
786+
not expr.getFieldName() = "GITHUB_TOKEN"
779787
)
780788
}
781789

@@ -803,16 +811,21 @@ class JobImpl extends AstNodeImpl, TJobNode {
803811
}
804812

805813
private predicate hasPrivilegedTrigger() {
806-
// For workflows that are triggered by the pull_request_target event, the GITHUB_TOKEN is granted read/write repository permission unless the permissions key is specified and the workflow can access secrets, even when it is triggered from a fork.
807-
// The Job is triggered by an event other than `pull_request`
814+
// the Job is triggered by an event other than `pull_request`
808815
count(this.getATriggerEvent()) = 1 and
809-
not this.getATriggerEvent().getName() = ["pull_request", "workflow_call"]
816+
not this.getATriggerEvent().getName() = "pull_request" and
817+
not this.getATriggerEvent().getName() = "workflow_call"
810818
or
811-
// The Workflow is a Reusable Workflow only and there is
812-
// a privileged caller workflow
813-
this.getEnclosingWorkflow().(ReusableWorkflowImpl).getACaller().isPrivileged()
819+
// the Workflow is a Reusable Workflow only and there is
820+
// a privileged caller workflow or we cant find a caller
821+
count(this.getATriggerEvent()) = 1 and
822+
this.getATriggerEvent().getName() = "workflow_call" and
823+
(
824+
this.getEnclosingWorkflow().(ReusableWorkflowImpl).getACaller().isPrivileged() or
825+
not exists(this.getEnclosingWorkflow().(ReusableWorkflowImpl).getACaller())
826+
)
814827
or
815-
// The Workflow has multiple triggers so at least one is not "pull_request"
828+
// the Workflow has multiple triggers so at least one is not "pull_request"
816829
count(this.getATriggerEvent()) > 1
817830
}
818831

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

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -108,8 +108,8 @@ class ActionsMutableRefCheckout extends MutableRefCheckoutStep instanceof UsesSt
108108
exists(StepsExpression e |
109109
this.getArgumentExpr("ref") = e and
110110
(
111-
e.getStepId().matches(["%head%", "%pull_request%", "%_pr_%"]) or
112-
e.getFieldName().matches(["%head%", "%pull_request%", "%_pr_%"])
111+
e.getStepId().matches("%" + ["head", "branch", "ref"] + "%") or
112+
e.getFieldName().matches("%" + ["head", "branch", "ref"] + "%")
113113
)
114114
)
115115
)
@@ -138,8 +138,8 @@ class ActionsSHACheckout extends SHACheckoutStep instanceof UsesStep {
138138
exists(StepsExpression e |
139139
this.getArgumentExpr("ref") = e and
140140
(
141-
e.getStepId().matches(["%sha%", "%commit%"]) or
142-
e.getFieldName().matches(["%sha%", "%commit%"])
141+
e.getStepId().matches("%" + ["head", "sha", "commit"] + "%") or
142+
e.getFieldName().matches("%" + ["head", "sha", "commit"] + "%")
143143
)
144144
)
145145
)

‎ql/lib/qlpack.yml

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@
22
library: true
33
warnOnImplicitThis: true
44
name: githubsecuritylab/actions-all
5-
version: 0.0.28
5+
version: 0.0.29
66
dependencies:
77
codeql/util: ^0.2.0
88
codeql/yaml: ^0.1.2

‎ql/src/qlpack.yml

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
---
22
library: false
33
name: githubsecuritylab/actions-queries
4-
version: 0.0.28
4+
version: 0.0.29
55
groups:
66
- actions
77
- queries

‎ql/test/query-tests/Security/CWE-094/CodeInjectionCritical.expected

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -315,6 +315,8 @@ subpaths
315315
| .github/workflows/pull_request_target.yml:14:19:14:69 | github.event.pull_request.head.repo.homepage | .github/workflows/pull_request_target.yml:14:19:14:69 | github.event.pull_request.head.repo.homepage | .github/workflows/pull_request_target.yml:14:19:14:69 | github.event.pull_request.head.repo.homepage | Potential code injection in $@, which may be controlled by an external user. | .github/workflows/pull_request_target.yml:14:19:14:69 | github.event.pull_request.head.repo.homepage | ${{ github.event.pull_request.head.repo.homepage }} |
316316
| .github/workflows/pull_request_target.yml:15:19:15:59 | github.event.pull_request.head.ref | .github/workflows/pull_request_target.yml:15:19:15:59 | github.event.pull_request.head.ref | .github/workflows/pull_request_target.yml:15:19:15:59 | github.event.pull_request.head.ref | Potential code injection in $@, which may be controlled by an external user. | .github/workflows/pull_request_target.yml:15:19:15:59 | github.event.pull_request.head.ref | ${{ github.event.pull_request.head.ref }} |
317317
| .github/workflows/pull_request_target.yml:16:19:16:40 | github.head_ref | .github/workflows/pull_request_target.yml:16:19:16:40 | github.head_ref | .github/workflows/pull_request_target.yml:16:19:16:40 | github.head_ref | Potential code injection in $@, which may be controlled by an external user. | .github/workflows/pull_request_target.yml:16:19:16:40 | github.head_ref | ${{ github.head_ref }} |
318+
| .github/workflows/reusable-workflow-1.yml:53:26:53:39 | env.log | .github/workflows/reusable-workflow-1.yml:44:19:44:56 | github.event.pull_request.title | .github/workflows/reusable-workflow-1.yml:53:26:53:39 | env.log | Potential code injection in $@, which may be controlled by an external user. | .github/workflows/reusable-workflow-1.yml:53:26:53:39 | env.log | ${{ env.log }} |
319+
| .github/workflows/reusable-workflow-2.yml:53:26:53:39 | env.log | .github/workflows/reusable-workflow-2.yml:44:19:44:56 | github.event.pull_request.title | .github/workflows/reusable-workflow-2.yml:53:26:53:39 | env.log | Potential code injection in $@, which may be controlled by an external user. | .github/workflows/reusable-workflow-2.yml:53:26:53:39 | env.log | ${{ env.log }} |
318320
| .github/workflows/self_needs.yml:19:15:19:47 | steps.source.outputs.value | .github/workflows/self_needs.yml:16:20:16:57 | github.event['comment']['body'] | .github/workflows/self_needs.yml:19:15:19:47 | steps.source.outputs.value | Potential code injection in $@, which may be controlled by an external user. | .github/workflows/self_needs.yml:19:15:19:47 | steps.source.outputs.value | ${{ steps.source.outputs.value }} |
319321
| .github/workflows/self_needs.yml:20:15:20:51 | needs.test1.outputs.job_output | .github/workflows/self_needs.yml:16:20:16:57 | github.event['comment']['body'] | .github/workflows/self_needs.yml:20:15:20:51 | needs.test1.outputs.job_output | Potential code injection in $@, which may be controlled by an external user. | .github/workflows/self_needs.yml:20:15:20:51 | needs.test1.outputs.job_output | ${{ needs.test1.outputs.job_output }} |
320322
| .github/workflows/simple2.yml:29:24:29:54 | steps.step.outputs.value | .github/workflows/simple2.yml:14:9:18:6 | Uses Step: source | .github/workflows/simple2.yml:29:24:29:54 | steps.step.outputs.value | Potential code injection in $@, which may be controlled by an external user. | .github/workflows/simple2.yml:29:24:29:54 | steps.step.outputs.value | ${{ steps.step.outputs.value }} |

‎ql/test/query-tests/Security/CWE-094/CodeInjectionMedium.expected

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -283,7 +283,5 @@ subpaths
283283
| .github/workflows/push.yml:14:19:14:64 | github.event.head_commit.committer.name | .github/workflows/push.yml:14:19:14:64 | github.event.head_commit.committer.name | .github/workflows/push.yml:14:19:14:64 | github.event.head_commit.committer.name | Potential code injection in $@, which may be controlled by an external user. | .github/workflows/push.yml:14:19:14:64 | github.event.head_commit.committer.name | ${{ github.event.head_commit.committer.name }} |
284284
| .github/workflows/push.yml:15:19:15:65 | github.event.commits[11].committer.email | .github/workflows/push.yml:15:19:15:65 | github.event.commits[11].committer.email | .github/workflows/push.yml:15:19:15:65 | github.event.commits[11].committer.email | Potential code injection in $@, which may be controlled by an external user. | .github/workflows/push.yml:15:19:15:65 | github.event.commits[11].committer.email | ${{ github.event.commits[11].committer.email }} |
285285
| .github/workflows/push.yml:16:19:16:64 | github.event.commits[11].committer.name | .github/workflows/push.yml:16:19:16:64 | github.event.commits[11].committer.name | .github/workflows/push.yml:16:19:16:64 | github.event.commits[11].committer.name | Potential code injection in $@, which may be controlled by an external user. | .github/workflows/push.yml:16:19:16:64 | github.event.commits[11].committer.name | ${{ github.event.commits[11].committer.name }} |
286-
| .github/workflows/reusable-workflow-1.yml:53:26:53:39 | env.log | .github/workflows/reusable-workflow-1.yml:44:19:44:56 | github.event.pull_request.title | .github/workflows/reusable-workflow-1.yml:53:26:53:39 | env.log | Potential code injection in $@, which may be controlled by an external user. | .github/workflows/reusable-workflow-1.yml:53:26:53:39 | env.log | ${{ env.log }} |
287-
| .github/workflows/reusable-workflow-2.yml:53:26:53:39 | env.log | .github/workflows/reusable-workflow-2.yml:44:19:44:56 | github.event.pull_request.title | .github/workflows/reusable-workflow-2.yml:53:26:53:39 | env.log | Potential code injection in $@, which may be controlled by an external user. | .github/workflows/reusable-workflow-2.yml:53:26:53:39 | env.log | ${{ env.log }} |
288286
| .github/workflows/simple1.yml:16:18:16:49 | steps.summary.outputs.value | .github/workflows/simple1.yml:11:20:11:58 | github.event.head_commit.message | .github/workflows/simple1.yml:16:18:16:49 | steps.summary.outputs.value | Potential code injection in $@, which may be controlled by an external user. | .github/workflows/simple1.yml:16:18:16:49 | steps.summary.outputs.value | ${{steps.summary.outputs.value}} |
289287
| .github/workflows/test.yml:49:20:49:56 | needs.job1.outputs['job_output'] | .github/workflows/test.yml:15:20:15:64 | github.event['head_commit']['message'] | .github/workflows/test.yml:49:20:49:56 | needs.job1.outputs['job_output'] | Potential code injection in $@, which may be controlled by an external user. | .github/workflows/test.yml:49:20:49:56 | needs.job1.outputs['job_output'] | ${{needs.job1.outputs['job_output']}} |

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

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,3 +9,4 @@
99
| .github/workflows/test11.yml:14:9:19:6 | Uses Step | Potential cache poisoning in the context of the default branch on step $@. | .github/workflows/test11.yml:19:9:23:6 | Uses Step | Uses Step |
1010
| .github/workflows/test15.yml:14:9:17:6 | Uses Step | Potential cache poisoning in the context of the default branch on step $@. | .github/workflows/test15.yml:17:9:21:6 | Uses Step | Uses Step |
1111
| .github/workflows/test16.yml:14:9:17:6 | Uses Step | Potential cache poisoning in the context of the default branch on step $@. | .github/workflows/test16.yml:17:9:21:6 | Uses Step | Uses Step |
12+
| .github/workflows/test17.yml:15:9:20:6 | Uses Step | Potential cache poisoning in the context of the default branch on step $@. | .github/workflows/test17.yml:22:9:26:31 | Uses Step | Uses Step |

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

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,33 @@
1+
name: Test
2+
3+
on:
4+
workflow_call:
5+
6+
env:
7+
API_KEY: ${{ secrets.API_KEY != '' && secrets.API_KEY }}
8+
9+
jobs:
10+
mend:
11+
runs-on: "ubuntu-latest"
12+
steps:
13+
- name: "Set the checkout ref"
14+
id: set_ref
15+
run: |
16+
if [[ "${{ github.event_name }}" == "pull_request_target" ]]; then
17+
echo "ref=${{ github.event.pull_request.head.sha }}" >> $GITHUB_OUTPUT
18+
else
19+
echo "ref=${{ github.ref }}" >> $GITHUB_OUTPUT
20+
fi
21+
22+
- name: "checkout"
23+
if: success()
24+
uses: "actions/checkout@v4"
25+
with:
26+
fetch-depth: 1
27+
ref: ${{ steps.set_ref.outputs.ref }}
28+
29+
- name: "setup ruby"
30+
if: success()
31+
uses: "ruby/setup-ruby@v1"
32+
with:
33+
ruby-version: 2.7

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

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,23 @@
1+
name: Test
2+
3+
on:
4+
pull_request:
5+
6+
permissions:
7+
contents: write
8+
pull-requests: write
9+
10+
jobs:
11+
test:
12+
runs-on: ubuntu-latest
13+
steps:
14+
- name: Check out repo on head ref
15+
uses: actions/checkout@93ea575cb5d8a053eaa0ac8fa3b40d7e05a33cc8
16+
with:
17+
ref: ${{ github.head_ref }}
18+
token: ${{ secrets.DOCUBOT_REPO_PAT }}
19+
20+
- run: |
21+
./cmd
22+
env:
23+
GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}

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

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -14,4 +14,5 @@
1414
| .github/workflows/label_trusted_checkout.yml:20:7:24:4 | Uses Step | Unpinned 3rd party Action 'label_trusted_checkout.yml' step $@ uses 'completely/fakeaction' with ref '2', not a pinned commit hash | .github/workflows/label_trusted_checkout.yml:20:7:24:4 | Uses Step | Uses Step |
1515
| .github/workflows/label_trusted_checkout.yml:24:7:27:21 | Uses Step | Unpinned 3rd party Action 'label_trusted_checkout.yml' step $@ uses 'fakerepo/comment-on-pr' with ref '1', not a pinned commit hash | .github/workflows/label_trusted_checkout.yml:24:7:27:21 | Uses Step | Uses Step |
1616
| .github/workflows/level0.yml:36:9:39:6 | Uses Step | Unpinned 3rd party Action 'Poutine Level 0' step $@ uses 'rlespinasse/github-slug-action' with ref '4', not a pinned commit hash | .github/workflows/level0.yml:36:9:39:6 | Uses Step | Uses Step |
17+
| .github/workflows/mend.yml:29:9:33:28 | Uses Step | Unpinned 3rd party Action 'Test' step $@ uses 'ruby/setup-ruby' with ref '1', not a pinned commit hash | .github/workflows/mend.yml:29:9:33:28 | Uses Step | Uses Step |
1718
| .github/workflows/unpinned_tags.yml:10:7:11:4 | Uses Step | Unpinned 3rd party Action 'unpinned_tags.yml' step $@ uses 'foo/bar' with ref '1', not a pinned commit hash | .github/workflows/unpinned_tags.yml:10:7:11:4 | Uses Step | Uses Step |

0 commit comments

Comments
 (0)