From e6311966c80fae7fdf43db6ff43c88600933d08e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alvaro=20Mu=C3=B1oz?= Date: Wed, 26 Jun 2024 16:17:07 +0200 Subject: [PATCH] Take explicit permission into account for privilege calculation --- ql/lib/codeql/actions/ast/internal/Ast.qll | 28 +++++++++-- .../CWE-349/.github/workflows/test20.yml | 46 +++++++++++++++++++ .../CWE-829/.github/workflows/test4.yml | 46 +++++++++++++++++++ .../UntrustedCheckoutCritical.expected | 1 - .../CWE-829/UntrustedCheckoutMedium.expected | 2 + 5 files changed, 118 insertions(+), 5 deletions(-) create mode 100644 ql/test/query-tests/Security/CWE-349/.github/workflows/test20.yml create mode 100644 ql/test/query-tests/Security/CWE-829/.github/workflows/test4.yml diff --git a/ql/lib/codeql/actions/ast/internal/Ast.qll b/ql/lib/codeql/actions/ast/internal/Ast.qll index 8d965c3e4c7..2deb987650c 100644 --- a/ql/lib/codeql/actions/ast/internal/Ast.qll +++ b/ql/lib/codeql/actions/ast/internal/Ast.qll @@ -848,11 +848,23 @@ class JobImpl extends AstNodeImpl, TJobNode { this.getPermissions().getAPermission().matches("%write") } + private predicate hasExplicitReadPermission() { + // the job has not an explicit write permission + exists(this.getPermissions().getAPermission()) and + not this.getPermissions().getAPermission().matches("%write") + } + private predicate hasImplicitWritePermission() { // the job has an explicit write permission this.getEnclosingWorkflow().getPermissions().getAPermission().matches("%write") } + private predicate hasImplicitReadPermission() { + // the job has not an explicit write permission + exists(this.getEnclosingWorkflow().getPermissions().getAPermission()) and + not this.getEnclosingWorkflow().getPermissions().getAPermission().matches("%write") + } + private predicate hasRuntimeData() { exists(string path, string trigger, string name, string secrets_source, string perms | workflowDataModel(path, trigger, name, secrets_source, perms, _) and @@ -892,8 +904,7 @@ class JobImpl extends AstNodeImpl, TJobNode { /** Holds if the action is privileged and externally triggerable. */ predicate isPrivilegedExternallyTriggerable() { - exists(EventImpl e | - this.getATriggerEvent() = e and + exists(EventImpl e | this.getATriggerEvent() = e | // job is triggereable by an external user e.isExternallyTriggerable() and // no matter if `pull_request` is granted write permissions or access to secrets @@ -903,9 +914,18 @@ class JobImpl extends AstNodeImpl, TJobNode { // job is privileged (write access or access to secrets) this.isPrivileged() or - // the trigger event is __normally__ privileged and we have no runtime data to prove otherwise + // the trigger event is __normally__ privileged + e.isPrivileged() and + // and we have no runtime data to prove otherwise not this.hasRuntimeData() and - e.isPrivileged() + // and the job is not explicitly non-privileged + not ( + ( + this.hasExplicitReadPermission() or + this.hasImplicitReadPermission() + ) and + not this.hasExplicitSecretAccess() + ) ) ) } diff --git a/ql/test/query-tests/Security/CWE-349/.github/workflows/test20.yml b/ql/test/query-tests/Security/CWE-349/.github/workflows/test20.yml new file mode 100644 index 00000000000..a07f2922fd7 --- /dev/null +++ b/ql/test/query-tests/Security/CWE-349/.github/workflows/test20.yml @@ -0,0 +1,46 @@ +name: Publish + +on: + push: + branches: + - main + pull_request_target: + workflow_dispatch: + workflow_call: + +jobs: + build-and-upload: + runs-on: ubuntu-latest + permissions: + contents: read + steps: + + - name: Checkout PR + if: ${{ github.event_name == 'pull_request_target' }} + uses: actions/checkout@v3 + with: + ref: ${{ github.event.pull_request.head.ref }} + repository: ${{ github.event.pull_request.head.repo.full_name }} + + - name: Checkout + if: ${{ github.event_name != 'pull_request_target' }} + uses: actions/checkout@v3 + with: + ref: main + + - name: Setup Pages + uses: actions/configure-pages@v1 + - name: Use Node.js + uses: actions/setup-node@v3 + with: + node-version: 18 + cache: npm + - name: Update npm to latest + run: npm i --prefer-online --no-fund --no-audit -g npm@latest + - run: npm -v + - run: npm i --ignore-scripts --no-audit --no-fund --package-lock + - run: npm run build -w www + - name: Upload artifact + uses: actions/upload-pages-artifact@v1 + with: + path: './workspaces/www/build' diff --git a/ql/test/query-tests/Security/CWE-829/.github/workflows/test4.yml b/ql/test/query-tests/Security/CWE-829/.github/workflows/test4.yml new file mode 100644 index 00000000000..a07f2922fd7 --- /dev/null +++ b/ql/test/query-tests/Security/CWE-829/.github/workflows/test4.yml @@ -0,0 +1,46 @@ +name: Publish + +on: + push: + branches: + - main + pull_request_target: + workflow_dispatch: + workflow_call: + +jobs: + build-and-upload: + runs-on: ubuntu-latest + permissions: + contents: read + steps: + + - name: Checkout PR + if: ${{ github.event_name == 'pull_request_target' }} + uses: actions/checkout@v3 + with: + ref: ${{ github.event.pull_request.head.ref }} + repository: ${{ github.event.pull_request.head.repo.full_name }} + + - name: Checkout + if: ${{ github.event_name != 'pull_request_target' }} + uses: actions/checkout@v3 + with: + ref: main + + - name: Setup Pages + uses: actions/configure-pages@v1 + - name: Use Node.js + uses: actions/setup-node@v3 + with: + node-version: 18 + cache: npm + - name: Update npm to latest + run: npm i --prefer-online --no-fund --no-audit -g npm@latest + - run: npm -v + - run: npm i --ignore-scripts --no-audit --no-fund --package-lock + - run: npm run build -w www + - name: Upload artifact + uses: actions/upload-pages-artifact@v1 + with: + path: './workspaces/www/build' diff --git a/ql/test/query-tests/Security/CWE-829/UntrustedCheckoutCritical.expected b/ql/test/query-tests/Security/CWE-829/UntrustedCheckoutCritical.expected index 0ff47fd2c53..92d5a0b5ce1 100644 --- a/ql/test/query-tests/Security/CWE-829/UntrustedCheckoutCritical.expected +++ b/ql/test/query-tests/Security/CWE-829/UntrustedCheckoutCritical.expected @@ -4,6 +4,5 @@ | .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. | | .github/workflows/mend.yml:22:9:29:6 | Uses Step | Potential unsafe checkout of untrusted pull request on privileged workflow. | -| .github/workflows/test3.yml:28:9:33:6 | Uses Step | Potential unsafe checkout of untrusted pull request on privileged workflow. | | .github/workflows/untrusted_checkout.yml:10:9:13:6 | Uses Step | Potential unsafe checkout of untrusted pull request on privileged workflow. | | .github/workflows/untrusted_checkout.yml:13:9:16:6 | Uses Step | Potential unsafe checkout of untrusted pull request on privileged workflow. | diff --git a/ql/test/query-tests/Security/CWE-829/UntrustedCheckoutMedium.expected b/ql/test/query-tests/Security/CWE-829/UntrustedCheckoutMedium.expected index 544d26da9b7..5bf0e56e1b7 100644 --- a/ql/test/query-tests/Security/CWE-829/UntrustedCheckoutMedium.expected +++ b/ql/test/query-tests/Security/CWE-829/UntrustedCheckoutMedium.expected @@ -1,3 +1,5 @@ | .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/priv_pull_request_checkout.yml:14:9:20:6 | Uses Step | Potential unsafe checkout of untrusted pull request on privileged workflow. | +| .github/workflows/test3.yml:28:9:33:6 | Uses Step | Potential unsafe checkout of untrusted pull request on privileged workflow. | +| .github/workflows/test4.yml:18:7:25:4 | Uses Step | Potential unsafe checkout of untrusted pull request on privileged workflow. |