Skip to content

Commit cd1827e

Browse files
author
Alvaro Muñoz
authored
Merge pull request #98 from github/improve_arginj
improve arginj
2 parents ef37e3c + 531f3d4 commit cd1827e

25 files changed

Lines changed: 440 additions & 129 deletions

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

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -293,6 +293,10 @@ class Run extends Step instanceof RunImpl {
293293
Expression getAnScriptExpr() { result = super.getAnScriptExpr() }
294294

295295
string getWorkingDirectory() { result = super.getWorkingDirectory() }
296+
297+
string getACommand() { result = super.getACommand() }
298+
299+
predicate getAnAssignment(string name, string value) { super.getAnAssignment(name, value) }
296300
}
297301

298302
abstract class SimpleReferenceExpression extends AstNode instanceof SimpleReferenceExpressionImpl {

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

Lines changed: 94 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -54,7 +54,6 @@ predicate isBashParameterExpansion(string expr, string parameter, string operato
5454
)
5555
}
5656

57-
// TODO, the followinr test fails
5857
bindingset[raw_content]
5958
predicate extractVariableAndValue(string raw_content, string key, string value) {
6059
exists(string regexp, string content | content = trimQuotes(raw_content) |
@@ -246,18 +245,14 @@ predicate inNonPrivilegedContext(AstNode node) {
246245
not node.getEnclosingJob().isPrivilegedExternallyTriggerable(_)
247246
}
248247

249-
string partialFileContentRegexp() {
250-
result = ["cat\\s+", "jq\\s+", "yq\\s+", "tail\\s+", "head\\s+", "ls\\s+"]
251-
}
252-
253248
bindingset[snippet]
254249
predicate outputsPartialFileContent(string snippet) {
255250
// e.g.
256251
// echo FOO=`yq '.foo' foo.yml` >> $GITHUB_ENV
257252
// echo "FOO=$(<foo.txt)" >> $GITHUB_ENV
258253
// yq '.foo' foo.yml >> $GITHUB_PATH
259254
// cat foo.txt >> $GITHUB_PATH
260-
snippet.regexpMatch(["(\\$\\(|`)<.*", ".*(\\b|^|\\s+)" + partialFileContentRegexp() + ".*"])
255+
Bash::getACommand(snippet).indexOf(["<", Bash::partialFileContentCommand() + " "]) = 0
261256
}
262257

263258
string defaultBranchNames() {
@@ -310,3 +305,96 @@ string normalizePath(string path) {
310305
*/
311306
bindingset[subpath, path]
312307
predicate isSubpath(string subpath, string path) { subpath.substring(0, path.length()) = path }
308+
309+
module Bash {
310+
string stmtSeparator() { result = ";" }
311+
312+
string commandSeparator() { result = ["&&", "||"] }
313+
314+
string pipeSeparator() { result = "|" }
315+
316+
string splitSeparators() {
317+
result = stmtSeparator() or result = commandSeparator() or result = pipeSeparator()
318+
}
319+
320+
string redirectionSeparator() { result = [">", ">>", "2>", "2>>", ">&", "2>&", "<", "<<<"] }
321+
322+
string partialFileContentCommand() { result = ["cat", "jq", "yq", "tail", "head"] }
323+
324+
bindingset[script]
325+
string getACommand(string script) {
326+
exists(string stmt_, string stmt, string subline2, string cmd |
327+
stmt_ = script.regexpReplaceAll("\\\\\\s*\n", "").splitAt("\n") and
328+
stmt =
329+
[
330+
// $() command substitution
331+
stmt_
332+
.regexpFind("\\$\\((?:[^()]+|\\((?:[^()]+|\\([^()]*\\))*\\))*\\)", _, _)
333+
.regexpReplaceAll("^\\$\\(", "")
334+
.regexpReplaceAll("\\)$", ""),
335+
// `...` command substitution
336+
stmt_
337+
.regexpFind("\\`[^\\`]+\\`", _, _)
338+
.regexpReplaceAll("^\\`", "")
339+
.regexpReplaceAll("\\`$", ""),
340+
// original line with no substitutions
341+
stmt_
342+
.regexpReplaceAll("\\`[^\\`]+\\`", "SUBCOMMAND")
343+
.regexpReplaceAll("\\$\\((?:[^()]+|\\((?:[^()]+|\\([^()]*\\))*\\))*\\)", "SUBCOMMAND")
344+
] and
345+
// We shoulg replace quoted arguments with a placeholder to avoid splitting them
346+
// eg: ls | grep -E "*.(tar.gz|zip)$"
347+
//subline2 = subline.regexpReplaceAll("\"([^\"]+)\"", "$0").regexpReplaceAll("'([^']+)'", "$0") and
348+
(
349+
stmt.regexpMatch(".*\"([^\"]+)\".*") and
350+
exists(int i |
351+
subline2 =
352+
stmt.replaceAll(stmt.regexpFind("\"([^\"]+)\"", _, i),
353+
stmt.regexpFind("\"([^\"]+)\"", _, i)
354+
.replaceAll("|", "::PIPE::")
355+
.replaceAll(";", "::SEMICOLON::")
356+
.replaceAll("&&", "::AND::")
357+
.replaceAll("||", "::OR::"))
358+
)
359+
or
360+
stmt.regexpMatch(".*'([^']+)'.*") and
361+
exists(int i |
362+
subline2 =
363+
stmt.replaceAll(stmt.regexpFind("'([^']+)'", _, i),
364+
stmt.regexpFind("'([^']+)'", _, i)
365+
.replaceAll("|", "::PIPE::")
366+
.replaceAll(";", "::SEMICOLON::")
367+
.replaceAll("&&", "::AND::")
368+
.replaceAll("||", "::OR::"))
369+
)
370+
or
371+
not stmt.regexpMatch(".*'([^']+)'.*") and
372+
not stmt.regexpMatch(".*\"([^\"]+)\".*") and
373+
subline2 = stmt
374+
) and
375+
cmd = subline2.splitAt(splitSeparators()).trim() and
376+
// when splitting the line with a separator that is not found, the result is the original line which may contain other separators
377+
// we only one the split parts that do not contain any of the separators
378+
not cmd.indexOf(splitSeparators()) > -1 and
379+
not cmd =
380+
[
381+
"", "for", "in", "do", "done", "if", "then", "else", "elif", "fi", "while", "until",
382+
"case", "esac", "{", "}"
383+
] and
384+
result =
385+
cmd.replaceAll("::PIPE::", "|")
386+
.replaceAll("::SEMICOLON::", ";")
387+
.replaceAll("::AND::", "&&")
388+
.replaceAll("::OR::", "||")
389+
)
390+
}
391+
392+
bindingset[script]
393+
predicate getAnAssignment(string script, string name, string value) {
394+
exists(string stmt |
395+
stmt = script.regexpReplaceAll("\\\\\\s*\n", "").splitAt("\n").trim() and
396+
name = stmt.regexpCapture("^([a-zA-Z0-9\\-_]+)=.*", 1) and
397+
value = stmt.regexpCapture("^[a-zA-Z0-9\\-_]+=(.*)", 1)
398+
)
399+
}
400+
}

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

Lines changed: 10 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -722,13 +722,10 @@ class EventImpl extends AstNodeImpl, TEventNode {
722722
not this.getName() = "workflow_run"
723723
or
724724
this.getName() = "workflow_run" and
725-
// workflow_run cannot be externally triggered if they triggering workflow runs in the context of the default branch
726-
// since an attacker can change the triggering workflow from any event to `pull_request` to trigger the workflow
727-
// but in that case, the triggering workflow will run in the context of the PR head branch
728-
(
729-
not exists(this.getAPropertyValue("branches")) or
730-
this.getAPropertyValue("branches").matches("%*%")
731-
)
725+
// workflow_run cannot be externally triggered if the triggering workflow runs in the context of the default branch
726+
// An attacker can change the triggering workflow from any event to `pull_request` to trigger the workflow
727+
// in that case, the triggering workflow will run in the context of the PR head branch
728+
not exists(this.getAPropertyValue("branches"))
732729
or
733730
// the event is `workflow_call` and there is a caller workflow that can be triggered externally
734731
this.getName() = "workflow_call" and
@@ -1322,6 +1319,12 @@ class RunImpl extends StepImpl {
13221319

13231320
string getScript() { result = script.getValue().regexpReplaceAll("\\\\\\s*\n", "") }
13241321

1322+
string getACommand() { result = Bash::getACommand(this.getScript()) }
1323+
1324+
predicate getAnAssignment(string name, string value) {
1325+
Bash::getAnAssignment(this.getScript(), name, value)
1326+
}
1327+
13251328
ScalarValueImpl getScriptScalar() { result = TScalarValueNode(script) }
13261329

13271330
ExpressionImpl getAnScriptExpr() { result.getParentNode().getNode() = script }

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

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -28,13 +28,13 @@ class ArgumentInjectionFromEnvVarSink extends ArgumentInjectionSink {
2828
)
2929
or
3030
exists(
31-
Run run, string line, string argument, string regexp, int argument_group, int command_group
31+
Run run, string cmd, string argument, string regexp, int argument_group, int command_group
3232
|
33-
run.getScript().splitAt("\n") = line and
33+
run.getACommand() = cmd and
3434
run.getScriptScalar() = this.asExpr() and
3535
argumentInjectionSinksDataModel(regexp, command_group, argument_group) and
36-
argument = line.regexpCapture(regexp, argument_group) and
37-
command = line.regexpCapture(regexp, command_group) and
36+
argument = cmd.regexpCapture(regexp, argument_group) and
37+
command = cmd.regexpCapture(regexp, command_group) and
3838
argument.regexpMatch(".*\\$(\\{)?(GITHUB_HEAD_REF).*")
3939
)
4040
}
@@ -60,12 +60,12 @@ private module ArgumentInjectionConfig implements DataFlow::ConfigSig {
6060
source instanceof RemoteFlowSource
6161
or
6262
exists(
63-
Run run, string argument, string line, string regexp, int command_group, int argument_group
63+
Run run, string argument, string cmd, string regexp, int command_group, int argument_group
6464
|
6565
run.getScriptScalar() = source.asExpr() and
66-
run.getScript().splitAt("\n") = line and
66+
run.getACommand() = cmd and
6767
argumentInjectionSinksDataModel(regexp, command_group, argument_group) and
68-
argument = line.regexpCapture(regexp, argument_group) and
68+
argument = cmd.regexpCapture(regexp, argument_group) and
6969
argument.regexpMatch(".*\\$(\\{)?(GITHUB_HEAD_REF).*")
7070
)
7171
}

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

Lines changed: 20 additions & 44 deletions
Original file line numberDiff line numberDiff line change
@@ -155,71 +155,54 @@ class ActionsGitHubScriptDownloadStep extends UntrustedArtifactDownloadStep, Use
155155
}
156156

157157
override string getPath() {
158-
if
159-
this.getAFollowingStep()
160-
.(Run)
161-
.getScript()
162-
.splitAt("\n")
163-
.regexpMatch(unzipRegexp() + unzipDirArgRegexp())
158+
if this.getAFollowingStep().(Run).getACommand().regexpMatch(unzipRegexp() + unzipDirArgRegexp())
164159
then
165160
result =
166161
normalizePath(trimQuotes(this.getAFollowingStep()
167162
.(Run)
168-
.getScript()
169-
.splitAt("\n")
163+
.getACommand()
170164
.regexpCapture(unzipRegexp() + unzipDirArgRegexp(), 2)))
171165
else
172-
if this.getAFollowingStep().(Run).getScript().splitAt("\n").regexpMatch(unzipRegexp())
166+
if this.getAFollowingStep().(Run).getACommand().regexpMatch(unzipRegexp())
173167
then result = "GITHUB_WORKSPACE/"
174168
else none()
175169
}
176170
}
177171

178172
class GHRunArtifactDownloadStep extends UntrustedArtifactDownloadStep, Run {
179-
string script;
180-
181173
GHRunArtifactDownloadStep() {
182174
// eg: - run: gh run download ${{ github.event.workflow_run.id }} --repo "${GITHUB_REPOSITORY}" --name "artifact_name"
183-
this.getScript() = script and
184-
script.splitAt("\n").regexpMatch(".*gh\\s+run\\s+download.*") and
185-
script.splitAt("\n").matches("%github.event.workflow_run.id%") and
175+
this.getACommand().regexpMatch(".*gh\\s+run\\s+download.*") and
176+
this.getACommand().matches("%github.event.workflow_run.id%") and
186177
(
187-
script.splitAt("\n").regexpMatch(unzipRegexp()) or
188-
this.getAFollowingStep().(Run).getScript().splitAt("\n").regexpMatch(unzipRegexp())
178+
this.getACommand().regexpMatch(unzipRegexp()) or
179+
this.getAFollowingStep().(Run).getACommand().regexpMatch(unzipRegexp())
189180
)
190181
}
191182

192183
override string getPath() {
193184
if
194-
this.getAFollowingStep()
195-
.(Run)
196-
.getScript()
197-
.splitAt("\n")
198-
.regexpMatch(unzipRegexp() + unzipDirArgRegexp()) or
199-
script.splitAt("\n").regexpMatch(unzipRegexp() + unzipDirArgRegexp())
185+
this.getAFollowingStep().(Run).getACommand().regexpMatch(unzipRegexp() + unzipDirArgRegexp()) or
186+
this.getACommand().regexpMatch(unzipRegexp() + unzipDirArgRegexp())
200187
then
201188
result =
202-
normalizePath(trimQuotes(script
203-
.splitAt("\n")
189+
normalizePath(trimQuotes(this.getACommand()
204190
.regexpCapture(unzipRegexp() + unzipDirArgRegexp(), 2))) or
205191
result =
206192
normalizePath(trimQuotes(this.getAFollowingStep()
207193
.(Run)
208-
.getScript()
209-
.splitAt("\n")
194+
.getACommand()
210195
.regexpCapture(unzipRegexp() + unzipDirArgRegexp(), 2)))
211196
else
212197
if
213-
this.getAFollowingStep().(Run).getScript().splitAt("\n").regexpMatch(unzipRegexp()) or
214-
script.splitAt("\n").regexpMatch(unzipRegexp())
198+
this.getAFollowingStep().(Run).getACommand().regexpMatch(unzipRegexp()) or
199+
this.getACommand().regexpMatch(unzipRegexp())
215200
then result = "GITHUB_WORKSPACE/"
216201
else none()
217202
}
218203
}
219204

220205
class DirectArtifactDownloadStep extends UntrustedArtifactDownloadStep, Run {
221-
string script;
222-
223206
DirectArtifactDownloadStep() {
224207
// eg:
225208
// run: |
@@ -230,32 +213,25 @@ class DirectArtifactDownloadStep extends UntrustedArtifactDownloadStep, Run {
230213
// gh api $url > "$name.zip"
231214
// unzip -d "$name" "$name.zip"
232215
// done
233-
this.getScript() = script and
234-
script.splitAt("\n").matches("%github.event.workflow_run.artifacts_url%") and
216+
this.getACommand().matches("%github.event.workflow_run.artifacts_url%") and
235217
(
236-
script.splitAt("\n").regexpMatch(unzipRegexp()) or
237-
this.getAFollowingStep().(Run).getScript().splitAt("\n").regexpMatch(unzipRegexp())
218+
this.getACommand().regexpMatch(unzipRegexp()) or
219+
this.getAFollowingStep().(Run).getACommand().regexpMatch(unzipRegexp())
238220
)
239221
}
240222

241223
override string getPath() {
242224
if
243-
script.splitAt("\n").regexpMatch(unzipRegexp() + unzipDirArgRegexp()) or
244-
this.getAFollowingStep()
245-
.(Run)
246-
.getScript()
247-
.splitAt("\n")
248-
.regexpMatch(unzipRegexp() + unzipDirArgRegexp())
225+
this.getACommand().regexpMatch(unzipRegexp() + unzipDirArgRegexp()) or
226+
this.getAFollowingStep().(Run).getACommand().regexpMatch(unzipRegexp() + unzipDirArgRegexp())
249227
then
250228
result =
251-
normalizePath(trimQuotes(script
252-
.splitAt("\n")
229+
normalizePath(trimQuotes(this.getACommand()
253230
.regexpCapture(unzipRegexp() + unzipDirArgRegexp(), 2))) or
254231
result =
255232
normalizePath(trimQuotes(this.getAFollowingStep()
256233
.(Run)
257-
.getScript()
258-
.splitAt("\n")
234+
.getACommand()
259235
.regexpCapture(unzipRegexp() + unzipDirArgRegexp(), 2)))
260236
else result = "GITHUB_WORKSPACE/"
261237
}

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

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -255,10 +255,13 @@ class PermissionActionCheck extends PermissionCheck instanceof UsesStep {
255255

256256
class BashCommentVsHeadDateCheck extends CommentVsHeadDateCheck, Run {
257257
BashCommentVsHeadDateCheck() {
258-
exists(string line |
259-
line = this.getScript().splitAt("\n") and
260-
line.toLowerCase()
261-
.regexpMatch(".*date\\s+-d.*(commit_at|pushed_at|comment_at|commented_at).*date\\s+-d.*(commit_at|pushed_at|comment_at|commented_at).*")
258+
// eg: if [[ $(date -d "$pushed_at" +%s) -gt $(date -d "$COMMENT_AT" +%s) ]]; then
259+
exists(string cmd1, string cmd2 |
260+
cmd1 = this.getACommand() and
261+
cmd2 = this.getACommand() and
262+
not cmd1 = cmd2 and
263+
cmd1.toLowerCase().regexpMatch("date\\s+-d.*(commit|pushed|comment|commented)_at.*") and
264+
cmd2.toLowerCase().regexpMatch("date\\s+-d.*(commit|pushed|comment|commented)_at.*")
262265
)
263266
}
264267
}

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

Lines changed: 2 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -37,11 +37,8 @@ class EnvPathInjectionFromFileReadSink extends EnvPathInjectionSink {
3737
// e.g.
3838
// FOO=$(cat test-results/sha-number)
3939
// echo "FOO=$FOO" >> $GITHUB_PATH
40-
exists(string line, string var_name, string var_value |
41-
run.getScript().splitAt("\n") = line
42-
|
43-
var_name = line.regexpCapture("([a-zA-Z0-9\\-_]+)=(.*)", 1) and
44-
var_value = line.regexpCapture("([a-zA-Z0-9\\-_]+)=(.*)", 2) and
40+
exists(string var_name, string var_value |
41+
run.getAnAssignment(var_name, var_value) and
4542
outputsPartialFileContent(var_value) and
4643
(
4744
value.matches("%$" + ["", "{", "ENV{"] + var_name + "%")

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

Lines changed: 2 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -42,11 +42,8 @@ class EnvVarInjectionFromFileReadSink extends EnvVarInjectionSink {
4242
// e.g.
4343
// FOO=$(cat test-results/sha-number)
4444
// echo "FOO=$FOO" >> $GITHUB_ENV
45-
exists(string line, string var_name, string var_value |
46-
run.getScript().splitAt("\n") = line
47-
|
48-
var_name = line.regexpCapture("([a-zA-Z0-9\\-_]+)=(.*)", 1) and
49-
var_value = line.regexpCapture("([a-zA-Z0-9\\-_]+)=(.*)", 2) and
45+
exists(string var_name, string var_value |
46+
run.getAnAssignment(var_name, var_value) and
5047
outputsPartialFileContent(var_value) and
5148
(
5249
value.matches("%$" + ["", "{", "ENV{"] + var_name + "%")

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

Lines changed: 4 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -56,11 +56,8 @@ class OutputClobberingFromFileReadSink extends OutputClobberingSink {
5656
// e.g.
5757
// FOO=$(cat test-results/sha-number)
5858
// echo "FOO=$FOO" >> $GITHUB_OUTPUT
59-
exists(string line, string var_name, string var_value |
60-
run.getScript().splitAt("\n") = line
61-
|
62-
var_name = line.regexpCapture("([a-zA-Z0-9\\-_]+)=(.*)", 1) and
63-
var_value = line.regexpCapture("([a-zA-Z0-9\\-_]+)=(.*)", 2) and
59+
exists(string var_name, string var_value |
60+
run.getAnAssignment(var_name, var_value) and
6461
outputsPartialFileContent(var_value) and
6562
(
6663
value.matches("%$" + ["", "{", "ENV{"] + var_name + "%")
@@ -154,11 +151,11 @@ class WorkflowCommandClobberingFromFileReadSink extends OutputClobberingSink {
154151
// A file is read and its content is printed to stdout
155152
// - run: echo "foo=$(<pr-id.txt)"
156153
clobbering_line.regexpMatch(".*echo\\s+(-e)?\\s*(\"|')?") and
157-
clobbering_line.regexpMatch(partialFileContentRegexp() + ".*")
154+
clobbering_line.regexpMatch(["ls", Bash::partialFileContentCommand()] + "\\s.*")
158155
or
159156
// A file content is printed to stdout
160157
// - run: cat pr-id.txt
161-
clobbering_line.regexpMatch(partialFileContentRegexp() + ".*")
158+
clobbering_line.regexpMatch(["ls", Bash::partialFileContentCommand()] + "\\s.*")
162159
)
163160
)
164161
}

0 commit comments

Comments
 (0)