Merge pull request #37 from github/toctou_refinements

Reduce FP for actor/association checks that cannot be bypassed this way

Author: Alvaro Muñoz

ql/src/Security/CWE-367/UntrustedCheckoutTOCTOUCritical.ql

@@ -20,6 +20,14 @@ where
   // the mutable checkout step is protected by an access check
   check = [checkout.getIf(), checkout.getEnclosingJob().getIf()] and
   // the checked-out code may lead to arbitrary code execution
-  checkout.getAFollowingStep() instanceof PoisonableStep
+  checkout.getAFollowingStep() instanceof PoisonableStep and
+  (
+    // label gates do not depend on the triggering event
+    check instanceof LabelControlCheck
+    or
+    // actor or Association gates apply to IssueOps only
+    (check instanceof AssociationControlCheck or check instanceof ActorControlCheck) and
+    check.getEnclosingJob().getATriggerEvent().getName().matches("%_comment")
+  )
 select checkout, "The checked-out code can be changed after the authorization check o step $@.",
   check, check.toString()

ql/src/Security/CWE-367/UntrustedCheckoutTOCTOUHigh.ql

@@ -20,6 +20,14 @@ where
   // the mutable checkout step is protected by an access check
   check = [checkout.getIf(), checkout.getEnclosingJob().getIf()] and
   // there are no evidences that the  checked-out code can lead to arbitrary code execution
-  not checkout.getAFollowingStep() instanceof PoisonableStep
+  not checkout.getAFollowingStep() instanceof PoisonableStep and
+  (
+    // label gates do not depend on the triggering event
+    check instanceof LabelControlCheck
+    or
+    // actor or Association gates apply to IssueOps only
+    (check instanceof AssociationControlCheck or check instanceof ActorControlCheck) and
+    check.getEnclosingJob().getATriggerEvent().getName().matches("%_comment")
+  )
 select checkout, "The checked-out code can be changed after the authorization check o step $@.",
   check, check.toString()

ql/test/query-tests/Security/CWE-367/.github/workflows/label_actor.yml

@@ -0,0 +1,17 @@
+# Making Label gates the only ones bypassable with TOCTOU races since actor or association ones should not be bypassable
+name: Label Trigger Test
+on:
+  pull_request_target:
+    types: [labeled]
+    branches: [main]
+
+jobs:
+  integration-tests:
+    runs-on: ubuntu-latest
+    if: github.repository_owner == 'npm' && github.actor == 'dependabot[bot]'
+    steps:
+      - uses: actions/checkout@v4
+        with:
+          ref: ${{ github.event.pull_request.head.ref }}
+          repository: ${{ github.event.pull_request.head.repo.full_name }}
+      - run: bash label_example/tests.sh