Skip to content

Commit 152d29d

Browse files
author
Alvaro Muñoz
committed
Add Artifact poisoning and Env Injection queries
1 parent bdfd461 commit 152d29d

23 files changed

+313
-26
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: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -4,20 +4,21 @@ private import codeql.actions.dataflow.ExternalFlow
44
import codeql.actions.dataflow.FlowSources
55
import codeql.actions.DataFlow
66

7-
predicate writeToGithubEnvSink(DataFlow::Node sink) {
8-
exists(Expression expr, Run run, string script, string line, string value |
7+
predicate writeToGithubEnvSink(DataFlow::Node exprNode, string key, string value) {
8+
exists(Expression expr, Run run, string script, string line |
99
script = run.getScript() and
1010
line = script.splitAt("\n") and
11-
value = line.regexpCapture("echo\\s+.*\\s*=(.*)>>\\s*\\$GITHUB_ENV", 1) and
12-
expr = sink.asExpr() 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
1314
run.getAnScriptExpr() = expr and
1415
value.indexOf(expr.getRawExpression()) > 0
1516
)
1617
}
1718

1819
private class EnvVarInjectionSink extends DataFlow::Node {
1920
EnvVarInjectionSink() {
20-
writeToGithubEnvSink(this) or
21+
writeToGithubEnvSink(this, _, _) or
2122
externallyDefinedSink(this, "envvar-injection")
2223
}
2324
}

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

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,26 @@
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+
from LocalJob job, ArtifactDownloadStep download, Step run
18+
where
19+
job.getWorkflow().getATriggerEvent() = "workflow_run" and
20+
(run instanceof Run or run instanceof UsesStep) and
21+
exists(int i, int j |
22+
job.getStep(i) = download and
23+
job.getStep(j) = run and
24+
i < j
25+
)
26+
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

‎ql/test/query-tests/Security/CWE-077/.github/workflows/test1.yml

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -21,5 +21,7 @@ jobs:
2121
- name: Extract Jira Key
2222
run: echo ISSUE_KEY=$(echo "${{ github.event.pull_request.title }}") >> $GITHUB_ENV
2323

24+
- name: Sink
25+
run: echo ${{ env.ISSUE_KEY }}
2426

2527

‎ql/test/query-tests/Security/CWE-077/EnvVarInjection.expected

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
edges
2+
nodes
3+
| .github/workflows/test1.yml:22:38:22:75 | github.event.pull_request.title | semmle.label | github.event.pull_request.title |
4+
subpaths
5+
#select
6+
| .github/workflows/test1.yml:22:38:22:75 | github.event.pull_request.title | .github/workflows/test1.yml:22:38:22:75 | github.event.pull_request.title | .github/workflows/test1.yml:22:38:22:75 | github.event.pull_request.title | Potential environment variable injection in $@, which may be controlled by an external user. | .github/workflows/test1.yml:22:38:22:75 | github.event.pull_request.title | ${{ github.event.pull_request.title }} |

0 commit comments

Comments
 (0)