diff --git a/actions/ql/lib/change-notes/2026-06-15-permission_check.md b/actions/ql/lib/change-notes/2026-06-15-permission_check.md index b3b8f5faf00..6c918922239 100644 --- a/actions/ql/lib/change-notes/2026-06-15-permission_check.md +++ b/actions/ql/lib/change-notes/2026-06-15-permission_check.md @@ -1,4 +1,4 @@ --- category: fix --- -* GitHub Actions support for reusable workflows was improved. \ No newline at end of file +* GitHub Actions queries now better account for permission checks on jobs that call reusable workflows. \ No newline at end of file diff --git a/actions/ql/lib/codeql/actions/security/ControlChecks.qll b/actions/ql/lib/codeql/actions/security/ControlChecks.qll index 093dd1dac16..5a57e59c317 100644 --- a/actions/ql/lib/codeql/actions/security/ControlChecks.qll +++ b/actions/ql/lib/codeql/actions/security/ControlChecks.qll @@ -62,7 +62,7 @@ abstract class ControlCheck extends AstNode { predicate protects(AstNode node, Event event, string category) { // The check dominates the step it should protect - this.dominates(node) and + this.dominates(node, event) and // The check is effective against the event and category this.protectsCategoryAndEvent(category, event.getName()) and // The check can be triggered by the event @@ -72,7 +72,7 @@ abstract class ControlCheck extends AstNode { /** * Holds if this control check must execute and pass before `node` can run. */ - predicate dominates(AstNode node) { + predicate dominates(AstNode node, Event event) { // Step-level: the check is an `if:` on the step containing `node`, // or on the enclosing job, or on a needed job/step. this instanceof If and @@ -104,26 +104,34 @@ abstract class ControlCheck extends AstNode { ) or // When the node is inside a (possibly nested) reusable workflow, - // check if the control check dominates any caller job in the chain. - exists(ExternalJob directCaller, ExternalJob caller | + // all direct callers for this event must be protected along their caller chain. + exists(ExternalJob directCaller | directCaller = node.getEnclosingWorkflow().(ReusableWorkflow).getACaller() and - caller = getAnOuterCaller*(directCaller) and - ( - this instanceof If and + directCaller.getATriggerEvent() = event + ) and + forall(ExternalJob directCaller | + directCaller = node.getEnclosingWorkflow().(ReusableWorkflow).getACaller() and + directCaller.getATriggerEvent() = event + | + exists(ExternalJob caller | + caller = getAnOuterCaller*(directCaller) and ( - caller.getIf() = this or - caller.getANeededJob().(LocalJob).getIf() = this or - caller.getANeededJob().(LocalJob).getAStep().getIf() = this + 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 ) - 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/src/Security/CWE-285/ImproperAccessControl.ql b/actions/ql/src/Security/CWE-285/ImproperAccessControl.ql index ba002f16a87..aa16f3ab21b 100644 --- a/actions/ql/src/Security/CWE-285/ImproperAccessControl.ql +++ b/actions/ql/src/Security/CWE-285/ImproperAccessControl.ql @@ -18,7 +18,7 @@ from LocalJob job, LabelCheck check, MutableRefCheckoutStep checkout, Event even where job.isPrivileged() and job.getAStep() = checkout and - check.dominates(checkout) and + check.dominates(checkout, event) and ( job.getATriggerEvent() = event and event.getName() = "pull_request_target" and diff --git a/actions/ql/src/Security/CWE-829/UntrustedCheckoutHigh.ql b/actions/ql/src/Security/CWE-829/UntrustedCheckoutHigh.ql index 9f28706c0d0..56dc65beb5f 100644 --- a/actions/ql/src/Security/CWE-829/UntrustedCheckoutHigh.ql +++ b/actions/ql/src/Security/CWE-829/UntrustedCheckoutHigh.ql @@ -34,8 +34,8 @@ where check instanceof AssociationCheck or check instanceof PermissionCheck ) and - check.dominates(checkout) and - date_check.dominates(checkout) + check.dominates(checkout, event) and + date_check.dominates(checkout, event) ) or // not issue_comment triggered workflows diff --git a/actions/ql/test/query-tests/Security/CWE-829/.github/workflows/untrusted_checkout_permission_check_reusable2.yml b/actions/ql/test/query-tests/Security/CWE-829/.github/workflows/untrusted_checkout_permission_check_reusable2.yml new file mode 100644 index 00000000000..fa4bbfd9774 --- /dev/null +++ b/actions/ql/test/query-tests/Security/CWE-829/.github/workflows/untrusted_checkout_permission_check_reusable2.yml @@ -0,0 +1,31 @@ +on: + pull_request_target: + +jobs: + is-collaborator: + 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 + build_unsafe: + # needs: is-collaborator + uses: TestOrg/TestRepo/.github/workflows/build.yml@main + with: + COMMIT_SHA: ${{ github.event.pull_request.head.sha }} # should alert since no permission check + build_safe: + needs: is-collaborator + uses: TestOrg/TestRepo/.github/workflows/build.yml@main + with: + COMMIT_SHA: ${{ github.event.pull_request.head.sha }} # shouldn't alert since permission check diff --git a/actions/ql/test/query-tests/Security/CWE-829/.github/workflows/untrusted_checkout_permissions_check.yml b/actions/ql/test/query-tests/Security/CWE-829/.github/workflows/untrusted_checkout_permissions_check.yml index 720ca82f0b9..c2349895863 100644 --- a/actions/ql/test/query-tests/Security/CWE-829/.github/workflows/untrusted_checkout_permissions_check.yml +++ b/actions/ql/test/query-tests/Security/CWE-829/.github/workflows/untrusted_checkout_permissions_check.yml @@ -29,3 +29,13 @@ jobs: ref: ${{ github.event.pull_request.head.sha }} # shouldn't alert since permission check fetch-depth: 2 - run: yarn test + build_unsafe: + runs-on: ubuntu-latest + # needs: is-collaborator + steps: + - name: Checkout repo + uses: actions/checkout@4 + with: + ref: ${{ github.event.pull_request.head.sha }} # should alert since no permission check + fetch-depth: 2 + - run: yarn test \ No newline at end of file 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 9a544ceba2e..1e585d2ed69 100644 --- a/actions/ql/test/query-tests/Security/CWE-829/UntrustedCheckoutCritical.expected +++ b/actions/ql/test/query-tests/Security/CWE-829/UntrustedCheckoutCritical.expected @@ -337,11 +337,13 @@ edges | .github/workflows/untrusted_checkout_6.yml:17:9:21:6 | Uses Step | .github/workflows/untrusted_checkout_6.yml:21:9:23:23 | Run Step | | .github/workflows/untrusted_checkout_no_needs.yml:8:9:16:6 | Uses Step: checkAccess | .github/workflows/untrusted_checkout_no_needs.yml:16:9:22:2 | Run Step | | .github/workflows/untrusted_checkout_no_needs.yml:26:9:31:6 | Uses Step | .github/workflows/untrusted_checkout_no_needs.yml:31:9:31:23 | Run Step | +| .github/workflows/untrusted_checkout_permission_check_reusable2.yml:8:9:16:6 | Uses Step: checkAccess | .github/workflows/untrusted_checkout_permission_check_reusable2.yml:16:9:22:2 | Run Step | | .github/workflows/untrusted_checkout_permission_check_reusable.yml:8:9:16:6 | Uses Step: checkAccess | .github/workflows/untrusted_checkout_permission_check_reusable.yml:16:9:22:2 | Run Step | | .github/workflows/untrusted_checkout_permission_check_reusable_level2.yml:8:9:16:6 | Uses Step: checkAccess | .github/workflows/untrusted_checkout_permission_check_reusable_level2.yml:16:9:22:2 | Run Step | | .github/workflows/untrusted_checkout_permission_check_reusable_no_needs.yml:8:9:16:6 | Uses Step: checkAccess | .github/workflows/untrusted_checkout_permission_check_reusable_no_needs.yml:16:9:22:2 | Run Step | | .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:31:23 | 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/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 | @@ -352,6 +354,7 @@ edges | .github/workflows/auto_ci.yml:67:9:74:6 | Uses Step | .github/workflows/auto_ci.yml:67:9:74:6 | Uses Step | .github/workflows/auto_ci.yml:79:9:84:6 | Run Step | Checkout of untrusted code in a privileged workflow with later potential execution (event trigger: $@). | .github/workflows/auto_ci.yml:6:3:6:21 | pull_request_target | pull_request_target | | .github/workflows/auto_ci.yml:67:9:74:6 | Uses Step | .github/workflows/auto_ci.yml:67:9:74:6 | Uses Step | .github/workflows/auto_ci.yml:84:9:93:6 | Run Step | Checkout of untrusted code in a privileged workflow with later potential execution (event trigger: $@). | .github/workflows/auto_ci.yml:6:3:6:21 | pull_request_target | pull_request_target | | .github/workflows/dependabot3.yml:15:9:20:6 | Uses Step | .github/workflows/dependabot3.yml:15:9:20:6 | Uses Step | .github/workflows/dependabot3.yml:25:9:48:6 | Run Step: set-milestone | Checkout of untrusted code in a privileged workflow with later potential execution (event trigger: $@). | .github/workflows/dependabot3.yml:3:5:3:23 | pull_request_target | pull_request_target | +| .github/workflows/external/TestOrg/TestRepo/.github/workflows/build.yml:11:9:14:6 | Uses Step | .github/workflows/external/TestOrg/TestRepo/.github/workflows/build.yml:11:9:14:6 | Uses Step | .github/workflows/external/TestOrg/TestRepo/.github/workflows/build.yml:14:9:17:7 | Run Step | Checkout of untrusted code in a privileged workflow with later potential execution (event trigger: $@). | .github/workflows/untrusted_checkout_permission_check_reusable2.yml:2:3:2:21 | pull_request_target | pull_request_target | | .github/workflows/external/TestOrg/TestRepo/.github/workflows/build.yml:11:9:14:6 | Uses Step | .github/workflows/external/TestOrg/TestRepo/.github/workflows/build.yml:11:9:14:6 | Uses Step | .github/workflows/external/TestOrg/TestRepo/.github/workflows/build.yml:14:9:17:7 | Run Step | Checkout of untrusted code in a privileged workflow with later potential execution (event trigger: $@). | .github/workflows/untrusted_checkout_permission_check_reusable_no_needs.yml:2:3:2:21 | pull_request_target | pull_request_target | | .github/workflows/external/TestOrg/TestRepo/.github/workflows/reusable.yml:23:9:26:6 | Uses Step | .github/workflows/external/TestOrg/TestRepo/.github/workflows/reusable.yml:23:9:26:6 | Uses Step | .github/workflows/external/TestOrg/TestRepo/.github/workflows/reusable.yml:26:9:29:7 | Run Step | Checkout of untrusted code in a privileged workflow with later potential execution (event trigger: $@). | .github/workflows/reusable_caller1.yaml:4:3:4:21 | pull_request_target | pull_request_target | | .github/workflows/gitcheckout.yml:10:11:18:8 | Run Step | .github/workflows/gitcheckout.yml:10:11:18:8 | Run Step | .github/workflows/gitcheckout.yml:21:11:23:22 | Run Step | Checkout of untrusted code in a privileged workflow with later potential execution (event trigger: $@). | .github/workflows/gitcheckout.yml:2:3:2:21 | pull_request_target | pull_request_target | @@ -387,3 +390,4 @@ edges | .github/workflows/untrusted_checkout.yml:8:9:11:6 | Uses Step | .github/workflows/untrusted_checkout.yml:8:9:11:6 | Uses Step | .github/workflows/untrusted_checkout.yml:15:9:18:2 | Run Step | Checkout of untrusted code in a privileged workflow with later potential execution (event trigger: $@). | .github/workflows/untrusted_checkout.yml:2:3:2:21 | pull_request_target | pull_request_target | | .github/workflows/untrusted_checkout.yml:23:9:26:6 | Uses Step | .github/workflows/untrusted_checkout.yml:23:9:26:6 | Uses Step | .github/workflows/untrusted_checkout.yml:30:9:32:23 | Run Step | Checkout of untrusted code in a privileged workflow with later potential execution (event trigger: $@). | .github/workflows/untrusted_checkout.yml:2:3:2:21 | pull_request_target | pull_request_target | | .github/workflows/untrusted_checkout_no_needs.yml:26:9:31:6 | Uses Step | .github/workflows/untrusted_checkout_no_needs.yml:26:9:31:6 | Uses Step | .github/workflows/untrusted_checkout_no_needs.yml:31:9:31:23 | Run Step | Checkout of untrusted code in a privileged workflow with later potential execution (event trigger: $@). | .github/workflows/untrusted_checkout_no_needs.yml:2:3:2:21 | pull_request_target | pull_request_target | +| .github/workflows/untrusted_checkout_permissions_check.yml:36:9:41:6 | Uses 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 | Checkout of untrusted code in a privileged workflow with later potential execution (event trigger: $@). | .github/workflows/untrusted_checkout_permissions_check.yml:2:3:2:21 | pull_request_target | pull_request_target |