Skip to content

Commit 8560772

Browse files
author
Alvaro Muñoz
authored
Merge pull request #72 from github/query/output_clobbering
feat(queries): Improve Output Clobbering query
2 parents c442f1b + 4732513 commit 8560772

6 files changed

Lines changed: 147 additions & 8 deletions

File tree

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

Lines changed: 6 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,7 @@ string wrapJsonRegexp(string regex) {
2020
}
2121

2222
bindingset[str]
23-
private string trimQuotes(string str) {
23+
string trimQuotes(string str) {
2424
result = str.trim().regexpReplaceAll("^(\"|')", "").regexpReplaceAll("(\"|')$", "")
2525
}
2626

@@ -279,19 +279,18 @@ predicate inNonPrivilegedContext(AstNode node) {
279279
inNonPrivilegedJob(node)
280280
}
281281

282+
string partialFileContentRegexp() {
283+
result = ["cat\\s+", "jq\\s+", "yq\\s+", "tail\\s+", "head\\s+", "ls\\s+"]
284+
}
285+
282286
bindingset[snippet]
283287
predicate outputsPartialFileContent(string snippet) {
284288
// e.g.
285289
// echo FOO=`yq '.foo' foo.yml` >> $GITHUB_ENV
286290
// echo "FOO=$(<foo.txt)" >> $GITHUB_ENV
287291
// yq '.foo' foo.yml >> $GITHUB_PATH
288292
// cat foo.txt >> $GITHUB_PATH
289-
snippet
290-
.regexpMatch([
291-
"(\\$\\(|`)<.*",
292-
".*(\\b|^|\\s+)" + ["cat\\s+", "jq\\s+", "yq\\s+", "tail\\s+", "head\\s+", "ls\\s+"] +
293-
".*"
294-
])
293+
snippet.regexpMatch(["(\\$\\(|`)<.*", ".*(\\b|^|\\s+)" + partialFileContentRegexp() + ".*"])
295294
}
296295

297296
string defaultBranchNames() {

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

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@ private import codeql.actions.DataFlow
88
private import codeql.actions.dataflow.FlowSources
99
private import codeql.actions.dataflow.ExternalFlow
1010
private import codeql.actions.security.ArtifactPoisoningQuery
11+
private import codeql.actions.security.OutputClobberingQuery
1112
private import codeql.actions.security.UntrustedCheckoutQuery
1213

1314
/**
@@ -114,7 +115,8 @@ predicate envToRunStep(DataFlow::Node pred, DataFlow::Node succ) {
114115
succ.asExpr() = run.getScriptScalar() and
115116
(
116117
envToSpecialFile(["GITHUB_ENV", "GITHUB_OUTPUT", "GITHUB_PATH"], var_name, run, _) or
117-
envToArgInjSink(var_name, run, _)
118+
envToArgInjSink(var_name, run, _) or
119+
exists(OutputClobberingSink n | n.asExpr() = run.getScriptScalar())
118120
)
119121
)
120122
}

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

Lines changed: 63 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -92,6 +92,69 @@ class OutputClobberingFromEnvVarSink extends OutputClobberingSink {
9292
}
9393
}
9494

95+
/**
96+
* - id: clob1
97+
* env:
98+
* BODY: ${{ github.event.comment.body }}
99+
* run: |
100+
* # VULNERABLE
101+
* echo $BODY
102+
* echo "::set-output name=OUTPUT::SAFE"
103+
* - id: clob2
104+
* env:
105+
* BODY: ${{ github.event.comment.body }}
106+
* run: |
107+
* # VULNERABLE
108+
* echo "::set-output name=OUTPUT::SAFE"
109+
* echo $BODY
110+
*/
111+
class WorkflowCommandClobberingFromEnvVarSink extends OutputClobberingSink {
112+
WorkflowCommandClobberingFromEnvVarSink() {
113+
exists(Run run, string output_line, string clobbering_line, string var_name |
114+
run.getScript().splitAt("\n") = output_line and
115+
singleLineWorkflowCmd(output_line, "set-output", _, _) and
116+
run.getScript().splitAt("\n") = clobbering_line and
117+
clobbering_line.regexpMatch(".*echo\\s+(-e\\s+)?(\"|')?\\$(\\{)?" + var_name + ".*") and
118+
exists(run.getInScopeEnvVarExpr(var_name)) and
119+
run.getScriptScalar() = this.asExpr()
120+
)
121+
}
122+
}
123+
124+
class WorkflowCommandClobberingFromFileReadSink extends OutputClobberingSink {
125+
WorkflowCommandClobberingFromFileReadSink() {
126+
exists(Run run, string output_line, string clobbering_line |
127+
run.getScriptScalar() = this.asExpr() and
128+
run.getScript().splitAt("\n") = output_line and
129+
singleLineWorkflowCmd(output_line, "set-output", _, _) and
130+
run.getScript().splitAt("\n") = clobbering_line and
131+
(
132+
// A file is read and its content is assigned to an env var that gets printed to stdout
133+
// - run: |
134+
// foo=$(<pr-id.txt)"
135+
// echo "${foo}"
136+
exists(string var_name, string value, string assign_line, string assignment_regexp |
137+
run.getScript().splitAt("\n") = assign_line and
138+
assignment_regexp = "([a-zA-Z0-9\\-_]+)=(.*)" and
139+
var_name = assign_line.regexpCapture(assignment_regexp, 1) and
140+
value = assign_line.regexpCapture(assignment_regexp, 2) and
141+
outputsPartialFileContent(trimQuotes(value)) and
142+
clobbering_line.regexpMatch(".*echo\\s+(-e\\s+)?(\"|')?\\$(\\{)?" + var_name + ".*")
143+
)
144+
or
145+
// A file is read and its content is printed to stdout
146+
// - run: echo "foo=$(<pr-id.txt)"
147+
clobbering_line.regexpMatch(".*echo\\s+(-e)?\\s*(\"|')?") and
148+
clobbering_line.regexpMatch(partialFileContentRegexp() + ".*")
149+
or
150+
// A file content is printed to stdout
151+
// - run: cat pr-id.txt
152+
clobbering_line.regexpMatch(partialFileContentRegexp() + ".*")
153+
)
154+
)
155+
}
156+
}
157+
95158
class OutputClobberingFromMaDSink extends OutputClobberingSink {
96159
OutputClobberingFromMaDSink() { madSink(this, "output-clobbering") }
97160
}

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

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,7 @@ where
3030
source.getNode().(RemoteFlowSource).getSourceType() = "artifact" and
3131
(
3232
sink.getNode() instanceof OutputClobberingFromFileReadSink or
33+
sink.getNode() instanceof WorkflowCommandClobberingFromFileReadSink or
3334
madSink(sink.getNode(), "output-clobbering")
3435
)
3536
)

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

Lines changed: 56 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,56 @@
1+
on:
2+
issue_comment:
3+
jobs:
4+
test1:
5+
runs-on: ubuntu-latest
6+
steps:
7+
- id: clob1
8+
env:
9+
BODY: ${{ github.event.comment.body }}
10+
run: |
11+
# VULNERABLE
12+
echo $BODY
13+
echo "::set-output name=OUTPUT::SAFE"
14+
- id: clob2
15+
env:
16+
BODY: ${{ github.event.comment.body }}
17+
run: |
18+
# VULNERABLE
19+
echo "::set-output name=OUTPUT::SAFE"
20+
echo $BODY
21+
- id: clob3
22+
run: |
23+
echo ${{ steps.clob1.outputs.OUTPUT }}
24+
test2:
25+
runs-on: ubuntu-latest
26+
steps:
27+
- id: clob1
28+
env:
29+
BODY: ${{ github.event.comment.body }}
30+
run: |
31+
# NOT VULNERABLE
32+
echo "::set-output name=OUTPUT::SAFE"
33+
test3:
34+
runs-on: ubuntu-latest
35+
steps:
36+
- name: Download artifact
37+
uses: dawidd6/action-download-artifact@v6
38+
with:
39+
run_id: ${{ github.event.workflow_run.id }}
40+
name: pr_number
41+
- id: clob1
42+
run: |
43+
# VULNERABLE
44+
PR="$(<pr-number)"
45+
echo "$PR"
46+
echo "::set-output name=OUTPUT::SAFE"
47+
- id: clob2
48+
run: |
49+
# VULNERABLE
50+
cat pr-number
51+
echo "::set-output name=OUTPUT::SAFE"
52+
- id: clob2
53+
run: |
54+
# VULNERABLE
55+
echo "::set-output name=OUTPUT::SAFE"
56+
ls *.txt

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

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,12 +1,30 @@
11
edges
22
| .github/workflows/output1.yml:9:18:9:49 | github.event.comment.body | .github/workflows/output1.yml:10:14:13:50 | # VULNERABLE\necho "OUTPUT_1=HARDCODED" >> $GITHUB_OUTPUT\necho "OUTPUT_2=$BODY" >> $GITHUB_OUTPUT\n | provenance | |
33
| .github/workflows/output1.yml:30:9:35:6 | Uses Step | .github/workflows/output1.yml:36:14:38:58 | echo "OUTPUT_1=HARDCODED" >> $GITHUB_OUTPUT\necho "OUTPUT_2=$(<pr-number)" >> $GITHUB_OUTPUT\n | provenance | |
4+
| .github/workflows/output2.yml:9:18:9:49 | github.event.comment.body | .github/workflows/output2.yml:10:14:13:48 | # VULNERABLE\necho $BODY\necho "::set-output name=OUTPUT::SAFE"\n | provenance | |
5+
| .github/workflows/output2.yml:16:18:16:49 | github.event.comment.body | .github/workflows/output2.yml:17:14:20:21 | # VULNERABLE\necho "::set-output name=OUTPUT::SAFE"\necho $BODY\n | provenance | |
6+
| .github/workflows/output2.yml:36:9:41:6 | Uses Step | .github/workflows/output2.yml:42:14:46:48 | # VULNERABLE\nPR="$(<pr-number)"\necho "$PR"\necho "::set-output name=OUTPUT::SAFE"\n | provenance | |
7+
| .github/workflows/output2.yml:36:9:41:6 | Uses Step | .github/workflows/output2.yml:48:14:51:48 | # VULNERABLE\ncat pr-number\necho "::set-output name=OUTPUT::SAFE"\n | provenance | |
8+
| .github/workflows/output2.yml:36:9:41:6 | Uses Step | .github/workflows/output2.yml:53:14:56:19 | # VULNERABLE\necho "::set-output name=OUTPUT::SAFE"\nls *.txt\n | provenance | |
49
nodes
510
| .github/workflows/output1.yml:9:18:9:49 | github.event.comment.body | semmle.label | github.event.comment.body |
611
| .github/workflows/output1.yml:10:14:13:50 | # VULNERABLE\necho "OUTPUT_1=HARDCODED" >> $GITHUB_OUTPUT\necho "OUTPUT_2=$BODY" >> $GITHUB_OUTPUT\n | semmle.label | # VULNERABLE\necho "OUTPUT_1=HARDCODED" >> $GITHUB_OUTPUT\necho "OUTPUT_2=$BODY" >> $GITHUB_OUTPUT\n |
712
| .github/workflows/output1.yml:30:9:35:6 | Uses Step | semmle.label | Uses Step |
813
| .github/workflows/output1.yml:36:14:38:58 | echo "OUTPUT_1=HARDCODED" >> $GITHUB_OUTPUT\necho "OUTPUT_2=$(<pr-number)" >> $GITHUB_OUTPUT\n | semmle.label | echo "OUTPUT_1=HARDCODED" >> $GITHUB_OUTPUT\necho "OUTPUT_2=$(<pr-number)" >> $GITHUB_OUTPUT\n |
14+
| .github/workflows/output2.yml:9:18:9:49 | github.event.comment.body | semmle.label | github.event.comment.body |
15+
| .github/workflows/output2.yml:10:14:13:48 | # VULNERABLE\necho $BODY\necho "::set-output name=OUTPUT::SAFE"\n | semmle.label | # VULNERABLE\necho $BODY\necho "::set-output name=OUTPUT::SAFE"\n |
16+
| .github/workflows/output2.yml:16:18:16:49 | github.event.comment.body | semmle.label | github.event.comment.body |
17+
| .github/workflows/output2.yml:17:14:20:21 | # VULNERABLE\necho "::set-output name=OUTPUT::SAFE"\necho $BODY\n | semmle.label | # VULNERABLE\necho "::set-output name=OUTPUT::SAFE"\necho $BODY\n |
18+
| .github/workflows/output2.yml:36:9:41:6 | Uses Step | semmle.label | Uses Step |
19+
| .github/workflows/output2.yml:42:14:46:48 | # VULNERABLE\nPR="$(<pr-number)"\necho "$PR"\necho "::set-output name=OUTPUT::SAFE"\n | semmle.label | # VULNERABLE\nPR="$(<pr-number)"\necho "$PR"\necho "::set-output name=OUTPUT::SAFE"\n |
20+
| .github/workflows/output2.yml:48:14:51:48 | # VULNERABLE\ncat pr-number\necho "::set-output name=OUTPUT::SAFE"\n | semmle.label | # VULNERABLE\ncat pr-number\necho "::set-output name=OUTPUT::SAFE"\n |
21+
| .github/workflows/output2.yml:53:14:56:19 | # VULNERABLE\necho "::set-output name=OUTPUT::SAFE"\nls *.txt\n | semmle.label | # VULNERABLE\necho "::set-output name=OUTPUT::SAFE"\nls *.txt\n |
922
subpaths
1023
#select
1124
| .github/workflows/output1.yml:10:14:13:50 | # VULNERABLE\necho "OUTPUT_1=HARDCODED" >> $GITHUB_OUTPUT\necho "OUTPUT_2=$BODY" >> $GITHUB_OUTPUT\n | .github/workflows/output1.yml:9:18:9:49 | github.event.comment.body | .github/workflows/output1.yml:10:14:13:50 | # VULNERABLE\necho "OUTPUT_1=HARDCODED" >> $GITHUB_OUTPUT\necho "OUTPUT_2=$BODY" >> $GITHUB_OUTPUT\n | Potential clobbering of a step output in $@. | .github/workflows/output1.yml:10:14:13:50 | # VULNERABLE\necho "OUTPUT_1=HARDCODED" >> $GITHUB_OUTPUT\necho "OUTPUT_2=$BODY" >> $GITHUB_OUTPUT\n | # VULNERABLE\necho "OUTPUT_1=HARDCODED" >> $GITHUB_OUTPUT\necho "OUTPUT_2=$BODY" >> $GITHUB_OUTPUT\n |
1225
| .github/workflows/output1.yml:36:14:38:58 | echo "OUTPUT_1=HARDCODED" >> $GITHUB_OUTPUT\necho "OUTPUT_2=$(<pr-number)" >> $GITHUB_OUTPUT\n | .github/workflows/output1.yml:30:9:35:6 | Uses Step | .github/workflows/output1.yml:36:14:38:58 | echo "OUTPUT_1=HARDCODED" >> $GITHUB_OUTPUT\necho "OUTPUT_2=$(<pr-number)" >> $GITHUB_OUTPUT\n | Potential clobbering of a step output in $@. | .github/workflows/output1.yml:36:14:38:58 | echo "OUTPUT_1=HARDCODED" >> $GITHUB_OUTPUT\necho "OUTPUT_2=$(<pr-number)" >> $GITHUB_OUTPUT\n | echo "OUTPUT_1=HARDCODED" >> $GITHUB_OUTPUT\necho "OUTPUT_2=$(<pr-number)" >> $GITHUB_OUTPUT\n |
26+
| .github/workflows/output2.yml:10:14:13:48 | # VULNERABLE\necho $BODY\necho "::set-output name=OUTPUT::SAFE"\n | .github/workflows/output2.yml:9:18:9:49 | github.event.comment.body | .github/workflows/output2.yml:10:14:13:48 | # VULNERABLE\necho $BODY\necho "::set-output name=OUTPUT::SAFE"\n | Potential clobbering of a step output in $@. | .github/workflows/output2.yml:10:14:13:48 | # VULNERABLE\necho $BODY\necho "::set-output name=OUTPUT::SAFE"\n | # VULNERABLE\necho $BODY\necho "::set-output name=OUTPUT::SAFE"\n |
27+
| .github/workflows/output2.yml:17:14:20:21 | # VULNERABLE\necho "::set-output name=OUTPUT::SAFE"\necho $BODY\n | .github/workflows/output2.yml:16:18:16:49 | github.event.comment.body | .github/workflows/output2.yml:17:14:20:21 | # VULNERABLE\necho "::set-output name=OUTPUT::SAFE"\necho $BODY\n | Potential clobbering of a step output in $@. | .github/workflows/output2.yml:17:14:20:21 | # VULNERABLE\necho "::set-output name=OUTPUT::SAFE"\necho $BODY\n | # VULNERABLE\necho "::set-output name=OUTPUT::SAFE"\necho $BODY\n |
28+
| .github/workflows/output2.yml:42:14:46:48 | # VULNERABLE\nPR="$(<pr-number)"\necho "$PR"\necho "::set-output name=OUTPUT::SAFE"\n | .github/workflows/output2.yml:36:9:41:6 | Uses Step | .github/workflows/output2.yml:42:14:46:48 | # VULNERABLE\nPR="$(<pr-number)"\necho "$PR"\necho "::set-output name=OUTPUT::SAFE"\n | Potential clobbering of a step output in $@. | .github/workflows/output2.yml:42:14:46:48 | # VULNERABLE\nPR="$(<pr-number)"\necho "$PR"\necho "::set-output name=OUTPUT::SAFE"\n | # VULNERABLE\nPR="$(<pr-number)"\necho "$PR"\necho "::set-output name=OUTPUT::SAFE"\n |
29+
| .github/workflows/output2.yml:48:14:51:48 | # VULNERABLE\ncat pr-number\necho "::set-output name=OUTPUT::SAFE"\n | .github/workflows/output2.yml:36:9:41:6 | Uses Step | .github/workflows/output2.yml:48:14:51:48 | # VULNERABLE\ncat pr-number\necho "::set-output name=OUTPUT::SAFE"\n | Potential clobbering of a step output in $@. | .github/workflows/output2.yml:48:14:51:48 | # VULNERABLE\ncat pr-number\necho "::set-output name=OUTPUT::SAFE"\n | # VULNERABLE\ncat pr-number\necho "::set-output name=OUTPUT::SAFE"\n |
30+
| .github/workflows/output2.yml:53:14:56:19 | # VULNERABLE\necho "::set-output name=OUTPUT::SAFE"\nls *.txt\n | .github/workflows/output2.yml:36:9:41:6 | Uses Step | .github/workflows/output2.yml:53:14:56:19 | # VULNERABLE\necho "::set-output name=OUTPUT::SAFE"\nls *.txt\n | Potential clobbering of a step output in $@. | .github/workflows/output2.yml:53:14:56:19 | # VULNERABLE\necho "::set-output name=OUTPUT::SAFE"\nls *.txt\n | # VULNERABLE\necho "::set-output name=OUTPUT::SAFE"\nls *.txt\n |

0 commit comments

Comments
 (0)