Improve checkout trigger events checks

Author: Alvaro Muñoz

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

@@ -13,7 +13,7 @@ string checkoutTriggers() {
  */
 private module ActionsMutableRefCheckoutConfig implements DataFlow::ConfigSig {
   predicate isSource(DataFlow::Node source) {
-    source.asExpr().getATriggerEvent().getName() = checkoutTriggers() and
+    //source.asExpr().getATriggerEvent().getName() = checkoutTriggers() and
     (
       // remote flow sources
       source instanceof ArtifactSource
@@ -209,29 +209,24 @@ abstract class SHACheckoutStep extends PRHeadCheckoutStep { }
 class ActionsMutableRefCheckout extends MutableRefCheckoutStep instanceof UsesStep {
   ActionsMutableRefCheckout() {
     this.getCallee() = "actions/checkout" and
+    //this.getEnclosingJob().getATriggerEvent().getName() = checkoutTriggers() and
     (
       exists(
         ActionsMutableRefCheckoutFlow::PathNode source, ActionsMutableRefCheckoutFlow::PathNode sink
       |
         ActionsMutableRefCheckoutFlow::flowPath(source, sink) and
-        sink.getNode().asExpr() = this.getArgumentExpr(["ref", "repository"])
+        this.getArgumentExpr(["ref", "repository"]) = sink.getNode().asExpr()
       )
       or
       // heuristic base on the step id and field name
-      exists(string value |
-        this.getArgumentExpr("ref")
-            .(SimpleReferenceExpression)
-            .getEnclosingJob()
-            .getATriggerEvent()
-            .getName() = checkoutTriggers() and
-        value.regexpMatch(".*(head|branch|ref).*")
+      exists(string value, Expression expr |
+        value.regexpMatch(".*(head|branch|ref).*") and expr = this.getArgumentExpr("ref")
       |
-        this.getArgumentExpr("ref").(StepsExpression).getStepId() = value or
-        this.getArgumentExpr("ref").(StepsExpression).getFieldName() = value or
-        this.getArgumentExpr("ref").(NeedsExpression).getNeededJobId() = value or
-        this.getArgumentExpr("ref").(NeedsExpression).getFieldName() = value or
-        this.getArgumentExpr("ref").(JsonReferenceExpression).getAccessPath() = value or
-        this.getArgumentExpr("ref").(JsonReferenceExpression).getInnerExpression() = value
+        expr.(StepsExpression).getStepId() = value or
+        expr.(SimpleReferenceExpression).getFieldName() = value or
+        expr.(NeedsExpression).getNeededJobId() = value or
+        expr.(JsonReferenceExpression).getAccessPath() = value or
+        expr.(JsonReferenceExpression).getInnerExpression() = value
       )
     )
   }
@@ -247,27 +242,22 @@ class ActionsMutableRefCheckout extends MutableRefCheckoutStep instanceof UsesSt
 class ActionsSHACheckout extends SHACheckoutStep instanceof UsesStep {
   ActionsSHACheckout() {
     this.getCallee() = "actions/checkout" and
+    //this.getEnclosingJob().getATriggerEvent().getName() = checkoutTriggers() and
     (
       exists(ActionsSHACheckoutFlow::PathNode source, ActionsSHACheckoutFlow::PathNode sink |
         ActionsSHACheckoutFlow::flowPath(source, sink) and
-        sink.getNode().asExpr() = this.getArgumentExpr(["ref", "repository"])
+        this.getArgumentExpr(["ref", "repository"]) = sink.getNode().asExpr()
       )
       or
       // heuristic base on the step id and field name
-      exists(string value |
-        this.getArgumentExpr("ref")
-            .(SimpleReferenceExpression)
-            .getEnclosingJob()
-            .getATriggerEvent()
-            .getName() = checkoutTriggers() and
-        value.regexpMatch(".*(head|sha|commit).*")
+      exists(string value, Expression expr |
+        value.regexpMatch(".*(head|sha|commit).*") and expr = this.getArgumentExpr("ref")
       |
-        this.getArgumentExpr("ref").(StepsExpression).getStepId() = value or
-        this.getArgumentExpr("ref").(StepsExpression).getFieldName() = value or
-        this.getArgumentExpr("ref").(NeedsExpression).getNeededJobId() = value or
-        this.getArgumentExpr("ref").(NeedsExpression).getFieldName() = value or
-        this.getArgumentExpr("ref").(JsonReferenceExpression).getAccessPath() = value or
-        this.getArgumentExpr("ref").(JsonReferenceExpression).getInnerExpression() = value
+        expr.(StepsExpression).getStepId() = value or
+        expr.(SimpleReferenceExpression).getFieldName() = value or
+        expr.(NeedsExpression).getNeededJobId() = value or
+        expr.(JsonReferenceExpression).getAccessPath() = value or
+        expr.(JsonReferenceExpression).getInnerExpression() = value
       )
     )
   }
@@ -283,7 +273,7 @@ class ActionsSHACheckout extends SHACheckoutStep instanceof UsesStep {
 class GitMutableRefCheckout extends MutableRefCheckoutStep instanceof Run {
   GitMutableRefCheckout() {
     exists(string cmd | this.getScript().getACommand() = cmd |
-      this.getATriggerEvent().getName() = checkoutTriggers() and
+      //this.getATriggerEvent().getName() = checkoutTriggers() and
       cmd.regexpMatch("git\\s+(fetch|pull).*") and
       (
         (containsHeadRef(cmd) or containsPullRequestNumber(cmd))
@@ -307,7 +297,7 @@ class GitMutableRefCheckout extends MutableRefCheckoutStep instanceof Run {
 class GitSHACheckout extends SHACheckoutStep instanceof Run {
   GitSHACheckout() {
     exists(string cmd | this.getScript().getACommand() = cmd |
-      this.getATriggerEvent().getName() = checkoutTriggers() and
+      //this.getATriggerEvent().getName() = checkoutTriggers() and
       cmd.regexpMatch("git\\s+(fetch|pull).*") and
       (
         containsHeadSHA(cmd)
@@ -328,7 +318,7 @@ class GitSHACheckout extends SHACheckoutStep instanceof Run {
 class GhMutableRefCheckout extends MutableRefCheckoutStep instanceof Run {
   GhMutableRefCheckout() {
     exists(string cmd | this.getScript().getACommand() = cmd |
-      this.getATriggerEvent().getName() = checkoutTriggers() and
+      //this.getATriggerEvent().getName() = checkoutTriggers() and
       cmd.regexpMatch(".*(gh|hub)\\s+pr\\s+checkout.*") and
       (
         (containsHeadRef(cmd) or containsPullRequestNumber(cmd))
@@ -351,7 +341,7 @@ class GhMutableRefCheckout extends MutableRefCheckoutStep instanceof Run {
 class GhSHACheckout extends SHACheckoutStep instanceof Run {
   GhSHACheckout() {
     exists(string cmd | this.getScript().getACommand() = cmd |
-      this.getATriggerEvent().getName() = checkoutTriggers() and
+      //this.getATriggerEvent().getName() = checkoutTriggers() and
       cmd.regexpMatch("gh\\s+pr\\s+checkout.*") and
       (
         containsHeadSHA(cmd)

ql/src/Security/CWE-829/UntrustedCheckoutCritical.ql

@@ -48,6 +48,7 @@ where
   // the checkout occurs in a privileged context
   inPrivilegedContext(poisonable, event) and
   inPrivilegedContext(checkout, event) and
+  event.getName() = checkoutTriggers() and
   not exists(ControlCheck check | check.protects(checkout, event, "untrusted-checkout")) and
   not exists(ControlCheck check | check.protects(poisonable, event, "untrusted-checkout"))
 select poisonable, checkout, poisonable,

ql/src/Security/CWE-829/UntrustedCheckoutHigh.ql

@@ -24,6 +24,7 @@ where
   not checkout.getAFollowingStep() instanceof PoisonableStep and
   // the checkout occurs in a privileged context
   inPrivilegedContext(checkout, event) and
+  event.getName() = checkoutTriggers() and
   (
     // issue_comment: check for date comparison checks and actor/access control checks
     event.getName() = "issue_comment" and