Skip to content

Commit 4f075f3

Browse files
author
Alvaro Muñoz
committed
feat: Improve sanitizer checks
1 parent 92f3b16 commit 4f075f3

21 files changed

+450
-81
lines changed

ql/lib/codeql/actions/Helper.qll

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -248,8 +248,7 @@ predicate inPrivilegedCompositeAction(AstNode node) {
248248
predicate inPrivilegedExternallyTriggerableJob(AstNode node) {
249249
exists(Job j |
250250
j = node.getEnclosingJob() and
251-
j.isPrivilegedExternallyTriggerable() and
252-
not exists(ControlCheck check, Event e | j.getATriggerEvent() = e | check.protects(node, e))
251+
j.isPrivilegedExternallyTriggerable()
253252
)
254253
}
255254

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

Lines changed: 92 additions & 56 deletions
Original file line numberDiff line numberDiff line change
@@ -1,17 +1,46 @@
11
import actions
22

3+
string any_relevant_category() {
4+
result =
5+
[
6+
"untrusted-checkout", "output-clobbering", "envpath-injection", "envvar-injection",
7+
"command-injection", "argument-injection", "code-injection", "cache-poisoning",
8+
"untrusted-checkout-toctou", "artifact-poisoning"
9+
]
10+
}
11+
12+
string any_non_toctou_category() {
13+
result = any_relevant_category() and not result = "untrusted-checkout-toctou"
14+
}
15+
16+
string any_relevant_event() {
17+
result =
18+
[
19+
"pull_request_target",
20+
"issue_comment",
21+
"pull_request_comment",
22+
"workflow_run",
23+
"issues",
24+
"fork",
25+
"watch",
26+
"discussion_comment",
27+
"discussion"
28+
]
29+
}
30+
331
/** An If node that contains an actor, user or label check */
432
abstract class ControlCheck extends AstNode {
533
ControlCheck() {
634
this instanceof If or
735
this instanceof Environment or
8-
this instanceof UsesStep
36+
this instanceof UsesStep or
37+
this instanceof Run
938
}
1039

11-
predicate protects(Step step, Event event) {
40+
predicate protects(Step step, Event event, string category) {
1241
event.getEnclosingWorkflow() = step.getEnclosingWorkflow() and
13-
this.getAProtectedEvent() = event.getName() and
14-
this.dominates(step)
42+
this.dominates(step) and
43+
this.protectsCategoryAndEvent(category, event.getName())
1544
}
1645

1746
predicate dominates(Step step) {
@@ -30,80 +59,71 @@ abstract class ControlCheck extends AstNode {
3059
step.getEnclosingJob().getANeededJob().getEnvironment() = this
3160
)
3261
or
33-
this.(UsesStep).getAFollowingStep() = step
62+
this.(Step).getAFollowingStep() = step
3463
}
3564

36-
abstract string getAProtectedEvent();
37-
38-
abstract boolean protectsAgainstRefMutationAttacks();
65+
abstract predicate protectsCategoryAndEvent(string category, string event);
3966
}
4067

4168
abstract class AssociationCheck extends ControlCheck {
42-
// checks who you are (identity)
43-
// association checks are effective against pull requests since they can control who is making the PR
44-
// they are not effective against issue_comment since the author of the comment may not be the same as the author of the PR
45-
// someone entitled to trigger the workflow with a comment, may no detect a malicious comment, or the comment may mutate after approval
46-
override string getAProtectedEvent() { result = ["pull_request", "pull_request_target"] }
47-
48-
override boolean protectsAgainstRefMutationAttacks() { result = true }
69+
// Checks if the actor is a COLLABORATOR of the repo
70+
// - they are effective against pull requests and workflow_run (since these are triggered by pull_requests) since they can control who is making the PR
71+
// - they are not effective against issue_comment since the author of the comment may not be the same as the author of the PR
72+
override predicate protectsCategoryAndEvent(string category, string event) {
73+
event = ["pull_request_target", "workflow_run"] and category = any_relevant_category()
74+
}
4975
}
5076

5177
abstract class ActorCheck extends ControlCheck {
52-
// checks who you are (identity)
53-
// actor checks are effective against pull requests since they can control who is making the PR
54-
// they are not effective against issue_comment since the author of the comment may not be the same as the author of the PR
55-
// someone entitled to trigger the workflow with a comment, may no detect a malicious comment, or the comment may mutate after approval
56-
override string getAProtectedEvent() { result = ["pull_request", "pull_request_target"] }
57-
58-
override boolean protectsAgainstRefMutationAttacks() { result = true }
78+
// checks for a specific actor
79+
// - they are effective against pull requests and workflow_run (since these are triggered by pull_requests) since they can control who is making the PR
80+
// - they are not effective against issue_comment since the author of the comment may not be the same as the author of the PR
81+
override predicate protectsCategoryAndEvent(string category, string event) {
82+
event = ["pull_request_target", "workflow_run"] and category = any_relevant_category()
83+
}
5984
}
6085

6186
abstract class RepositoryCheck extends ControlCheck {
62-
// repository checks are effective against pull requests since they can control where the code is coming from
63-
// they are not effective against issue_comment since the repository will always be the same
64-
// who you are (identity)
65-
override string getAProtectedEvent() { result = ["pull_request", "pull_request_target"] }
66-
67-
override boolean protectsAgainstRefMutationAttacks() { result = true }
87+
// checks that the origin of the code is the same as the repository.
88+
// for pull_requests, that means that it triggers only on local branches or repos from the same org
89+
// - they are effective against pull requests/workflow_run since they can control where the code is coming from
90+
// - they are not effective against issue_comment since the repository will always be the same
91+
override predicate protectsCategoryAndEvent(string category, string event) {
92+
event = ["pull_request_target", "workflow_run"] and category = any_relevant_category()
93+
}
6894
}
6995

7096
abstract class PermissionCheck extends ControlCheck {
71-
// permission checks are effective against pull requests since they can control who can make changes
72-
// they are not effective against issue_comment since the author of the comment may not be the same as the author of the PR
73-
// someone entitled to trigger the workflow with a comment, may no detect a malicious comment, or the comment may mutate after approval
74-
// who you are (identity)
75-
override string getAProtectedEvent() { result = ["pull_request", "pull_request_target"] }
76-
77-
override boolean protectsAgainstRefMutationAttacks() { result = true }
97+
// checks that the actor has a specific permission level
98+
// - they are effective against pull requests/workflow_run since they can control who can make changes
99+
// - they are not effective against issue_comment since the author of the comment may not be the same as the author of the PR
100+
override predicate protectsCategoryAndEvent(string category, string event) {
101+
event = ["pull_request_target", "workflow_run", "issue_comment"] and
102+
category = any_relevant_category()
103+
}
78104
}
79105

80106
abstract class LabelCheck extends ControlCheck {
81-
// does it protect injection attacks but not pwn requests?
82-
// pwn requests are susceptible to checkout of mutable code
83-
// but injection attacks are not, although a branch name can be changed after approval and perhaps also some other things
84-
// they do actually protext against untrusted code execution (sha)
85-
// what you have (approval)
86-
// TODO: A check should be a combination of:
87-
// - event type (pull_request, issue_comment, etc)
88-
// - category (untrusted mutable code, untrusted immutable code, code injection, etc)
89-
// - we dont know this unless we pass category to inPrivilegedContext and into ControlCheck.protects
90-
// - we can decide if a control check is effective based only on the ast node
91-
override string getAProtectedEvent() { result = ["pull_request", "pull_request_target"] }
92-
93-
// ref can be mutated after approval
94-
override boolean protectsAgainstRefMutationAttacks() { result = false }
107+
// checks if the issue/pull_request is labeled, which implies that it could have been approved
108+
// - they dont protect against mutation attacks
109+
override predicate protectsCategoryAndEvent(string category, string event) {
110+
event = ["pull_request_target", "workflow_run"] and category = any_non_toctou_category()
111+
}
95112
}
96113

97114
class EnvironmentCheck extends ControlCheck instanceof Environment {
98115
// Environment checks are not effective against any mutable attacks
99-
// they do actually protext against untrusted code execution (sha)
100-
// what you have (approval)
101-
EnvironmentCheck() { any() }
102-
103-
override string getAProtectedEvent() { result = ["pull_request", "pull_request_target"] }
116+
// they do actually protect against untrusted code execution (sha)
117+
override predicate protectsCategoryAndEvent(string category, string event) {
118+
event = ["pull_request_target", "workflow_run"] and category = any_non_toctou_category()
119+
}
120+
}
104121

105-
// ref can be mutated after approval
106-
override boolean protectsAgainstRefMutationAttacks() { result = false }
122+
abstract class CommentVsHeadDateCheck extends ControlCheck {
123+
override predicate protectsCategoryAndEvent(string category, string event) {
124+
// by itself, this check is not effective against any attacks
125+
none()
126+
}
107127
}
108128

109129
/* Specific implementations of control checks */
@@ -184,6 +204,12 @@ class AssociationActionCheck extends AssociationCheck instanceof UsesStep {
184204

185205
class PermissionActionCheck extends PermissionCheck instanceof UsesStep {
186206
PermissionActionCheck() {
207+
this.getCallee() = "sushichop/action-repository-permission" and
208+
this.getArgument("required-permission") = ["write", "admin"]
209+
or
210+
this.getCallee() = "prince-chrismc/check-actor-permissions-action" and
211+
this.getArgument("permission") = ["write", "admin"]
212+
or
187213
this.getCallee() = "lannonbr/repo-permission-check-action" and
188214
this.getArgument("permission") = ["write", "admin"]
189215
or
@@ -195,3 +221,13 @@ class PermissionActionCheck extends PermissionCheck instanceof UsesStep {
195221
)
196222
}
197223
}
224+
225+
class BashCommentVsHeadDateCheck extends CommentVsHeadDateCheck, Run {
226+
BashCommentVsHeadDateCheck() {
227+
exists(string line |
228+
line = this.getScript().splitAt("\n") and
229+
line.toLowerCase()
230+
.regexpMatch(".*date\\s+-d.*(commit_at|pushed_at|comment_at|commented_at).*date\\s+-d.*(commit_at|pushed_at|comment_at|commented_at).*")
231+
)
232+
}
233+
}

ql/lib/ext/config/externally_triggereable_events.yml

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -6,13 +6,14 @@ extensions:
66
- ["discussion"]
77
- ["discussion_comment"]
88
- ["fork"]
9+
- ["watch"]
910
- ["issue_comment"]
1011
- ["issues"]
11-
- ["pull_request"]
12+
- ["pull_request"] # non-privileged
1213
- ["pull_request_comment"]
1314
- ["pull_request_review"]
1415
- ["pull_request_review_comment"]
1516
- ["pull_request_target"]
16-
- ["workflow_run"] # depending on trigger workflow
17+
- ["workflow_run"] # depending on branch filter
1718
- ["workflow_call"] # depending on caller
1819

ql/src/Security/CWE-074/OutputClobberingHigh.ql

Lines changed: 13 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -16,16 +16,28 @@ import actions
1616
import codeql.actions.security.OutputClobberingQuery
1717
import codeql.actions.dataflow.ExternalFlow
1818
import OutputClobberingFlow::PathGraph
19+
import codeql.actions.security.ControlChecks
1920

2021
from OutputClobberingFlow::PathNode source, OutputClobberingFlow::PathNode sink
2122
where
2223
OutputClobberingFlow::flowPath(source, sink) and
2324
inPrivilegedContext(sink.getNode().asExpr()) and
2425
// exclude paths to file read sinks from non-artifact sources
2526
(
26-
not source.getNode().(RemoteFlowSource).getSourceType() = "artifact"
27+
not source.getNode().(RemoteFlowSource).getSourceType() = "artifact" and
28+
not exists(ControlCheck check |
29+
check
30+
.protects(sink.getNode().asExpr(),
31+
source.getNode().asExpr().getEnclosingJob().getATriggerEvent(), "code-injection")
32+
)
2733
or
2834
source.getNode().(RemoteFlowSource).getSourceType() = "artifact" and
35+
not exists(ControlCheck check |
36+
check
37+
.protects(sink.getNode().asExpr(),
38+
source.getNode().asExpr().getEnclosingJob().getATriggerEvent(),
39+
["untrusted-checkout", "artifact-poisoning"])
40+
) and
2941
(
3042
sink.getNode() instanceof OutputClobberingFromFileReadSink or
3143
sink.getNode() instanceof WorkflowCommandClobberingFromFileReadSink or

ql/src/Security/CWE-077/EnvPathInjectionCritical.ql

Lines changed: 13 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -15,15 +15,27 @@
1515
import actions
1616
import codeql.actions.security.EnvPathInjectionQuery
1717
import EnvPathInjectionFlow::PathGraph
18+
import codeql.actions.security.ControlChecks
1819

1920
from EnvPathInjectionFlow::PathNode source, EnvPathInjectionFlow::PathNode sink
2021
where
2122
EnvPathInjectionFlow::flowPath(source, sink) and
2223
inPrivilegedContext(sink.getNode().asExpr()) and
2324
(
24-
not source.getNode().(RemoteFlowSource).getSourceType() = "artifact"
25+
not source.getNode().(RemoteFlowSource).getSourceType() = "artifact" and
26+
not exists(ControlCheck check |
27+
check
28+
.protects(sink.getNode().asExpr(),
29+
source.getNode().asExpr().getEnclosingJob().getATriggerEvent(), "code-injection")
30+
)
2531
or
2632
source.getNode().(RemoteFlowSource).getSourceType() = "artifact" and
33+
not exists(ControlCheck check |
34+
check
35+
.protects(sink.getNode().asExpr(),
36+
source.getNode().asExpr().getEnclosingJob().getATriggerEvent(),
37+
["untrusted-checkout", "artifact-poisoning"])
38+
) and
2739
sink.getNode() instanceof EnvPathInjectionFromFileReadSink
2840
)
2941
select sink.getNode(), source, sink,

ql/src/Security/CWE-077/EnvVarInjectionCritical.ql

Lines changed: 18 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -16,16 +16,33 @@ import actions
1616
import codeql.actions.security.EnvVarInjectionQuery
1717
import codeql.actions.dataflow.ExternalFlow
1818
import EnvVarInjectionFlow::PathGraph
19+
import codeql.actions.security.ControlChecks
1920

2021
from EnvVarInjectionFlow::PathNode source, EnvVarInjectionFlow::PathNode sink
2122
where
2223
EnvVarInjectionFlow::flowPath(source, sink) and
2324
inPrivilegedContext(sink.getNode().asExpr()) and
25+
not exists(ControlCheck check |
26+
check
27+
.protects(sink.getNode().asExpr(),
28+
source.getNode().asExpr().getEnclosingJob().getATriggerEvent(), "envvar-injection")
29+
) and
2430
// exclude paths to file read sinks from non-artifact sources
2531
(
26-
not source.getNode().(RemoteFlowSource).getSourceType() = "artifact"
32+
not source.getNode().(RemoteFlowSource).getSourceType() = "artifact" and
33+
not exists(ControlCheck check |
34+
check
35+
.protects(sink.getNode().asExpr(),
36+
source.getNode().asExpr().getEnclosingJob().getATriggerEvent(), "code-injection")
37+
)
2738
or
2839
source.getNode().(RemoteFlowSource).getSourceType() = "artifact" and
40+
not exists(ControlCheck check |
41+
check
42+
.protects(sink.getNode().asExpr(),
43+
source.getNode().asExpr().getEnclosingJob().getATriggerEvent(),
44+
["untrusted-checkout", "artifact-poisoning"])
45+
) and
2946
(
3047
sink.getNode() instanceof EnvVarInjectionFromFileReadSink or
3148
madSink(sink.getNode(), "envvar-injection")

ql/src/Security/CWE-088/ArgumentInjectionCritical.ql

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -14,11 +14,17 @@
1414
import actions
1515
import codeql.actions.security.ArgumentInjectionQuery
1616
import ArgumentInjectionFlow::PathGraph
17+
import codeql.actions.security.ControlChecks
1718

1819
from ArgumentInjectionFlow::PathNode source, ArgumentInjectionFlow::PathNode sink
1920
where
2021
ArgumentInjectionFlow::flowPath(source, sink) and
21-
inPrivilegedContext(sink.getNode().asExpr())
22+
inPrivilegedContext(sink.getNode().asExpr()) and
23+
not exists(ControlCheck check |
24+
check
25+
.protects(sink.getNode().asExpr(),
26+
source.getNode().asExpr().getEnclosingJob().getATriggerEvent(), "argument-injection")
27+
)
2228
select sink.getNode(), source, sink,
2329
"Potential argument injection in $@ command, which may be controlled by an external user.", sink,
2430
sink.getNode().(ArgumentInjectionSink).getCommand()

ql/src/Security/CWE-094/CodeInjectionCritical.ql

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,11 +17,17 @@
1717
import actions
1818
import codeql.actions.security.CodeInjectionQuery
1919
import CodeInjectionFlow::PathGraph
20+
import codeql.actions.security.ControlChecks
2021

2122
from CodeInjectionFlow::PathNode source, CodeInjectionFlow::PathNode sink
2223
where
2324
CodeInjectionFlow::flowPath(source, sink) and
2425
inPrivilegedContext(sink.getNode().asExpr()) and
26+
not exists(ControlCheck check |
27+
check
28+
.protects(sink.getNode().asExpr(),
29+
source.getNode().asExpr().getEnclosingJob().getATriggerEvent(), "code-injection")
30+
) and
2531
// exclude cases where the sink is a JS script and the expression uses toJson
2632
not exists(UsesStep script |
2733
script.getCallee() = "actions/github-script" and

ql/src/Security/CWE-349/CachePoisoningViaCodeInjection.ql

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,9 @@ where
2626
// job can be triggered by an external user
2727
e.isExternallyTriggerable() and
2828
// the checkout is not controlled by an access check
29-
not exists(ControlCheck check | check.protects(source.getNode().asExpr(), j.getATriggerEvent())) and
29+
not exists(ControlCheck check |
30+
check.protects(source.getNode().asExpr(), j.getATriggerEvent(), "code-injection")
31+
) and
3032
// excluding privileged workflows since they can be exploited in easier circumstances
3133
not j.isPrivileged() and
3234
(

ql/src/Security/CWE-349/CachePoisoningViaDirectCache.ql

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -57,7 +57,9 @@ where
5757
path = source.(UntrustedArtifactDownloadStep).getPath()
5858
) and
5959
// the checkout/download is not controlled by an access check
60-
not exists(ControlCheck check | check.protects(source, j.getATriggerEvent())) and
60+
not exists(ControlCheck check |
61+
check.protects(source, j.getATriggerEvent(), ["untrusted-checkout", "artifact-poisoning"])
62+
) and
6163
j.getATriggerEvent() = e and
6264
// job can be triggered by an external user
6365
e.isExternallyTriggerable() and

0 commit comments

Comments
 (0)