diff --git a/ql/lib/codeql/actions/dataflow/FlowSources.qll b/ql/lib/codeql/actions/dataflow/FlowSources.qll index 2fca425642e..cf1763b1c03 100644 --- a/ql/lib/codeql/actions/dataflow/FlowSources.qll +++ b/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" } diff --git a/ql/lib/codeql/actions/security/ArtifactPoisoningQuery.qll b/ql/lib/codeql/actions/security/ArtifactPoisoningQuery.qll index 31427287b0c..d8d5f83c867 100644 --- a/ql/lib/codeql/actions/security/ArtifactPoisoningQuery.qll +++ b/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%"]) ) } diff --git a/ql/lib/codeql/actions/security/OutputClobberingQuery.qll b/ql/lib/codeql/actions/security/OutputClobberingQuery.qll index e6cc0d06a46..1d0de83afa3 100644 --- a/ql/lib/codeql/actions/security/OutputClobberingQuery.qll +++ b/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 diff --git a/ql/lib/codeql/actions/security/UntrustedCheckoutQuery.qll b/ql/lib/codeql/actions/security/UntrustedCheckoutQuery.qll index 9653ae2beda..1a75f8a96c1 100644 --- a/ql/lib/codeql/actions/security/UntrustedCheckoutQuery.qll +++ b/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();