Skip to content

Commit 8590a0b

Browse files
author
Alvaro Muñoz
committed
Refactor runOnDefaultBranch
1 parent 4d61204 commit 8590a0b

File tree

3 files changed

+53
-26
lines changed

3 files changed

+53
-26
lines changed

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

Lines changed: 50 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,56 @@ string defaultBranchTriggerEvent() {
1010
]
1111
}
1212

13+
predicate test(Event e) {
14+
e.getName() = "pull_request_target" and
15+
// branches and branches-ignore filters
16+
e.hasProperty("branches") and
17+
e.hasProperty("branches-ignore") and
18+
e.getAPropertyValue("branches") = ["main", "master", "default"] and
19+
not e.getAPropertyValue("branches-ignore") = ["main", "master", "default"]
20+
}
21+
22+
predicate runsOnDefaultBranch(Job j) {
23+
exists(Event e |
24+
j.getATriggerEvent() = e and
25+
(
26+
e.getName() = defaultBranchTriggerEvent() and
27+
not e.getName() = "pull_request_target"
28+
or
29+
e.getName() = "push" and
30+
e.getAPropertyValue("branches") = ["main", "master", "default"]
31+
or
32+
e.getName() = "pull_request_target" and
33+
(
34+
// no filtering
35+
not e.hasProperty("branches") and not e.hasProperty("branches-ignore")
36+
or
37+
// only branches-ignore filter
38+
e.hasProperty("branches-ignore") and
39+
not e.hasProperty("branches") and
40+
not e.getAPropertyValue("branches-ignore") = ["main", "master", "default"]
41+
or
42+
// only branches filter
43+
e.hasProperty("branches") and
44+
not e.hasProperty("branches-ignore") and
45+
e.getAPropertyValue("branches") = ["main", "master", "default"]
46+
or
47+
// branches and branches-ignore filters
48+
e.hasProperty("branches") and
49+
e.hasProperty("branches-ignore") and
50+
e.getAPropertyValue("branches") = ["main", "master", "default"] and
51+
not e.getAPropertyValue("branches-ignore") = ["main", "master", "default"]
52+
)
53+
)
54+
)
55+
or
56+
j.getATriggerEvent().getName() = "workflow_call" and
57+
exists(ExternalJob call |
58+
call.getCallee() = j.getLocation().getFile().getRelativePath() and
59+
runsOnDefaultBranch(call)
60+
)
61+
}
62+
1363
abstract class CacheWritingStep extends Step { }
1464

1565
class CacheActionUsesStep extends CacheWritingStep, UsesStep {

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

Lines changed: 1 addition & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -21,19 +21,7 @@ where
2121
// Excluding privileged workflows since they can be easily exploited in similar circumstances
2222
not j.isPrivileged() and
2323
// The workflow runs in the context of the default branch
24-
// TODO: (require to collect trigger types)
25-
// - add push to default branch?
26-
// - exclude pull_request_target when branches_ignore includes default branch or when branches does not include the default branch
27-
(
28-
j.getEnclosingWorkflow().hasTriggerEvent(defaultBranchTriggerEvent())
29-
or
30-
j.getEnclosingWorkflow().hasTriggerEvent("workflow_call") and
31-
exists(ExternalJob call, Workflow caller |
32-
call.getCallee() = j.getLocation().getFile().getRelativePath() and
33-
caller = call.getWorkflow() and
34-
caller.hasTriggerEvent(defaultBranchTriggerEvent())
35-
)
36-
) and
24+
runsOnDefaultBranch(j) and
3725
// The job checkouts untrusted code from a pull request
3826
j.getAStep() = checkout and
3927
(

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

Lines changed: 2 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -21,21 +21,10 @@ from CodeInjectionFlow::PathNode source, CodeInjectionFlow::PathNode sink, Local
2121
where
2222
CodeInjectionFlow::flowPath(source, sink) and
2323
j = sink.getNode().asExpr().getEnclosingJob() and
24+
// Excluding privileged workflows since they can be easily exploited in similar circumstances
2425
not j.isPrivileged() and
2526
// The workflow runs in the context of the default branch
26-
// TODO: (require to collect trigger types)
27-
// - add push to default branch?
28-
// - exclude pull_request_target when branches_ignore includes default branch or when branches does not include the default branch
29-
(
30-
j.getEnclosingWorkflow().hasTriggerEvent(defaultBranchTriggerEvent())
31-
or
32-
j.getEnclosingWorkflow().hasTriggerEvent("workflow_call") and
33-
exists(ExternalJob call, Workflow caller |
34-
call.getCallee() = j.getLocation().getFile().getRelativePath() and
35-
caller = call.getWorkflow() and
36-
caller.hasTriggerEvent(defaultBranchTriggerEvent())
37-
)
38-
)
27+
runsOnDefaultBranch(j)
3928
select sink.getNode(), source, sink,
4029
"Unprivileged code injection in $@, which may lead to cache poisoning.", sink,
4130
sink.getNode().asExpr().(Expression).getRawExpression()

0 commit comments

Comments
 (0)