Skip to content

Actions: Diff-informed queries: phase 3 (non-trivial locations) #20072

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -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

Expand Down Expand Up @@ -88,6 +89,19 @@ 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
exists(Event event | result = event.getLocation() |
inPrivilegedContext(sink.asExpr(), event) and
not exists(ControlCheck check | check.protects(sink.asExpr(), event, "argument-injection"))
)
}
}

/** Tracks flow of unsafe user input that is used to construct and evaluate a code script. */
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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+.*" }

Expand Down Expand Up @@ -318,6 +319,19 @@ 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
exists(Event event | result = event.getLocation() |
inPrivilegedContext(sink.asExpr(), event) and
not exists(ControlCheck check | check.protects(sink.asExpr(), event, "artifact-poisoning"))
)
}
}

/** Tracks flow of unsafe artifacts that is used in an insecure way. */
Expand Down
49 changes: 49 additions & 0 deletions actions/ql/lib/codeql/actions/security/CodeInjectionQuery.qll
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
Expand Down Expand Up @@ -35,6 +37,53 @@ 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
// where clause from CodeInjectionCritical.ql
exists(Event event, RemoteFlowSource source | result = event.getLocation() |
inPrivilegedContext(sink.asExpr(), event) and
isSource(source) and
source.getEventName() = event.getName() and
not exists(ControlCheck check | check.protects(sink.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.asExpr() and
exists(getAToJsonReferenceExpression(sink.asExpr().(Expression).getExpression(), _))
)
)
or
// where clause from CachePoisoningViaCodeInjection.ql
exists(Event event, LocalJob job, DataFlow::Node source | result = event.getLocation() |
job = sink.asExpr().getEnclosingJob() and
job.getATriggerEvent() = event and
// job can be triggered by an external user
event.isExternallyTriggerable() and
// the checkout is not controlled by an access check
isSource(source) and
not exists(ControlCheck check | check.protects(source.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())
)
)
)
}
}

/** Tracks flow of unsafe user input that is used to construct and evaluate a code script. */
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ 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") }
Expand All @@ -16,6 +17,22 @@ 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
// where clause from CommandInjectionCritical.ql
exists(Event event | result = event.getLocation() |
inPrivilegedContext(sink.asExpr(), event) and
not exists(ControlCheck check |
check.protects(sink.asExpr(), event, ["command-injection", "code-injection"])
)
)
}
}

/** Tracks flow of unsafe user input that is used to construct and evaluate a system command. */
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,30 @@ 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
// where clause from EnvPathInjectionCritical.ql
exists(Event event, RemoteFlowSource source | result = event.getLocation() |
inPrivilegedContext(sink.asExpr(), event) and
isSource(source) and
(
not source.getSourceType() = "artifact" and
not exists(ControlCheck check | check.protects(sink.asExpr(), event, "code-injection"))
or
source.getSourceType() = "artifact" and
not exists(ControlCheck check |
check.protects(sink.asExpr(), event, ["untrusted-checkout", "artifact-poisoning"])
) and
sink instanceof EnvPathInjectionFromFileReadSink
)
)
}
}

/** Tracks flow of unsafe user input that is used to construct and evaluate the PATH environment variable. */
Expand Down
34 changes: 34 additions & 0 deletions actions/ql/lib/codeql/actions/security/EnvVarInjectionQuery.qll
Original file line number Diff line number Diff line change
Expand Up @@ -163,6 +163,40 @@ 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
// where clause from EnvVarInjectionCritical.ql
exists(Event event, RemoteFlowSource source | result = event.getLocation() |
inPrivilegedContext(sink.asExpr(), event) and
isSource(source) and
// exclude paths to file read sinks from non-artifact sources
(
// source is text
not source.getSourceType() = "artifact" and
not exists(ControlCheck check |
check.protects(sink.asExpr(), event, ["envvar-injection", "code-injection"])
)
or
// source is an artifact or a file from an untrusted checkout
source.getSourceType() = "artifact" and
not exists(ControlCheck check |
check
.protects(sink.asExpr(), event,
["envvar-injection", "untrusted-checkout", "artifact-poisoning"])
) and
(
sink instanceof EnvVarInjectionFromFileReadSink or
madSink(sink, "envvar-injection")
)
)
)
}
}

/** Tracks flow of unsafe user input that is used to construct and evaluate an environment variable. */
Expand Down