From 1fc45eb2969202fda4690a095dd9889746e16e88 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alvaro=20Mu=C3=B1oz?= Date: Fri, 24 May 2024 09:33:35 +0200 Subject: [PATCH] Improve ControlCheck for untrusted checkouts --- .../security/UntrustedCheckoutQuery.qll | 28 ++++++-- .../CWE-367/UntrustedCheckoutTOCTOUHigh.ql | 6 +- .../CWE-829/UntrustedCheckoutCritical.ql | 7 +- .../Security/CWE-829/UntrustedCheckoutHigh.ql | 13 ++-- .../CWE-829/UntrustedCheckoutMedium.ql | 6 +- .../CWE-829/.github/workflows/dependabot1.yml | 45 ++++++++++++ .../CWE-829/.github/workflows/dependabot2.yml | 68 +++++++++++++++++++ .../CWE-829/.github/workflows/test2.yml | 20 ++++++ .../UntrustedCheckoutCritical.expected | 2 + .../CWE-829/UntrustedCheckoutHigh.expected | 1 + 10 files changed, 177 insertions(+), 19 deletions(-) create mode 100644 ql/test/query-tests/Security/CWE-829/.github/workflows/dependabot1.yml create mode 100644 ql/test/query-tests/Security/CWE-829/.github/workflows/dependabot2.yml create mode 100644 ql/test/query-tests/Security/CWE-829/.github/workflows/test2.yml diff --git a/ql/lib/codeql/actions/security/UntrustedCheckoutQuery.qll b/ql/lib/codeql/actions/security/UntrustedCheckoutQuery.qll index 6c3b042d1e7..ba31b0de500 100644 --- a/ql/lib/codeql/actions/security/UntrustedCheckoutQuery.qll +++ b/ql/lib/codeql/actions/security/UntrustedCheckoutQuery.qll @@ -227,7 +227,14 @@ class GhSHACheckout extends SHACheckoutStep instanceof Run { } /** An If node that contains an actor, user or label check */ -abstract class ControlCheck extends If { } +abstract class ControlCheck extends If { + predicate dominates(Step step) { + step.getIf() = this or + step.getEnclosingJob().getIf() = this or + step.getEnclosingJob().getANeededJob().(LocalJob).getAStep().getIf() = this or + step.getEnclosingJob().getANeededJob().(LocalJob).getIf() = this + } +} class LabelControlCheck extends ControlCheck { LabelControlCheck() { @@ -244,15 +251,28 @@ class LabelControlCheck extends ControlCheck { class ActorControlCheck extends ControlCheck { ActorControlCheck() { - // eg: contains(github.actor, 'dependabot') - // eg: github.triggering_actor != 'CI Agent' + // eg: github.actor == 'dependabot[bot]' + // eg: github.triggering_actor == 'CI Agent' // eg: github.event.pull_request.user.login == 'mybot' exists( normalizeExpr(this.getCondition()) .regexpFind([ "\\bgithub\\.actor\\b", "\\bgithub\\.triggering_actor\\b", "\\bgithub\\.event\\.comment\\.user\\.login\\b", - "\\bgithub\\.event\\.pull_request\\.user\\.login\\b", + "\\bgithub\\.event\\.pull_request\\.user\\.login\\b", + ], _, _) + ) + } +} + +class RepositoryControlCheck extends ControlCheck { + RepositoryControlCheck() { + // eg: github.repository == 'test/foo' + exists( + normalizeExpr(this.getCondition()) + .regexpFind([ + "\\bgithub\\.repository\\b", + "\\bgithub\\.repository_owner\\b", ], _, _) ) } diff --git a/ql/src/Security/CWE-367/UntrustedCheckoutTOCTOUHigh.ql b/ql/src/Security/CWE-367/UntrustedCheckoutTOCTOUHigh.ql index c1118bc00ca..ca1b855c6ec 100644 --- a/ql/src/Security/CWE-367/UntrustedCheckoutTOCTOUHigh.ql +++ b/ql/src/Security/CWE-367/UntrustedCheckoutTOCTOUHigh.ql @@ -2,9 +2,9 @@ * @name Untrusted Checkout TOCTOU * @description Untrusted Checkout is protected by a security check but the checked-out branch can be changed after the check. * @kind problem - * @problem.severity warning - * @precision medium - * @security-severity 5.3 + * @problem.severity error + * @precision high + * @security-severity 7.5 * @id actions/untrusted-checkout-toctou/high * @tags actions * security diff --git a/ql/src/Security/CWE-829/UntrustedCheckoutCritical.ql b/ql/src/Security/CWE-829/UntrustedCheckoutCritical.ql index 0a597ee3fa4..eae580ebd52 100644 --- a/ql/src/Security/CWE-829/UntrustedCheckoutCritical.ql +++ b/ql/src/Security/CWE-829/UntrustedCheckoutCritical.ql @@ -21,10 +21,11 @@ from LocalJob j, PRHeadCheckoutStep checkout where j = checkout.getEnclosingJob() and j.getAStep() = checkout and + // the checkout is followed by a known poisonable step checkout.getAFollowingStep() instanceof PoisonableStep and - not exists(ControlCheck check | - checkout.getIf() = check or checkout.getEnclosingJob().getIf() = check - ) and + // the checkout is not controlled by an access check + not exists(ControlCheck check | check.dominates(checkout)) and + // the checkout occurs in a privileged context ( inPrivilegedCompositeAction(checkout) or diff --git a/ql/src/Security/CWE-829/UntrustedCheckoutHigh.ql b/ql/src/Security/CWE-829/UntrustedCheckoutHigh.ql index 29a15accdf2..9faab24dbcb 100644 --- a/ql/src/Security/CWE-829/UntrustedCheckoutHigh.ql +++ b/ql/src/Security/CWE-829/UntrustedCheckoutHigh.ql @@ -4,9 +4,9 @@ * By explicitly checking out and running the build script from a fork the untrusted code is running in an environment * that is able to push to the base repository and to access secrets. * @kind problem - * @problem.severity warning - * @precision medium - * @security-severity 5.3 + * @problem.severity error + * @precision high + * @security-severity 7.5 * @id actions/untrusted-checkout/high * @tags actions * security @@ -21,10 +21,11 @@ from LocalJob j, PRHeadCheckoutStep checkout where j = checkout.getEnclosingJob() and j.getAStep() = checkout and + // the checkout is NOT followed by a known poisonable step not checkout.getAFollowingStep() instanceof PoisonableStep and - not exists(ControlCheck check | - checkout.getIf() = check or checkout.getEnclosingJob().getIf() = check - ) and + // the checkout is not controlled by an access check + not exists(ControlCheck check | check.dominates(checkout)) and + // the checkout occurs in a privileged context ( inPrivilegedCompositeAction(checkout) or diff --git a/ql/src/Security/CWE-829/UntrustedCheckoutMedium.ql b/ql/src/Security/CWE-829/UntrustedCheckoutMedium.ql index aa62a88935b..574c2d7bffe 100644 --- a/ql/src/Security/CWE-829/UntrustedCheckoutMedium.ql +++ b/ql/src/Security/CWE-829/UntrustedCheckoutMedium.ql @@ -21,9 +21,9 @@ from LocalJob j, PRHeadCheckoutStep checkout where j = checkout.getEnclosingJob() and j.getAStep() = checkout and - not exists(ControlCheck check | - checkout.getIf() = check or checkout.getEnclosingJob().getIf() = check - ) and + // the checkout is not controlled by an access check + not exists(ControlCheck check | check.dominates(checkout)) and + // the checkout occurs in a non-privileged context ( inNonPrivilegedCompositeAction(checkout) or inNonPrivilegedJob(checkout) diff --git a/ql/test/query-tests/Security/CWE-829/.github/workflows/dependabot1.yml b/ql/test/query-tests/Security/CWE-829/.github/workflows/dependabot1.yml new file mode 100644 index 00000000000..afe1dfab038 --- /dev/null +++ b/ql/test/query-tests/Security/CWE-829/.github/workflows/dependabot1.yml @@ -0,0 +1,45 @@ +name: Check dist + +on: + pull_request: + push: + branches: + - main + - 'releases/*' + +jobs: + verify-build: # make sure the checked in dist/ folder matches the output of a rebuild + runs-on: ubuntu-latest + + steps: + - uses: actions/checkout@v4 + with: + ref: ${{ github.event.pull_request.head.sha }} + + - name: Read .nvmrc + id: nvm + run: echo "NVMRC=$(cat .nvmrc)" >> $GITHUB_OUTPUT + + - name: Setup Node.js + uses: actions/setup-node@v4 + with: + node-version: ${{ steps.nvm.outputs.NVMRC }} + + - name: Install npm dependencies + run: npm clean-install + + - name: Rebuild the dist/ directory + run: npm run package + + - name: Compare the expected and actual dist/ directories + run: script/check-diff + verify-index-js: # make sure the entrypoint js files run on a clean machine without compiling first + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + with: + ref: ${{ github.event.pull_request.head.sha }} + + - uses: ./ + with: + milliseconds: 1000 diff --git a/ql/test/query-tests/Security/CWE-829/.github/workflows/dependabot2.yml b/ql/test/query-tests/Security/CWE-829/.github/workflows/dependabot2.yml new file mode 100644 index 00000000000..072eae4b1d2 --- /dev/null +++ b/ql/test/query-tests/Security/CWE-829/.github/workflows/dependabot2.yml @@ -0,0 +1,68 @@ +name: Compile dependabot updates + +on: + pull_request: + +permissions: + pull-requests: write + contents: write +jobs: + fetch-dependabot-metadata: + runs-on: ubuntu-latest + # We only want to check the metadata on pull_request events from Dependabot itself, + # any subsequent pushes to the PR should just skip this step so we don't go into + # a loop on commits created by the `build-dependabot-changes` job + if: ${{ github.actor == 'dependabot[bot]' }} + # Map the step output to a job output for subsequent jobs + outputs: + dependency-type: ${{ steps.dependabot-metadata.outputs.dependency-type }} + package-ecosystem: ${{ steps.dependabot-metadata.outputs.package-ecosystem }} + steps: + - name: Fetch dependabot metadata + id: dependabot-metadata + uses: dependabot/fetch-metadata@c9c4182bf1b97f5224aee3906fd373f6b61b4526 # v1.6.0 + with: + github-token: "${{ secrets.GITHUB_TOKEN }}" + build-dependabot-changes: + runs-on: ubuntu-latest + needs: [fetch-dependabot-metadata] + # We only need to build the dist/ folder if the PR relates to Docker or an npm dependency + if: needs.fetch-dependabot-metadata.outputs.package-ecosystem == 'docker' || needs.fetch-dependabot-metadata.outputs.package-ecosystem == 'npm_and_yarn' + steps: + # Check out using a PAT so any pushed changes will trigger checkruns + - uses: actions/checkout@v4 + with: + ref: ${{ github.event.pull_request.head.ref }} + token: ${{ secrets.DEPENDABOT_AUTOBUILD }} + + - name: Read .nvmrc + id: nvm + run: echo "NVMRC=$(cat .nvmrc)" >> $GITHUB_OUTPUT + + - name: Setup Node.js + uses: actions/setup-node@v4 + with: + node-version: ${{ steps.nvm.outputs.NVMRC }} + + - name: Install npm dependencies + run: npm clean-install + + # If we're reacting to a Docker PR, we have on extra step to refresh and check in the container manifest, + # this **must** happen before rebuilding dist/ so it uses the new version of the manifest + - name: Rebuild docker/containers.json + if: needs.fetch-dependabot-metadata.outputs.package-ecosystem == 'docker' + run: | + npm run update-container-manifest + git add docker/containers.json + + - name: Rebuild the dist/ directory + run: npm run package + + - name: Check in any change to dist/ + run: | + git add dist/ + # Specifying the full email allows the avatar to show up: https://github.com/orgs/community/discussions/26560 + git config user.name "github-actions[bot]" + git config user.email "41898282+github-actions[bot]@users.noreply.github.com" + git commit -m "[dependabot skip] Update dist/ with build changes" || exit 0 + git push diff --git a/ql/test/query-tests/Security/CWE-829/.github/workflows/test2.yml b/ql/test/query-tests/Security/CWE-829/.github/workflows/test2.yml new file mode 100644 index 00000000000..64e4992b5ca --- /dev/null +++ b/ql/test/query-tests/Security/CWE-829/.github/workflows/test2.yml @@ -0,0 +1,20 @@ +name: "Frogbot Scan Pull Request" +on: + pull_request_target: + types: [ opened, synchronize ] +permissions: + pull-requests: write + contents: read +jobs: + scan-pull-request: + runs-on: ubuntu-latest + environment: frogbot + steps: + - uses: actions/checkout@v2 + with: + ref: ${{ github.event.pull_request.head.sha }} + - uses: jfrog/frogbot@8fbeca612957ae5f5f0c03a19cb6e59e237026f3 # v2.10.0 + env: + JF_URL: ${{ secrets.JF_URL }} + JF_ACCESS_TOKEN: ${{ secrets.JF_ACCESS_TOKEN }} + JF_GIT_TOKEN: ${{ secrets.GITHUB_TOKEN }} diff --git a/ql/test/query-tests/Security/CWE-829/UntrustedCheckoutCritical.expected b/ql/test/query-tests/Security/CWE-829/UntrustedCheckoutCritical.expected index 1f90c56607d..2660a726ab6 100644 --- a/ql/test/query-tests/Security/CWE-829/UntrustedCheckoutCritical.expected +++ b/ql/test/query-tests/Security/CWE-829/UntrustedCheckoutCritical.expected @@ -1,5 +1,7 @@ | .github/workflows/auto_ci.yml:20:9:27:6 | Uses Step | Potential unsafe checkout of untrusted pull request on privileged workflow. | | .github/workflows/auto_ci.yml:67:9:74:6 | Uses Step | Potential unsafe checkout of untrusted pull request on privileged workflow. | +| .github/workflows/dependabot1.yml:15:9:19:6 | Uses Step | Potential unsafe checkout of untrusted pull request on privileged workflow. | +| .github/workflows/dependabot1.yml:39:9:43:6 | Uses Step | Potential unsafe checkout of untrusted pull request on privileged workflow. | | .github/workflows/gitcheckout.yml:10:11:18:8 | Run Step | Potential unsafe checkout of untrusted pull request on privileged workflow. | | .github/workflows/level0.yml:99:9:103:6 | Uses Step | Potential unsafe checkout of untrusted pull request on privileged workflow. | | .github/workflows/level0.yml:125:9:129:6 | Uses Step | Potential unsafe checkout of untrusted pull request on privileged workflow. | diff --git a/ql/test/query-tests/Security/CWE-829/UntrustedCheckoutHigh.expected b/ql/test/query-tests/Security/CWE-829/UntrustedCheckoutHigh.expected index a40ab1fa771..9015e85b3d0 100644 --- a/ql/test/query-tests/Security/CWE-829/UntrustedCheckoutHigh.expected +++ b/ql/test/query-tests/Security/CWE-829/UntrustedCheckoutHigh.expected @@ -15,6 +15,7 @@ | .github/workflows/issue_comment_octokit.yml:79:9:83:2 | Uses Step | Potential unsafe checkout of untrusted pull request on privileged workflow. | | .github/workflows/issue_comment_octokit.yml:95:9:100:2 | Uses Step | Potential unsafe checkout of untrusted pull request on privileged workflow. | | .github/workflows/issue_comment_octokit.yml:109:9:114:66 | Uses Step | Potential unsafe checkout of untrusted pull request on privileged workflow. | +| .github/workflows/test2.yml:13:9:16:6 | Uses Step | Potential unsafe checkout of untrusted pull request on privileged workflow. | | .github/workflows/untrusted_checkout2.yml:14:9:19:72 | Run Step | Potential unsafe checkout of untrusted pull request on privileged workflow. | | .github/workflows/workflow_run_untrusted_checkout.yml:13:9:16:6 | Uses Step | Potential unsafe checkout of untrusted pull request on privileged workflow. | | .github/workflows/workflow_run_untrusted_checkout.yml:16:9:18:31 | Uses Step | Potential unsafe checkout of untrusted pull request on privileged workflow. |