Skip to content

Commit 1fc45eb

Browse files
author
Alvaro Muñoz
committed
Improve ControlCheck for untrusted checkouts
1 parent 16a7522 commit 1fc45eb

File tree

10 files changed

+177
-19
lines changed

10 files changed

+177
-19
lines changed

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

Lines changed: 24 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -227,7 +227,14 @@ class GhSHACheckout extends SHACheckoutStep instanceof Run {
227227
}
228228

229229
/** An If node that contains an actor, user or label check */
230-
abstract class ControlCheck extends If { }
230+
abstract class ControlCheck extends If {
231+
predicate dominates(Step step) {
232+
step.getIf() = this or
233+
step.getEnclosingJob().getIf() = this or
234+
step.getEnclosingJob().getANeededJob().(LocalJob).getAStep().getIf() = this or
235+
step.getEnclosingJob().getANeededJob().(LocalJob).getIf() = this
236+
}
237+
}
231238

232239
class LabelControlCheck extends ControlCheck {
233240
LabelControlCheck() {
@@ -244,15 +251,28 @@ class LabelControlCheck extends ControlCheck {
244251

245252
class ActorControlCheck extends ControlCheck {
246253
ActorControlCheck() {
247-
// eg: contains(github.actor, 'dependabot')
248-
// eg: github.triggering_actor != 'CI Agent'
254+
// eg: github.actor == 'dependabot[bot]'
255+
// eg: github.triggering_actor == 'CI Agent'
249256
// eg: github.event.pull_request.user.login == 'mybot'
250257
exists(
251258
normalizeExpr(this.getCondition())
252259
.regexpFind([
253260
"\\bgithub\\.actor\\b", "\\bgithub\\.triggering_actor\\b",
254261
"\\bgithub\\.event\\.comment\\.user\\.login\\b",
255-
"\\bgithub\\.event\\.pull_request\\.user\\.login\\b",
262+
"\\bgithub\\.event\\.pull_request\\.user\\.login\\b",
263+
], _, _)
264+
)
265+
}
266+
}
267+
268+
class RepositoryControlCheck extends ControlCheck {
269+
RepositoryControlCheck() {
270+
// eg: github.repository == 'test/foo'
271+
exists(
272+
normalizeExpr(this.getCondition())
273+
.regexpFind([
274+
"\\bgithub\\.repository\\b",
275+
"\\bgithub\\.repository_owner\\b",
256276
], _, _)
257277
)
258278
}

‎ql/src/Security/CWE-367/UntrustedCheckoutTOCTOUHigh.ql

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -2,9 +2,9 @@
22
* @name Untrusted Checkout TOCTOU
33
* @description Untrusted Checkout is protected by a security check but the checked-out branch can be changed after the check.
44
* @kind problem
5-
* @problem.severity warning
6-
* @precision medium
7-
* @security-severity 5.3
5+
* @problem.severity error
6+
* @precision high
7+
* @security-severity 7.5
88
* @id actions/untrusted-checkout-toctou/high
99
* @tags actions
1010
* security

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

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -21,10 +21,11 @@ from LocalJob j, PRHeadCheckoutStep checkout
2121
where
2222
j = checkout.getEnclosingJob() and
2323
j.getAStep() = checkout and
24+
// the checkout is followed by a known poisonable step
2425
checkout.getAFollowingStep() instanceof PoisonableStep and
25-
not exists(ControlCheck check |
26-
checkout.getIf() = check or checkout.getEnclosingJob().getIf() = check
27-
) and
26+
// the checkout is not controlled by an access check
27+
not exists(ControlCheck check | check.dominates(checkout)) and
28+
// the checkout occurs in a privileged context
2829
(
2930
inPrivilegedCompositeAction(checkout)
3031
or

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

Lines changed: 7 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -4,9 +4,9 @@
44
* By explicitly checking out and running the build script from a fork the untrusted code is running in an environment
55
* that is able to push to the base repository and to access secrets.
66
* @kind problem
7-
* @problem.severity warning
8-
* @precision medium
9-
* @security-severity 5.3
7+
* @problem.severity error
8+
* @precision high
9+
* @security-severity 7.5
1010
* @id actions/untrusted-checkout/high
1111
* @tags actions
1212
* security
@@ -21,10 +21,11 @@ from LocalJob j, PRHeadCheckoutStep checkout
2121
where
2222
j = checkout.getEnclosingJob() and
2323
j.getAStep() = checkout and
24+
// the checkout is NOT followed by a known poisonable step
2425
not checkout.getAFollowingStep() instanceof PoisonableStep and
25-
not exists(ControlCheck check |
26-
checkout.getIf() = check or checkout.getEnclosingJob().getIf() = check
27-
) and
26+
// the checkout is not controlled by an access check
27+
not exists(ControlCheck check | check.dominates(checkout)) and
28+
// the checkout occurs in a privileged context
2829
(
2930
inPrivilegedCompositeAction(checkout)
3031
or

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

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -21,9 +21,9 @@ from LocalJob j, PRHeadCheckoutStep checkout
2121
where
2222
j = checkout.getEnclosingJob() and
2323
j.getAStep() = checkout and
24-
not exists(ControlCheck check |
25-
checkout.getIf() = check or checkout.getEnclosingJob().getIf() = check
26-
) and
24+
// the checkout is not controlled by an access check
25+
not exists(ControlCheck check | check.dominates(checkout)) and
26+
// the checkout occurs in a non-privileged context
2727
(
2828
inNonPrivilegedCompositeAction(checkout) or
2929
inNonPrivilegedJob(checkout)

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

Lines changed: 45 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,45 @@
1+
name: Check dist
2+
3+
on:
4+
pull_request:
5+
push:
6+
branches:
7+
- main
8+
- 'releases/*'
9+
10+
jobs:
11+
verify-build: # make sure the checked in dist/ folder matches the output of a rebuild
12+
runs-on: ubuntu-latest
13+
14+
steps:
15+
- uses: actions/checkout@v4
16+
with:
17+
ref: ${{ github.event.pull_request.head.sha }}
18+
19+
- name: Read .nvmrc
20+
id: nvm
21+
run: echo "NVMRC=$(cat .nvmrc)" >> $GITHUB_OUTPUT
22+
23+
- name: Setup Node.js
24+
uses: actions/setup-node@v4
25+
with:
26+
node-version: ${{ steps.nvm.outputs.NVMRC }}
27+
28+
- name: Install npm dependencies
29+
run: npm clean-install
30+
31+
- name: Rebuild the dist/ directory
32+
run: npm run package
33+
34+
- name: Compare the expected and actual dist/ directories
35+
run: script/check-diff
36+
verify-index-js: # make sure the entrypoint js files run on a clean machine without compiling first
37+
runs-on: ubuntu-latest
38+
steps:
39+
- uses: actions/checkout@v4
40+
with:
41+
ref: ${{ github.event.pull_request.head.sha }}
42+
43+
- uses: ./
44+
with:
45+
milliseconds: 1000

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

Lines changed: 68 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,68 @@
1+
name: Compile dependabot updates
2+
3+
on:
4+
pull_request:
5+
6+
permissions:
7+
pull-requests: write
8+
contents: write
9+
jobs:
10+
fetch-dependabot-metadata:
11+
runs-on: ubuntu-latest
12+
# We only want to check the metadata on pull_request events from Dependabot itself,
13+
# any subsequent pushes to the PR should just skip this step so we don't go into
14+
# a loop on commits created by the `build-dependabot-changes` job
15+
if: ${{ github.actor == 'dependabot[bot]' }}
16+
# Map the step output to a job output for subsequent jobs
17+
outputs:
18+
dependency-type: ${{ steps.dependabot-metadata.outputs.dependency-type }}
19+
package-ecosystem: ${{ steps.dependabot-metadata.outputs.package-ecosystem }}
20+
steps:
21+
- name: Fetch dependabot metadata
22+
id: dependabot-metadata
23+
uses: dependabot/fetch-metadata@c9c4182bf1b97f5224aee3906fd373f6b61b4526 # v1.6.0
24+
with:
25+
github-token: "${{ secrets.GITHUB_TOKEN }}"
26+
build-dependabot-changes:
27+
runs-on: ubuntu-latest
28+
needs: [fetch-dependabot-metadata]
29+
# We only need to build the dist/ folder if the PR relates to Docker or an npm dependency
30+
if: needs.fetch-dependabot-metadata.outputs.package-ecosystem == 'docker' || needs.fetch-dependabot-metadata.outputs.package-ecosystem == 'npm_and_yarn'
31+
steps:
32+
# Check out using a PAT so any pushed changes will trigger checkruns
33+
- uses: actions/checkout@v4
34+
with:
35+
ref: ${{ github.event.pull_request.head.ref }}
36+
token: ${{ secrets.DEPENDABOT_AUTOBUILD }}
37+
38+
- name: Read .nvmrc
39+
id: nvm
40+
run: echo "NVMRC=$(cat .nvmrc)" >> $GITHUB_OUTPUT
41+
42+
- name: Setup Node.js
43+
uses: actions/setup-node@v4
44+
with:
45+
node-version: ${{ steps.nvm.outputs.NVMRC }}
46+
47+
- name: Install npm dependencies
48+
run: npm clean-install
49+
50+
# If we're reacting to a Docker PR, we have on extra step to refresh and check in the container manifest,
51+
# this **must** happen before rebuilding dist/ so it uses the new version of the manifest
52+
- name: Rebuild docker/containers.json
53+
if: needs.fetch-dependabot-metadata.outputs.package-ecosystem == 'docker'
54+
run: |
55+
npm run update-container-manifest
56+
git add docker/containers.json
57+
58+
- name: Rebuild the dist/ directory
59+
run: npm run package
60+
61+
- name: Check in any change to dist/
62+
run: |
63+
git add dist/
64+
# Specifying the full email allows the avatar to show up: https://github.com/orgs/community/discussions/26560
65+
git config user.name "github-actions[bot]"
66+
git config user.email "41898282+github-actions[bot]@users.noreply.github.com"
67+
git commit -m "[dependabot skip] Update dist/ with build changes" || exit 0
68+
git push

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

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,20 @@
1+
name: "Frogbot Scan Pull Request"
2+
on:
3+
pull_request_target:
4+
types: [ opened, synchronize ]
5+
permissions:
6+
pull-requests: write
7+
contents: read
8+
jobs:
9+
scan-pull-request:
10+
runs-on: ubuntu-latest
11+
environment: frogbot
12+
steps:
13+
- uses: actions/checkout@v2
14+
with:
15+
ref: ${{ github.event.pull_request.head.sha }}
16+
- uses: jfrog/frogbot@8fbeca612957ae5f5f0c03a19cb6e59e237026f3 # v2.10.0
17+
env:
18+
JF_URL: ${{ secrets.JF_URL }}
19+
JF_ACCESS_TOKEN: ${{ secrets.JF_ACCESS_TOKEN }}
20+
JF_GIT_TOKEN: ${{ secrets.GITHUB_TOKEN }}

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

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,7 @@
11
| .github/workflows/auto_ci.yml:20:9:27:6 | Uses Step | Potential unsafe checkout of untrusted pull request on privileged workflow. |
22
| .github/workflows/auto_ci.yml:67:9:74:6 | Uses Step | Potential unsafe checkout of untrusted pull request on privileged workflow. |
3+
| .github/workflows/dependabot1.yml:15:9:19:6 | Uses Step | Potential unsafe checkout of untrusted pull request on privileged workflow. |
4+
| .github/workflows/dependabot1.yml:39:9:43:6 | Uses Step | Potential unsafe checkout of untrusted pull request on privileged workflow. |
35
| .github/workflows/gitcheckout.yml:10:11:18:8 | Run Step | Potential unsafe checkout of untrusted pull request on privileged workflow. |
46
| .github/workflows/level0.yml:99:9:103:6 | Uses Step | Potential unsafe checkout of untrusted pull request on privileged workflow. |
57
| .github/workflows/level0.yml:125:9:129:6 | Uses Step | Potential unsafe checkout of untrusted pull request on privileged workflow. |

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

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@
1515
| .github/workflows/issue_comment_octokit.yml:79:9:83:2 | Uses Step | Potential unsafe checkout of untrusted pull request on privileged workflow. |
1616
| .github/workflows/issue_comment_octokit.yml:95:9:100:2 | Uses Step | Potential unsafe checkout of untrusted pull request on privileged workflow. |
1717
| .github/workflows/issue_comment_octokit.yml:109:9:114:66 | Uses Step | Potential unsafe checkout of untrusted pull request on privileged workflow. |
18+
| .github/workflows/test2.yml:13:9:16:6 | Uses Step | Potential unsafe checkout of untrusted pull request on privileged workflow. |
1819
| .github/workflows/untrusted_checkout2.yml:14:9:19:72 | Run Step | Potential unsafe checkout of untrusted pull request on privileged workflow. |
1920
| .github/workflows/workflow_run_untrusted_checkout.yml:13:9:16:6 | Uses Step | Potential unsafe checkout of untrusted pull request on privileged workflow. |
2021
| .github/workflows/workflow_run_untrusted_checkout.yml:16:9:18:31 | Uses Step | Potential unsafe checkout of untrusted pull request on privileged workflow. |

0 commit comments

Comments
 (0)