Skip to content

Commit f8be8e7

Browse files
KyFaStKyFaSt
authored
Merge branch 'master' into immutable-actions
2 parents df0c1e2 + dbcf113 commit f8be8e7

55 files changed

Lines changed: 1740 additions & 323 deletions

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

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

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,8 @@ class AstNode instanceof AstNodeImpl {
1717

1818
Job getEnclosingJob() { result = super.getEnclosingJob() }
1919

20+
Event getATriggerEvent() { result = super.getATriggerEvent() }
21+
2022
Workflow getEnclosingWorkflow() { result = super.getEnclosingWorkflow() }
2123

2224
CompositeAction getEnclosingCompositeAction() { result = super.getEnclosingCompositeAction() }
@@ -100,8 +102,6 @@ class Workflow extends AstNode instanceof WorkflowImpl {
100102

101103
Job getJob(string jobId) { result = super.getJob(jobId) }
102104

103-
Event getATriggerEvent() { result = super.getATriggerEvent() }
104-
105105
Permissions getPermissions() { result = super.getPermissions() }
106106

107107
Strategy getStrategy() { result = super.getStrategy() }
@@ -200,8 +200,6 @@ abstract class Job extends AstNode instanceof JobImpl {
200200

201201
Permissions getPermissions() { result = super.getPermissions() }
202202

203-
Event getATriggerEvent() { result = super.getATriggerEvent() }
204-
205203
Strategy getStrategy() { result = super.getStrategy() }
206204

207205
string getARunsOnLabel() { result = super.getARunsOnLabel() }

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

Lines changed: 10 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -111,6 +111,11 @@ abstract class AstNodeImpl extends TAstNode {
111111
result = this.getEnclosingCompositeAction().getACallerJob()
112112
}
113113

114+
/**
115+
* Gets and Event triggering this node.
116+
*/
117+
EventImpl getATriggerEvent() { result = this.getEnclosingJob().getATriggerEvent() }
118+
114119
/**
115120
* Gets the enclosing Step.
116121
*/
@@ -447,7 +452,7 @@ class CompositeActionImpl extends AstNodeImpl, TCompositeAction {
447452
)
448453
}
449454

450-
EventImpl getATriggerEvent() { result = this.getACallerJob().getATriggerEvent() }
455+
override EventImpl getATriggerEvent() { result = this.getACallerJob().getATriggerEvent() }
451456
}
452457

453458
class WorkflowImpl extends AstNodeImpl, TWorkflowNode {
@@ -486,7 +491,7 @@ class WorkflowImpl extends AstNodeImpl, TWorkflowNode {
486491
PermissionsImpl getPermissions() { result.getNode() = n.lookup("permissions") }
487492

488493
/** Gets the trigger event that starts this workflow. */
489-
EventImpl getATriggerEvent() { this.getOn().getAnEvent() = result }
494+
override EventImpl getATriggerEvent() { this.getOn().getAnEvent() = result }
490495

491496
/** Gets the strategy for this workflow. */
492497
StrategyImpl getStrategy() { result.getNode() = n.lookup("strategy") }
@@ -918,7 +923,7 @@ class JobImpl extends AstNodeImpl, TJobNode {
918923
StrategyImpl getStrategy() { result.getNode() = n.lookup("strategy") }
919924

920925
/** Gets the trigger event that starts this workflow. */
921-
EventImpl getATriggerEvent() {
926+
override EventImpl getATriggerEvent() {
922927
if this.getEnclosingWorkflow() instanceof ReusableWorkflowImpl
923928
then
924929
result = this.getEnclosingWorkflow().(ReusableWorkflowImpl).getACaller().getATriggerEvent()
@@ -1174,6 +1179,8 @@ class StepImpl extends AstNodeImpl, TStepNode {
11741179
result = super.getEnclosingJob()
11751180
}
11761181

1182+
override EventImpl getATriggerEvent() { result = this.getEnclosingJob().getATriggerEvent() }
1183+
11771184
EnvImpl getEnv() { result.getNode() = n.lookup("env") }
11781185

11791186
/** Gets the ID of this step, if any. */

‎ql/lib/codeql/actions/config/Config.qll‎

Lines changed: 12 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -136,6 +136,16 @@ predicate immutableActionsDataModel(
136136
* - cmd_regex: Regular expression for matching untrusted git commands
137137
* - flag: Flag for the command
138138
*/
139-
predicate untrustedGitCommandsDataModel(string cmd_regex, string flag) {
140-
Extensions::untrustedGitCommandsDataModel(cmd_regex, flag)
139+
predicate untrustedGitCommandDataModel(string cmd_regex, string flag) {
140+
Extensions::untrustedGitCommandDataModel(cmd_regex, flag)
141+
}
142+
143+
/**
144+
* MaD models for untrusted gh commands
145+
* Fields:
146+
* - cmd_regex: Regular expression for matching untrusted gh commands
147+
* - flag: Flag for the command
148+
*/
149+
predicate untrustedGhCommandDataModel(string cmd_regex, string flag) {
150+
Extensions::untrustedGhCommandDataModel(cmd_regex, flag)
141151
}

‎ql/lib/codeql/actions/config/ConfigExtensions.qll‎

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -68,4 +68,9 @@ extensible predicate immutableActionsDataModel(
6868
/**
6969
* Holds for git commands that may introduce untrusted data when called on an attacker controlled branch.
7070
*/
71-
extensible predicate untrustedGitCommandsDataModel(string cmd_regex, string flag);
71+
extensible predicate untrustedGitCommandDataModel(string cmd_regex, string flag);
72+
73+
/**
74+
* Holds for gh commands that may introduce untrusted data
75+
*/
76+
extensible predicate untrustedGhCommandDataModel(string cmd_regex, string flag);

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

Lines changed: 61 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -40,9 +40,10 @@ class GitHubCtxSource extends RemoteFlowSource {
4040

4141
class GitHubEventCtxSource extends RemoteFlowSource {
4242
string flag;
43+
string context;
4344

4445
GitHubEventCtxSource() {
45-
exists(Expression e, string context, string regexp |
46+
exists(Expression e, string regexp |
4647
this.asExpr() = e and
4748
context = e.getExpression() and
4849
(
@@ -62,6 +63,8 @@ class GitHubEventCtxSource extends RemoteFlowSource {
6263
}
6364

6465
override string getSourceType() { result = flag }
66+
67+
string getContext() { result = context }
6568
}
6669

6770
abstract class CommandSource extends RemoteFlowSource {
@@ -77,7 +80,7 @@ class GitCommandSource extends RemoteFlowSource, CommandSource {
7780

7881
GitCommandSource() {
7982
exists(Step checkout, string cmd_regex |
80-
// This shoould be:
83+
// This should be:
8184
// source instanceof PRHeadCheckoutStep
8285
// but PRHeadCheckoutStep uses Taint Tracking anc causes a non-Monolitic Recursion error
8386
// so we list all the subclasses of PRHeadCheckoutStep here and use actions/checkout as a workaround
@@ -87,7 +90,8 @@ class GitCommandSource extends RemoteFlowSource, CommandSource {
8790
checkout = uses and
8891
uses.getCallee() = "actions/checkout" and
8992
exists(uses.getArgument("ref")) and
90-
not uses.getArgument("ref").matches("%base%")
93+
not uses.getArgument("ref").matches("%base%") and
94+
uses.getATriggerEvent().getName() = checkoutTriggers()
9195
)
9296
or
9397
checkout instanceof GitMutableRefCheckout
@@ -102,8 +106,8 @@ class GitCommandSource extends RemoteFlowSource, CommandSource {
102106
checkout.getAFollowingStep() = run and
103107
run.getScript().getAStmt() = cmd and
104108
cmd.indexOf("git") = 0 and
105-
untrustedGitCommandsDataModel(cmd_regex, flag) and
106-
cmd.regexpMatch(".*" + cmd_regex + ".*")
109+
untrustedGitCommandDataModel(cmd_regex, flag) and
110+
cmd.regexpMatch(cmd_regex + ".*")
107111
)
108112
}
109113

@@ -114,6 +118,34 @@ class GitCommandSource extends RemoteFlowSource, CommandSource {
114118
override Run getEnclosingRun() { result = run }
115119
}
116120

121+
class GhCLICommandSource extends RemoteFlowSource, CommandSource {
122+
Run run;
123+
string cmd;
124+
string flag;
125+
126+
GhCLICommandSource() {
127+
exists(string cmd_regex |
128+
this.asExpr() = run.getScript() and
129+
run.getScript().getAStmt() = cmd and
130+
cmd.indexOf("gh ") = 0 and
131+
untrustedGhCommandDataModel(cmd_regex, flag) and
132+
cmd.regexpMatch(cmd_regex + ".*") and
133+
(
134+
cmd.regexpMatch(".*\\b(pr|pulls)\\b.*") and
135+
run.getATriggerEvent().getName() = checkoutTriggers()
136+
or
137+
not cmd.regexpMatch(".*\\b(pr|pulls)\\b.*")
138+
)
139+
)
140+
}
141+
142+
override string getSourceType() { result = flag }
143+
144+
override Run getEnclosingRun() { result = run }
145+
146+
override string getCommand() { result = cmd }
147+
}
148+
117149
class GitHubEventPathSource extends RemoteFlowSource, CommandSource {
118150
string cmd;
119151
string flag;
@@ -203,7 +235,7 @@ class ArtifactSource extends RemoteFlowSource, FileSource {
203235
*/
204236
private class CheckoutSource extends RemoteFlowSource, FileSource {
205237
CheckoutSource() {
206-
// This shoould be:
238+
// This should be:
207239
// source instanceof PRHeadCheckoutStep
208240
// but PRHeadCheckoutStep uses Taint Tracking anc causes a non-Monolitic Recursion error
209241
// so we list all the subclasses of PRHeadCheckoutStep here and use actions/checkout as a workaround
@@ -212,7 +244,8 @@ private class CheckoutSource extends RemoteFlowSource, FileSource {
212244
this.asExpr() = uses and
213245
uses.getCallee() = "actions/checkout" and
214246
exists(uses.getArgument("ref")) and
215-
not uses.getArgument("ref").matches("%base%")
247+
not uses.getArgument("ref").matches("%base%") and
248+
uses.getATriggerEvent().getName() = checkoutTriggers()
216249
)
217250
or
218251
this.asExpr() instanceof GitMutableRefCheckout
@@ -295,3 +328,24 @@ class Xt0rtedSlashCommandSource extends RemoteFlowSource {
295328

296329
override string getSourceType() { result = "text" }
297330
}
331+
332+
class OctokitRequestActionSource extends RemoteFlowSource {
333+
OctokitRequestActionSource() {
334+
exists(UsesStep u, string route |
335+
u.getCallee() = "octokit/request-action" and
336+
route = u.getArgument("route").trim() and
337+
route.indexOf("GET") = 0 and
338+
(
339+
route.matches("%/commits%") or
340+
route.matches("%/comments%") or
341+
route.matches("%/pulls%") or
342+
route.matches("%/issues%") or
343+
route.matches("%/users%") or
344+
route.matches("%github.event.issue.pull_request.url%")
345+
) and
346+
this.asExpr() = u
347+
)
348+
}
349+
350+
override string getSourceType() { result = "text" }
351+
}

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

Lines changed: 35 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -91,11 +91,45 @@ predicate xt0rtedSlashCommandActionTaintStep(DataFlow::Node pred, DataFlow::Node
9191
)
9292
}
9393

94+
/**
95+
* A read of user-controlled field of the octokit/request-action action.
96+
*/
97+
predicate octokitRequestActionTaintStep(DataFlow::Node pred, DataFlow::Node succ) {
98+
exists(StepsExpression o |
99+
pred instanceof OctokitRequestActionSource and
100+
o.getTarget() = pred.asExpr() and
101+
o.getStepId() = pred.asExpr().(UsesStep).getId() and
102+
succ.asExpr() = o and
103+
(
104+
not o instanceof JsonReferenceExpression and
105+
o.getFieldName() = "data"
106+
or
107+
o instanceof JsonReferenceExpression and
108+
o.(JsonReferenceExpression).getInnerExpression().matches("%.data") and
109+
o.(JsonReferenceExpression)
110+
.getAccessPath()
111+
.matches([
112+
"%.title",
113+
"%.user.login",
114+
"%.body",
115+
"%.head.ref",
116+
"%.head.repo.full_name",
117+
"%.commit.author.email",
118+
"%.commit.commiter.email",
119+
"%.commit.message",
120+
"%.email",
121+
"%.name",
122+
])
123+
)
124+
)
125+
}
126+
94127
class TaintSteps extends AdditionalTaintStep {
95128
override predicate step(DataFlow::Node node1, DataFlow::Node node2) {
96129
dornyPathsFilterTaintStep(node1, node2) or
97130
tjActionsChangedFilesTaintStep(node1, node2) or
98131
tjActionsVerifyChangedFilesTaintStep(node1, node2) or
99-
xt0rtedSlashCommandActionTaintStep(node1, node2)
132+
xt0rtedSlashCommandActionTaintStep(node1, node2) or
133+
octokitRequestActionTaintStep(node1, node2)
100134
}
101135
}

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

Lines changed: 13 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -4,9 +4,9 @@ import codeql.actions.DataFlow
44
import codeql.actions.dataflow.FlowSources
55
import codeql.actions.security.PoisonableSteps
66

7-
string unzipRegexp() { result = ".*(unzip|tar)\\s+.*" }
7+
string unzipRegexp() { result = "(unzip|tar)\\s+.*" }
88

9-
string unzipDirArgRegexp() { result = "-d\\s+([^ ]+).*" }
9+
string unzipDirArgRegexp() { result = "(-d|-C)\\s+([^ ]+).*" }
1010

1111
abstract class UntrustedArtifactDownloadStep extends Step {
1212
abstract string getPath();
@@ -166,7 +166,7 @@ class ActionsGitHubScriptDownloadStep extends UntrustedArtifactDownloadStep, Use
166166
.(Run)
167167
.getScript()
168168
.getACommand()
169-
.regexpCapture(unzipRegexp() + unzipDirArgRegexp(), 2)))
169+
.regexpCapture(unzipRegexp() + unzipDirArgRegexp(), 3)))
170170
else
171171
if this.getAFollowingStep().(Run).getScript().getACommand().regexpMatch(unzipRegexp())
172172
then result = "GITHUB_WORKSPACE/"
@@ -197,13 +197,13 @@ class GHRunArtifactDownloadStep extends UntrustedArtifactDownloadStep, Run {
197197
result =
198198
normalizePath(trimQuotes(this.getScript()
199199
.getACommand()
200-
.regexpCapture(unzipRegexp() + unzipDirArgRegexp(), 2))) or
200+
.regexpCapture(unzipRegexp() + unzipDirArgRegexp(), 3))) or
201201
result =
202202
normalizePath(trimQuotes(this.getAFollowingStep()
203203
.(Run)
204204
.getScript()
205205
.getACommand()
206-
.regexpCapture(unzipRegexp() + unzipDirArgRegexp(), 2)))
206+
.regexpCapture(unzipRegexp() + unzipDirArgRegexp(), 3)))
207207
else
208208
if
209209
this.getAFollowingStep().(Run).getScript().getACommand().regexpMatch(unzipRegexp()) or
@@ -243,13 +243,13 @@ class DirectArtifactDownloadStep extends UntrustedArtifactDownloadStep, Run {
243243
result =
244244
normalizePath(trimQuotes(this.getScript()
245245
.getACommand()
246-
.regexpCapture(unzipRegexp() + unzipDirArgRegexp(), 2))) or
246+
.regexpCapture(unzipRegexp() + unzipDirArgRegexp(), 3))) or
247247
result =
248248
normalizePath(trimQuotes(this.getAFollowingStep()
249249
.(Run)
250250
.getScript()
251251
.getACommand()
252-
.regexpCapture(unzipRegexp() + unzipDirArgRegexp(), 2)))
252+
.regexpCapture(unzipRegexp() + unzipDirArgRegexp(), 3)))
253253
else result = "GITHUB_WORKSPACE/"
254254
}
255255
}
@@ -276,7 +276,12 @@ class ArtifactPoisoningSink extends DataFlow::Node {
276276
)
277277
or
278278
poisonable.(UsesStep) = this.asExpr() and
279-
download.getPath() = "GITHUB_WORKSPACE/"
279+
(
280+
not poisonable instanceof LocalActionUsesStep and
281+
download.getPath() = "GITHUB_WORKSPACE/"
282+
or
283+
isSubpath(poisonable.(LocalActionUsesStep).getPath(), download.getPath())
284+
)
280285
)
281286
}
282287

0 commit comments

Comments
 (0)