Fix false negatives when one of the jobs had proper checks and the other didn't

2026-06-25 14:47:04 +02:00 · 2026-06-18 12:02:56 +00:00
parent d51a9a3e1a
commit 2d6feb1255
7 changed files with 77 additions and 24 deletions
--- 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.
+* GitHub Actions queries now better account for permission checks on jobs that call reusable workflows.
--- 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
      )
    )
  }
--- 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
--- 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
--- 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
@@ -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
--- 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
--- 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 |