Merge pull request #92 from github/fix/direct_cache_poison

Improve path checks for Artifact and Cache poisoning queries

Author: Alvaro Muñoz

ql/lib/codeql/actions/Ast.qll

@@ -289,6 +289,8 @@ class Run extends Step instanceof RunImpl {
   ScalarValue getScriptScalar() { result = super.getScriptScalar() }
 
   Expression getAnScriptExpr() { result = super.getAnScriptExpr() }
+
+  string getWorkingDirectory() { result = super.getWorkingDirectory() }
 }
 
 abstract class SimpleReferenceExpression extends AstNode instanceof SimpleReferenceExpressionImpl {

ql/lib/codeql/actions/Helper.qll

@@ -283,3 +283,30 @@ string getRepoRoot() {
     result = ""
   )
 }
+
+bindingset[path]
+string normalizePath(string path) {
+  exists(string trimmed_path | trimmed_path = trimQuotes(path) |
+    // ./foo -> GITHUB_WORKSPACE/foo
+    if path.indexOf("./") = 0
+    then result = path.replaceAll("./", "GITHUB_WORKSPACE/")
+    else
+      // GITHUB_WORKSPACE/foo -> GITHUB_WORKSPACE/foo
+      if path.indexOf("GITHUB_WORKSPACE/") = 0
+      then result = path
+      else
+        // foo -> GITHUB_WORKSPACE/foo
+        if path.regexpMatch("^[^/~].*")
+        then result = "GITHUB_WORKSPACE/" + path.regexpReplaceAll("/$", "")
+        else
+          // ~/foo -> ~/foo
+          // /foo -> /foo
+          result = path
+  )
+}
+
+/**
+ * Holds if the path cache_path is a subpath of the path untrusted_path.
+ */
+bindingset[subpath, path]
+predicate isSubpath(string subpath, string path) { subpath.substring(0, path.length()) = path }

ql/lib/codeql/actions/ast/internal/Ast.qll

@@ -1317,6 +1317,18 @@ class RunImpl extends StepImpl {
   override string toString() {
     if exists(this.getId()) then result = "Run Step: " + this.getId() else result = "Run Step"
   }
+
+  /** Gets the working directory for this `runs` mapping. */
+  string getWorkingDirectory() {
+    if exists(n.lookup("working-directory").(YamlString).getValue())
+    then
+      result =
+        n.lookup("working-directory")
+            .(YamlString)
+            .getValue()
+            .regexpReplaceAll("^\\./", "GITHUB_WORKSPACE/")
+    else result = "GITHUB_WORKSPACE/"
+  }
 }
 
 /**

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

@@ -35,7 +35,9 @@ class GitHubDownloadArtifactActionStep extends UntrustedArtifactDownloadStep, Us
   }
 
   override string getPath() {
-    if exists(this.getArgument("path")) then result = this.getArgument("path") else result = ""
+    if exists(this.getArgument("path"))
+    then result = normalizePath(this.getArgument("path"))
+    else result = "GITHUB_WORKSPACE/"
   }
 }
 
@@ -79,11 +81,11 @@ class DownloadArtifactActionStep extends UntrustedArtifactDownloadStep, UsesStep
 
   override string getPath() {
     if exists(this.getArgument(["path", "download_path"]))
-    then result = this.getArgument(["path", "download_path"])
+    then result = normalizePath(this.getArgument(["path", "download_path"]))
     else
       if exists(this.getArgument("paths"))
-      then result = this.getArgument("paths").splitAt(" ")
-      else result = ""
+      then result = normalizePath(this.getArgument("paths").splitAt(" "))
+      else result = "GITHUB_WORKSPACE/"
   }
 }
 
@@ -114,8 +116,8 @@ class LegitLabsDownloadArtifactActionStep extends UntrustedArtifactDownloadStep,
 
   override string getPath() {
     if exists(this.getArgument("path"))
-    then result = this.getArgument("path")
-    else result = "./artifacts"
+    then result = normalizePath(this.getArgument("path"))
+    else result = "GITHUB_WORKSPACE/artifacts"
   }
 }
 
@@ -161,14 +163,14 @@ class ActionsGitHubScriptDownloadStep extends UntrustedArtifactDownloadStep, Use
           .regexpMatch(unzipRegexp() + unzipDirArgRegexp())
     then
       result =
-        trimQuotes(this.getAFollowingStep()
-              .(Run)
-              .getScript()
-              .splitAt("\n")
-              .regexpCapture(unzipRegexp() + unzipDirArgRegexp(), 2))
+        normalizePath(trimQuotes(this.getAFollowingStep()
+                .(Run)
+                .getScript()
+                .splitAt("\n")
+                .regexpCapture(unzipRegexp() + unzipDirArgRegexp(), 2)))
     else
       if this.getAFollowingStep().(Run).getScript().splitAt("\n").regexpMatch(unzipRegexp())
-      then result = ""
+      then result = "GITHUB_WORKSPACE/"
       else none()
   }
 }
@@ -197,18 +199,20 @@ class GHRunArtifactDownloadStep extends UntrustedArtifactDownloadStep, Run {
       script.splitAt("\n").regexpMatch(unzipRegexp() + unzipDirArgRegexp())
     then
       result =
-        trimQuotes(script.splitAt("\n").regexpCapture(unzipRegexp() + unzipDirArgRegexp(), 2)) or
+        normalizePath(trimQuotes(script
+                .splitAt("\n")
+                .regexpCapture(unzipRegexp() + unzipDirArgRegexp(), 2))) or
       result =
-        trimQuotes(this.getAFollowingStep()
-              .(Run)
-              .getScript()
-              .splitAt("\n")
-              .regexpCapture(unzipRegexp() + unzipDirArgRegexp(), 2))
+        normalizePath(trimQuotes(this.getAFollowingStep()
+                .(Run)
+                .getScript()
+                .splitAt("\n")
+                .regexpCapture(unzipRegexp() + unzipDirArgRegexp(), 2)))
     else
       if
         this.getAFollowingStep().(Run).getScript().splitAt("\n").regexpMatch(unzipRegexp()) or
         script.splitAt("\n").regexpMatch(unzipRegexp())
-      then result = ""
+      then result = "GITHUB_WORKSPACE/"
       else none()
   }
 }
@@ -244,14 +248,16 @@ class DirectArtifactDownloadStep extends UntrustedArtifactDownloadStep, Run {
           .regexpMatch(unzipRegexp() + unzipDirArgRegexp())
     then
       result =
-        trimQuotes(script.splitAt("\n").regexpCapture(unzipRegexp() + unzipDirArgRegexp(), 2)) or
+        normalizePath(trimQuotes(script
+                .splitAt("\n")
+                .regexpCapture(unzipRegexp() + unzipDirArgRegexp(), 2))) or
       result =
-        trimQuotes(this.getAFollowingStep()
-              .(Run)
-              .getScript()
-              .splitAt("\n")
-              .regexpCapture(unzipRegexp() + unzipDirArgRegexp(), 2))
-    else result = ""
+        normalizePath(trimQuotes(this.getAFollowingStep()
+                .(Run)
+                .getScript()
+                .splitAt("\n")
+                .regexpCapture(unzipRegexp() + unzipDirArgRegexp(), 2)))
+    else result = "GITHUB_WORKSPACE/"
   }
 }
 
@@ -268,18 +274,16 @@ class ArtifactPoisoningSink extends DataFlow::Node {
       (
         // Check if the poisonable step is a local script execution step
         // and the path of the command or script matches the path of the downloaded artifact
+        isSubpath(poisonable.(LocalScriptExecutionRunStep).getPath(), download.getPath())
+        or
         // Checking the path for non local script execution steps is very difficult
         not poisonable instanceof LocalScriptExecutionRunStep
-        or
-        // TODO: account for Run's working directory
-        poisonable
-            .(LocalScriptExecutionRunStep)
-            .getCommand()
-            .matches(["./", ""] + download.getPath() + "%")
+        // Its not easy to extract the path from a non-local script execution step so skipping this check for now
+        // and isSubpath(poisonable.(Run).getWorkingDirectory(), download.getPath())
       )
       or
       poisonable.(UsesStep) = this.asExpr() and
-      download.getPath() = ""
+      download.getPath() = "GITHUB_WORKSPACE/"
     )
   }
 

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

@@ -51,13 +51,17 @@ abstract class CacheWritingStep extends Step {
 class CacheActionUsesStep extends CacheWritingStep, UsesStep {
   CacheActionUsesStep() { this.getCallee() = "actions/cache" }
 
-  override string getPath() { result = this.(UsesStep).getArgument("path").splitAt("\n") }
+  override string getPath() {
+    result = normalizePath(this.(UsesStep).getArgument("path").splitAt("\n"))
+  }
 }
 
 class CacheActionSaveUsesStep extends CacheWritingStep, UsesStep {
   CacheActionSaveUsesStep() { this.getCallee() = "actions/cache/save" }
 
-  override string getPath() { result = this.(UsesStep).getArgument("path").splitAt("\n") }
+  override string getPath() {
+    result = normalizePath(this.(UsesStep).getArgument("path").splitAt("\n"))
+  }
 }
 
 class SetupRubyUsesStep extends CacheWritingStep, UsesStep {
@@ -66,5 +70,5 @@ class SetupRubyUsesStep extends CacheWritingStep, UsesStep {
     this.getArgument("bundler-cache") = "true"
   }
 
-  override string getPath() { result = "vendor/bundle" }
+  override string getPath() { result = normalizePath("vendor/bundle") }
 }

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

@@ -36,18 +36,18 @@ class JavascriptImportnUsesStep extends PoisonableStep, UsesStep {
 }
 
 class LocalScriptExecutionRunStep extends PoisonableStep, Run {
-  string cmd;
+  string path;
 
   LocalScriptExecutionRunStep() {
-    exists(string line, string regexp, int command_group |
+    exists(string line, string regexp, int path_group |
       line = this.getScript().splitAt("\n").trim()
     |
-      poisonableLocalScriptsDataModel(regexp, command_group) and
-      cmd = line.regexpCapture(regexp, command_group)
+      poisonableLocalScriptsDataModel(regexp, path_group) and
+      path = line.regexpCapture(regexp, path_group)
     )
   }
 
-  string getCommand() { result = cmd }
+  string getPath() { result = normalizePath(path.splitAt(" ")) }
 }
 
 class LocalActionUsesStep extends PoisonableStep, UsesStep {

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

@@ -151,12 +151,6 @@ predicate containsHeadRef(string s) {
   )
 }
 
-private string getStepCWD() {
-  // TODO: This should be the path of the git command.
-  // Read if from the step's CWD, workspace or look for a cd command.
-  result = "?"
-}
-
 /** Checkout of a Pull Request HEAD */
 abstract class PRHeadCheckoutStep extends Step {
   abstract string getPath();
@@ -208,7 +202,7 @@ class ActionsMutableRefCheckout extends MutableRefCheckoutStep instanceof UsesSt
   override string getPath() {
     if exists(this.(UsesStep).getArgument("path"))
     then result = this.(UsesStep).getArgument("path")
-    else result = "?"
+    else result = "GITHUB_WORKSPACE/"
   }
 }
 
@@ -252,7 +246,7 @@ class ActionsSHACheckout extends SHACheckoutStep instanceof UsesStep {
   override string getPath() {
     if exists(this.(UsesStep).getArgument("path"))
     then result = this.(UsesStep).getArgument("path")
-    else result = "?"
+    else result = "GITHUB_WORKSPACE/"
   }
 }
 
@@ -277,7 +271,7 @@ class GitMutableRefCheckout extends MutableRefCheckoutStep instanceof Run {
     )
   }
 
-  override string getPath() { result = getStepCWD() }
+  override string getPath() { result = this.(Run).getWorkingDirectory() }
 }
 
 /** Checkout of a Pull Request HEAD ref using git within a Run step */
@@ -298,7 +292,7 @@ class GitSHACheckout extends SHACheckoutStep instanceof Run {
     )
   }
 
-  override string getPath() { result = getStepCWD() }
+  override string getPath() { result = this.(Run).getWorkingDirectory() }
 }
 
 /** Checkout of a Pull Request HEAD ref using gh within a Run step */
@@ -321,7 +315,7 @@ class GhMutableRefCheckout extends MutableRefCheckoutStep instanceof Run {
     )
   }
 
-  override string getPath() { result = getStepCWD() }
+  override string getPath() { result = this.(Run).getWorkingDirectory() }
 }
 
 /** Checkout of a Pull Request HEAD ref using gh within a Run step */
@@ -341,5 +335,5 @@ class GhSHACheckout extends SHACheckoutStep instanceof Run {
     )
   }
 
-  override string getPath() { result = getStepCWD() }
+  override string getPath() { result = this.(Run).getWorkingDirectory() }
 }

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

@@ -18,29 +18,6 @@ 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 job, Event event, Step source, Step step, string message, string path
@@ -86,7 +63,7 @@ where
     step.(CacheWritingStep).getPath() = "?"
     or
     // the cache writing step reads from a path the attacker can control
-    not path = "?" and controlledCachePath(step.(CacheWritingStep).getPath(), path)
+    not path = "?" and isSubpath(step.(CacheWritingStep).getPath(), path)
   ) and
   not step instanceof PoisonableStep
 select step, source, step,

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

@@ -1,7 +1,7 @@
 name: Test
 
 on:
-  issue_comment:
+  pull_request_target:
 
 permissions:
   actions: write
@@ -11,6 +11,8 @@ jobs:
     runs-on: ubuntu-latest
     steps:
       - uses: actions/checkout@v4
+        with:
+          ref: ${{ github.event.pull_request.head.sha }}
       - name: Set up Python 3.10
         uses: actions/setup-python@v5
         with:
@@ -22,14 +24,3 @@ jobs:
           path: ./results/pip
           key: ${{ runner.os }}-pip-${{ hashFiles('**/pyproject.toml') }}
           restore-keys: ${{ runner.os }}-pip-
-      - name: Download artifact
-        uses: dawidd6/action-download-artifact@v2
-        with:
-          name: results
-          path: results/
-      - name: Upload results
-        uses: actions/upload-artifact@v4
-        with:
-          name: results
-          path: results/
-          if-no-files-found: ignore