Skip to content

Commit 9c90db3

Browse files
author
Alvaro Muñoz
authored
Merge pull request #41 from GitHubSecurityLab/env_injection
New Artifact Poisoning and EnvVar Injection queries
2 parents c7b3148 + a2bbf70 commit 9c90db3

29 files changed

+580
-21
lines changed

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

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -38,7 +38,9 @@ class AstNode instanceof AstNodeImpl {
3838
Expression getInScopeEnvVarExpr(string name) { result = super.getInScopeEnvVarExpr(name) }
3939
}
4040

41-
class ScalarValue extends AstNode instanceof ScalarValueImpl { }
41+
class ScalarValue extends AstNode instanceof ScalarValueImpl {
42+
string getValue() { result = super.getValue() }
43+
}
4244

4345
class Expression extends AstNode instanceof ExpressionImpl {
4446
string expression;
@@ -218,6 +220,8 @@ abstract class Uses extends AstNode instanceof UsesImpl {
218220

219221
string getVersion() { result = super.getVersion() }
220222

223+
string getArgument(string argName) { result = super.getArgument(argName) }
224+
221225
Expression getArgumentExpr(string argName) { result = super.getArgumentExpr(argName) }
222226
}
223227

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

Lines changed: 15 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -128,6 +128,8 @@ class ScalarValueImpl extends AstNodeImpl, TScalarValueNode {
128128
override Location getLocation() { result = value.getLocation() }
129129

130130
override YamlScalar getNode() { result = value }
131+
132+
string getValue() { result = value.getValue() }
131133
}
132134

133135
class ExpressionImpl extends AstNodeImpl, TExpressionNode {
@@ -687,7 +689,19 @@ abstract class UsesImpl extends AstNodeImpl {
687689

688690
abstract string getVersion();
689691

690-
abstract ExpressionImpl getArgumentExpr(string key);
692+
/** Gets the argument expression for the given key. */
693+
string getArgument(string key) {
694+
exists(ScalarValueImpl scalar |
695+
scalar.getNode() = this.getNode().(YamlMapping).lookup("with").(YamlMapping).lookup(key) and
696+
result = scalar.getValue()
697+
)
698+
}
699+
700+
/** Gets the argument expression for the given key (if it exists). */
701+
ExpressionImpl getArgumentExpr(string key) {
702+
result.getParentNode().getNode() =
703+
this.getNode().(YamlMapping).lookup("with").(YamlMapping).lookup(key)
704+
}
691705
}
692706

693707
/**
@@ -719,11 +733,6 @@ class UsesStepImpl extends StepImpl, UsesImpl {
719733
/** Gets the version reference used when checking out the Action, e.g. `v2` in `actions/checkout@v2`. */
720734
override string getVersion() { result = u.getValue().regexpCapture(usesParser(), 3) }
721735

722-
/** Gets the argument expression for the given key. */
723-
override ExpressionImpl getArgumentExpr(string key) {
724-
result.getParentNode().getNode() = n.lookup("with").(YamlMapping).lookup(key)
725-
}
726-
727736
override string toString() {
728737
if exists(this.getId()) then result = "Uses Step: " + this.getId() else result = "Uses Step"
729738
}
@@ -763,11 +772,6 @@ class ExternalJobImpl extends JobImpl, UsesImpl {
763772
else none()
764773
)
765774
}
766-
767-
/** Gets the argument expression for the given key. */
768-
override ExpressionImpl getArgumentExpr(string key) {
769-
result.getParentNode().getNode() = n.lookup("with").(YamlMapping).lookup(key)
770-
}
771775
}
772776

773777
class RunImpl extends StepImpl {

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

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -33,19 +33,19 @@ class AdditionalTaintStep extends Unit {
3333
* echo "foo=$(echo $BODY)" >> $GITHUB_OUTPUT
3434
* echo "foo=$(echo $BODY)" >> "$GITHUB_OUTPUT"
3535
*/
36-
predicate runEnvToScriptStoreStep(DataFlow::Node pred, DataFlow::Node succ, DataFlow::ContentSet c) {
37-
exists(Run r, string varName, string output |
36+
predicate envToOutputStoreStep(DataFlow::Node pred, DataFlow::Node succ, DataFlow::ContentSet c) {
37+
exists(Run run, string varName, string output |
3838
c = any(DataFlow::FieldContent ct | ct.getName() = output.replaceAll("output\\.", "")) and
39-
r.getInScopeEnvVarExpr(varName) = pred.asExpr() and
39+
run.getInScopeEnvVarExpr(varName) = pred.asExpr() and
4040
exists(string script, string line |
41-
script = r.getScript() and
41+
script = run.getScript() and
4242
line = script.splitAt("\n") and
4343
(
4444
output = line.regexpCapture(".*::set-output\\s+name=(.*)::.*", 1) or
4545
output = line.regexpCapture(".*echo\\s*\"(.*)=.*\\s*>>\\s*(\")?\\$GITHUB_OUTPUT.*", 1)
4646
) and
4747
line.indexOf("$" + ["", "{", "ENV{"] + varName) > 0
4848
) and
49-
succ.asExpr() = r
49+
succ.asExpr() = run
5050
)
5151
}

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

Lines changed: 18 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -232,8 +232,24 @@ predicate envCtxLocalStep(Node nodeFrom, Node nodeTo) {
232232
astFrom = nodeFrom.asExpr() and
233233
astTo = nodeTo.asExpr() and
234234
(
235-
externallyDefinedSource(nodeFrom, _, "env." + astTo.getFieldName()) or
235+
externallyDefinedSource(nodeFrom, _, "env." + astTo.getFieldName())
236+
or
236237
astTo.getTarget() = astFrom
238+
or
239+
// e.g:
240+
// - run: echo ISSUE_KEY=$(echo "${{ github.event.pull_request.title }}") >> $GITHUB_ENV
241+
// - run: echo ${{ env.ISSUE_KEY }}
242+
exists(Run run, string script, Expression expr, string line, string key, string value |
243+
run.getScript() = script and
244+
run.getAnScriptExpr() = expr and
245+
line = script.splitAt("\n") and
246+
key = line.regexpCapture("echo\\s+([^=]+)\\s*=(.*)>>\\s*\\$GITHUB_ENV", 1) and
247+
value = line.regexpCapture("echo\\s+([^=]+)\\s*=(.*)>>\\s*\\$GITHUB_ENV", 2) and
248+
value.indexOf(expr.getRawExpression()) > 0 and
249+
key = astTo.getFieldName() and
250+
expr = astFrom and
251+
expr.getEnclosingWorkflow() = run.getEnclosingWorkflow()
252+
)
237253
)
238254
)
239255
}
@@ -312,7 +328,7 @@ predicate fieldStoreStep(Node node1, Node node2, ContentSet c) {
312328
predicate storeStep(Node node1, ContentSet c, Node node2) {
313329
fieldStoreStep(node1, node2, c) or
314330
externallyDefinedStoreStep(node1, node2, c) or
315-
runEnvToScriptStoreStep(node1, node2, c)
331+
envToOutputStoreStep(node1, node2, c)
316332
}
317333

318334
/**

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

Lines changed: 50 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,50 @@
1+
import actions
2+
3+
class ArtifactDownloadStep extends Step {
4+
ArtifactDownloadStep() {
5+
// eg: - uses: dawidd6/action-download-artifact@v2
6+
this.(UsesStep).getCallee() = "dawidd6/action-download-artifact" and
7+
// exclude downloads outside the current directory
8+
// TODO: add more checks to make sure the artifacts can be controlled
9+
not exists(this.(UsesStep).getArgumentExpr("path"))
10+
or
11+
// eg:
12+
// - uses: actions/github-script@v6
13+
// with:
14+
// script: |
15+
// let allArtifacts = await github.rest.actions.listWorkflowRunArtifacts({
16+
// owner: context.repo.owner,
17+
// repo: context.repo.repo,
18+
// run_id: context.payload.workflow_run.id,
19+
// });
20+
// let matchArtifact = allArtifacts.data.artifacts.filter((artifact) => {
21+
// return artifact.name == "<ARTEFACT_NAME>"
22+
// })[0];
23+
// let download = await github.rest.actions.downloadArtifact({
24+
// owner: context.repo.owner,
25+
// repo: context.repo.repo,
26+
// artifact_id: matchArtifact.id,
27+
// archive_format: 'zip',
28+
// });
29+
this.(UsesStep).getCallee() = "actions/github-script" and
30+
exists(string script |
31+
this.(UsesStep).getArgument("script") = script and
32+
script.matches("%listWorkflowRunArtifacts(%") and
33+
script.matches("%downloadArtifact(%")
34+
)
35+
or
36+
// eg: - run: gh run download "${WORKFLOW_RUN_ID}" --repo "${GITHUB_REPOSITORY}" --name "artifact_name"
37+
this.(Run).getScript().splitAt("\n").regexpMatch(".*gh\\s+run\\s+download.*")
38+
or
39+
// eg:
40+
// run: |
41+
// artifacts_url=${{ github.event.workflow_run.artifacts_url }}
42+
// gh api "$artifacts_url" -q '.artifacts[] | [.name, .archive_download_url] | @tsv' | while read artifact
43+
// do
44+
// IFS=$'\t' read name url <<< "$artifact"
45+
// gh api $url > "$name.zip"
46+
// unzip -d "$name" "$name.zip"
47+
// done
48+
this.(Run).getScript().splitAt("\n").matches("%github.event.workflow_run.artifacts_url%")
49+
}
50+
}

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

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,37 @@
1+
private import actions
2+
private import codeql.actions.TaintTracking
3+
private import codeql.actions.dataflow.ExternalFlow
4+
import codeql.actions.dataflow.FlowSources
5+
import codeql.actions.DataFlow
6+
7+
predicate writeToGithubEnvSink(DataFlow::Node exprNode, string key, string value) {
8+
exists(Expression expr, Run run, string script, string line |
9+
script = run.getScript() and
10+
line = script.splitAt("\n") and
11+
key = line.regexpCapture("echo\\s+([^=]+)\\s*=(.*)>>\\s*\\$GITHUB_ENV", 1) and
12+
value = line.regexpCapture("echo\\s+([^=]+)\\s*=(.*)>>\\s*\\$GITHUB_ENV", 2) and
13+
expr = exprNode.asExpr() and
14+
run.getAnScriptExpr() = expr and
15+
value.indexOf(expr.getRawExpression()) > 0
16+
)
17+
}
18+
19+
private class EnvVarInjectionSink extends DataFlow::Node {
20+
EnvVarInjectionSink() {
21+
writeToGithubEnvSink(this, _, _) or
22+
externallyDefinedSink(this, "envvar-injection")
23+
}
24+
}
25+
26+
/**
27+
* A taint-tracking configuration for unsafe user input
28+
* that is used to construct and evaluate an environment variable.
29+
*/
30+
private module EnvVarInjectionConfig implements DataFlow::ConfigSig {
31+
predicate isSource(DataFlow::Node source) { source instanceof RemoteFlowSource }
32+
33+
predicate isSink(DataFlow::Node sink) { sink instanceof EnvVarInjectionSink }
34+
}
35+
36+
/** Tracks flow of unsafe user input that is used to construct and evaluate an environment variable. */
37+
module EnvVarInjectionFlow = TaintTracking::Global<EnvVarInjectionConfig>;

‎ql/src/Security/CWE-077/EnvVarInjection.ql‎

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,23 @@
1+
/**
2+
* @name Enviroment Variable built from user-controlled sources
3+
* @description Building an environment variable from user-controlled sources may alter the execution of following system commands
4+
* @kind path-problem
5+
* @problem.severity warning
6+
* @security-severity 5.0
7+
* @precision high
8+
* @id actions/envvar-injection
9+
* @tags actions
10+
* security
11+
* external/cwe/cwe-077
12+
* external/cwe/cwe-020
13+
*/
14+
15+
import actions
16+
import codeql.actions.security.EnvVarInjectionQuery
17+
import EnvVarInjectionFlow::PathGraph
18+
19+
from EnvVarInjectionFlow::PathNode source, EnvVarInjectionFlow::PathNode sink
20+
where EnvVarInjectionFlow::flowPath(source, sink)
21+
select sink.getNode(), source, sink,
22+
"Potential environment variable injection in $@, which may be controlled by an external user.",
23+
sink, sink.getNode().asExpr().(Expression).getRawExpression()

‎ql/src/Security/CWE-077/PrivilegedEnvVarInjection.ql‎

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,31 @@
1+
/**
2+
* @name Enviroment Variable built from user-controlled sources
3+
* @description Building an environment variable from user-controlled sources may alter the execution of following system commands
4+
* @kind path-problem
5+
* @problem.severity error
6+
* @security-severity 9
7+
* @precision high
8+
* @id actions/privileged-envvar-injection
9+
* @tags actions
10+
* security
11+
* external/cwe/cwe-077
12+
* external/cwe/cwe-020
13+
*/
14+
15+
import actions
16+
import codeql.actions.security.EnvVarInjectionQuery
17+
import EnvVarInjectionFlow::PathGraph
18+
19+
predicate isSingleTriggerWorkflow(Workflow w, string trigger) {
20+
w.getATriggerEvent() = trigger and
21+
count(string t | w.getATriggerEvent() = t | t) = 1
22+
}
23+
24+
from EnvVarInjectionFlow::PathNode source, EnvVarInjectionFlow::PathNode sink, Workflow w
25+
where
26+
EnvVarInjectionFlow::flowPath(source, sink) and
27+
w = source.getNode().asExpr().getEnclosingWorkflow() and
28+
not isSingleTriggerWorkflow(w, "pull_request")
29+
select sink.getNode(), source, sink,
30+
"Potential privileged environment variable injection in $@, which may be controlled by an external user.",
31+
sink, sink.getNode().asExpr().(Expression).getRawExpression()

‎ql/src/Security/CWE-829/ArtifactPoisoning.ql‎

Lines changed: 44 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,44 @@
1+
/**
2+
* @name Artifact poisoning
3+
* @description An attacker may be able to poison the workflow's artifacts and influence on consequent steps.
4+
* @kind problem
5+
* @problem.severity warning
6+
* @precision medium
7+
* @security-severity 9.3
8+
* @id actions/artifact-poisoning
9+
* @tags actions
10+
* security
11+
* external/cwe/cwe-829
12+
*/
13+
14+
import actions
15+
import codeql.actions.security.ArtifactPoisoningQuery
16+
17+
predicate isSingleTriggerWorkflow(Workflow w, string trigger) {
18+
w.getATriggerEvent() = trigger and
19+
count(string t | w.getATriggerEvent() = t | t) = 1
20+
}
21+
22+
from Workflow w, LocalJob job, ArtifactDownloadStep download, Step run
23+
where
24+
w = job.getWorkflow() and
25+
(
26+
// The Workflow is triggered by an event other than `pull_request`
27+
not isSingleTriggerWorkflow(w, "pull_request")
28+
or
29+
// The Workflow is only triggered by `workflow_call` and there is
30+
// a caller workflow triggered by an event other than `pull_request`
31+
isSingleTriggerWorkflow(w, "workflow_call") and
32+
exists(ExternalJob call, Workflow caller |
33+
call.getCallee() = w.getLocation().getFile().getRelativePath() and
34+
caller = call.getWorkflow() and
35+
not isSingleTriggerWorkflow(caller, "pull_request")
36+
)
37+
) and
38+
(run instanceof Run or run instanceof UsesStep) and
39+
exists(int i, int j |
40+
job.getStep(i) = download and
41+
job.getStep(j) = run and
42+
i < j
43+
)
44+
select download, "Potential artifact poisoning."

‎ql/src/Security/CWE-829/UntrustedCheckout.ql‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,7 @@
55
* that is able to push to the base repository and to access secrets.
66
* @kind problem
77
* @problem.severity warning
8-
* @precision low
8+
* @precision medium
99
* @security-severity 9.3
1010
* @id actions/untrusted-checkout
1111
* @tags actions

0 commit comments

Comments
 (0)