refactor common code to identify untrusted checkouts

Author: Alvaro Muñoz

ql/lib/codeql/actions/dataflow/FlowSources.qll

@@ -92,28 +92,7 @@ class GitCommandSource extends RemoteFlowSource, CommandSource {
 
   GitCommandSource() {
     exists(Step checkout, string cmd_regex |
-      // This should be:
-      // source instanceof PRHeadCheckoutStep
-      // but PRHeadCheckoutStep uses Taint Tracking anc causes a non-Monolitic Recursion error
-      // so we list all the subclasses of PRHeadCheckoutStep here and use actions/checkout as a workaround
-      // instead of using  ActionsMutableRefCheckout and ActionsSHACheckout
-      (
-        exists(Uses uses |
-          checkout = uses and
-          uses.getCallee() = "actions/checkout" and
-          exists(uses.getArgument("ref")) and
-          not uses.getArgument("ref").matches("%base%") and
-          uses.getATriggerEvent().getName() = checkoutTriggers()
-        )
-        or
-        checkout instanceof GitMutableRefCheckout
-        or
-        checkout instanceof GitSHACheckout
-        or
-        checkout instanceof GhMutableRefCheckout
-        or
-        checkout instanceof GhSHACheckout
-      ) and
+      checkout instanceof SimplePRHeadCheckoutStep and
       this.asExpr() = run.getScript() and
       checkout.getAFollowingStep() = run and
       run.getScript().getAStmt() = cmd and
@@ -255,29 +234,7 @@ class ArtifactSource extends RemoteFlowSource, FileSource {
 private class CheckoutSource extends RemoteFlowSource, FileSource {
   Event event;
 
-  CheckoutSource() {
-    // This should be:
-    // source instanceof PRHeadCheckoutStep
-    // but PRHeadCheckoutStep uses Taint Tracking anc causes a non-Monolitic Recursion error
-    // so we list all the subclasses of PRHeadCheckoutStep here and use actions/checkout as a workaround
-    // instead of using  ActionsMutableRefCheckout and ActionsSHACheckout
-    exists(Uses uses |
-      this.asExpr() = uses and
-      uses.getCallee() = "actions/checkout" and
-      exists(uses.getArgument("ref")) and
-      not uses.getArgument("ref").matches("%base%") and
-      event = uses.getATriggerEvent() and
-      event.getName() = checkoutTriggers()
-    )
-    or
-    this.asExpr() instanceof GitMutableRefCheckout
-    or
-    this.asExpr() instanceof GitSHACheckout
-    or
-    this.asExpr() instanceof GhMutableRefCheckout
-    or
-    this.asExpr() instanceof GhSHACheckout
-  }
+  CheckoutSource() { this.asExpr() instanceof SimplePRHeadCheckoutStep }
 
   override string getSourceType() { result = "artifact" }
 

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

@@ -3,6 +3,7 @@ private import codeql.actions.TaintTracking
 import codeql.actions.DataFlow
 import codeql.actions.dataflow.FlowSources
 import codeql.actions.security.PoisonableSteps
+import codeql.actions.security.UntrustedCheckoutQuery
 
 string unzipRegexp() { result = "(unzip|tar)\\s+.*" }
 
@@ -22,11 +23,10 @@ class GitHubDownloadArtifactActionStep extends UntrustedArtifactDownloadStep, Us
       exists(this.getArgument("github-token"))
       or
       // There is an artifact upload step in the same workflow which can be influenced by an attacker on a checkout step
-      exists(LocalJob job, UsesStep checkout, UsesStep upload |
+      exists(LocalJob job, SimplePRHeadCheckoutStep checkout, UsesStep upload |
         this.getEnclosingWorkflow().getAJob() = job and
         job.getAStep() = checkout and
-        job.getATriggerEvent().getName() = "pull_request_target" and
-        checkout.getCallee() = "actions/checkout" and
+        checkout.getATriggerEvent().getName() = "pull_request_target" and
         checkout.getAFollowingStep() = upload and
         upload.getCallee() = "actions/upload-artifact"
       )
@@ -55,8 +55,10 @@ class DownloadArtifactActionStep extends UntrustedArtifactDownloadStep, UsesStep
         "ma-ve/action-download-artifact-with-retry"
       ] and
     (
-      not exists(this.getArgument(["branch", "branch_name"])) or
-      not this.getArgument(["branch", "branch_name"]) = ["main", "master"]
+      not exists(this.getArgument(["branch", "branch_name"]))
+      or
+      exists(this.getArgument(["branch", "branch_name"])) and
+      this.getArgument("allow_forks") = "true"
     ) and
     (
       not exists(this.getArgument(["commit", "commitHash", "commit_sha"])) or
@@ -74,7 +76,8 @@ class DownloadArtifactActionStep extends UntrustedArtifactDownloadStep, UsesStep
     ) and
     (
       not exists(this.getArgument("pr")) or
-      not this.getArgument("pr").matches("%github.event.pull_request.number%")
+      not this.getArgument("pr")
+          .matches(["%github.event.pull_request.number%", "%github.event.number%"])
     )
   }
 

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

@@ -20,26 +20,7 @@ class OutputClobberingFromFileReadSink extends OutputClobberingSink {
       (
         step instanceof UntrustedArtifactDownloadStep
         or
-        // This should be:
-        // artifact instanceof PRHeadCheckoutStep
-        // but PRHeadCheckoutStep uses Taint Tracking anc causes a non-Monolitic Recursion error
-        // so we list all the subclasses of PRHeadCheckoutStep here and use actions/checkout as a workaround
-        // instead of using  ActionsMutableRefCheckout and ActionsSHACheckout
-        exists(Uses uses |
-          step = uses and
-          uses.getCallee() = "actions/checkout" and
-          exists(uses.getArgument("ref")) and
-          not uses.getArgument("ref").matches("%base%") and
-          uses.getATriggerEvent().getName() = checkoutTriggers()
-        )
-        or
-        step instanceof GitMutableRefCheckout
-        or
-        step instanceof GitSHACheckout
-        or
-        step instanceof GhMutableRefCheckout
-        or
-        step instanceof GhSHACheckout
+        step instanceof SimplePRHeadCheckoutStep
       ) and
       step.getAFollowingStep() = run and
       this.asExpr() = run.getScript() and

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

@@ -193,6 +193,31 @@ predicate containsHeadRef(string s) {
   )
 }
 
+class SimplePRHeadCheckoutStep extends Step {
+  SimplePRHeadCheckoutStep() {
+    // This should be:
+    // artifact instanceof PRHeadCheckoutStep
+    // but PRHeadCheckoutStep uses Taint Tracking anc causes a non-Monolitic Recursion error
+    // so we list all the subclasses of PRHeadCheckoutStep here and use actions/checkout as a workaround
+    // instead of using ActionsMutableRefCheckout and ActionsSHACheckout
+    exists(Uses uses |
+      this = uses and
+      uses.getCallee() = "actions/checkout" and
+      exists(uses.getArgument("ref")) and
+      not uses.getArgument("ref").matches("%base%") and
+      uses.getATriggerEvent().getName() = checkoutTriggers()
+    )
+    or
+    this instanceof GitMutableRefCheckout
+    or
+    this instanceof GitSHACheckout
+    or
+    this instanceof GhMutableRefCheckout
+    or
+    this instanceof GhSHACheckout
+  }
+}
+
 /** Checkout of a Pull Request HEAD */
 abstract class PRHeadCheckoutStep extends Step {
   abstract string getPath();