Skip to content

Commit 4be3011

Browse files
author
Alvaro Muñoz
authored
Merge pull request #4 from github/refactor_untrusted_checkout
Refactor untrusted checkout queries
2 parents 9843f37 + 16c77cb commit 4be3011

19 files changed

+383
-244
lines changed

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

Lines changed: 12 additions & 73 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@ private import codeql.actions.TaintTracking
33
import codeql.actions.DataFlow
44
private import codeql.actions.dataflow.ExternalFlow
55
import codeql.actions.dataflow.FlowSources
6+
import codeql.actions.security.PoisonableSteps
67

78
string unzipRegexp() { result = ".*(unzip|tar)\\s+.*" }
89

@@ -228,81 +229,19 @@ class DirectArtifactDownloadStep extends UntrustedArtifactDownloadStep, Run {
228229
}
229230
}
230231

231-
abstract class PoisonableStep extends Step { }
232-
233-
// source: https://github.com/boostsecurityio/poutine/blob/main/opa/rego/rules/untrusted_checkout_exec.rego#L16
234-
private string dangerousActions() {
235-
result =
236-
["pre-commit/action", "oxsecurity/megalinter", "bridgecrewio/checkov-action", "ruby/setup-ruby"]
237-
}
238-
239-
class DangerousActionUsesStep extends PoisonableStep, UsesStep {
240-
DangerousActionUsesStep() {
241-
exists(UntrustedArtifactDownloadStep step |
242-
step.getAFollowingStep() = this and
243-
this.getCallee() = dangerousActions()
244-
)
245-
}
246-
}
247-
248-
// source: https://github.com/boostsecurityio/poutine/blob/main/opa/rego/rules/untrusted_checkout_exec.rego#L23
249-
private string dangerousCommands() {
250-
result =
251-
[
252-
"npm install", "npm run ", "yarn ", "npm ci(\\b|$)", "make ", "terraform plan",
253-
"terraform apply", "gomplate ", "pre-commit run", "pre-commit install", "go generate",
254-
"msbuild ", "mvn ", "./mvnw ", "gradle ", "./gradlew ", "bundle install", "bundle exec ",
255-
"^ant ", "mkdocs build", "pytest"
256-
]
257-
}
258-
259-
class BuildRunStep extends PoisonableStep, Run {
260-
BuildRunStep() {
261-
exists(UntrustedArtifactDownloadStep step |
262-
step.getAFollowingStep() = this and
263-
exists(
264-
this.getScript().splitAt("\n").trim().regexpFind("([^a-z]|^)" + dangerousCommands(), _, _)
265-
)
266-
)
267-
}
268-
}
269-
270-
class LocalCommandExecutionRunStep extends PoisonableStep, Run {
271-
LocalCommandExecutionRunStep() {
272-
exists(UntrustedArtifactDownloadStep step |
273-
step.getAFollowingStep() = this and
274-
// Heuristic:
275-
// Run step with a command starting with `./xxxx`, `sh xxxx`, ...
276-
exists(
277-
this.getScript()
278-
.splitAt("\n")
279-
.trim()
280-
.regexpFind("([^a-z]|^)(./|(ba|z|fi)?sh\\s+)" + step.getPath(), _, _)
281-
)
282-
)
283-
}
284-
}
285-
286-
class EnvVarInjectionRunStep extends PoisonableStep, Run {
287-
EnvVarInjectionRunStep() {
288-
exists(UntrustedArtifactDownloadStep step, string value |
289-
step.getAFollowingStep() = this and
290-
// Heuristic:
291-
// Run step with env var definition based on file content.
292-
// eg: `echo "sha=$(cat test-results/sha-number)" >> $GITHUB_ENV`
293-
// eg: `echo "sha=$(<test-results/sha-number)" >> $GITHUB_ENV`
294-
Utils::writeToGitHubEnv(this, _, value) and
295-
// TODO: add support for other commands like `<`, `jq`, ...
296-
value.regexpMatch(["\\$\\(", "`"] + ["ls\\s+", "cat\\s+", "<"] + ".*" + ["`", "\\)"])
297-
)
298-
}
299-
}
300-
301232
class ArtifactPoisoningSink extends DataFlow::Node {
302233
ArtifactPoisoningSink() {
303-
exists(PoisonableStep step |
304-
step.(Run).getScriptScalar() = this.asExpr() or
305-
step.(UsesStep) = this.asExpr()
234+
exists(UntrustedArtifactDownloadStep download, PoisonableStep poisonable |
235+
download.getAFollowingStep() = poisonable and
236+
(
237+
poisonable.(Run).getScriptScalar() = this.asExpr()
238+
or
239+
poisonable.(UsesStep) = this.asExpr()
240+
) and
241+
(
242+
not poisonable instanceof LocalCommandExecutionRunStep or
243+
poisonable.(LocalCommandExecutionRunStep).getCommand().matches(download.getPath() + "%")
244+
)
306245
)
307246
}
308247
}

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

Lines changed: 64 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,64 @@
1+
import actions
2+
3+
abstract class PoisonableStep extends Step { }
4+
5+
// source: https://github.com/boostsecurityio/poutine/blob/main/opa/rego/rules/untrusted_checkout_exec.rego#L16
6+
private string dangerousActions() {
7+
result =
8+
["pre-commit/action", "oxsecurity/megalinter", "bridgecrewio/checkov-action", "ruby/setup-ruby"]
9+
}
10+
11+
class DangerousActionUsesStep extends PoisonableStep, UsesStep {
12+
DangerousActionUsesStep() { this.getCallee() = dangerousActions() }
13+
}
14+
15+
// source: https://github.com/boostsecurityio/poutine/blob/main/opa/rego/rules/untrusted_checkout_exec.rego#L23
16+
private string dangerousCommands() {
17+
result =
18+
[
19+
"npm install", "npm run ", "yarn ", "npm ci(\\b|$)", "make ", "terraform plan",
20+
"terraform apply", "gomplate ", "pre-commit run", "pre-commit install", "go generate",
21+
"msbuild ", "mvn ", "./mvnw ", "gradle ", "./gradlew ", "bundle install", "bundle exec ",
22+
"^ant ", "mkdocs build", "pytest"
23+
]
24+
}
25+
26+
class BuildRunStep extends PoisonableStep, Run {
27+
BuildRunStep() {
28+
exists(
29+
this.getScript().splitAt("\n").trim().regexpFind("([^a-z]|^)" + dangerousCommands(), _, _)
30+
)
31+
}
32+
}
33+
34+
class LocalCommandExecutionRunStep extends PoisonableStep, Run {
35+
string cmd;
36+
37+
LocalCommandExecutionRunStep() {
38+
// Heuristic:
39+
// Run step with a command starting with `./xxxx`, `sh xxxx`, ...
40+
exists(string line | line = this.getScript().splitAt("\n").trim() |
41+
// ./xxxx
42+
cmd = line.regexpCapture("(^|\\s+)\\.\\/(.*)", 2)
43+
or
44+
// sh xxxx
45+
cmd = line.regexpCapture("(^|\\s+)(ba|z|fi)?sh\\s+(.*)", 3)
46+
)
47+
}
48+
49+
string getCommand() { result = cmd }
50+
}
51+
52+
class EnvVarInjectionRunStep extends PoisonableStep, Run {
53+
EnvVarInjectionRunStep() {
54+
exists(string value |
55+
// Heuristic:
56+
// Run step with env var definition based on file content.
57+
// eg: `echo "sha=$(cat test-results/sha-number)" >> $GITHUB_ENV`
58+
// eg: `echo "sha=$(<test-results/sha-number)" >> $GITHUB_ENV`
59+
Utils::writeToGitHubEnv(this, _, value) and
60+
// TODO: add support for other commands like `<`, `jq`, ...
61+
value.regexpMatch(["\\$\\(", "`"] + ["ls\\s+", "cat\\s+", "<"] + ".*" + ["`", "\\)"])
62+
)
63+
}
64+
}

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

Lines changed: 229 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,229 @@
1+
import actions
2+
import codeql.actions.DataFlow
3+
4+
bindingset[s]
5+
predicate containsPullRequestNumber(string s) {
6+
exists(
7+
Utils::normalizeExpr(s)
8+
.regexpFind([
9+
"\\bgithub\\.event\\.number\\b", "\\bgithub\\.event\\.issue\\.number\\b",
10+
"\\bgithub\\.event\\.pull_request\\.id\\b",
11+
"\\bgithub\\.event\\.pull_request\\.number\\b",
12+
"\\bgithub\\.event\\.check_suite\\.pull_requests\\[\\d+\\]\\.id\\b",
13+
"\\bgithub\\.event\\.check_suite\\.pull_requests\\[\\d+\\]\\.number\\b",
14+
"\\bgithub\\.event\\.check_run\\.check_suite\\.pull_requests\\[\\d+\\]\\.id\\b",
15+
"\\bgithub\\.event\\.check_run\\.check_suite\\.pull_requests\\[\\d+\\]\\.number\\b",
16+
"\\bgithub\\.event\\.check_run\\.pull_requests\\[\\d+\\]\\.id\\b",
17+
"\\bgithub\\.event\\.check_run\\.pull_requests\\[\\d+\\]\\.number\\b",
18+
// heuristics
19+
"\\bpr_number\\b", "\\bpr_id\\b"
20+
], _, _)
21+
)
22+
}
23+
24+
bindingset[s]
25+
predicate containsHeadSHA(string s) {
26+
exists(
27+
Utils::normalizeExpr(s)
28+
.regexpFind([
29+
"\\bgithub\\.event\\.pull_request\\.head\\.sha\\b",
30+
"\\bgithub\\.event\\.pull_request\\.merge_commit_sha\\b",
31+
"\\bgithub\\.event\\.workflow_run\\.head_commit\\.id\\b",
32+
"\\bgithub\\.event\\.workflow_run\\.head_sha\\b",
33+
"\\bgithub\\.event\\.check_suite\\.after\\b",
34+
"\\bgithub\\.event\\.check_suite\\.head_commit\\.id\\b",
35+
"\\bgithub\\.event\\.check_suite\\.head_sha\\b",
36+
"\\bgithub\\.event\\.check_suite\\.pull_requests\\[\\d+\\]\\.head\\.sha\\b",
37+
"\\bgithub\\.event\\.check_run\\.check_suite\\.after\\b",
38+
"\\bgithub\\.event\\.check_run\\.check_suite\\.head_commit\\.id\\b",
39+
"\\bgithub\\.event\\.check_run\\.check_suite\\.head_sha\\b",
40+
"\\bgithub\\.event\\.check_run\\.check_suite\\.pull_requests\\[\\d+\\]\\.head\\.sha\\b",
41+
"\\bgithub\\.event\\.check_run\\.head_sha\\b",
42+
"\\bgithub\\.event\\.check_run\\.pull_requests\\[\\d+\\]\\.head\\.sha\\b",
43+
// heuristics
44+
"\\bhead\\.sha\\b", "\\bhead_sha\\b", "\\bpr_head_sha\\b"
45+
], _, _)
46+
)
47+
}
48+
49+
bindingset[s]
50+
predicate containsHeadRef(string s) {
51+
exists(
52+
Utils::normalizeExpr(s)
53+
.regexpFind([
54+
"\\bgithub\\.event\\.pull_request\\.head\\.ref\\b", "\\bgithub\\.head_ref\\b",
55+
"\\bgithub\\.event\\.workflow_run\\.head_branch\\b",
56+
"\\bgithub\\.event\\.check_suite\\.pull_requests\\[\\d+\\]\\.head\\.ref\\b",
57+
"\\bgithub\\.event\\.check_run\\.check_suite\\.pull_requests\\[\\d+\\]\\.head\\.ref\\b",
58+
"\\bgithub\\.event\\.check_run\\.pull_requests\\[\\d+\\]\\.head\\.ref\\b",
59+
// heuristics
60+
"\\bhead\\.ref\\b", "\\bhead_ref\\b", "\\bpr_head_ref\\b",
61+
// env vars
62+
"\\benv\\.GITHUB_HEAD_REF\\b",
63+
], _, _)
64+
)
65+
}
66+
67+
/** Checkout of a Pull Request HEAD ref */
68+
abstract class PRHeadCheckoutStep extends Step { }
69+
70+
/** Checkout of a Pull Request HEAD ref using actions/checkout action */
71+
class ActionsMutableRefCheckout extends PRHeadCheckoutStep instanceof UsesStep {
72+
ActionsMutableRefCheckout() {
73+
this.getCallee() = "actions/checkout" and
74+
(
75+
// ref argument contains the PR id/number or head ref/sha
76+
exists(Expression e |
77+
(
78+
containsHeadRef(e.getExpression()) or
79+
containsPullRequestNumber(e.getExpression())
80+
) and
81+
DataFlow::hasLocalFlowExpr(e, this.getArgumentExpr("ref"))
82+
)
83+
or
84+
// 3rd party actions returning the PR head sha/ref
85+
exists(UsesStep step |
86+
step.getCallee() = ["eficode/resolve-pr-refs", "xt0rted/pull-request-comment-branch"] and
87+
// TODO: This should be read step of the head_sha or head_ref output vars
88+
this.getArgument("ref").regexpMatch(".*head_ref.*") and
89+
DataFlow::hasLocalFlowExpr(step, this.getArgumentExpr("ref"))
90+
)
91+
or
92+
// heuristic base on the step id and field name
93+
exists(StepsExpression e |
94+
this.getArgumentExpr("ref") = e and
95+
(
96+
e.getStepId().matches(["%ref%", "%branch%"]) or
97+
e.getFieldName().matches(["%ref%", "%branch%"])
98+
)
99+
)
100+
)
101+
}
102+
}
103+
104+
/** Checkout of a Pull Request HEAD ref using actions/checkout action */
105+
class ActionsSHACheckout extends PRHeadCheckoutStep instanceof UsesStep {
106+
ActionsSHACheckout() {
107+
this.getCallee() = "actions/checkout" and
108+
(
109+
// ref argument contains the PR id/number or head ref/sha
110+
exists(Expression e |
111+
containsHeadSHA(e.getExpression()) and
112+
DataFlow::hasLocalFlowExpr(e, this.getArgumentExpr("ref"))
113+
)
114+
or
115+
// 3rd party actions returning the PR head sha/ref
116+
exists(UsesStep step |
117+
step.getCallee() = ["eficode/resolve-pr-refs", "xt0rted/pull-request-comment-branch"] and
118+
this.getArgument("ref").regexpMatch(".*head_sha.*") and
119+
DataFlow::hasLocalFlowExpr(step, this.getArgumentExpr("ref"))
120+
)
121+
or
122+
// heuristic base on the step id and field name
123+
exists(StepsExpression e |
124+
this.getArgumentExpr("ref") = e and
125+
(
126+
e.getStepId().matches(["%sha%", "%commit%"]) or
127+
e.getFieldName().matches(["%sha%", "%commit%"])
128+
)
129+
)
130+
)
131+
}
132+
}
133+
134+
/** Checkout of a Pull Request HEAD ref using git within a Run step */
135+
class GitMutableRefCheckout extends PRHeadCheckoutStep instanceof Run {
136+
GitMutableRefCheckout() {
137+
exists(string line |
138+
this.getScript().splitAt("\n") = line and
139+
line.regexpMatch(".*git\\s+(fetch|pull).*") and
140+
(
141+
(containsHeadRef(line) or containsPullRequestNumber(line))
142+
or
143+
exists(string varname, string expr |
144+
expr = this.getInScopeEnvVarExpr(varname).getExpression() and
145+
(
146+
containsHeadRef(expr) or
147+
containsPullRequestNumber(expr)
148+
) and
149+
exists(line.regexpFind(varname, _, _))
150+
)
151+
)
152+
)
153+
}
154+
}
155+
156+
/** Checkout of a Pull Request HEAD ref using git within a Run step */
157+
class GitSHACheckout extends PRHeadCheckoutStep instanceof Run {
158+
GitSHACheckout() {
159+
exists(string line |
160+
this.getScript().splitAt("\n") = line and
161+
line.regexpMatch(".*git\\s+(fetch|pull).*") and
162+
(
163+
containsHeadSHA(line)
164+
or
165+
exists(string varname, string expr |
166+
expr = this.getInScopeEnvVarExpr(varname).getExpression() and
167+
containsHeadSHA(expr) and
168+
exists(line.regexpFind(varname, _, _))
169+
)
170+
)
171+
)
172+
}
173+
}
174+
175+
/** Checkout of a Pull Request HEAD ref using gh within a Run step */
176+
class GhMutableRefCheckout extends PRHeadCheckoutStep instanceof Run {
177+
GhMutableRefCheckout() {
178+
exists(string line |
179+
this.getScript().splitAt("\n") = line and
180+
line.regexpMatch(".*gh\\s+pr\\s+checkout.*") and
181+
(
182+
(containsHeadRef(line) or containsPullRequestNumber(line))
183+
or
184+
exists(string varname |
185+
(
186+
containsHeadRef(this.getInScopeEnvVarExpr(varname).getExpression()) or
187+
containsPullRequestNumber(this.getInScopeEnvVarExpr(varname).getExpression())
188+
) and
189+
exists(line.regexpFind(varname, _, _))
190+
)
191+
)
192+
)
193+
}
194+
}
195+
196+
/** Checkout of a Pull Request HEAD ref using gh within a Run step */
197+
class GhSHACheckout extends PRHeadCheckoutStep instanceof Run {
198+
GhSHACheckout() {
199+
exists(string line |
200+
this.getScript().splitAt("\n") = line and
201+
line.regexpMatch(".*gh\\s+pr\\s+checkout.*") and
202+
(
203+
containsHeadSHA(line)
204+
or
205+
exists(string varname |
206+
containsHeadSHA(this.getInScopeEnvVarExpr(varname).getExpression()) and
207+
exists(line.regexpFind(varname, _, _))
208+
)
209+
)
210+
)
211+
}
212+
}
213+
214+
/** An If node that contains an actor, user or label check */
215+
class ControlCheck extends If {
216+
ControlCheck() {
217+
exists(
218+
Utils::normalizeExpr(this.getCondition())
219+
.regexpFind([
220+
"\\bgithub\\.actor\\b", // actor
221+
"\\bgithub\\.triggering_actor\\b", // actor
222+
"\\bgithub\\.event\\.comment\\.user\\.login\\b", //user
223+
"\\bgithub\\.event\\.pull_request\\.user\\.login\\b", //user
224+
"\\bgithub\\.event\\.pull_request\\.labels\\b", // label
225+
"\\bgithub\\.event\\.label\\.name\\b" // label
226+
], _, _)
227+
)
228+
}
229+
}

0 commit comments

Comments
 (0)