diff --git a/ql/lib/codeql/actions/security/UntrustedCheckoutQuery.qll b/ql/lib/codeql/actions/security/UntrustedCheckoutQuery.qll index 01da214b6ea..ffbb6fac263 100644 --- a/ql/lib/codeql/actions/security/UntrustedCheckoutQuery.qll +++ b/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) diff --git a/ql/src/Security/CWE-829/UntrustedCheckoutCritical.ql b/ql/src/Security/CWE-829/UntrustedCheckoutCritical.ql index be3b02ae477..07602af0ac4 100644 --- a/ql/src/Security/CWE-829/UntrustedCheckoutCritical.ql +++ b/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, diff --git a/ql/src/Security/CWE-829/UntrustedCheckoutHigh.ql b/ql/src/Security/CWE-829/UntrustedCheckoutHigh.ql index e130ba5dbb8..39cd1860097 100644 --- a/ql/src/Security/CWE-829/UntrustedCheckoutHigh.ql +++ b/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