From 6df70d1a455f67ce3a174ea0dda7ea9384fec8ad Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Alvaro=20Mu=C3=B1oz?= <pwntester@github.com>
Date: Sun, 23 Jun 2024 21:34:30 +0200
Subject: [PATCH] Do not consider priv events if runtime data is available

---
 ql/lib/codeql/actions/ast/internal/Ast.qll    | 21 +++++++---
 .../CWE-829/.github/workflows/test3.yml       | 41 +++++++++++++++++++
 .../UntrustedCheckoutCritical.expected        |  1 +
 3 files changed, 58 insertions(+), 5 deletions(-)
 create mode 100644 ql/test/query-tests/Security/CWE-829/.github/workflows/test3.yml

diff --git a/ql/lib/codeql/actions/ast/internal/Ast.qll b/ql/lib/codeql/actions/ast/internal/Ast.qll
index d4864a80e54..da54833e9a6 100644
--- a/ql/lib/codeql/actions/ast/internal/Ast.qll
+++ b/ql/lib/codeql/actions/ast/internal/Ast.qll
@@ -853,6 +853,14 @@ class JobImpl extends AstNodeImpl, TJobNode {
     this.getEnclosingWorkflow().getPermissions().getAPermission().matches("%write")
   }
 
+  private predicate hasRuntimeData() {
+    exists(string path, string trigger, string name, string secrets_source, string perms |
+      workflowDataModel(path, trigger, name, secrets_source, perms, _) and
+      path.trim() = this.getLocation().getFile().getRelativePath() and
+      name.trim().matches(this.getId() + "%")
+    )
+  }
+
   private predicate hasRuntimeWritePermissions() {
     // the effective runtime permissions have write access
     exists(string path, string trigger, string name, string secrets_source, string perms |
@@ -885,15 +893,18 @@ class JobImpl extends AstNodeImpl, TJobNode {
   /** Holds if the action is privileged and externally triggerable. */
   predicate isPrivilegedExternallyTriggerable() {
     exists(EventImpl e |
-      // job is triggereable by an external user
       this.getATriggerEvent() = e and
+      // job is triggereable by an external user
       e.isExternallyTriggerable() and
-      // job is privileged (write access or access to secrets)
+      // no matter if `pull_request` is granted write permissions or access to secrets
+      // when the job is triggered by a `pull_request` event from a fork, they will get revoked
+      not e.getName() = "pull_request" and
       (
-        this.isPrivileged() and
-        not e.getName() = "pull_request"
+        // job is privileged (write access or access to secrets)
+        this.isPrivileged()
         or
-        not this.isPrivileged() and
+        // the trigger event is __normally__ privileged and we have no runtime data to prove otherwise
+        not this.hasRuntimeData() and
         e.isPrivileged()
       )
     )
diff --git a/ql/test/query-tests/Security/CWE-829/.github/workflows/test3.yml b/ql/test/query-tests/Security/CWE-829/.github/workflows/test3.yml
new file mode 100644
index 00000000000..d9aa2973e00
--- /dev/null
+++ b/ql/test/query-tests/Security/CWE-829/.github/workflows/test3.yml
@@ -0,0 +1,41 @@
+name: "Test"
+permissions:
+  actions: none
+  checks: none
+  contents: read
+  deployments: none
+  id-token: none
+  issues: none
+  discussions: none
+  packages: none
+  pages: none
+  pull-requests: read
+  repository-projects: none
+  security-events: none
+  statuses: none
+on:
+  pull_request_target:
+    types:
+      - opened
+      - edited
+      - synchronize
+
+jobs:
+  main:
+    name: Test Pull Request
+    runs-on: ubuntu-latest
+    steps:
+      - name: Checkout
+        uses: actions/checkout@v4
+        with:
+          ref: ${{ github.event.pull_request.head.ref }}
+          repository: ${{ github.event.pull_request.head.repo.full_name }}
+      - run: npm install
+        working-directory: scripts/github-actions/semantic-pull-request/
+      - name: Lint PR Title
+        if: github.event_name == 'pull_request_target'
+        uses: actions/github-script@v7
+        with:
+          script: |
+            const verifyPullRequest = require('./scripts/github-actions/semantic-pull-request')
+            await verifyPullRequest({ context, core, github })
diff --git a/ql/test/query-tests/Security/CWE-829/UntrustedCheckoutCritical.expected b/ql/test/query-tests/Security/CWE-829/UntrustedCheckoutCritical.expected
index 92d5a0b5ce1..0ff47fd2c53 100644
--- a/ql/test/query-tests/Security/CWE-829/UntrustedCheckoutCritical.expected
+++ b/ql/test/query-tests/Security/CWE-829/UntrustedCheckoutCritical.expected
@@ -4,5 +4,6 @@
 | .github/workflows/level0.yml:99:9:103:6 | Uses Step | Potential unsafe checkout of untrusted pull request on privileged workflow. |
 | .github/workflows/level0.yml:125:9:129:6 | Uses Step | Potential unsafe checkout of untrusted pull request on privileged workflow. |
 | .github/workflows/mend.yml:22:9:29:6 | Uses Step | Potential unsafe checkout of untrusted pull request on privileged workflow. |
+| .github/workflows/test3.yml:28:9:33:6 | Uses Step | Potential unsafe checkout of untrusted pull request on privileged workflow. |
 | .github/workflows/untrusted_checkout.yml:10:9:13:6 | Uses Step | Potential unsafe checkout of untrusted pull request on privileged workflow. |
 | .github/workflows/untrusted_checkout.yml:13:9:16:6 | Uses Step | Potential unsafe checkout of untrusted pull request on privileged workflow. |