Merge pull request #101 from github/control_checks/toctou_split

Improve control checks to better account for toctou issues

Author: Alvaro Muñoz

ql/lib/codeql/actions/security/ControlChecks.qll

@@ -1,30 +1,44 @@
 import actions
 
-string any_relevant_category() {
+string any_category() {
   result =
     [
       "untrusted-checkout", "output-clobbering", "envpath-injection", "envvar-injection",
       "command-injection", "argument-injection", "code-injection", "cache-poisoning",
-      "untrusted-checkout-toctou", "artifact-poisoning"
+      "untrusted-checkout-toctou", "artifact-poisoning", "artifact-poisoning-toctou"
     ]
 }
 
-string any_non_toctou_category() {
-  result = any_relevant_category() and not result = "untrusted-checkout-toctou"
+string non_toctou_category() {
+  result = any_category() and not result = "untrusted-checkout-toctou"
 }
 
-string any_relevant_event() {
+string toctou_category() { result = ["untrusted-checkout-toctou", "artifact-poisoning-toctou"] }
+
+string any_event() { result = actor_not_attacker_event() or result = actor_is_attacker_event() }
+
+string actor_is_attacker_event() {
   result =
     [
+      // actor and attacker have to be the same
       "pull_request_target",
-      "issue_comment",
-      "pull_request_comment",
       "workflow_run",
+      "discussion_comment",
+      "discussion",
       "issues",
       "fork",
-      "watch",
-      "discussion_comment",
-      "discussion"
+      "watch"
+    ]
+}
+
+string actor_not_attacker_event() {
+  result =
+    [
+      // actor and attacker can be different
+      // actor may be a collaborator, but the attacker is may be the author of the PR that gets commented
+      // therefore it may be vulnerable to TOCTOU races where the actor reviews one thing and the attacker changes it
+      "issue_comment",
+      "pull_request_comment",
     ]
 }
 
@@ -81,7 +95,9 @@ abstract class AssociationCheck extends ControlCheck {
   // - they are effective against pull requests and workflow_run (since these are triggered by pull_requests) since they can control who is making the PR
   // - they are not effective against issue_comment since the author of the comment may not be the same as the author of the PR
   override predicate protectsCategoryAndEvent(string category, string event) {
-    event = ["pull_request_target", "workflow_run"] and category = any_relevant_category()
+    event = actor_is_attacker_event() and category = any_category()
+    or
+    event = actor_not_attacker_event() and category = non_toctou_category()
   }
 }
 
@@ -90,7 +106,9 @@ abstract class ActorCheck extends ControlCheck {
   // - they are effective against pull requests and workflow_run (since these are triggered by pull_requests) since they can control who is making the PR
   // - they are not effective against issue_comment since the author of the comment may not be the same as the author of the PR
   override predicate protectsCategoryAndEvent(string category, string event) {
-    event = ["pull_request_target", "workflow_run"] and category = any_relevant_category()
+    event = actor_is_attacker_event() and category = any_category()
+    or
+    event = actor_not_attacker_event() and category = non_toctou_category()
   }
 }
 
@@ -106,31 +124,36 @@ abstract class PermissionCheck extends ControlCheck {
   // - they are effective against pull requests/workflow_run since they can control who can make changes
   // - they are not effective against issue_comment since the author of the comment may not be the same as the author of the PR
   override predicate protectsCategoryAndEvent(string category, string event) {
-    event = ["pull_request_target", "workflow_run", "issue_comment"] and
-    category = any_relevant_category()
+    event = actor_is_attacker_event() and category = any_category()
+    or
+    event = actor_not_attacker_event() and category = non_toctou_category()
   }
 }
 
 abstract class LabelCheck extends ControlCheck {
   // checks if the issue/pull_request is labeled, which implies that it could have been approved
   // - they dont protect against mutation attacks
   override predicate protectsCategoryAndEvent(string category, string event) {
-    event = ["pull_request_target", "workflow_run"] and category = any_non_toctou_category()
+    event = actor_is_attacker_event() and category = any_category()
+    or
+    event = actor_not_attacker_event() and category = non_toctou_category()
   }
 }
 
 class EnvironmentCheck extends ControlCheck instanceof Environment {
   // Environment checks are not effective against any mutable attacks
   // they do actually protect against untrusted code execution (sha)
   override predicate protectsCategoryAndEvent(string category, string event) {
-    event = ["pull_request_target", "workflow_run"] and category = any_non_toctou_category()
+    event = actor_is_attacker_event() and category = any_category()
+    or
+    event = actor_not_attacker_event() and category = non_toctou_category()
   }
 }
 
 abstract class CommentVsHeadDateCheck extends ControlCheck {
   override predicate protectsCategoryAndEvent(string category, string event) {
     // by itself, this check is not effective against any attacks
-    none()
+    event = actor_not_attacker_event() and category = toctou_category()
   }
 }
 
@@ -187,7 +210,7 @@ class PullRequestTargetRepositoryIfCheck extends RepositoryCheck instanceof If {
   }
 
   override predicate protectsCategoryAndEvent(string category, string event) {
-    event = "pull_request_target" and category = any_relevant_category()
+    event = "pull_request_target" and category = any_category()
   }
 }
 
@@ -205,7 +228,7 @@ class WorkflowRunRepositoryIfCheck extends RepositoryCheck instanceof If {
   }
 
   override predicate protectsCategoryAndEvent(string category, string event) {
-    event = "workflow_run" and category = any_relevant_category()
+    event = "workflow_run" and category = any_category()
   }
 }
 
@@ -250,6 +273,9 @@ class PermissionActionCheck extends PermissionCheck instanceof UsesStep {
       not exists(this.getArgument("permission-level")) or
       this.getArgument("permission-level") = ["write", "admin"]
     )
+    or
+    this.getCallee() = "actions/github-script" and
+    this.getArgument("script").splitAt("\n").matches("%getCollaboratorPermissionLevel%")
   }
 }
 

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

@@ -18,18 +18,14 @@ import codeql.actions.security.ControlChecks
 
 query predicate edges(Step a, Step b) { a.getNextStep() = b }
 
-from
-  LocalJob job, MutableRefCheckoutStep checkout, PoisonableStep step, ControlCheck check,
-  Event event
+from MutableRefCheckoutStep checkout, PoisonableStep step, Event event
 where
-  job.getAStep() = checkout and
   // the checked-out code may lead to arbitrary code execution
   checkout.getAFollowingStep() = step and
   // the checkout occurs in a privileged context
   inPrivilegedContext(checkout, event) and
   // the mutable checkout step is protected by an Insufficient access check
-  check.protects(checkout, event, "untrusted-checkout") and
-  not check.protects(checkout, event, "untrusted-checkout-toctou")
+  exists(ControlCheck check1 | check1.protects(checkout, event, "untrusted-checkout")) and
+  not exists(ControlCheck check2 | check2.protects(checkout, event, "untrusted-checkout-toctou"))
 select step, checkout, step,
-  "Insufficient protection against execution of untrusted code on a privileged workflow on check $@.",
-  check, check.toString()
+  "Insufficient protection against execution of untrusted code on a privileged workflow."

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

@@ -16,17 +16,14 @@ import codeql.actions.security.UntrustedCheckoutQuery
 import codeql.actions.security.PoisonableSteps
 import codeql.actions.security.ControlChecks
 
-from LocalJob job, MutableRefCheckoutStep checkout, ControlCheck check, Event event
+from MutableRefCheckoutStep checkout, Event event
 where
-  job.getAStep() = checkout and
   // there are no evidences that the checked-out gets executed
   not checkout.getAFollowingStep() instanceof PoisonableStep and
   // the checkout occurs in a privileged context
   inPrivilegedContext(checkout, event) and
-  event = job.getATriggerEvent() and
   // the mutable checkout step is protected by an Insufficient access check
-  check.protects(checkout, event, "untrusted-checkout") and
-  not check.protects(checkout, event, "untrusted-checkout-toctou")
+  exists(ControlCheck check1 | check1.protects(checkout, event, "untrusted-checkout")) and
+  not exists(ControlCheck check2 | check2.protects(checkout, event, "untrusted-checkout-toctou"))
 select checkout,
-  "Insufficient protection against execution of untrusted code on a privileged workflow on step $@.",
-  check, check.toString()
+  "Insufficient protection against execution of untrusted code on a privileged workflow."

ql/src/Security/CWE-829/UntrustedCheckoutCritical.ql

@@ -26,22 +26,6 @@ where
   checkout.getAFollowingStep() = step and
   // the checkout occurs in a privileged context
   inPrivilegedContext(step, event) and
-  (
-    // issue_comment: check for date comparison checks and actor/access control checks
-    event.getName() = "issue_comment" and
-    not exists(ControlCheck check, CommentVsHeadDateCheck date_check |
-      (
-        check instanceof ActorCheck or
-        check instanceof AssociationCheck or
-        check instanceof PermissionCheck
-      ) and
-      check.dominates(step) and
-      date_check.dominates(step)
-    )
-    or
-    // not issue_comment triggered workflows
-    not event.getName() = "issue_comment" and
-    not exists(ControlCheck check | check.protects(step, event, "untrusted-checkout"))
-  )
+  not exists(ControlCheck check | check.protects(step, event, "untrusted-checkout"))
 select step, checkout, step, "Execution of untrusted code on a privileged workflow. $@", event,
   event.getLocation().getFile().toString()

…CWE-367/.github/workflows/deployment.yml …WE-367/.github/workflows/deployment1.ymlql/test/query-tests/Security/CWE-367/.github/workflows/deployment.yml renamed to ql/test/query-tests/Security/CWE-367/.github/workflows/deployment1.yml ql/test/query-tests/Security/CWE-367/.github/workflows/deployment.yml renamed to ql/test/query-tests/Security/CWE-367/.github/workflows/deployment1.yml

@@ -0,0 +1,31 @@
+# https://github.com/AdnaneKhan/ActionsTOCTOU/blob/main/.github/workflows/deployment_victim.yml
+name: Environment PR Check
+
+on:
+  pull_request_target:
+    branches:
+      - main
+    paths:
+      - 'README.md'
+  workflow_dispatch:
+jobs:
+   test:
+     environment: Public CI
+     runs-on: ubuntu-latest
+     steps:
+       - name: Checkout from PR branch  
+         uses: actions/checkout@v4
+         with: 
+          repository: ${{ github.event.pull_request.head.repo.full_name }}
+          ref: ${{ github.event.pull_request.head.sha }}
+
+       - name: Set Node.js 20.x for GitHub Action
+         uses: actions/setup-node@v4
+         with:
+           node-version: 20.x
+
+       - name: installing node_modules
+         run: cd deployment_example && npm install 
+
+       - name: Build GitHub Action
+         run: cd deployment_example && npm run build

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

@@ -0,0 +1,68 @@
+# https://github.com/AdnaneKhan/ActionsTOCTOU/blob/main/.github/workflows/comment_victim.yml
+name: Comment Triggered Test
+on:
+  issue_comment:
+    types: [created]
+permissions: 'write-all'
+jobs:
+  test1:
+    if: ${{ github.event.issue.pull_request && contains(fromJson('["MEMBER", "OWNER"]'), github.event.comment.author_association) && startsWith(github.event.comment.body, '/run-tests ') }}
+    runs-on: ubuntu-latest
+    steps:
+
+      - uses: actions/github-script@v6
+        name: Get PR branch
+        id: issue
+        with:
+          script: |
+            const pr = context.payload.issue.number
+            const data = await github.rest.pulls.get({
+              owner: context.repo.owner,
+              repo: context.repo.repo,
+              pull_number: pr
+            })
+            return {
+              ref: data.data.head.ref,
+              sha: data.data.head.sha,
+            }
+      - uses: actions/checkout@v4
+        with:
+          submodules: recursive
+          ref: ${{ fromJson(steps.issue.outputs.result).sha }}
+      - run: bash comment_example/tests.sh
+
+  test2:
+    if: ${{ github.event.issue.pull_request && contains(fromJson('["MEMBER", "OWNER"]'), github.event.comment.author_association) && startsWith(github.event.comment.body, '/run-tests ') }}
+    runs-on: ubuntu-latest
+    steps:
+
+      - uses: actions/github-script@v6
+        name: Get PR branch
+        id: issue
+        with:
+          script: |
+            const pr = context.payload.issue.number
+            const data = await github.rest.pulls.get({
+              owner: context.repo.owner,
+              repo: context.repo.repo,
+              pull_number: pr
+            })
+            return {
+              ref: data.data.head.ref,
+              sha: data.data.head.sha,
+            }
+      - uses: actions/checkout@v4
+        with:
+          submodules: recursive
+          ref: ${{ fromJson(steps.issue.outputs.result).ref }}
+      - run: bash comment_example/tests.sh
+
+  test3:
+    if: ${{ github.event.issue.pull_request && contains(fromJson('["MEMBER", "OWNER"]'), github.event.comment.author_association) && startsWith(github.event.comment.body, '/run-tests ') }}
+    runs-on: ubuntu-latest
+    steps:
+      - uses: actions/checkout@v4
+        with:
+          submodules: recursive
+          ref: "refs/pull/${{ github.event.number }}/merge"
+      - run: bash comment_example/tests.sh