Improve query

Author: Alvaro Muñoz

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

@@ -21,7 +21,14 @@ class SetupJavaUsesStep extends CacheWritingStep, UsesStep {
 }
 
 class SetupGoUsesStep extends CacheWritingStep, UsesStep {
-  SetupGoUsesStep() { this.getCallee() = "actions/setup-go" }
+  SetupGoUsesStep() {
+    this.getCallee() = "actions/setup-go" and
+    (
+      not exists(this.getArgument("cache"))
+      or
+      this.getArgument("cache") = "true"
+    )
+  }
 }
 
 class SetupNodeUsesStep extends CacheWritingStep, UsesStep {
@@ -48,7 +55,7 @@ class SetupDotnetUsesStep extends CacheWritingStep, UsesStep {
   SetupDotnetUsesStep() {
     this.getCallee() = "actions/setup-dotnet" and
     (
-      exists(this.getArgument("cache")) or
+      this.getArgument("cache") = "true" or
       exists(this.getArgument("cache-dependency-path"))
     )
   }

ql/src/Security/CWE-349/CachePoisoning.ql

@@ -14,8 +14,9 @@
 import actions
 import codeql.actions.security.UntrustedCheckoutQuery
 import codeql.actions.security.CachePoisoningQuery
+import codeql.actions.security.PoisonableSteps
 
-from LocalJob j
+from LocalJob j, PRHeadCheckoutStep checkout
 where
   // The workflow runs in the context of the default branch
   // TODO: (require to collect trigger types)
@@ -29,7 +30,14 @@ where
           "repository_dispatch", "schedule", "watch", "workflow_run"
         ]) and
   // The job checkouts untrusted code from a pull request
-  j.getAStep() instanceof PRHeadCheckoutStep and
-  // The job writes to the cache
-  j.getAStep() instanceof CacheWritingStep
+  j.getAStep() = checkout and
+  (
+    // The job writes to the cache
+    // (No need to follow the checkout step as the cache writing is normally done after the job completes)
+    j.getAStep() instanceof CacheWritingStep
+    or
+    // The job executes checked-out code
+    // (The cache specific token can be leaked even for non-privileged workflows)
+    checkout.getAFollowingStep() instanceof PoisonableStep
+  )
 select j.getAStep().(CacheWritingStep), "Potential cache poisoning on privileged workflow."

ql/test/query-tests/Security/CWE-349/.github/workflows/test5.yml

@@ -0,0 +1,17 @@
+name: Cache Poisoning
+
+on: pull_request_target
+
+jobs:
+  poison:
+    runs-on: ubuntu-latest
+    steps:
+      - uses: actions/checkout@v3
+        with:
+          ref: ${{ github.event.pull_request.head.sha }}
+      - uses: actions/setup-go@v2
+        with:
+          go-version-file: 'go.mod'
+          cache: false
+      - run: do some go stuff
+          

ql/test/query-tests/Security/CWE-349/.github/workflows/test6.yml

@@ -0,0 +1,17 @@
+name: Cache Poisoning
+
+on: pull_request_target
+
+jobs:
+  poison:
+    runs-on: ubuntu-latest
+    steps:
+      - uses: actions/checkout@v3
+        with:
+          ref: ${{ github.event.pull_request.head.sha }}
+      - uses: actions/setup-go@v2
+        with:
+          go-version-file: 'go.mod'
+          cache: true
+      - run: do some go stuff
+          

ql/test/query-tests/Security/CWE-349/.github/workflows/test7.yml

@@ -0,0 +1,16 @@
+name: Cache Poisoning
+
+on: pull_request_target
+
+jobs:
+  poison:
+    runs-on: ubuntu-latest
+    steps:
+      - uses: actions/checkout@v3
+        with:
+          ref: ${{ github.event.pull_request.head.sha }}
+      - uses: actions/setup-go@v2
+        with:
+          go-version-file: 'go.mod'
+      - run: do some go stuff
+          

ql/test/query-tests/Security/CWE-349/CachePoisoning.expected

@@ -1,3 +1,5 @@
 | .github/workflows/test1.yml:17:9:21:6 | Uses Step | Potential cache poisoning on privileged workflow. |
 | .github/workflows/test2.yml:12:9:16:6 | Uses Step | Potential cache poisoning on privileged workflow. |
 | .github/workflows/test3.yml:12:9:20:6 | Uses Step | Potential cache poisoning on privileged workflow. |
+| .github/workflows/test6.yml:12:9:16:6 | Uses Step | Potential cache poisoning on privileged workflow. |
+| .github/workflows/test7.yml:12:9:15:6 | Uses Step | Potential cache poisoning on privileged workflow. |