From 2273aadb4bc0b80bc48eec448de0b6405a5e32ac Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Alvaro=20Mu=C3=B1oz?= <pwntester@github.com>
Date: Mon, 5 Aug 2024 23:47:00 +0200
Subject: [PATCH] Improve Cache Poisoning query

The untrusted files path is compared with the path written to the cache
to check if the cache can really be poisoned
---
 ql/src/Security/CWE-349/CachePoisoning.ql | 52 ++++++++++++++++++++---
 1 file changed, 45 insertions(+), 7 deletions(-)

diff --git a/ql/src/Security/CWE-349/CachePoisoning.ql b/ql/src/Security/CWE-349/CachePoisoning.ql
index 6609dae2b7f..3f2bb8db472 100644
--- a/ql/src/Security/CWE-349/CachePoisoning.ql
+++ b/ql/src/Security/CWE-349/CachePoisoning.ql
@@ -18,19 +18,47 @@ import codeql.actions.security.CachePoisoningQuery
 import codeql.actions.security.PoisonableSteps
 import codeql.actions.security.ControlChecks
 
+/**
+ * Holds if the path cache_path is a subpath of the path untrusted_path.
+ */
+bindingset[cache_path, untrusted_path]
+predicate controlledCachePath(string cache_path, string untrusted_path) {
+  exists(string normalized_cache_path, string normalized_untrusted_path |
+    (
+      cache_path.regexpMatch("^[a-zA-Z0-9_-].*") and
+      normalized_cache_path = "./" + cache_path.regexpReplaceAll("/$", "")
+      or
+      normalized_cache_path = cache_path.regexpReplaceAll("/$", "")
+    ) and
+    (
+      untrusted_path.regexpMatch("^[a-zA-Z0-9_-].*") and
+      normalized_untrusted_path = "./" + untrusted_path.regexpReplaceAll("/$", "")
+      or
+      normalized_untrusted_path = untrusted_path.regexpReplaceAll("/$", "")
+    ) and
+    normalized_cache_path.substring(0, normalized_untrusted_path.length()) =
+      normalized_untrusted_path
+  )
+}
+
 query predicate edges(Step a, Step b) { a.getNextStep() = b }
 
-from LocalJob j, Event e, Step artifact, Step s
+from LocalJob j, Event e, Step source, Step s, string message, string path
 where
   (
-    artifact instanceof PRHeadCheckoutStep or
-    artifact instanceof UntrustedArtifactDownloadStep
+    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
   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(artifact, j.getATriggerEvent())) and
+  not exists(ControlCheck check | check.protects(source, j.getATriggerEvent())) and
   (
     // the workflow runs in the context of the default branch
     runsOnDefaultBranch(e)
@@ -43,19 +71,29 @@ where
     )
   ) and
   // the job checkouts untrusted code from a pull request
-  j.getAStep() = artifact and
+  j.getAStep() = source 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
     or
     // the job executes checked-out code
     // (The cache specific token can be leaked even for non-privileged workflows)
-    artifact.getAFollowingStep() = s and
+    source.getAFollowingStep() = s and
     s instanceof PoisonableStep and
     // excluding privileged workflows since they can be exploited in easier circumstances
     not j.isPrivileged()
   )
-select s, artifact, s, "Potential cache poisoning in the context of the default branch"
+select s, source, s, "Potential cache poisoning in the context of the default branch" + message