From e2347a5c7d69fd3651a88dd92a7b4e2d8f546cf5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jaroslav=20Loba=C4=8Devski?= Date: Tue, 23 Jun 2026 11:52:11 +0000 Subject: [PATCH] Fix for independent checks --- .../codeql/actions/security/ControlChecks.qll | 72 +++++++++++-------- ...ed_checkout_two_callers_both_protected.yml | 48 +++++++++++++ .../UntrustedCheckoutCritical.expected | 2 + 3 files changed, 94 insertions(+), 28 deletions(-) create mode 100644 actions/ql/test/query-tests/Security/CWE-829/.github/workflows/untrusted_checkout_two_callers_both_protected.yml diff --git a/actions/ql/lib/codeql/actions/security/ControlChecks.qll b/actions/ql/lib/codeql/actions/security/ControlChecks.qll index 0181aab8a64..c3250ca09c0 100644 --- a/actions/ql/lib/codeql/actions/security/ControlChecks.qll +++ b/actions/ql/lib/codeql/actions/security/ControlChecks.qll @@ -66,7 +66,22 @@ abstract class ControlCheck extends AstNode { // The check is effective against the event and category this.protectsCategoryAndEvent(category, event.getName()) and // The check can be triggered by the event - this.getATriggerEvent() = event + this.getATriggerEvent() = event and + // For reusable workflows, ALL callers for this event must be protected by SOME check + ( + not node.getEnclosingWorkflow() instanceof ReusableWorkflow + or + forall(ExternalJob directCaller | + directCaller = node.getEnclosingWorkflow().(ReusableWorkflow).getACaller() and + directCaller.getATriggerEvent() = event + | + exists(ControlCheck check | + check.protectsCategoryAndEvent(category, event.getName()) and + check.getATriggerEvent() = event and + check.dominatesViaCaller(node, event, directCaller) + ) + ) + ) } /** @@ -103,35 +118,36 @@ abstract class ControlCheck extends AstNode { node.getEnclosingJob().getANeededJob().(LocalJob).getAStep() = this ) or - // When the node is inside a (possibly nested) reusable workflow, - // all direct callers for this event must be protected along their caller chain. - exists(ExternalJob directCaller | - directCaller = node.getEnclosingWorkflow().(ReusableWorkflow).getACaller() and - directCaller.getATriggerEvent() = event - ) and - forall(ExternalJob directCaller | - directCaller = node.getEnclosingWorkflow().(ReusableWorkflow).getACaller() and - directCaller.getATriggerEvent() = event - | - exists(ExternalJob caller | - caller = getAnOuterCaller*(directCaller) and + // When the node is inside a reusable workflow, + // this check dominates via at least one caller chain. + this.dominatesViaCaller(node, event, _) + } + + /** + * Holds if this control check dominates `node` in a reusable workflow + * via the caller chain starting at `directCaller`. + */ + predicate dominatesViaCaller(AstNode node, Event event, ExternalJob directCaller) { + directCaller = node.getEnclosingWorkflow().(ReusableWorkflow).getACaller() and + directCaller.getATriggerEvent() = event and + exists(ExternalJob caller | + caller = getAnOuterCaller*(directCaller) and + ( + this instanceof If and ( - this instanceof If and - ( - caller.getIf() = this or - caller.getANeededJob().(LocalJob).getIf() = this or - caller.getANeededJob().(LocalJob).getAStep().getIf() = this - ) - or - this instanceof Environment and - ( - caller.getEnvironment() = this or - caller.getANeededJob().getEnvironment() = this - ) - or - (this instanceof Run or this instanceof UsesStep) and - caller.getANeededJob().(LocalJob).getAStep() = this + caller.getIf() = this or + caller.getANeededJob().(LocalJob).getIf() = this or + caller.getANeededJob().(LocalJob).getAStep().getIf() = this ) + or + this instanceof Environment and + ( + caller.getEnvironment() = this or + caller.getANeededJob().getEnvironment() = this + ) + or + (this instanceof Run or this instanceof UsesStep) and + caller.getANeededJob().(LocalJob).getAStep() = this ) ) } diff --git a/actions/ql/test/query-tests/Security/CWE-829/.github/workflows/untrusted_checkout_two_callers_both_protected.yml b/actions/ql/test/query-tests/Security/CWE-829/.github/workflows/untrusted_checkout_two_callers_both_protected.yml new file mode 100644 index 00000000000..6e842fc158e --- /dev/null +++ b/actions/ql/test/query-tests/Security/CWE-829/.github/workflows/untrusted_checkout_two_callers_both_protected.yml @@ -0,0 +1,48 @@ +on: + pull_request_target: + +jobs: + is-collaborator-a: + runs-on: ubuntu-latest + steps: + - name: Get User Permission + id: checkAccess + uses: actions-cool/check-user-permission@cd622002ff25c2311d2e7fb82107c0d24be83f9b + with: + require: write + username: ${{ github.actor }} + env: + GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} + - name: Check User Permission + if: steps.checkAccess.outputs.require-result == 'false' + run: | + echo "${{ github.actor }} does not have permissions on this repo." + echo "Current permission level is ${{ steps.checkAccess.outputs.user-permission }}" + exit 1 + caller-a: + needs: is-collaborator-a + uses: TestOrg/TestRepo/.github/workflows/build.yml@main + with: + COMMIT_SHA: ${{ github.event.pull_request.head.sha }} + is-collaborator-b: + runs-on: ubuntu-latest + steps: + - name: Get User Permission + id: checkAccess + uses: actions-cool/check-user-permission@cd622002ff25c2311d2e7fb82107c0d24be83f9b + with: + require: write + username: ${{ github.actor }} + env: + GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} + - name: Check User Permission + if: steps.checkAccess.outputs.require-result == 'false' + run: | + echo "${{ github.actor }} does not have permissions on this repo." + echo "Current permission level is ${{ steps.checkAccess.outputs.user-permission }}" + exit 1 + caller-b: + needs: is-collaborator-b + uses: TestOrg/TestRepo/.github/workflows/build.yml@main + with: + COMMIT_SHA: ${{ github.event.pull_request.head.sha }} diff --git a/actions/ql/test/query-tests/Security/CWE-829/UntrustedCheckoutCritical.expected b/actions/ql/test/query-tests/Security/CWE-829/UntrustedCheckoutCritical.expected index 1e585d2ed69..2da0e4f61ec 100644 --- a/actions/ql/test/query-tests/Security/CWE-829/UntrustedCheckoutCritical.expected +++ b/actions/ql/test/query-tests/Security/CWE-829/UntrustedCheckoutCritical.expected @@ -344,6 +344,8 @@ edges | .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 | | .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 | | .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 | +| .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 | +| .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 | | .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 | | .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 | | .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 |