Skip to content

Commit 27d0a34

Browse files
author
Alvaro Muñoz
committed
Improve Env path/var injection queries
1 parent 39308fd commit 27d0a34

33 files changed

+1061
-420
lines changed

‎ql/lib/codeql/actions/Ast.qll

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -298,6 +298,8 @@ abstract class Job extends AstNode instanceof JobImpl {
298298
Strategy getStrategy() { result = super.getStrategy() }
299299

300300
predicate isPrivileged() { super.isPrivileged() }
301+
302+
string getARunsOnLabel() { result = super.getARunsOnLabel() }
301303
}
302304

303305
class LocalJob extends Job instanceof LocalJobImpl {
@@ -352,6 +354,8 @@ class ExternalJob extends Job, Uses instanceof ExternalJobImpl { }
352354
class Run extends Step instanceof RunImpl {
353355
string getScript() { result = super.getScript() }
354356

357+
ScalarValue getScriptScalar() { result = super.getScriptScalar() }
358+
355359
Expression getAnScriptExpr() { result = super.getAnScriptExpr() }
356360
}
357361

‎ql/lib/codeql/actions/ast/internal/Ast.qll

Lines changed: 18 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -579,10 +579,12 @@ class JobImpl extends AstNodeImpl, TJobNode {
579579
YamlMapping n;
580580
string jobId;
581581
WorkflowImpl workflow;
582+
YamlMappingLikeNode runson;
582583

583584
JobImpl() {
584585
this = TJobNode(n) and
585-
workflow.getNode().lookup("jobs").(YamlMapping).lookup(jobId) = n
586+
workflow.getNode().lookup("jobs").(YamlMapping).lookup(jobId) = n and
587+
runson = n.lookup("runs-on").(YamlMappingLikeNode)
586588
}
587589

588590
override string toString() { result = "Job: " + jobId }
@@ -660,6 +662,19 @@ class JobImpl extends AstNodeImpl, TJobNode {
660662
// The enclosing workflow is privileged
661663
this.getEnclosingWorkflow().isPrivileged()
662664
}
665+
666+
/** Gets the runs-on field of the job. */
667+
string getARunsOnLabel() {
668+
exists(string lbl, YamlNode r |
669+
(
670+
r = runson.getNode(lbl) and
671+
not lbl = ["group", "labels"]
672+
or
673+
r = runson.getNode("labels").(YamlMappingLikeNode).getNode(lbl)
674+
) and
675+
result = lbl.trim().regexpReplaceAll("^('|\")", "").regexpReplaceAll("('|\")$", "").trim()
676+
)
677+
}
663678
}
664679

665680
class LocalJobImpl extends JobImpl {
@@ -865,6 +880,8 @@ class RunImpl extends StepImpl {
865880

866881
string getScript() { result = script.getValue() }
867882

883+
ScalarValueImpl getScriptScalar() { result = TScalarValueNode(script) }
884+
868885
ExpressionImpl getAnScriptExpr() { result.getParentNode().getNode() = script }
869886

870887
override string toString() {

‎ql/lib/codeql/actions/controlflow/internal/Cfg.qll

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -260,7 +260,11 @@ private class RunTree extends StandardPreOrderTree instanceof Run {
260260
override ControlFlowTree getChildNode(int i) {
261261
result =
262262
rank[i](AstNode child, Location l |
263-
(child = super.getInScopeEnvVarExpr(_) or child = super.getAnScriptExpr()) and
263+
(
264+
child = super.getInScopeEnvVarExpr(_) or
265+
child = super.getAnScriptExpr() or
266+
child = super.getScriptScalar()
267+
) and
264268
l = child.getLocation()
265269
|
266270
child
@@ -291,3 +295,5 @@ private class InputTree extends LeafTree instanceof Input { }
291295
private class ScalarValueLeaf extends LeafTree instanceof ScalarValue { }
292296

293297
private class ExpressionLeaf extends LeafTree instanceof Expression { }
298+
299+
predicate test(ScalarValueLeaf f) { any() }

‎ql/lib/codeql/actions/dataflow/FlowSteps.qll

Lines changed: 19 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -33,9 +33,13 @@ class AdditionalTaintStep extends Unit {
3333
predicate envToRunStep(DataFlow::Node pred, DataFlow::Node succ) {
3434
exists(Run run, string varName, string value |
3535
run.getInScopeEnvVarExpr(varName) = pred.asExpr() and
36-
Utils::writeToGitHubEnv(run, _, value) and
37-
value.indexOf("$" + ["", "{", "ENV{"] + varName) > 0 and
38-
succ.asExpr() = run
36+
(
37+
Utils::writeToGitHubEnv(run, _, value) or
38+
Utils::writeToGitHubOutput(run, _, value) or
39+
Utils::writeToGitHubPath(run, value)
40+
) and
41+
value.matches("%$" + ["", "{", "ENV{"] + varName + "%") and
42+
succ.asExpr() = run.getScriptScalar()
3943
)
4044
}
4145

@@ -85,12 +89,9 @@ predicate artifactToOutputStoreStep(DataFlow::Node pred, DataFlow::Node succ, Da
8589
exists(Run run, string key, string value, UntrustedArtifactDownloadStep download |
8690
c = any(DataFlow::FieldContent ct | ct.getName() = key) and
8791
download.getAFollowingStep() = run and
88-
pred.asExpr() = run and
92+
pred.asExpr() = run.getScriptScalar() and
8993
succ.asExpr() = run and
90-
(
91-
Utils::writeToGitHubOutput(run, key, value) or
92-
Utils::writeToGitHubEnv(run, key, value)
93-
) and
94+
Utils::writeToGitHubOutput(run, key, value) and
9495
value.regexpMatch(["\\$\\(", "`"] + ["cat\\s+", "<"] + ".*" + ["`", "\\)"])
9596
)
9697
}
@@ -99,7 +100,7 @@ predicate artifactToEnvStoreStep(DataFlow::Node pred, DataFlow::Node succ, DataF
99100
exists(Run run, string key, string value, UntrustedArtifactDownloadStep download |
100101
c = any(DataFlow::FieldContent ct | ct.getName() = key) and
101102
download.getAFollowingStep() = run and
102-
pred.asExpr() = run and
103+
pred.asExpr() = run.getScriptScalar() and
103104
// we store the taint on the enclosing job since the may not exist an implicit env attribute
104105
succ.asExpr() = run.getEnclosingJob() and
105106
Utils::writeToGitHubEnv(run, key, value) and
@@ -110,12 +111,16 @@ predicate artifactToEnvStoreStep(DataFlow::Node pred, DataFlow::Node succ, DataF
110111
/**
111112
* A download artifact step followed by a step that may use downloaded artifacts.
112113
*/
114+
predicate artifactDownloadToUseStep(DataFlow::Node pred, DataFlow::Node succ) {
115+
exists(UntrustedArtifactDownloadStep download, Run run |
116+
pred.asExpr() = download and
117+
succ.asExpr() = run.getScriptScalar() and
118+
download.getAFollowingStep() = run
119+
)
120+
}
121+
113122
class ArtifactDownloadToUseTaintStep extends AdditionalTaintStep {
114123
override predicate step(DataFlow::Node node1, DataFlow::Node node2) {
115-
exists(UntrustedArtifactDownloadStep download, Run run |
116-
node1.asExpr() = download and
117-
node2.asExpr() = run and
118-
download.getAFollowingStep() = run
119-
)
124+
artifactDownloadToUseStep(node1, node2)
120125
}
121126
}

‎ql/lib/codeql/actions/dataflow/internal/DataFlowPrivate.qll

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -61,7 +61,8 @@ class DataFlowExpr extends Cfg::Node {
6161
this.getAstNode() instanceof Uses or
6262
this.getAstNode() instanceof Run or
6363
this.getAstNode() instanceof Outputs or
64-
this.getAstNode() instanceof Input
64+
this.getAstNode() instanceof Input or
65+
this.getAstNode() instanceof ScalarValue
6566
}
6667
}
6768

‎ql/lib/codeql/actions/security/ArtifactPoisoningQuery.qll

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -299,7 +299,12 @@ class EnvVarInjectionRunStep extends PoisonableStep, Run {
299299
}
300300

301301
class ArtifactPoisoningSink extends DataFlow::Node {
302-
ArtifactPoisoningSink() { this.asExpr() instanceof PoisonableStep }
302+
ArtifactPoisoningSink() {
303+
exists(PoisonableStep step |
304+
step.(Run).getScriptScalar() = this.asExpr() or
305+
step.(UsesStep) = this.asExpr()
306+
)
307+
}
303308
}
304309

305310
/**

‎ql/lib/codeql/actions/security/EnvPathInjectionQuery.qll

Lines changed: 25 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -5,23 +5,18 @@ import codeql.actions.dataflow.FlowSources
55
private import codeql.actions.security.ArtifactPoisoningQuery
66
import codeql.actions.DataFlow
77

8-
predicate envPathInjectionFromExprSink(DataFlow::Node sink) {
9-
exists(Expression expr, Run run, string value |
10-
Utils::writeToGitHubPath(run, value) and
11-
expr = sink.asExpr() and
12-
run.getAnScriptExpr() = expr and
13-
value.indexOf(expr.getExpression()) > 0
14-
)
15-
}
8+
abstract class EnvPathInjectionSink extends DataFlow::Node { }
169

17-
predicate envPathInjectionFromFileSink(DataFlow::Node sink) {
18-
exists(Run run, UntrustedArtifactDownloadStep step, string value |
19-
sink.asExpr() = run and
20-
step.getAFollowingStep() = run and
21-
Utils::writeToGitHubPath(run, value) and
22-
// TODO: add support for other commands like `<`, `jq`, ...
23-
value.regexpMatch(["\\$\\(", "`"] + ["cat\\s+", "<"] + ".*" + ["`", "\\)"])
24-
)
10+
class EnvPathInjectionFromFileReadSink extends EnvPathInjectionSink {
11+
EnvPathInjectionFromFileReadSink() {
12+
exists(Run run, UntrustedArtifactDownloadStep step, string value |
13+
this.asExpr() = run.getScriptScalar() and
14+
step.getAFollowingStep() = run and
15+
Utils::writeToGitHubPath(run, value) and
16+
// TODO: add support for other commands like `<`, `jq`, ...
17+
value.regexpMatch(["\\$\\(", "`"] + ["cat\\s+", "<"] + ".*" + ["`", "\\)"])
18+
)
19+
}
2520
}
2621

2722
/**
@@ -32,26 +27,23 @@ predicate envPathInjectionFromFileSink(DataFlow::Node sink) {
3227
* run: |
3328
* echo "$BODY" >> $GITHUB_PATH
3429
*/
35-
predicate envPathInjectionFromEnvSink(DataFlow::Node sink) {
36-
exists(Run run, Expression expr, string varname, string value |
37-
sink.asExpr().getInScopeEnvVarExpr(varname) = expr and
38-
run = sink.asExpr() and
39-
Utils::writeToGitHubPath(run, value) and
40-
(
41-
value = ["$" + varname, "${" + varname + "}", "$ENV{" + varname + "}"]
42-
or
43-
value.matches("$(echo %") and value.indexOf(varname) > 0
30+
class EnvPathInjectionFromEnvVarSink extends EnvPathInjectionSink {
31+
EnvPathInjectionFromEnvVarSink() {
32+
exists(Run run, Expression expr, string varname, string value |
33+
this.asExpr().getInScopeEnvVarExpr(varname) = expr and
34+
run.getScriptScalar() = this.asExpr() and
35+
Utils::writeToGitHubPath(run, value) and
36+
(
37+
value.matches("%$" + ["", "{", "ENV{"] + varname + "%")
38+
or
39+
value.matches("$(echo %") and value.indexOf(varname) > 0
40+
)
4441
)
45-
)
42+
}
4643
}
4744

48-
private class EnvPathInjectionSink extends DataFlow::Node {
49-
EnvPathInjectionSink() {
50-
envPathInjectionFromExprSink(this) or
51-
envPathInjectionFromFileSink(this) or
52-
envPathInjectionFromEnvSink(this) or
53-
externallyDefinedSink(this, "envpath-injection")
54-
}
45+
class EnvPathInjectionFromMaDSink extends EnvPathInjectionSink {
46+
EnvPathInjectionFromMaDSink() { externallyDefinedSink(this, "envpath-injection") }
5547
}
5648

5749
/**

‎ql/lib/codeql/actions/security/EnvVarInjectionQuery.qll

Lines changed: 31 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -5,33 +5,43 @@ import codeql.actions.dataflow.FlowSources
55
private import codeql.actions.security.ArtifactPoisoningQuery
66
import codeql.actions.DataFlow
77

8-
predicate envVarInjectionFromExprSink(DataFlow::Node sink) {
9-
exists(Expression expr, Run run, string key, string value |
10-
Utils::writeToGitHubEnv(run, key, value) and
11-
expr = sink.asExpr() and
12-
run.getAnScriptExpr() = expr and
13-
value.indexOf(expr.getExpression()) > 0
14-
)
15-
}
8+
abstract class EnvVarInjectionSink extends DataFlow::Node { }
169

17-
predicate envVarInjectionFromFileSink(DataFlow::Node sink) {
18-
exists(Run run, UntrustedArtifactDownloadStep step, string value |
19-
sink.asExpr() = run and
20-
step.getAFollowingStep() = run and
21-
Utils::writeToGitHubEnv(run, _, value) and
22-
// TODO: add support for other commands like `<`, `jq`, ...
23-
value.regexpMatch(["\\$\\(", "`"] + ["cat\\s+", "<"] + ".*" + ["`", "\\)"])
24-
)
10+
// predicate envVarInjectionFromEnvVarSink(DataFlow::Node sink) {
11+
// exists(Expression expr, Run run, string varName, string key, string value |
12+
// expr = run.getInScopeEnvVarExpr(varName) and
13+
// Utils::writeToGitHubEnv(run, key, value) and
14+
// expr = sink.asExpr() and
15+
// value.matches("%$" + ["", "{", "ENV{"] + varName + "%")
16+
// )
17+
// }
18+
class EnvVarInjectionFromEnvVarSink extends EnvVarInjectionSink {
19+
EnvVarInjectionFromEnvVarSink() {
20+
exists(Run run, Expression expr, string varname, string key, string value |
21+
expr = run.getInScopeEnvVarExpr(varname) and
22+
Utils::writeToGitHubEnv(run, key, value) and
23+
run.getScriptScalar() = this.asExpr() and
24+
value.matches("%$" + ["", "{", "ENV{"] + varname + "%")
25+
)
26+
}
2527
}
2628

27-
private class EnvVarInjectionSink extends DataFlow::Node {
28-
EnvVarInjectionSink() {
29-
envVarInjectionFromExprSink(this) or
30-
envVarInjectionFromFileSink(this) or
31-
externallyDefinedSink(this, "envvar-injection")
29+
class EnvVarInjectionFromFileReadSink extends EnvVarInjectionSink {
30+
EnvVarInjectionFromFileReadSink() {
31+
exists(Run run, UntrustedArtifactDownloadStep step, string value |
32+
this.asExpr() = run.getScriptScalar() and
33+
step.getAFollowingStep() = run and
34+
Utils::writeToGitHubEnv(run, _, value) and
35+
// TODO: add support for other commands like `<`, `jq`, ...
36+
value.regexpMatch(["\\$\\(", "`"] + ["cat\\s+", "<"] + ".*" + ["`", "\\)"])
37+
)
3238
}
3339
}
3440

41+
class EnvVarInjectionFromMaDSink extends EnvVarInjectionSink {
42+
EnvVarInjectionFromMaDSink() { externallyDefinedSink(this, "envvar-injection") }
43+
}
44+
3545
/**
3646
* A taint-tracking configuration for unsafe user input
3747
* that is used to construct and evaluate an environment variable.

‎ql/lib/qlpack.yml

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@
22
library: true
33
warnOnImplicitThis: true
44
name: githubsecuritylab/actions-all
5-
version: 0.0.17
5+
version: 0.0.18
66
dependencies:
77
codeql/util: ^0.2.0
88
codeql/yaml: ^0.1.2

‎ql/src/Security/CWE-284/CodeExecutionOnSelfHostedRunner.ql

Lines changed: 47 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,47 @@
1+
/**
2+
* @name Pull Request code execution on self-hosted runner
3+
* @description Running untrusted code on a public repository's self-hosted runner can lead to the compromise of the runner machine
4+
* @kind problem
5+
* @problem.severity error
6+
* @security-severity 9.0
7+
* @precision high
8+
* @id actions/pr-on-self-hosted-runner
9+
* @tags actions
10+
* security
11+
* external/cwe/cwe-284
12+
*/
13+
14+
import actions
15+
import codeql.actions.dataflow.ExternalFlow
16+
17+
/**
18+
* This predicate uses data available in the workflow file to identify self-hosted runners.
19+
* It does not know if the repository is public or private.
20+
* It is a best-effort approach to identify self-hosted runners.
21+
*/
22+
predicate staticallyIdentifiedSelfHostedRunner(Job job) {
23+
exists(string label |
24+
job.getEnclosingWorkflow().getATriggerEvent() =
25+
["pull_request", "pull_request_review", "pull_request_review_comment", "pull_request_target"] and
26+
label = job.getARunsOnLabel() and
27+
// source: https://github.com/boostsecurityio/poutine/blob/main/opa/rego/poutine/utils.rego#L49C3-L49C136
28+
not label
29+
.regexpMatch("(?i)^((ubuntu-(([0-9]{2})\\.04|latest)|macos-([0-9]{2}|latest)(-x?large)?|windows-(20[0-9]{2}|latest)|(buildjet|warp)-[a-z0-9-]+))$")
30+
)
31+
}
32+
33+
/**
34+
* This predicate uses data available in the job log files to identify self-hosted runners.
35+
* It is a best-effort approach to identify self-hosted runners.
36+
*/
37+
predicate dynamicallyIdentifiedSelfHostedRunner(Job job) {
38+
exists(string runner_info |
39+
workflowDataModel(job.getEnclosingWorkflow().getLocation().getFile().getRelativePath(),
40+
"public", job.getId(), _, _, runner_info) and
41+
runner_info.matches("self-hosted:true")
42+
)
43+
}
44+
45+
from Job job
46+
where staticallyIdentifiedSelfHostedRunner(job) or dynamicallyIdentifiedSelfHostedRunner(job)
47+
select job, "Job runs on self-hosted runner"

0 commit comments

Comments
 (0)