Skip to content

Commit c31e9dd

Browse files
author
Alvaro Muñoz
committed
Add EnvPathInjection query
1 parent ab7196a commit c31e9dd

File tree

8 files changed

+195
-1
lines changed

8 files changed

+195
-1
lines changed

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

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -89,6 +89,24 @@ module Utils {
8989
)
9090
}
9191

92+
bindingset[line]
93+
predicate extractPathAssignment(string line, string value) {
94+
exists(string path |
95+
// single path assignment
96+
path =
97+
line.regexpCapture("(echo|Write-Output)\\s+(.*)>>\\s*(\"|')?\\$(\\{)?GITHUB_PATH(\\})?(\"|')?",
98+
2) and
99+
value = trimQuotes(path)
100+
or
101+
// workflow command assignment
102+
path =
103+
line.regexpCapture("(echo|Write-Output)\\s+(\"|')?::add-path::(.*)(\"|')?", 3)
104+
.regexpReplaceAll("^\"", "")
105+
.regexpReplaceAll("\"$", "") and
106+
value = trimQuotes(path)
107+
)
108+
}
109+
92110
predicate writeToGitHubEnv(Run run, string key, string value) {
93111
extractLineAssignment(run.getScript().splitAt("\n"), "ENV", key, value) or
94112
extractMultilineAssignment(run.getScript(), "ENV", key, value)
@@ -98,6 +116,10 @@ module Utils {
98116
extractLineAssignment(run.getScript().splitAt("\n"), "OUTPUT", key, value) or
99117
extractMultilineAssignment(run.getScript(), "OUTPUT", key, value)
100118
}
119+
120+
predicate writeToGitHubPath(Run run, string value) {
121+
extractPathAssignment(run.getScript().splitAt("\n"), value)
122+
}
101123
}
102124

103125
class AstNode instanceof AstNodeImpl {

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

Lines changed: 68 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,68 @@
1+
private import actions
2+
private import codeql.actions.TaintTracking
3+
private import codeql.actions.dataflow.ExternalFlow
4+
import codeql.actions.dataflow.FlowSources
5+
private import codeql.actions.security.ArtifactPoisoningQuery
6+
import codeql.actions.DataFlow
7+
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+
}
16+
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+
)
25+
}
26+
27+
/**
28+
* Holds if a Run step declares an environment variable, uses it to declare a PATH env var.
29+
* e.g.
30+
* env:
31+
* BODY: ${{ github.event.comment.body }}
32+
* run: |
33+
* echo "$BODY" >> $GITHUB_PATH
34+
*/
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
44+
)
45+
)
46+
}
47+
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+
}
55+
}
56+
57+
/**
58+
* A taint-tracking configuration for unsafe user input
59+
* that is used to construct and evaluate an environment variable.
60+
*/
61+
private module EnvPathInjectionConfig implements DataFlow::ConfigSig {
62+
predicate isSource(DataFlow::Node source) { source instanceof RemoteFlowSource }
63+
64+
predicate isSink(DataFlow::Node sink) { sink instanceof EnvPathInjectionSink }
65+
}
66+
67+
/** Tracks flow of unsafe user input that is used to construct and evaluate the PATH environment variable. */
68+
module EnvPathInjectionFlow = TaintTracking::Global<EnvPathInjectionConfig>;

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,7 @@ predicate envVarInjectionFromExprSink(DataFlow::Node sink) {
1010
Utils::writeToGitHubEnv(run, key, value) and
1111
expr = sink.asExpr() and
1212
run.getAnScriptExpr() = expr and
13-
value.indexOf(expr.getRawExpression()) > 0
13+
value.indexOf(expr.getExpression()) > 0
1414
)
1515
}
1616

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

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,32 @@
1+
/**
2+
* @name PATH Enviroment Variable built from user-controlled sources
3+
* @description Building the PATH 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/envpath-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.EnvPathInjectionQuery
17+
import EnvPathInjectionFlow::PathGraph
18+
19+
from EnvPathInjectionFlow::PathNode source, EnvPathInjectionFlow::PathNode sink
20+
where
21+
EnvPathInjectionFlow::flowPath(source, sink) and
22+
(
23+
exists(source.getNode().asExpr().getEnclosingCompositeAction())
24+
or
25+
exists(Workflow w |
26+
w = source.getNode().asExpr().getEnclosingWorkflow() and
27+
not w.isPrivileged()
28+
)
29+
)
30+
select sink.getNode(), source, sink,
31+
"Potential PATH environment variable injection in $@, which may be controlled by an external user.",
32+
sink, sink.getNode().toString()

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

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,28 @@
1+
/**
2+
* @name PATH Enviroment Variable built from user-controlled sources
3+
* @description Building the PATH 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-envpath-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.EnvPathInjectionQuery
17+
import EnvPathInjectionFlow::PathGraph
18+
19+
from EnvPathInjectionFlow::PathNode source, EnvPathInjectionFlow::PathNode sink
20+
where
21+
EnvPathInjectionFlow::flowPath(source, sink) and
22+
exists(Workflow w |
23+
w = source.getNode().asExpr().getEnclosingWorkflow() and
24+
w.isPrivileged()
25+
)
26+
select sink.getNode(), source, sink,
27+
"Potential privileged PATH environment variable injection in $@, which may be controlled by an external user.",
28+
sink, sink.getNode().toString()

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

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,33 @@
1+
name: Pull Request Open
2+
3+
on:
4+
pull_request_target:
5+
6+
jobs:
7+
test:
8+
runs-on: ubuntu-latest
9+
steps:
10+
11+
- run: echo "${{ github.event.pull_request.title }}" >> $GITHUB_PATH
12+
- env:
13+
PATHINJ: ${{ github.event.pull_request.title }}
14+
run: echo $(echo "$PATHINJ") >> $GITHUB_PATH
15+
- env:
16+
PATHINJ: ${{ github.event.pull_request.title }}
17+
run: echo $PATHINJ >> $GITHUB_PATH
18+
- env:
19+
PATHINJ: ${{ github.event.pull_request.title }}
20+
run: echo ${PATHINJ} >> $GITHUB_PATH
21+
- uses: dawidd6/action-download-artifact@v2
22+
with:
23+
name: artifact_name
24+
path: foo
25+
- run: echo "$(cat foo/bar)" >> $GITHUB_PATH
26+
- env:
27+
ACTIONS_ALLOW_UNSECURE_COMMANDS: true
28+
PATHINJ: ${{ github.event.pull_request.title }}
29+
run: echo "::add-path::$PATHINJ"
30+
31+
32+
33+

‎ql/test/query-tests/Security/CWE-077/EnvPathInjection.actual

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,10 @@
1+
edges
2+
| .github/workflows/path1.yml:21:9:25:6 | Uses Step | .github/workflows/path1.yml:25:9:26:6 | Run Step |
3+
| .github/workflows/path1.yml:21:9:25:6 | Uses Step | .github/workflows/path1.yml:26:9:29:41 | Run Step |
4+
nodes
5+
| .github/workflows/path1.yml:11:21:11:58 | github.event.pull_request.title | semmle.label | github.event.pull_request.title |
6+
| .github/workflows/path1.yml:21:9:25:6 | Uses Step | semmle.label | Uses Step |
7+
| .github/workflows/path1.yml:25:9:26:6 | Run Step | semmle.label | Run Step |
8+
| .github/workflows/path1.yml:26:9:29:41 | Run Step | semmle.label | Run Step |
9+
subpaths
10+
#select

‎ql/test/query-tests/Security/CWE-077/EnvPathInjection.qlref

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
Security/CWE-077/EnvPathInjection.ql

0 commit comments

Comments
 (0)