diff --git a/actions/ql/lib/codeql/actions/security/ArgumentInjectionQuery.qll b/actions/ql/lib/codeql/actions/security/ArgumentInjectionQuery.qll index 1d461cca3df2..679b8977cf91 100644 --- a/actions/ql/lib/codeql/actions/security/ArgumentInjectionQuery.qll +++ b/actions/ql/lib/codeql/actions/security/ArgumentInjectionQuery.qll @@ -1,6 +1,7 @@ private import actions private import codeql.actions.TaintTracking private import codeql.actions.dataflow.ExternalFlow +private import codeql.actions.security.ControlChecks import codeql.actions.dataflow.FlowSources import codeql.actions.DataFlow @@ -65,6 +66,16 @@ class ArgumentInjectionFromMaDSink extends ArgumentInjectionSink { override string getCommand() { result = "unknown" } } +/** + * Gets the event that is relevant for the given node in the context of argument injection. + * + * This is used to highlight the event in the query results when an alert is raised. + */ +Event getRelevantEventInPrivilegedContext(DataFlow::Node node) { + inPrivilegedContext(node.asExpr(), result) and + not exists(ControlCheck check | check.protects(node.asExpr(), result, "argument-injection")) +} + /** * A taint-tracking configuration for unsafe user input * that is used to construct and evaluate a code script. @@ -88,6 +99,16 @@ private module ArgumentInjectionConfig implements DataFlow::ConfigSig { run.getScript().getAnEnvReachingArgumentInjectionSink(var, _, _) ) } + + predicate observeDiffInformedIncrementalMode() { any() } + + Location getASelectedSourceLocation(DataFlow::Node source) { none() } + + Location getASelectedSinkLocation(DataFlow::Node sink) { + result = sink.getLocation() + or + result = getRelevantEventInPrivilegedContext(sink).getLocation() + } } /** Tracks flow of unsafe user input that is used to construct and evaluate a code script. */ diff --git a/actions/ql/lib/codeql/actions/security/ArtifactPoisoningQuery.qll b/actions/ql/lib/codeql/actions/security/ArtifactPoisoningQuery.qll index f0649bb8174e..76025a9ba0db 100644 --- a/actions/ql/lib/codeql/actions/security/ArtifactPoisoningQuery.qll +++ b/actions/ql/lib/codeql/actions/security/ArtifactPoisoningQuery.qll @@ -4,6 +4,7 @@ import codeql.actions.DataFlow import codeql.actions.dataflow.FlowSources import codeql.actions.security.PoisonableSteps import codeql.actions.security.UntrustedCheckoutQuery +import codeql.actions.security.ControlChecks string unzipRegexp() { result = "(unzip|tar)\\s+.*" } @@ -292,6 +293,16 @@ class ArtifactPoisoningSink extends DataFlow::Node { string getPath() { result = download.getPath() } } +/** + * Gets the event that is relevant for the given node in the context of artifact poisoning. + * + * This is used to highlight the event in the query results when an alert is raised. + */ +Event getRelevantEventInPrivilegedContext(DataFlow::Node node) { + inPrivilegedContext(node.asExpr(), result) and + not exists(ControlCheck check | check.protects(node.asExpr(), result, "artifact-poisoning")) +} + /** * A taint-tracking configuration for unsafe artifacts * that is used may lead to artifact poisoning @@ -318,6 +329,16 @@ private module ArtifactPoisoningConfig implements DataFlow::ConfigSig { exists(run.getScript().getAFileReadCommand()) ) } + + predicate observeDiffInformedIncrementalMode() { any() } + + Location getASelectedSourceLocation(DataFlow::Node source) { none() } + + Location getASelectedSinkLocation(DataFlow::Node sink) { + result = sink.getLocation() + or + result = getRelevantEventInPrivilegedContext(sink).getLocation() + } } /** Tracks flow of unsafe artifacts that is used in an insecure way. */ diff --git a/actions/ql/lib/codeql/actions/security/CodeInjectionQuery.qll b/actions/ql/lib/codeql/actions/security/CodeInjectionQuery.qll index fac498f72dab..c58e3949a024 100644 --- a/actions/ql/lib/codeql/actions/security/CodeInjectionQuery.qll +++ b/actions/ql/lib/codeql/actions/security/CodeInjectionQuery.qll @@ -3,6 +3,8 @@ private import codeql.actions.TaintTracking private import codeql.actions.dataflow.ExternalFlow import codeql.actions.dataflow.FlowSources import codeql.actions.DataFlow +import codeql.actions.security.ControlChecks +import codeql.actions.security.CachePoisoningQuery class CodeInjectionSink extends DataFlow::Node { CodeInjectionSink() { @@ -11,6 +13,46 @@ class CodeInjectionSink extends DataFlow::Node { } } +/** + * Get the relevant event for the sink in CodeInjectionCritical.ql. + */ +Event getRelevantCriticalEventForSink(DataFlow::Node sink) { + inPrivilegedContext(sink.asExpr(), result) and + not exists(ControlCheck check | check.protects(sink.asExpr(), result, "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 + script.getArgumentExpr("script") = sink.asExpr() and + exists(getAToJsonReferenceExpression(sink.asExpr().(Expression).getExpression(), _)) + ) +} + +/** + * Get the relevant event for the sink in CachePoisoningViaCodeInjection.ql. + */ +Event getRelevantCachePoisoningEventForSink(DataFlow::Node sink) { + exists(LocalJob job | + job = sink.asExpr().getEnclosingJob() and + job.getATriggerEvent() = result and + // job can be triggered by an external user + result.isExternallyTriggerable() and + // excluding privileged workflows since they can be exploited in easier circumstances + // which is covered by `actions/code-injection/critical` + not job.isPrivilegedExternallyTriggerable(result) and + ( + // the workflow runs in the context of the default branch + runsOnDefaultBranch(result) + or + // the workflow caller runs in the context of the default branch + result.getName() = "workflow_call" and + exists(ExternalJob caller | + caller.getCallee() = job.getLocation().getFile().getRelativePath() and + runsOnDefaultBranch(caller.getATriggerEvent()) + ) + ) + ) +} + /** * A taint-tracking configuration for unsafe user input * that is used to construct and evaluate a code script. @@ -35,6 +77,18 @@ private module CodeInjectionConfig implements DataFlow::ConfigSig { exists(run.getScript().getAFileReadCommand()) ) } + + predicate observeDiffInformedIncrementalMode() { any() } + + Location getASelectedSourceLocation(DataFlow::Node source) { none() } + + Location getASelectedSinkLocation(DataFlow::Node sink) { + result = sink.getLocation() + or + result = getRelevantCriticalEventForSink(sink).getLocation() + or + result = getRelevantCachePoisoningEventForSink(sink).getLocation() + } } /** Tracks flow of unsafe user input that is used to construct and evaluate a code script. */ diff --git a/actions/ql/lib/codeql/actions/security/CommandInjectionQuery.qll b/actions/ql/lib/codeql/actions/security/CommandInjectionQuery.qll index 59d523cd5827..ffa16b0f9406 100644 --- a/actions/ql/lib/codeql/actions/security/CommandInjectionQuery.qll +++ b/actions/ql/lib/codeql/actions/security/CommandInjectionQuery.qll @@ -3,11 +3,20 @@ private import codeql.actions.TaintTracking private import codeql.actions.dataflow.ExternalFlow import codeql.actions.dataflow.FlowSources import codeql.actions.DataFlow +import codeql.actions.security.ControlChecks private class CommandInjectionSink extends DataFlow::Node { CommandInjectionSink() { madSink(this, "command-injection") } } +/** Get the relevant event for the sink in CommandInjectionCritical.ql. */ +Event getRelevantEventInPrivilegedContext(DataFlow::Node sink) { + inPrivilegedContext(sink.asExpr(), result) and + not exists(ControlCheck check | + check.protects(sink.asExpr(), result, ["command-injection", "code-injection"]) + ) +} + /** * A taint-tracking configuration for unsafe user input * that is used to construct and evaluate a system command. @@ -16,6 +25,16 @@ private module CommandInjectionConfig implements DataFlow::ConfigSig { predicate isSource(DataFlow::Node source) { source instanceof RemoteFlowSource } predicate isSink(DataFlow::Node sink) { sink instanceof CommandInjectionSink } + + predicate observeDiffInformedIncrementalMode() { any() } + + Location getASelectedSourceLocation(DataFlow::Node source) { none() } + + Location getASelectedSinkLocation(DataFlow::Node sink) { + result = sink.getLocation() + or + result = getRelevantEventInPrivilegedContext(sink).getLocation() + } } /** Tracks flow of unsafe user input that is used to construct and evaluate a system command. */ diff --git a/actions/ql/lib/codeql/actions/security/EnvPathInjectionQuery.qll b/actions/ql/lib/codeql/actions/security/EnvPathInjectionQuery.qll index 33efc9b1bc8f..46c1c4d32006 100644 --- a/actions/ql/lib/codeql/actions/security/EnvPathInjectionQuery.qll +++ b/actions/ql/lib/codeql/actions/security/EnvPathInjectionQuery.qll @@ -72,6 +72,25 @@ class EnvPathInjectionFromMaDSink extends EnvPathInjectionSink { EnvPathInjectionFromMaDSink() { madSink(this, "envpath-injection") } } +/** + * Get the relevant event for a sink in EnvPathInjectionCritical.ql where the source type is "artifact". + */ +Event getRelevantArtifactEventInPrivilegedContext(DataFlow::Node sink) { + inPrivilegedContext(sink.asExpr(), result) and + not exists(ControlCheck check | + check.protects(sink.asExpr(), result, ["untrusted-checkout", "artifact-poisoning"]) + ) and + sink instanceof EnvPathInjectionFromFileReadSink +} + +/** + * Get the relevant event for a sink in EnvPathInjectionCritical.ql where the source type is not "artifact". + */ +Event getRelevantNonArtifactEventInPrivilegedContext(DataFlow::Node sink) { + inPrivilegedContext(sink.asExpr(), result) and + not exists(ControlCheck check | check.protects(sink.asExpr(), result, "code-injection")) +} + /** * A taint-tracking configuration for unsafe user input * that is used to construct and evaluate an environment variable. @@ -108,6 +127,18 @@ private module EnvPathInjectionConfig implements DataFlow::ConfigSig { exists(run.getScript().getAFileReadCommand()) ) } + + predicate observeDiffInformedIncrementalMode() { any() } + + Location getASelectedSourceLocation(DataFlow::Node source) { none() } + + Location getASelectedSinkLocation(DataFlow::Node sink) { + result = sink.getLocation() + or + result = getRelevantArtifactEventInPrivilegedContext(sink).getLocation() + or + result = getRelevantNonArtifactEventInPrivilegedContext(sink).getLocation() + } } /** Tracks flow of unsafe user input that is used to construct and evaluate the PATH environment variable. */ diff --git a/actions/ql/lib/codeql/actions/security/EnvVarInjectionQuery.qll b/actions/ql/lib/codeql/actions/security/EnvVarInjectionQuery.qll index 656ea1207b51..2022e3dca998 100644 --- a/actions/ql/lib/codeql/actions/security/EnvVarInjectionQuery.qll +++ b/actions/ql/lib/codeql/actions/security/EnvVarInjectionQuery.qll @@ -126,6 +126,32 @@ class EnvVarInjectionFromMaDSink extends EnvVarInjectionSink { EnvVarInjectionFromMaDSink() { madSink(this, "envvar-injection") } } +/** + * Get the relevant event for a sink in EnvVarInjectionCritical.ql where the source type is "artifact". + */ +Event getRelevantArtifactEventInPrivilegedContext(DataFlow::Node sink) { + inPrivilegedContext(sink.asExpr(), result) and + not exists(ControlCheck check | + check + .protects(sink.asExpr(), result, + ["envvar-injection", "untrusted-checkout", "artifact-poisoning"]) + ) and + ( + sink instanceof EnvVarInjectionFromFileReadSink or + madSink(sink, "envvar-injection") + ) +} + +/** + * Get the relevant event for a sink in EnvVarInjectionCritical.ql where the source type is not "artifact". + */ +Event getRelevantNonArtifactEventInPrivilegedContext(DataFlow::Node sink) { + inPrivilegedContext(sink.asExpr(), result) and + not exists(ControlCheck check | + check.protects(sink.asExpr(), result, ["envvar-injection", "code-injection"]) + ) +} + /** * A taint-tracking configuration for unsafe user input * that is used to construct and evaluate an environment variable. @@ -163,6 +189,18 @@ private module EnvVarInjectionConfig implements DataFlow::ConfigSig { exists(run.getScript().getAFileReadCommand()) ) } + + predicate observeDiffInformedIncrementalMode() { any() } + + Location getASelectedSourceLocation(DataFlow::Node source) { none() } + + Location getASelectedSinkLocation(DataFlow::Node sink) { + result = sink.getLocation() + or + result = getRelevantArtifactEventInPrivilegedContext(sink).getLocation() + or + result = getRelevantNonArtifactEventInPrivilegedContext(sink).getLocation() + } } /** Tracks flow of unsafe user input that is used to construct and evaluate an environment variable. */ diff --git a/actions/ql/src/Security/CWE-077/EnvPathInjectionCritical.ql b/actions/ql/src/Security/CWE-077/EnvPathInjectionCritical.ql index 3e6d63a4604d..30bb0fd366fb 100644 --- a/actions/ql/src/Security/CWE-077/EnvPathInjectionCritical.ql +++ b/actions/ql/src/Security/CWE-077/EnvPathInjectionCritical.ql @@ -21,18 +21,12 @@ import codeql.actions.security.ControlChecks from EnvPathInjectionFlow::PathNode source, EnvPathInjectionFlow::PathNode sink, Event event where EnvPathInjectionFlow::flowPath(source, sink) and - inPrivilegedContext(sink.getNode().asExpr(), event) and ( not source.getNode().(RemoteFlowSource).getSourceType() = "artifact" and - not exists(ControlCheck check | - check.protects(sink.getNode().asExpr(), event, "code-injection") - ) + event = getRelevantNonArtifactEventInPrivilegedContext(sink.getNode()) or source.getNode().(RemoteFlowSource).getSourceType() = "artifact" and - not exists(ControlCheck check | - check.protects(sink.getNode().asExpr(), event, ["untrusted-checkout", "artifact-poisoning"]) - ) and - sink.getNode() instanceof EnvPathInjectionFromFileReadSink + event = getRelevantArtifactEventInPrivilegedContext(sink.getNode()) ) select sink.getNode(), source, sink, "Potential PATH environment variable injection in $@, which may be controlled by an external user ($@).", diff --git a/actions/ql/src/Security/CWE-077/EnvVarInjectionCritical.ql b/actions/ql/src/Security/CWE-077/EnvVarInjectionCritical.ql index 28ad3b5b5d28..6f0d9729d6d3 100644 --- a/actions/ql/src/Security/CWE-077/EnvVarInjectionCritical.ql +++ b/actions/ql/src/Security/CWE-077/EnvVarInjectionCritical.ql @@ -22,26 +22,15 @@ import codeql.actions.security.ControlChecks from EnvVarInjectionFlow::PathNode source, EnvVarInjectionFlow::PathNode sink, Event event where EnvVarInjectionFlow::flowPath(source, sink) and - inPrivilegedContext(sink.getNode().asExpr(), event) and // exclude paths to file read sinks from non-artifact sources ( // source is text not source.getNode().(RemoteFlowSource).getSourceType() = "artifact" and - not exists(ControlCheck check | - check.protects(sink.getNode().asExpr(), event, ["envvar-injection", "code-injection"]) - ) + event = getRelevantNonArtifactEventInPrivilegedContext(sink.getNode()) or // source is an artifact or a file from an untrusted checkout source.getNode().(RemoteFlowSource).getSourceType() = "artifact" and - not exists(ControlCheck check | - check - .protects(sink.getNode().asExpr(), event, - ["envvar-injection", "untrusted-checkout", "artifact-poisoning"]) - ) and - ( - sink.getNode() instanceof EnvVarInjectionFromFileReadSink or - madSink(sink.getNode(), "envvar-injection") - ) + event = getRelevantArtifactEventInPrivilegedContext(sink.getNode()) ) select sink.getNode(), source, sink, "Potential environment variable injection in $@, which may be controlled by an external user ($@).", diff --git a/actions/ql/src/Security/CWE-094/CodeInjectionCritical.ql b/actions/ql/src/Security/CWE-094/CodeInjectionCritical.ql index c4ab00837ca7..ed30e4da71c8 100644 --- a/actions/ql/src/Security/CWE-094/CodeInjectionCritical.ql +++ b/actions/ql/src/Security/CWE-094/CodeInjectionCritical.ql @@ -22,15 +22,8 @@ import codeql.actions.security.ControlChecks from CodeInjectionFlow::PathNode source, CodeInjectionFlow::PathNode sink, Event event where CodeInjectionFlow::flowPath(source, sink) and - inPrivilegedContext(sink.getNode().asExpr(), event) and - source.getNode().(RemoteFlowSource).getEventName() = event.getName() and - not exists(ControlCheck check | check.protects(sink.getNode().asExpr(), event, "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 - script.getArgumentExpr("script") = sink.getNode().asExpr() and - exists(getAToJsonReferenceExpression(sink.getNode().asExpr().(Expression).getExpression(), _)) - ) + event = getRelevantCriticalEventForSink(sink.getNode()) and + source.getNode().(RemoteFlowSource).getEventName() = event.getName() select sink.getNode(), source, sink, "Potential code injection in $@, which may be controlled by an external user ($@).", sink, sink.getNode().asExpr().(Expression).getRawExpression(), event, event.getName() diff --git a/actions/ql/src/Security/CWE-349/CachePoisoningViaCodeInjection.ql b/actions/ql/src/Security/CWE-349/CachePoisoningViaCodeInjection.ql index 23e1f223073f..2fe792aba1e6 100644 --- a/actions/ql/src/Security/CWE-349/CachePoisoningViaCodeInjection.ql +++ b/actions/ql/src/Security/CWE-349/CachePoisoningViaCodeInjection.ql @@ -18,30 +18,13 @@ import codeql.actions.security.CachePoisoningQuery import CodeInjectionFlow::PathGraph import codeql.actions.security.ControlChecks -from CodeInjectionFlow::PathNode source, CodeInjectionFlow::PathNode sink, LocalJob job, Event event +from CodeInjectionFlow::PathNode source, CodeInjectionFlow::PathNode sink, Event event where CodeInjectionFlow::flowPath(source, sink) and - job = sink.getNode().asExpr().getEnclosingJob() and - job.getATriggerEvent() = event and - // job can be triggered by an external user - event.isExternallyTriggerable() and + event = getRelevantCachePoisoningEventForSink(sink.getNode()) and // the checkout is not controlled by an access check not exists(ControlCheck check | check.protects(source.getNode().asExpr(), event, "code-injection") - ) and - // excluding privileged workflows since they can be exploited in easier circumstances - // which is covered by `actions/code-injection/critical` - not job.isPrivilegedExternallyTriggerable(event) and - ( - // the workflow runs in the context of the default branch - runsOnDefaultBranch(event) - or - // the workflow caller runs in the context of the default branch - event.getName() = "workflow_call" and - exists(ExternalJob caller | - caller.getCallee() = job.getLocation().getFile().getRelativePath() and - runsOnDefaultBranch(caller.getATriggerEvent()) - ) ) select sink.getNode(), source, sink, "Unprivileged code injection in $@, which may lead to cache poisoning ($@).", sink, diff --git a/actions/ql/src/Security/CWE-829/ArtifactPoisoningCritical.ql b/actions/ql/src/Security/CWE-829/ArtifactPoisoningCritical.ql index afef7bdd82b2..24ecb4b03397 100644 --- a/actions/ql/src/Security/CWE-829/ArtifactPoisoningCritical.ql +++ b/actions/ql/src/Security/CWE-829/ArtifactPoisoningCritical.ql @@ -19,10 +19,7 @@ import codeql.actions.security.ControlChecks from ArtifactPoisoningFlow::PathNode source, ArtifactPoisoningFlow::PathNode sink, Event event where ArtifactPoisoningFlow::flowPath(source, sink) and - inPrivilegedContext(sink.getNode().asExpr(), event) and - not exists(ControlCheck check | - check.protects(sink.getNode().asExpr(), event, "artifact-poisoning") - ) + event = getRelevantEventInPrivilegedContext(sink.getNode()) select sink.getNode(), source, sink, "Potential artifact poisoning in $@, which may be controlled by an external user ($@).", sink, sink.getNode().toString(), event, event.getName() diff --git a/actions/ql/src/experimental/Security/CWE-078/CommandInjectionCritical.ql b/actions/ql/src/experimental/Security/CWE-078/CommandInjectionCritical.ql index 7d45b25b1a29..2ed98d714dab 100644 --- a/actions/ql/src/experimental/Security/CWE-078/CommandInjectionCritical.ql +++ b/actions/ql/src/experimental/Security/CWE-078/CommandInjectionCritical.ql @@ -21,10 +21,7 @@ import codeql.actions.security.ControlChecks from CommandInjectionFlow::PathNode source, CommandInjectionFlow::PathNode sink, Event event where CommandInjectionFlow::flowPath(source, sink) and - inPrivilegedContext(sink.getNode().asExpr(), event) and - not exists(ControlCheck check | - check.protects(sink.getNode().asExpr(), event, ["command-injection", "code-injection"]) - ) + event = getRelevantEventInPrivilegedContext(sink.getNode()) select sink.getNode(), source, sink, "Potential command injection in $@, which may be controlled by an external user ($@).", sink, sink.getNode().asExpr().(Expression).getRawExpression(), event, event.getName() diff --git a/actions/ql/src/experimental/Security/CWE-088/ArgumentInjectionCritical.ql b/actions/ql/src/experimental/Security/CWE-088/ArgumentInjectionCritical.ql index 6930e2f684a4..ecab01dd114d 100644 --- a/actions/ql/src/experimental/Security/CWE-088/ArgumentInjectionCritical.ql +++ b/actions/ql/src/experimental/Security/CWE-088/ArgumentInjectionCritical.ql @@ -20,10 +20,7 @@ import codeql.actions.security.ControlChecks from ArgumentInjectionFlow::PathNode source, ArgumentInjectionFlow::PathNode sink, Event event where ArgumentInjectionFlow::flowPath(source, sink) and - inPrivilegedContext(sink.getNode().asExpr(), event) and - not exists(ControlCheck check | - check.protects(sink.getNode().asExpr(), event, "argument-injection") - ) + event = getRelevantEventInPrivilegedContext(sink.getNode()) select sink.getNode(), source, sink, "Potential argument injection in $@ command, which may be controlled by an external user ($@).", sink, sink.getNode().(ArgumentInjectionSink).getCommand(), event, event.getName()