diff --git a/ql/lib/codeql/actions/Helper.qll b/ql/lib/codeql/actions/Helper.qll index 1d88f6f6511..9ac67575b8b 100644 --- a/ql/lib/codeql/actions/Helper.qll +++ b/ql/lib/codeql/actions/Helper.qll @@ -248,8 +248,7 @@ predicate inPrivilegedCompositeAction(AstNode node) { predicate inPrivilegedExternallyTriggerableJob(AstNode node) { exists(Job j | j = node.getEnclosingJob() and - j.isPrivilegedExternallyTriggerable() and - not exists(ControlCheck check, Event e | j.getATriggerEvent() = e | check.protects(node, e)) + j.isPrivilegedExternallyTriggerable() ) } diff --git a/ql/lib/codeql/actions/security/ControlChecks.qll b/ql/lib/codeql/actions/security/ControlChecks.qll index 2d8e60dca37..26bee3ca3a6 100644 --- a/ql/lib/codeql/actions/security/ControlChecks.qll +++ b/ql/lib/codeql/actions/security/ControlChecks.qll @@ -1,17 +1,46 @@ import actions +string any_relevant_category() { + result = + [ + "untrusted-checkout", "output-clobbering", "envpath-injection", "envvar-injection", + "command-injection", "argument-injection", "code-injection", "cache-poisoning", + "untrusted-checkout-toctou", "artifact-poisoning" + ] +} + +string any_non_toctou_category() { + result = any_relevant_category() and not result = "untrusted-checkout-toctou" +} + +string any_relevant_event() { + result = + [ + "pull_request_target", + "issue_comment", + "pull_request_comment", + "workflow_run", + "issues", + "fork", + "watch", + "discussion_comment", + "discussion" + ] +} + /** An If node that contains an actor, user or label check */ abstract class ControlCheck extends AstNode { ControlCheck() { this instanceof If or this instanceof Environment or - this instanceof UsesStep + this instanceof UsesStep or + this instanceof Run } - predicate protects(Step step, Event event) { + predicate protects(Step step, Event event, string category) { event.getEnclosingWorkflow() = step.getEnclosingWorkflow() and - this.getAProtectedEvent() = event.getName() and - this.dominates(step) + this.dominates(step) and + this.protectsCategoryAndEvent(category, event.getName()) } predicate dominates(Step step) { @@ -30,80 +59,71 @@ abstract class ControlCheck extends AstNode { step.getEnclosingJob().getANeededJob().getEnvironment() = this ) or - this.(UsesStep).getAFollowingStep() = step + this.(Step).getAFollowingStep() = step } - abstract string getAProtectedEvent(); - - abstract boolean protectsAgainstRefMutationAttacks(); + abstract predicate protectsCategoryAndEvent(string category, string event); } abstract class AssociationCheck extends ControlCheck { - // checks who you are (identity) - // association checks are effective against pull requests since they can control who is making the PR - // they are not effective against issue_comment since the author of the comment may not be the same as the author of the PR - // someone entitled to trigger the workflow with a comment, may no detect a malicious comment, or the comment may mutate after approval - override string getAProtectedEvent() { result = ["pull_request", "pull_request_target"] } - - override boolean protectsAgainstRefMutationAttacks() { result = true } + // Checks if the actor is a MEMBER/OWNER the repo + // - they are effective against pull requests and workflow_run (since these are triggered by pull_requests) since they can control who is making the PR + // - they are not effective against issue_comment since the author of the comment may not be the same as the author of the PR + override predicate protectsCategoryAndEvent(string category, string event) { + event = ["pull_request_target", "workflow_run"] and category = any_relevant_category() + } } abstract class ActorCheck extends ControlCheck { - // checks who you are (identity) - // actor checks are effective against pull requests since they can control who is making the PR - // they are not effective against issue_comment since the author of the comment may not be the same as the author of the PR - // someone entitled to trigger the workflow with a comment, may no detect a malicious comment, or the comment may mutate after approval - override string getAProtectedEvent() { result = ["pull_request", "pull_request_target"] } - - override boolean protectsAgainstRefMutationAttacks() { result = true } + // checks for a specific actor + // - they are effective against pull requests and workflow_run (since these are triggered by pull_requests) since they can control who is making the PR + // - they are not effective against issue_comment since the author of the comment may not be the same as the author of the PR + override predicate protectsCategoryAndEvent(string category, string event) { + event = ["pull_request_target", "workflow_run"] and category = any_relevant_category() + } } abstract class RepositoryCheck extends ControlCheck { - // repository checks are effective against pull requests since they can control where the code is coming from - // they are not effective against issue_comment since the repository will always be the same - // who you are (identity) - override string getAProtectedEvent() { result = ["pull_request", "pull_request_target"] } - - override boolean protectsAgainstRefMutationAttacks() { result = true } + // checks that the origin of the code is the same as the repository. + // for pull_requests, that means that it triggers only on local branches or repos from the same org + // - they are effective against pull requests/workflow_run since they can control where the code is coming from + // - they are not effective against issue_comment since the repository will always be the same + override predicate protectsCategoryAndEvent(string category, string event) { + event = ["pull_request_target", "workflow_run"] and category = any_relevant_category() + } } abstract class PermissionCheck extends ControlCheck { - // permission checks are effective against pull requests since they can control who can make changes - // they are not effective against issue_comment since the author of the comment may not be the same as the author of the PR - // someone entitled to trigger the workflow with a comment, may no detect a malicious comment, or the comment may mutate after approval - // who you are (identity) - override string getAProtectedEvent() { result = ["pull_request", "pull_request_target"] } - - override boolean protectsAgainstRefMutationAttacks() { result = true } + // checks that the actor has a specific permission level + // - they are effective against pull requests/workflow_run since they can control who can make changes + // - they are not effective against issue_comment since the author of the comment may not be the same as the author of the PR + override predicate protectsCategoryAndEvent(string category, string event) { + event = ["pull_request_target", "workflow_run", "issue_comment"] and + category = any_relevant_category() + } } abstract class LabelCheck extends ControlCheck { - // does it protect injection attacks but not pwn requests? - // pwn requests are susceptible to checkout of mutable code - // but injection attacks are not, although a branch name can be changed after approval and perhaps also some other things - // they do actually protext against untrusted code execution (sha) - // what you have (approval) - // TODO: A check should be a combination of: - // - event type (pull_request, issue_comment, etc) - // - category (untrusted mutable code, untrusted immutable code, code injection, etc) - // - we dont know this unless we pass category to inPrivilegedContext and into ControlCheck.protects - // - we can decide if a control check is effective based only on the ast node - override string getAProtectedEvent() { result = ["pull_request", "pull_request_target"] } - - // ref can be mutated after approval - override boolean protectsAgainstRefMutationAttacks() { result = false } + // checks if the issue/pull_request is labeled, which implies that it could have been approved + // - they dont protect against mutation attacks + override predicate protectsCategoryAndEvent(string category, string event) { + event = ["pull_request_target", "workflow_run"] and category = any_non_toctou_category() + } } class EnvironmentCheck extends ControlCheck instanceof Environment { // Environment checks are not effective against any mutable attacks - // they do actually protext against untrusted code execution (sha) - // what you have (approval) - EnvironmentCheck() { any() } + // they do actually protect against untrusted code execution (sha) + override predicate protectsCategoryAndEvent(string category, string event) { + event = ["pull_request_target", "workflow_run"] and category = any_non_toctou_category() + } +} - override string getAProtectedEvent() { result = ["pull_request", "pull_request_target"] } - - // ref can be mutated after approval - override boolean protectsAgainstRefMutationAttacks() { result = false } +abstract class CommentVsHeadDateCheck extends ControlCheck { + override predicate protectsCategoryAndEvent(string category, string event) { + // by itself, this check is not effective against any attacks + none() + } } /* Specific implementations of control checks */ @@ -162,28 +182,37 @@ class RepositoryIfCheck extends RepositoryCheck instanceof If { class AssociationIfCheck extends AssociationCheck instanceof If { AssociationIfCheck() { // eg: contains(fromJson('["MEMBER", "OWNER"]'), github.event.comment.author_association) - exists( - normalizeExpr(this.getCondition()) - .regexpFind([ - "\\bgithub\\.event\\.comment\\.author_association\\b", - "\\bgithub\\.event\\.issue\\.author_association\\b", - "\\bgithub\\.event\\.pull_request\\.author_association\\b", - ], _, _) - ) + normalizeExpr(this.getCondition()) + .splitAt("\n") + .regexpMatch([ + ".*\\bgithub\\.event\\.comment\\.author_association\\b.*", + ".*\\bgithub\\.event\\.issue\\.author_association\\b.*", + ".*\\bgithub\\.event\\.pull_request\\.author_association\\b.*", + ]) and + normalizeExpr(this.getCondition()).splitAt("\n").regexpMatch(".*\\bMEMBER\\b.*") and + normalizeExpr(this.getCondition()).splitAt("\n").regexpMatch(".*\\bOWNER\\b.*") } } class AssociationActionCheck extends AssociationCheck instanceof UsesStep { AssociationActionCheck() { this.getCallee() = "TheModdingInquisition/actions-team-membership" and - not exists(this.getArgument("exit")) - or - this.getArgument("exit") = "true" + ( + not exists(this.getArgument("exit")) + or + this.getArgument("exit") = "true" + ) } } class PermissionActionCheck extends PermissionCheck instanceof UsesStep { PermissionActionCheck() { + this.getCallee() = "sushichop/action-repository-permission" and + this.getArgument("required-permission") = ["write", "admin"] + or + this.getCallee() = "prince-chrismc/check-actor-permissions-action" and + this.getArgument("permission") = ["write", "admin"] + or this.getCallee() = "lannonbr/repo-permission-check-action" and this.getArgument("permission") = ["write", "admin"] or @@ -195,3 +224,13 @@ class PermissionActionCheck extends PermissionCheck instanceof UsesStep { ) } } + +class BashCommentVsHeadDateCheck extends CommentVsHeadDateCheck, Run { + BashCommentVsHeadDateCheck() { + exists(string line | + line = this.getScript().splitAt("\n") and + line.toLowerCase() + .regexpMatch(".*date\\s+-d.*(commit_at|pushed_at|comment_at|commented_at).*date\\s+-d.*(commit_at|pushed_at|comment_at|commented_at).*") + ) + } +} diff --git a/ql/lib/ext/config/externally_triggereable_events.yml b/ql/lib/ext/config/externally_triggereable_events.yml index 88d17c728b7..028671c243d 100644 --- a/ql/lib/ext/config/externally_triggereable_events.yml +++ b/ql/lib/ext/config/externally_triggereable_events.yml @@ -6,13 +6,14 @@ extensions: - ["discussion"] - ["discussion_comment"] - ["fork"] + - ["watch"] - ["issue_comment"] - ["issues"] - - ["pull_request"] + - ["pull_request"] # non-privileged - ["pull_request_comment"] - ["pull_request_review"] - ["pull_request_review_comment"] - ["pull_request_target"] - - ["workflow_run"] # depending on trigger workflow + - ["workflow_run"] # depending on branch filter - ["workflow_call"] # depending on caller diff --git a/ql/src/Security/CWE-074/OutputClobberingHigh.ql b/ql/src/Security/CWE-074/OutputClobberingHigh.ql index c53489f9628..0ead5aa7689 100644 --- a/ql/src/Security/CWE-074/OutputClobberingHigh.ql +++ b/ql/src/Security/CWE-074/OutputClobberingHigh.ql @@ -16,6 +16,7 @@ import actions import codeql.actions.security.OutputClobberingQuery import codeql.actions.dataflow.ExternalFlow import OutputClobberingFlow::PathGraph +import codeql.actions.security.ControlChecks from OutputClobberingFlow::PathNode source, OutputClobberingFlow::PathNode sink where @@ -23,9 +24,20 @@ where inPrivilegedContext(sink.getNode().asExpr()) and // exclude paths to file read sinks from non-artifact sources ( - not source.getNode().(RemoteFlowSource).getSourceType() = "artifact" + not source.getNode().(RemoteFlowSource).getSourceType() = "artifact" and + not exists(ControlCheck check | + check + .protects(sink.getNode().asExpr(), + source.getNode().asExpr().getEnclosingJob().getATriggerEvent(), "code-injection") + ) or source.getNode().(RemoteFlowSource).getSourceType() = "artifact" and + not exists(ControlCheck check | + check + .protects(sink.getNode().asExpr(), + source.getNode().asExpr().getEnclosingJob().getATriggerEvent(), + ["untrusted-checkout", "artifact-poisoning"]) + ) and ( sink.getNode() instanceof OutputClobberingFromFileReadSink or sink.getNode() instanceof WorkflowCommandClobberingFromFileReadSink or diff --git a/ql/src/Security/CWE-077/EnvPathInjectionCritical.ql b/ql/src/Security/CWE-077/EnvPathInjectionCritical.ql index 4ff86eb0fbd..9fa066d195c 100644 --- a/ql/src/Security/CWE-077/EnvPathInjectionCritical.ql +++ b/ql/src/Security/CWE-077/EnvPathInjectionCritical.ql @@ -15,15 +15,27 @@ import actions import codeql.actions.security.EnvPathInjectionQuery import EnvPathInjectionFlow::PathGraph +import codeql.actions.security.ControlChecks from EnvPathInjectionFlow::PathNode source, EnvPathInjectionFlow::PathNode sink where EnvPathInjectionFlow::flowPath(source, sink) and inPrivilegedContext(sink.getNode().asExpr()) and ( - not source.getNode().(RemoteFlowSource).getSourceType() = "artifact" + not source.getNode().(RemoteFlowSource).getSourceType() = "artifact" and + not exists(ControlCheck check | + check + .protects(sink.getNode().asExpr(), + source.getNode().asExpr().getEnclosingJob().getATriggerEvent(), "code-injection") + ) or source.getNode().(RemoteFlowSource).getSourceType() = "artifact" and + not exists(ControlCheck check | + check + .protects(sink.getNode().asExpr(), + source.getNode().asExpr().getEnclosingJob().getATriggerEvent(), + ["untrusted-checkout", "artifact-poisoning"]) + ) and sink.getNode() instanceof EnvPathInjectionFromFileReadSink ) select sink.getNode(), source, sink, diff --git a/ql/src/Security/CWE-077/EnvVarInjectionCritical.ql b/ql/src/Security/CWE-077/EnvVarInjectionCritical.ql index 89e1ddd3cc2..806bae2a91d 100644 --- a/ql/src/Security/CWE-077/EnvVarInjectionCritical.ql +++ b/ql/src/Security/CWE-077/EnvVarInjectionCritical.ql @@ -16,16 +16,33 @@ import actions import codeql.actions.security.EnvVarInjectionQuery import codeql.actions.dataflow.ExternalFlow import EnvVarInjectionFlow::PathGraph +import codeql.actions.security.ControlChecks from EnvVarInjectionFlow::PathNode source, EnvVarInjectionFlow::PathNode sink where EnvVarInjectionFlow::flowPath(source, sink) and inPrivilegedContext(sink.getNode().asExpr()) and + not exists(ControlCheck check | + check + .protects(sink.getNode().asExpr(), + source.getNode().asExpr().getEnclosingJob().getATriggerEvent(), "envvar-injection") + ) and // exclude paths to file read sinks from non-artifact sources ( - not source.getNode().(RemoteFlowSource).getSourceType() = "artifact" + not source.getNode().(RemoteFlowSource).getSourceType() = "artifact" and + not exists(ControlCheck check | + check + .protects(sink.getNode().asExpr(), + source.getNode().asExpr().getEnclosingJob().getATriggerEvent(), "code-injection") + ) or source.getNode().(RemoteFlowSource).getSourceType() = "artifact" and + not exists(ControlCheck check | + check + .protects(sink.getNode().asExpr(), + source.getNode().asExpr().getEnclosingJob().getATriggerEvent(), + ["untrusted-checkout", "artifact-poisoning"]) + ) and ( sink.getNode() instanceof EnvVarInjectionFromFileReadSink or madSink(sink.getNode(), "envvar-injection") diff --git a/ql/src/Security/CWE-088/ArgumentInjectionCritical.ql b/ql/src/Security/CWE-088/ArgumentInjectionCritical.ql index affa372f14e..6f1f6008a06 100644 --- a/ql/src/Security/CWE-088/ArgumentInjectionCritical.ql +++ b/ql/src/Security/CWE-088/ArgumentInjectionCritical.ql @@ -14,11 +14,17 @@ import actions import codeql.actions.security.ArgumentInjectionQuery import ArgumentInjectionFlow::PathGraph +import codeql.actions.security.ControlChecks from ArgumentInjectionFlow::PathNode source, ArgumentInjectionFlow::PathNode sink where ArgumentInjectionFlow::flowPath(source, sink) and - inPrivilegedContext(sink.getNode().asExpr()) + inPrivilegedContext(sink.getNode().asExpr()) and + not exists(ControlCheck check | + check + .protects(sink.getNode().asExpr(), + source.getNode().asExpr().getEnclosingJob().getATriggerEvent(), "argument-injection") + ) select sink.getNode(), source, sink, "Potential argument injection in $@ command, which may be controlled by an external user.", sink, sink.getNode().(ArgumentInjectionSink).getCommand() diff --git a/ql/src/Security/CWE-094/CodeInjectionCritical.ql b/ql/src/Security/CWE-094/CodeInjectionCritical.ql index 9319718b7fc..ec4925d24a0 100644 --- a/ql/src/Security/CWE-094/CodeInjectionCritical.ql +++ b/ql/src/Security/CWE-094/CodeInjectionCritical.ql @@ -17,11 +17,17 @@ import actions import codeql.actions.security.CodeInjectionQuery import CodeInjectionFlow::PathGraph +import codeql.actions.security.ControlChecks from CodeInjectionFlow::PathNode source, CodeInjectionFlow::PathNode sink where CodeInjectionFlow::flowPath(source, sink) and inPrivilegedContext(sink.getNode().asExpr()) and + not exists(ControlCheck check | + check + .protects(sink.getNode().asExpr(), + source.getNode().asExpr().getEnclosingJob().getATriggerEvent(), "code-injection") + ) and // exclude cases where the sink is a JS script and the expression uses toJson not exists(UsesStep script | script.getCallee() = "actions/github-script" and diff --git a/ql/src/Security/CWE-349/CachePoisoningViaCodeInjection.ql b/ql/src/Security/CWE-349/CachePoisoningViaCodeInjection.ql index 685bdcca401..67b615d115a 100644 --- a/ql/src/Security/CWE-349/CachePoisoningViaCodeInjection.ql +++ b/ql/src/Security/CWE-349/CachePoisoningViaCodeInjection.ql @@ -26,7 +26,9 @@ where // job can be triggered by an external user e.isExternallyTriggerable() and // the checkout is not controlled by an access check - not exists(ControlCheck check | check.protects(source.getNode().asExpr(), j.getATriggerEvent())) and + not exists(ControlCheck check | + check.protects(source.getNode().asExpr(), j.getATriggerEvent(), "code-injection") + ) and // excluding privileged workflows since they can be exploited in easier circumstances not j.isPrivileged() and ( diff --git a/ql/src/Security/CWE-349/CachePoisoningViaDirectCache.ql b/ql/src/Security/CWE-349/CachePoisoningViaDirectCache.ql index ea36bcf0be1..b6df022329d 100644 --- a/ql/src/Security/CWE-349/CachePoisoningViaDirectCache.ql +++ b/ql/src/Security/CWE-349/CachePoisoningViaDirectCache.ql @@ -57,7 +57,9 @@ where path = source.(UntrustedArtifactDownloadStep).getPath() ) and // the checkout/download is not controlled by an access check - not exists(ControlCheck check | check.protects(source, j.getATriggerEvent())) and + not exists(ControlCheck check | + check.protects(source, j.getATriggerEvent(), ["untrusted-checkout", "artifact-poisoning"]) + ) and j.getATriggerEvent() = e and // job can be triggered by an external user e.isExternallyTriggerable() and diff --git a/ql/src/Security/CWE-349/CachePoisoningViaPoisonableStep.ql b/ql/src/Security/CWE-349/CachePoisoningViaPoisonableStep.ql index ee2719f0611..0750a02930e 100644 --- a/ql/src/Security/CWE-349/CachePoisoningViaPoisonableStep.ql +++ b/ql/src/Security/CWE-349/CachePoisoningViaPoisonableStep.ql @@ -34,7 +34,9 @@ where path = source.(UntrustedArtifactDownloadStep).getPath() ) and // the checkout/download is not controlled by an access check - not exists(ControlCheck check | check.protects(source, j.getATriggerEvent())) and + not exists(ControlCheck check | + check.protects(source, j.getATriggerEvent(), ["untrusted-checkout", "artifact-poisoning"]) + ) and j.getATriggerEvent() = e and // job can be triggered by an external user e.isExternallyTriggerable() and diff --git a/ql/src/Security/CWE-367/UntrustedCheckoutTOCTOUCritical.ql b/ql/src/Security/CWE-367/UntrustedCheckoutTOCTOUCritical.ql index a97309ce187..7c7ab15de31 100644 --- a/ql/src/Security/CWE-367/UntrustedCheckoutTOCTOUCritical.ql +++ b/ql/src/Security/CWE-367/UntrustedCheckoutTOCTOUCritical.ql @@ -24,11 +24,10 @@ where // the checked-out code may lead to arbitrary code execution checkout.getAFollowingStep() = s and // the checkout occurs in a privileged context - j.isPrivilegedExternallyTriggerable() and + inPrivilegedContext(checkout) and // the mutable checkout step is protected by an Insufficient access check - check.dominates(checkout) and - check.protects(checkout, j.getATriggerEvent()) and - check.protectsAgainstRefMutationAttacks() = false + check.protects(checkout, j.getATriggerEvent(), "untrusted-checkout") and + not check.protects(checkout, j.getATriggerEvent(), "untrusted-checkout-toctou") select s, checkout, s, "Insufficient protection against execution of untrusted code on a privileged workflow on check $@.", check, check.toString() diff --git a/ql/src/Security/CWE-367/UntrustedCheckoutTOCTOUHigh.ql b/ql/src/Security/CWE-367/UntrustedCheckoutTOCTOUHigh.ql index 0a83cc54ad6..7f584e00c9a 100644 --- a/ql/src/Security/CWE-367/UntrustedCheckoutTOCTOUHigh.ql +++ b/ql/src/Security/CWE-367/UntrustedCheckoutTOCTOUHigh.ql @@ -22,11 +22,10 @@ where // there are no evidences that the checked-out gets executed not checkout.getAFollowingStep() instanceof PoisonableStep and // the checkout occurs in a privileged context - j.isPrivilegedExternallyTriggerable() and + inPrivilegedContext(checkout) and // the mutable checkout step is protected by an Insufficient access check - check.dominates(checkout) and - check.protects(checkout, j.getATriggerEvent()) and - check.protectsAgainstRefMutationAttacks() = false + check.protects(checkout, j.getATriggerEvent(), "untrusted-checkout") and + not check.protects(checkout, j.getATriggerEvent(), "untrusted-checkout-toctou") select checkout, "Insufficient protection against execution of untrusted code on a privileged workflow on step $@.", check, check.toString() diff --git a/ql/src/Security/CWE-829/UntrustedCheckoutCritical.ql b/ql/src/Security/CWE-829/UntrustedCheckoutCritical.ql index 2026a784d05..499abc047b6 100644 --- a/ql/src/Security/CWE-829/UntrustedCheckoutCritical.ql +++ b/ql/src/Security/CWE-829/UntrustedCheckoutCritical.ql @@ -20,10 +20,33 @@ import codeql.actions.security.ControlChecks query predicate edges(Step a, Step b) { a.getNextStep() = b } -from PRHeadCheckoutStep checkout, PoisonableStep s +from PRHeadCheckoutStep checkout, PoisonableStep step where // the checkout is followed by a known poisonable step - checkout.getAFollowingStep() = s and + checkout.getAFollowingStep() = step and // the checkout occurs in a privileged context - inPrivilegedContext(checkout) -select s, checkout, s, "Execution of untrusted code on a privileged workflow." + inPrivilegedContext(checkout) and + ( + // issue_comment: check for date comparison checks and actor/access control checks + exists(Event event | + event.getName() = "issue_comment" and + event = checkout.getEnclosingJob().getATriggerEvent() and + not exists(ControlCheck check, CommentVsHeadDateCheck date_check | + ( + check instanceof ActorCheck or + check instanceof AssociationCheck or + check instanceof PermissionCheck + ) and + check.dominates(checkout) and + date_check.dominates(checkout) + ) + ) + or + // not issue_comment triggered workflows + exists(Event event | + not event.getName() = "issue_comment" and + event = checkout.getEnclosingJob().getATriggerEvent() and + not exists(ControlCheck check | check.protects(checkout, event, "untrusted-checkout")) + ) + ) +select step, checkout, step, "Execution of untrusted code on a privileged workflow." diff --git a/ql/src/Security/CWE-829/UntrustedCheckoutHigh.ql b/ql/src/Security/CWE-829/UntrustedCheckoutHigh.ql index 0675603af0f..8577218800e 100644 --- a/ql/src/Security/CWE-829/UntrustedCheckoutHigh.ql +++ b/ql/src/Security/CWE-829/UntrustedCheckoutHigh.ql @@ -23,5 +23,26 @@ where // the checkout is NOT followed by a known poisonable step not checkout.getAFollowingStep() instanceof PoisonableStep and // the checkout occurs in a privileged context - inPrivilegedContext(checkout) + inPrivilegedContext(checkout) and + ( + // issue_comment: check for date comparison checks and actor/access control checks + exists(Event e | + e.getName() = "issue_comment" and + checkout.getEnclosingJob().getATriggerEvent() = e and + not exists(ControlCheck write_check, CommentVsHeadDateCheck data_check | + (write_check instanceof ActorCheck or write_check instanceof AssociationCheck) and + write_check.dominates(checkout) and + data_check.dominates(checkout) + ) + ) + or + // not issue_comment triggered workflows + exists(Event event | + not event.getName() = "issue_comment" and + not exists(ControlCheck check | + check + .protects(checkout, checkout.getEnclosingJob().getATriggerEvent(), "untrusted-checkout") + ) + ) + ) select checkout, "Potential execution of untrusted code on a privileged workflow." diff --git a/ql/test/query-tests/Security/CWE-829/.github/workflows/test11.yml b/ql/test/query-tests/Security/CWE-829/.github/workflows/test11.yml new file mode 100644 index 00000000000..16bb6bf876c --- /dev/null +++ b/ql/test/query-tests/Security/CWE-829/.github/workflows/test11.yml @@ -0,0 +1,94 @@ +name: Test + +on: + + issue_comment: + types: [created] + +jobs: + + deploy: + name: Update deployment + if: >- + ${{ + github.event.issue.pull_request && + (contains(github.event.comment.body, '/deploy') || contains(github.event.comment.body, '/rollback')) && + contains(github.event.issue.labels.*.name, 'Deployment Update') && + github.event.comment.user.type != 'Bot' + }} + + runs-on: [self-hosted, production] + + permissions: + contents: write + issues: write + pull-requests: write + statuses: write + + steps: + + - name: Check comment keywords + shell: bash + env: + COMMENT_BODY: ${{ github.event.comment.body }} + PR_COMMENT_ALLOW_LIST: ${{ secrets.PR_COMMENT_ALLOW_LIST }} + run: | + function list_subset { local list1="$1"; local list2="$2"; result=0; for item in $list2; do if ! [[ $list1 =~ (^|[[:space:]])"$item"($|[[:space:]]) ]]; then result=1; fi; done; return $result; } + + if `list_subset "echo $PR_COMMENT_ALLOW_LIST" "echo $COMMENT_BODY"` ; then + echo "Command keywords allowed. Proceeding!" + else + echo "Command keywords not allowed. Skipping!" + exit 1 + fi + + - name: Get environment from comment + id: environment + shell: bash + env: + COMMENT_BODY: ${{ github.event.comment.body }} + COMMENT_AT: ${{ github.event.comment.created_at }} + GH_REPO: ${{ github.repository }} + PR_NUMBER: ${{ github.event.issue.number }} + GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} + run: | + pr="$(gh api /repos/${GH_REPO}/pulls/${PR_NUMBER})" + head_sha="$(echo "$pr" | jq -r .head.sha)" + pushed_at="$(echo "$pr" | jq -r .pushed_at)" + + if [[ $(date -d "$pushed_at" +%s) -gt $(date -d "$COMMENT_AT" +%s) ]]; then + echo "Deployment not allowed because the PR was pushed to (at $pushed_at) after the triggering comment was issued (at $COMMENT_AT)" + exit 1 + fi + + target=$(echo "$COMMENT_BODY" | sed 's/.* //') && \ + deploy_type=$(echo "$COMMENT_BODY" | sed 's/ .*//') + + if [[ $target == "scorer" ]]; then + echo "env=async scorer" >> $GITHUB_OUTPUT + else + env=$(echo "$target") + echo "env=$env" >> $GITHUB_OUTPUT + fi + + if [[ $deploy_type == "/deploy" ]]; then + echo "depl=deployment" >> $GITHUB_OUTPUT + elif [[ $deploy_type == "/rollback" ]]; then + echo "depl=rollback" >> $GITHUB_OUTPUT + else + echo "depl=unknown deployment type" >> $GITHUB_OUTPUT + fi + + echo "head_sha=$head_sha" >> $GITHUB_OUTPUT + + - name: Checkout PR branch + if: contains(github.event.comment.body, '/deploy') + uses: actions/checkout@692973e3d937129bcbf40652eb9f2f61becf3332 # v4 + with: + ref: ${{ steps.environment.outputs.head_sha }} + + - name: Environment setup + uses: ./.github/actions/setup-env + with: + azure_creds: ${{ secrets.AZURE_CREDENTIALS }} + diff --git a/ql/test/query-tests/Security/CWE-829/.github/workflows/test12.yml b/ql/test/query-tests/Security/CWE-829/.github/workflows/test12.yml new file mode 100644 index 00000000000..878b8377961 --- /dev/null +++ b/ql/test/query-tests/Security/CWE-829/.github/workflows/test12.yml @@ -0,0 +1,96 @@ +name: Test + +on: + + issue_comment: + types: [created] + +jobs: + + deploy: + name: Update deployment + if: > + github.event.issue.pull_request && + (contains(github.event.comment.body, '/deploy') || contains(github.event.comment.body, '/rollback')) && + contains(github.event.issue.labels.*.name, 'Deployment Update') && + github.event.comment.user.type != 'Bot' && + ( + github.event.issue.author_association == 'OWNER' || + github.event.issue.author_association == 'COLLABORATOR' || + github.event.issue.author_association == 'MEMBER' + ) + runs-on: [self-hosted, production] + + permissions: + contents: write + issues: write + pull-requests: write + statuses: write + + steps: + + - name: Check comment keywords + shell: bash + env: + COMMENT_BODY: ${{ github.event.comment.body }} + PR_COMMENT_ALLOW_LIST: ${{ secrets.PR_COMMENT_ALLOW_LIST }} + run: | + function list_subset { local list1="$1"; local list2="$2"; result=0; for item in $list2; do if ! [[ $list1 =~ (^|[[:space:]])"$item"($|[[:space:]]) ]]; then result=1; fi; done; return $result; } + + if `list_subset "echo $PR_COMMENT_ALLOW_LIST" "echo $COMMENT_BODY"` ; then + echo "Command keywords allowed. Proceeding!" + else + echo "Command keywords not allowed. Skipping!" + exit 1 + fi + + - name: Get environment from comment + id: environment + shell: bash + env: + COMMENT_BODY: ${{ github.event.comment.body }} + COMMENT_AT: ${{ github.event.comment.created_at }} + GH_REPO: ${{ github.repository }} + PR_NUMBER: ${{ github.event.issue.number }} + GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} + run: | + pr="$(gh api /repos/${GH_REPO}/pulls/${PR_NUMBER})" + head_sha="$(echo "$pr" | jq -r .head.sha)" + pushed_at="$(echo "$pr" | jq -r .pushed_at)" + + if [[ $(date -d "$pushed_at" +%s) -gt $(date -d "$COMMENT_AT" +%s) ]]; then + echo "Deployment not allowed because the PR was pushed to (at $pushed_at) after the triggering comment was issued (at $COMMENT_AT)" + exit 1 + fi + + target=$(echo "$COMMENT_BODY" | sed 's/.* //') && \ + deploy_type=$(echo "$COMMENT_BODY" | sed 's/ .*//') + + if [[ $target == "scorer" ]]; then + echo "env=async scorer" >> $GITHUB_OUTPUT + else + env=$(echo "$target") + echo "env=$env" >> $GITHUB_OUTPUT + fi + + if [[ $deploy_type == "/deploy" ]]; then + echo "depl=deployment" >> $GITHUB_OUTPUT + elif [[ $deploy_type == "/rollback" ]]; then + echo "depl=rollback" >> $GITHUB_OUTPUT + else + echo "depl=unknown deployment type" >> $GITHUB_OUTPUT + fi + + echo "head_sha=$head_sha" >> $GITHUB_OUTPUT + + - name: Checkout PR branch + if: contains(github.event.comment.body, '/deploy') + uses: actions/checkout@692973e3d937129bcbf40652eb9f2f61becf3332 # v4 + with: + ref: ${{ steps.environment.outputs.head_sha }} + + - name: Environment setup + uses: ./.github/actions/setup-env + with: + azure_creds: ${{ secrets.AZURE_CREDENTIALS }} + diff --git a/ql/test/query-tests/Security/CWE-829/.github/workflows/test13.yml b/ql/test/query-tests/Security/CWE-829/.github/workflows/test13.yml new file mode 100644 index 00000000000..0a73e86d5fc --- /dev/null +++ b/ql/test/query-tests/Security/CWE-829/.github/workflows/test13.yml @@ -0,0 +1,31 @@ +on: + issue_comment: + types: + - created +jobs: + danger-for-external: + name: Danger for external - Node.js 16 + if: | + github.event_name == 'issue_comment' && github.event.action == 'created' + && github.event.issue.pull_request != null + && startsWith(github.event.comment.body, '/danger') + runs-on: ubuntu-latest + steps: + - name: Check repository permission for user + uses: sushichop/action-repository-permission@v2 + with: + required-permission: write + reaction-permitted: rocket + comment-not-permitted: Sorry, you don't have enough permission to execute `/danger`... + - name: Clone the PR source + uses: actions/checkout@v3 + with: + ref: refs/pull/${{ github.event.issue.number }}/head + fetch-depth: 0 + - uses: actions/setup-node@v3 + with: + node-version: 16 + - name: Danger JS + run: npx danger ci + env: + GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} diff --git a/ql/test/query-tests/Security/CWE-829/UnpinnedActionsTag.expected b/ql/test/query-tests/Security/CWE-829/UnpinnedActionsTag.expected index c91470d5cc8..5d38b397a42 100644 --- a/ql/test/query-tests/Security/CWE-829/UnpinnedActionsTag.expected +++ b/ql/test/query-tests/Security/CWE-829/UnpinnedActionsTag.expected @@ -27,4 +27,5 @@ | .github/workflows/pr-workflow.yml:452:9:453:6 | Uses Step | Unpinned 3rd party Action 'pr-workflow' step $@ uses 'determinatesystems/magic-nix-cache-action' with ref 'main', not a pinned commit hash | .github/workflows/pr-workflow.yml:452:9:453:6 | Uses Step | Uses Step | | .github/workflows/pr-workflow.yml:453:9:459:6 | Uses Step | Unpinned 3rd party Action 'pr-workflow' step $@ uses 'cachix/cachix-action' with ref 'master', not a pinned commit hash | .github/workflows/pr-workflow.yml:453:9:459:6 | Uses Step | Uses Step | | .github/workflows/test7.yml:24:9:27:6 | Uses Step | Unpinned 3rd party Action 'Benchmark' step $@ uses 'pnpm/action-setup' with ref 'v3', not a pinned commit hash | .github/workflows/test7.yml:24:9:27:6 | Uses Step | Uses Step | +| .github/workflows/test13.yml:14:7:20:4 | Uses Step | Unpinned 3rd party Action 'test13.yml' step $@ uses 'sushichop/action-repository-permission' with ref 'v2', not a pinned commit hash | .github/workflows/test13.yml:14:7:20:4 | Uses Step | Uses Step | | .github/workflows/unpinned_tags.yml:10:7:11:4 | Uses Step | Unpinned 3rd party Action 'unpinned_tags.yml' step $@ uses 'foo/bar' with ref 'v1', not a pinned commit hash | .github/workflows/unpinned_tags.yml:10:7:11:4 | Uses Step | Uses Step | diff --git a/ql/test/query-tests/Security/CWE-829/UntrustedCheckoutCritical.expected b/ql/test/query-tests/Security/CWE-829/UntrustedCheckoutCritical.expected index 7313ffd9ae3..8bb9e02559c 100644 --- a/ql/test/query-tests/Security/CWE-829/UntrustedCheckoutCritical.expected +++ b/ql/test/query-tests/Security/CWE-829/UntrustedCheckoutCritical.expected @@ -175,6 +175,15 @@ edges | .github/workflows/test8.yml:20:9:26:6 | Uses Step | .github/workflows/test8.yml:26:9:29:2 | Run Step | | .github/workflows/test9.yml:11:9:16:6 | Uses Step | .github/workflows/test9.yml:16:9:17:48 | Run Step | | .github/workflows/test10.yml:20:9:25:6 | Uses Step | .github/workflows/test10.yml:25:9:30:2 | Run Step | +| .github/workflows/test11.yml:30:7:45:4 | Run Step | .github/workflows/test11.yml:45:7:84:4 | Run Step: environment | +| .github/workflows/test11.yml:45:7:84:4 | Run Step: environment | .github/workflows/test11.yml:84:7:90:4 | Uses Step | +| .github/workflows/test11.yml:84:7:90:4 | Uses Step | .github/workflows/test11.yml:90:7:93:54 | Uses Step | +| .github/workflows/test12.yml:32:7:47:4 | Run Step | .github/workflows/test12.yml:47:7:86:4 | Run Step: environment | +| .github/workflows/test12.yml:47:7:86:4 | Run Step: environment | .github/workflows/test12.yml:86:7:92:4 | Uses Step | +| .github/workflows/test12.yml:86:7:92:4 | Uses Step | .github/workflows/test12.yml:92:7:95:54 | Uses Step | +| .github/workflows/test13.yml:14:7:20:4 | Uses Step | .github/workflows/test13.yml:20:7:25:4 | Uses Step | +| .github/workflows/test13.yml:20:7:25:4 | Uses Step | .github/workflows/test13.yml:25:7:28:4 | Uses Step | +| .github/workflows/test13.yml:25:7:28:4 | Uses Step | .github/workflows/test13.yml:28:7:31:50 | Run Step | | .github/workflows/test.yml:13:9:14:6 | Uses Step | .github/workflows/test.yml:14:9:25:6 | Run Step | | .github/workflows/test.yml:14:9:25:6 | Run Step | .github/workflows/test.yml:25:9:33:6 | Run Step | | .github/workflows/test.yml:25:9:33:6 | Run Step | .github/workflows/test.yml:33:9:37:34 | Run Step | @@ -223,7 +232,7 @@ edges | .github/workflows/test7.yml:49:9:58:20 | Run Step: benchmark-pr | .github/workflows/test7.yml:19:9:24:6 | Uses Step | .github/workflows/test7.yml:49:9:58:20 | Run Step: benchmark-pr | Execution of untrusted code on a privileged workflow. | | .github/workflows/test9.yml:16:9:17:48 | Run Step | .github/workflows/test9.yml:11:9:16:6 | Uses Step | .github/workflows/test9.yml:16:9:17:48 | Run Step | Execution of untrusted code on a privileged workflow. | | .github/workflows/test10.yml:25:9:30:2 | Run Step | .github/workflows/test10.yml:20:9:25:6 | Uses Step | .github/workflows/test10.yml:25:9:30:2 | Run Step | Execution of untrusted code on a privileged workflow. | -| .github/workflows/untrusted_checkout3.yml:13:9:13:23 | Run Step | .github/actions/dangerous-git-checkout/action.yml:6:7:11:4 | Uses Step | .github/workflows/untrusted_checkout3.yml:13:9:13:23 | Run Step | Execution of untrusted code on a privileged workflow. | +| .github/workflows/test11.yml:90:7:93:54 | Uses Step | .github/workflows/test11.yml:84:7:90:4 | Uses Step | .github/workflows/test11.yml:90:7:93:54 | Uses Step | Execution of untrusted code on a privileged workflow. | | .github/workflows/untrusted_checkout4.yml:61:7:67:4 | Run Step | .github/workflows/untrusted_checkout4.yml:55:7:61:4 | Uses Step | .github/workflows/untrusted_checkout4.yml:61:7:67:4 | Run Step | Execution of untrusted code on a privileged workflow. | | .github/workflows/untrusted_checkout4.yml:67:7:73:4 | Run Step | .github/workflows/untrusted_checkout4.yml:55:7:61:4 | Uses Step | .github/workflows/untrusted_checkout4.yml:67:7:73:4 | Run Step | Execution of untrusted code on a privileged workflow. | | .github/workflows/untrusted_checkout4.yml:73:7:79:4 | Run Step | .github/workflows/untrusted_checkout4.yml:55:7:61:4 | Uses Step | .github/workflows/untrusted_checkout4.yml:73:7:79:4 | Run Step | Execution of untrusted code on a privileged workflow. | diff --git a/ql/test/query-tests/Security/CWE-829/UntrustedCheckoutHigh.expected b/ql/test/query-tests/Security/CWE-829/UntrustedCheckoutHigh.expected index b9cf0e547ca..181bd5673bc 100644 --- a/ql/test/query-tests/Security/CWE-829/UntrustedCheckoutHigh.expected +++ b/ql/test/query-tests/Security/CWE-829/UntrustedCheckoutHigh.expected @@ -18,6 +18,7 @@ | .github/workflows/pr-workflow.yml:103:9:109:6 | Uses Step | Potential execution of untrusted code on a privileged workflow. | | .github/workflows/pr-workflow.yml:139:9:144:6 | Uses Step | Potential execution of untrusted code on a privileged workflow. | | .github/workflows/pr-workflow.yml:444:9:449:6 | Uses Step | Potential execution of untrusted code on a privileged workflow. | +| .github/workflows/test13.yml:20:7:25:4 | Uses Step | Potential execution of untrusted code on a privileged workflow. | | .github/workflows/untrusted_checkout2.yml:14:9:19:72 | Run Step | Potential execution of untrusted code on a privileged workflow. | | .github/workflows/workflow_run_untrusted_checkout.yml:13:9:16:6 | Uses Step | Potential execution of untrusted code on a privileged workflow. | | .github/workflows/workflow_run_untrusted_checkout.yml:16:9:18:31 | Uses Step | Potential execution of untrusted code on a privileged workflow. |