Split Cache Poisoning queries in 3

Split them into 3 queries depending of how the cache can be poisoned:
- control of cached files
- execution of controlled code
- code injection

Remove `setup-XXX` actions from CacheWriting class since the cached
files are not in the CWD

Author: Alvaro Muñoz

ql/src/Security/CWE-349/CachePoisoningByCodeInjection.ql renamed to ql/src/Security/CWE-349/CachePoisoningViaCodeInjection.ql

@@ -1,5 +1,5 @@
 /**
- * @name Cache Poisoning via low-privilege code injection
+ * @name Cache Poisoning via low-privileged code injection
  * @description The cache can be poisoned by untrusted code, leading to a cache poisoning attack.
  * @kind path-problem
  * @problem.severity error

ql/src/Security/CWE-349/CachePoisoning.ql renamed to ql/src/Security/CWE-349/CachePoisoningViaDirectCache.ql

@@ -1,11 +1,11 @@
 /**
- * @name Cache Poisoning
+ * @name Cache Poisoning via caching of untrusted files
  * @description The cache can be poisoned by untrusted code, leading to a cache poisoning attack.
  * @kind path-problem
  * @problem.severity error
  * @precision high
  * @security-severity 7.5
- * @id actions/cache-poisoning
+ * @id actions/cache-poisoning/direct-cache
  * @tags actions
  *       security
  *       external/cwe/cwe-349
@@ -45,6 +45,8 @@ query predicate edges(Step a, Step b) { a.getNextStep() = b }
 
 from LocalJob j, Event e, Step source, Step s, string message, string path
 where
+  // the job checkouts untrusted code from a pull request or downloads an untrusted artifact
+  j.getAStep() = source and
   (
     source instanceof PRHeadCheckoutStep and
     message = "due to privilege checkout of untrusted code." and
@@ -54,46 +56,35 @@ where
     message = "due to downloading an untrusted artifact." and
     path = source.(UntrustedArtifactDownloadStep).getPath()
   ) and
+  // the checkout/download is not controlled by an access check
+  not exists(ControlCheck check | check.protects(source, j.getATriggerEvent())) and
   j.getATriggerEvent() = e and
   // job can be triggered by an external user
   e.isExternallyTriggerable() and
-  // the checkout is not controlled by an access check
-  not exists(ControlCheck check | check.protects(source, j.getATriggerEvent())) and
   (
     // the workflow runs in the context of the default branch
     runsOnDefaultBranch(e)
     or
-    // the workflow caller runs in the context of the default branch
+    // the workflow's caller runs in the context of the default branch
     e.getName() = "workflow_call" and
     exists(ExternalJob caller |
       caller.getCallee() = j.getLocation().getFile().getRelativePath() and
       runsOnDefaultBranch(caller.getATriggerEvent())
     )
   ) and
-  // the job checkouts untrusted code from a pull request
-  j.getAStep() = source and
+  // the job writes to the cache
+  // (No need to follow the checkout/download step since the cache is normally write after the job completes)
+  j.getAStep() = s and
+  s instanceof CacheWritingStep 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() = s and
-    s instanceof CacheWritingStep and
-    (
-      // we dont know what code can be controlled by the attacker
-      path = "?"
-      or
-      // we dont know what files are being cached
-      s.(CacheWritingStep).getPath() = "?"
-      or
-      // the cache writing step reads from the path the attacker can control
-      not path = "?" and controlledCachePath(s.(CacheWritingStep).getPath(), path)
-    ) and
-    not s instanceof PoisonableStep
+    // we dont know what code can be controlled by the attacker
+    path = "?"
     or
-    // the job executes checked-out code
-    // (The cache specific token can be leaked even for non-privileged workflows)
-    source.getAFollowingStep() = s and
-    s instanceof PoisonableStep and
-    // excluding privileged workflows since they can be exploited in easier circumstances
-    not j.isPrivileged()
-  )
+    // we dont know what files are being cached
+    s.(CacheWritingStep).getPath() = "?"
+    or
+    // the cache writing step reads from a path the attacker can control
+    not path = "?" and controlledCachePath(s.(CacheWritingStep).getPath(), path)
+  ) and
+  not s instanceof PoisonableStep
 select s, source, s, "Potential cache poisoning in the context of the default branch " + message

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

@@ -0,0 +1,58 @@
+/**
+ * @name Cache Poisoning via execution of untrusted code
+ * @description The cache can be poisoned by untrusted code, leading to a cache poisoning attack.
+ * @kind path-problem
+ * @problem.severity error
+ * @precision high
+ * @security-severity 7.5
+ * @id actions/cache-poisoning/poisonable-step
+ * @tags actions
+ *       security
+ *       external/cwe/cwe-349
+ */
+
+import actions
+import codeql.actions.security.ArtifactPoisoningQuery
+import codeql.actions.security.UntrustedCheckoutQuery
+import codeql.actions.security.CachePoisoningQuery
+import codeql.actions.security.PoisonableSteps
+import codeql.actions.security.ControlChecks
+
+query predicate edges(Step a, Step b) { a.getNextStep() = b }
+
+from LocalJob j, Event e, Step source, Step s, string message, string path
+where
+  // the job checkouts untrusted code from a pull request or downloads an untrusted artifact
+  j.getAStep() = source and
+  (
+    source instanceof PRHeadCheckoutStep and
+    message = "due to privilege checkout of untrusted code." and
+    path = source.(PRHeadCheckoutStep).getPath()
+    or
+    source instanceof UntrustedArtifactDownloadStep and
+    message = "due to downloading an untrusted artifact." and
+    path = source.(UntrustedArtifactDownloadStep).getPath()
+  ) and
+  // the checkout/download is not controlled by an access check
+  not exists(ControlCheck check | check.protects(source, j.getATriggerEvent())) and
+  j.getATriggerEvent() = e and
+  // job can be triggered by an external user
+  e.isExternallyTriggerable() and
+  (
+    // the workflow runs in the context of the default branch
+    runsOnDefaultBranch(e)
+    or
+    // the workflow's caller runs in the context of the default branch
+    e.getName() = "workflow_call" and
+    exists(ExternalJob caller |
+      caller.getCallee() = j.getLocation().getFile().getRelativePath() and
+      runsOnDefaultBranch(caller.getATriggerEvent())
+    )
+  ) and
+  // the job executes checked-out code
+  // (The cache specific token can be leaked even for non-privileged workflows)
+  source.getAFollowingStep() = s and
+  s instanceof PoisonableStep and
+  // excluding privileged workflows since they can be exploited in easier circumstances
+  not j.isPrivileged()
+select s, source, s, "Potential cache poisoning in the context of the default branch " + message

ql/test/query-tests/Security/CWE-349/.github/workflows/test9.yml renamed to ql/test/query-tests/Security/CWE-349/.github/workflows/code_injection1.yml

@@ -0,0 +1,16 @@
+name: Test
+
+on:
+  pull_request_target:
+    branches: [ master, main, dev ]
+
+jobs:
+  test:
+    name: Test
+    runs-on: ubuntu-latest
+    steps:
+      - id: modified_files
+        uses: trilom/[email protected]
+        with:
+          output: ","
+      - run: echo "${{ steps.modified_files.outputs.files_modified }}"