Skip to content

Commit 5f6b37d

Browse files
authored
Merge pull request #22240 from JarLob/fix/actions-output-clobbering-regex
Actions: The regex wasn't escaping external input
2 parents dc0be20 + 6c38d83 commit 5f6b37d

4 files changed

Lines changed: 24 additions & 1 deletion

File tree

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

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -196,7 +196,10 @@ class WorkflowCommandClobberingFromFileReadSink extends OutputClobberingSink {
196196
clobbering_cmd.regexpMatch(["ls", Bash::fileReadCommand()] + "\\s.*") and
197197
(
198198
// - run: echo "foo=$(<pr-id.txt)"
199-
clobbering_stmt.regexpMatch("echo.*" + clobbering_cmd + ".*")
199+
exists(string echo, int echoOffset |
200+
echo = clobbering_stmt.regexpFind("\\becho\\s+", _, echoOffset) and
201+
clobbering_stmt.indexOf(clobbering_cmd, 0, echoOffset + echo.length()) >= 0
202+
)
200203
or
201204
// A file content is printed to stdout
202205
// - run: cat pr-id.txt
Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
---
2+
category: fix
3+
---
4+
* Fixed a performance issue in the `actions/output-clobbering/high` query caused by using unescaped source-code input in a regular expression.

actions/ql/test/query-tests/Security/CWE-074/.github/workflows/output2.yml

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -114,3 +114,13 @@ jobs:
114114
run: |
115115
# VULNERABLE: halt_error emits its input without JSON encoding
116116
jq '.value | halt_error(1)' pr-number.json
117+
- id: clob18
118+
run: |
119+
# VULNERABLE: the file name contains regex metacharacters
120+
echo "VALUE=$(cat 'pr[number](final).txt')"
121+
echo "::set-output name=OUTPUT::SAFE"
122+
- id: clob19
123+
run: |
124+
# VULNERABLE: echo is invoked through env
125+
env echo "VALUE=$(cat 'pr[number](final).txt')"
126+
echo "::set-output name=OUTPUT::SAFE"

actions/ql/test/query-tests/Security/CWE-074/OutputClobberingHigh.expected

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,8 @@ edges
1616
| .github/workflows/output2.yml:36:9:41:6 | Uses Step | .github/workflows/output2.yml:106:14:108:51 | # VULNERABLE: raw-output0 emits strings without JSON encoding\njq '.value' --raw-output0 pr-number.json\n | provenance | Config |
1717
| .github/workflows/output2.yml:36:9:41:6 | Uses Step | .github/workflows/output2.yml:110:14:112:46 | # VULNERABLE: stderr emits its input without JSON encoding\njq '.value \| stderr' pr-number.json\n | provenance | Config |
1818
| .github/workflows/output2.yml:36:9:41:6 | Uses Step | .github/workflows/output2.yml:114:14:116:53 | # VULNERABLE: halt_error emits its input without JSON encoding\njq '.value \| halt_error(1)' pr-number.json\n | provenance | Config |
19+
| .github/workflows/output2.yml:36:9:41:6 | Uses Step | .github/workflows/output2.yml:118:14:121:48 | # VULNERABLE: the file name contains regex metacharacters\necho "VALUE=$(cat 'pr[number](final).txt')"\necho "::set-output name=OUTPUT::SAFE"\n | provenance | Config |
20+
| .github/workflows/output2.yml:36:9:41:6 | Uses Step | .github/workflows/output2.yml:123:14:126:48 | # VULNERABLE: echo is invoked through env\nenv echo "VALUE=$(cat 'pr[number](final).txt')"\necho "::set-output name=OUTPUT::SAFE"\n | provenance | Config |
1921
nodes
2022
| .github/workflows/output1.yml:9:18:9:49 | github.event.comment.body | semmle.label | github.event.comment.body |
2123
| .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 |
@@ -39,6 +41,8 @@ nodes
3941
| .github/workflows/output2.yml:106:14:108:51 | # VULNERABLE: raw-output0 emits strings without JSON encoding\njq '.value' --raw-output0 pr-number.json\n | semmle.label | # VULNERABLE: raw-output0 emits strings without JSON encoding\njq '.value' --raw-output0 pr-number.json\n |
4042
| .github/workflows/output2.yml:110:14:112:46 | # VULNERABLE: stderr emits its input without JSON encoding\njq '.value \| stderr' pr-number.json\n | semmle.label | # VULNERABLE: stderr emits its input without JSON encoding\njq '.value \| stderr' pr-number.json\n |
4143
| .github/workflows/output2.yml:114:14:116:53 | # VULNERABLE: halt_error emits its input without JSON encoding\njq '.value \| halt_error(1)' pr-number.json\n | semmle.label | # VULNERABLE: halt_error emits its input without JSON encoding\njq '.value \| halt_error(1)' pr-number.json\n |
44+
| .github/workflows/output2.yml:118:14:121:48 | # VULNERABLE: the file name contains regex metacharacters\necho "VALUE=$(cat 'pr[number](final).txt')"\necho "::set-output name=OUTPUT::SAFE"\n | semmle.label | # VULNERABLE: the file name contains regex metacharacters\necho "VALUE=$(cat 'pr[number](final).txt')"\necho "::set-output name=OUTPUT::SAFE"\n |
45+
| .github/workflows/output2.yml:123:14:126:48 | # VULNERABLE: echo is invoked through env\nenv echo "VALUE=$(cat 'pr[number](final).txt')"\necho "::set-output name=OUTPUT::SAFE"\n | semmle.label | # VULNERABLE: echo is invoked through env\nenv echo "VALUE=$(cat 'pr[number](final).txt')"\necho "::set-output name=OUTPUT::SAFE"\n |
4246
| .github/workflows/output3.yml:10:20:10:51 | github.event.comment.body | semmle.label | github.event.comment.body |
4347
subpaths
4448
#select
@@ -59,4 +63,6 @@ subpaths
5963
| .github/workflows/output2.yml:106:14:108:51 | # VULNERABLE: raw-output0 emits strings without JSON encoding\njq '.value' --raw-output0 pr-number.json\n | .github/workflows/output2.yml:36:9:41:6 | Uses Step | .github/workflows/output2.yml:106:14:108:51 | # VULNERABLE: raw-output0 emits strings without JSON encoding\njq '.value' --raw-output0 pr-number.json\n | Attacker-controlled data printed to standard output may forge a `set-output` workflow command and overwrite step outputs in $@. | .github/workflows/output2.yml:106:14:108:51 | # VULNERABLE: raw-output0 emits strings without JSON encoding\njq '.value' --raw-output0 pr-number.json\n | this step |
6064
| .github/workflows/output2.yml:110:14:112:46 | # VULNERABLE: stderr emits its input without JSON encoding\njq '.value \| stderr' pr-number.json\n | .github/workflows/output2.yml:36:9:41:6 | Uses Step | .github/workflows/output2.yml:110:14:112:46 | # VULNERABLE: stderr emits its input without JSON encoding\njq '.value \| stderr' pr-number.json\n | Attacker-controlled data printed to standard output may forge a `set-output` workflow command and overwrite step outputs in $@. | .github/workflows/output2.yml:110:14:112:46 | # VULNERABLE: stderr emits its input without JSON encoding\njq '.value \| stderr' pr-number.json\n | this step |
6165
| .github/workflows/output2.yml:114:14:116:53 | # VULNERABLE: halt_error emits its input without JSON encoding\njq '.value \| halt_error(1)' pr-number.json\n | .github/workflows/output2.yml:36:9:41:6 | Uses Step | .github/workflows/output2.yml:114:14:116:53 | # VULNERABLE: halt_error emits its input without JSON encoding\njq '.value \| halt_error(1)' pr-number.json\n | Attacker-controlled data printed to standard output may forge a `set-output` workflow command and overwrite step outputs in $@. | .github/workflows/output2.yml:114:14:116:53 | # VULNERABLE: halt_error emits its input without JSON encoding\njq '.value \| halt_error(1)' pr-number.json\n | this step |
66+
| .github/workflows/output2.yml:118:14:121:48 | # VULNERABLE: the file name contains regex metacharacters\necho "VALUE=$(cat 'pr[number](final).txt')"\necho "::set-output name=OUTPUT::SAFE"\n | .github/workflows/output2.yml:36:9:41:6 | Uses Step | .github/workflows/output2.yml:118:14:121:48 | # VULNERABLE: the file name contains regex metacharacters\necho "VALUE=$(cat 'pr[number](final).txt')"\necho "::set-output name=OUTPUT::SAFE"\n | Attacker-controlled data printed to standard output may forge a `set-output` workflow command and overwrite step outputs in $@. | .github/workflows/output2.yml:118:14:121:48 | # VULNERABLE: the file name contains regex metacharacters\necho "VALUE=$(cat 'pr[number](final).txt')"\necho "::set-output name=OUTPUT::SAFE"\n | this step |
67+
| .github/workflows/output2.yml:123:14:126:48 | # VULNERABLE: echo is invoked through env\nenv echo "VALUE=$(cat 'pr[number](final).txt')"\necho "::set-output name=OUTPUT::SAFE"\n | .github/workflows/output2.yml:36:9:41:6 | Uses Step | .github/workflows/output2.yml:123:14:126:48 | # VULNERABLE: echo is invoked through env\nenv echo "VALUE=$(cat 'pr[number](final).txt')"\necho "::set-output name=OUTPUT::SAFE"\n | Attacker-controlled data printed to standard output may forge a `set-output` workflow command and overwrite step outputs in $@. | .github/workflows/output2.yml:123:14:126:48 | # VULNERABLE: echo is invoked through env\nenv echo "VALUE=$(cat 'pr[number](final).txt')"\necho "::set-output name=OUTPUT::SAFE"\n | this step |
6268
| .github/workflows/output3.yml:10:20:10:51 | github.event.comment.body | .github/workflows/output3.yml:10:20:10:51 | github.event.comment.body | .github/workflows/output3.yml:10:20:10:51 | github.event.comment.body | Attacker-controlled data may inject or overwrite step outputs in $@. | .github/workflows/output3.yml:10:20:10:51 | github.event.comment.body | this action |

0 commit comments

Comments
 (0)