Skip to content

Commit b2d7c82

Browse files
author
Alvaro Muñoz
authored
Merge pull request #25 from github/support_trigger_events
New `On` and `Event` classes
2 parents a30c2aa + 510cefe commit b2d7c82

File tree

12 files changed

+246
-49
lines changed

12 files changed

+246
-49
lines changed

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

Lines changed: 18 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -254,13 +254,13 @@ class Workflow extends AstNode instanceof WorkflowImpl {
254254

255255
Job getJob(string jobId) { result = super.getJob(jobId) }
256256

257-
predicate hasTriggerEvent(string trigger) { super.hasTriggerEvent(trigger) }
258-
259-
string getATriggerEvent() { result = super.getATriggerEvent() }
257+
Event getATriggerEvent() { result = super.getATriggerEvent() }
260258

261259
Permissions getPermissions() { result = super.getPermissions() }
262260

263261
Strategy getStrategy() { result = super.getStrategy() }
262+
263+
On getOn() { result = super.getOn() }
264264
}
265265

266266
class ReusableWorkflow extends Workflow instanceof ReusableWorkflowImpl {
@@ -305,6 +305,20 @@ class Needs extends AstNode instanceof NeedsImpl {
305305
Job getANeededJob() { result = super.getANeededJob() }
306306
}
307307

308+
class On extends AstNode instanceof OnImpl {
309+
Event getAnEvent() { result = super.getAnEvent() }
310+
}
311+
312+
class Event extends AstNode instanceof EventImpl {
313+
string getName() { result = super.getName() }
314+
315+
string getAnActivityType() { result = super.getAnActivityType() }
316+
317+
string getAPropertyValue(string prop) { result = super.getAPropertyValue(prop) }
318+
319+
predicate hasProperty(string prop) { super.hasProperty(prop) }
320+
}
321+
308322
/**
309323
* An Actions job within a workflow.
310324
* See https://docs.github.com/en/actions/reference/workflow-syntax-for-github-actions#jobs.
@@ -328,9 +342,7 @@ abstract class Job extends AstNode instanceof JobImpl {
328342

329343
Permissions getPermissions() { result = super.getPermissions() }
330344

331-
predicate hasTriggerEvent(string trigger) { super.hasTriggerEvent(trigger) }
332-
333-
string getATriggerEvent() { result = super.getATriggerEvent() }
345+
Event getATriggerEvent() { result = super.getATriggerEvent() }
334346

335347
Strategy getStrategy() { result = super.getStrategy() }
336348

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

Lines changed: 77 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -68,6 +68,16 @@ private newtype TAstNode =
6868
TStrategyNode(YamlMapping n) { exists(YamlMapping m | m.lookup("strategy") = n) } or
6969
TNeedsNode(YamlMappingLikeNode n) { exists(YamlMapping m | m.lookup("needs") = n) } or
7070
TJobNode(YamlMapping n) { exists(YamlMapping w | w.lookup("jobs").(YamlMapping).lookup(_) = n) } or
71+
TOnNode(YamlMappingLikeNode n) { exists(YamlMapping w | w.lookup("on") = n) } or
72+
TEventNode(YamlScalar event, YamlMappingLikeNode n) {
73+
exists(OnImpl o |
74+
o.getNode().(YamlMapping).maps(event, n)
75+
or
76+
o.getNode().(YamlSequence).getAChildNode() = event and event = n
77+
or
78+
o.getNode().(YamlScalar) = n and event = n
79+
)
80+
} or
7181
TStepNode(YamlMapping n) {
7282
exists(YamlMapping m | m.lookup("steps").(YamlSequence).getElementNode(_) = n)
7383
} or
@@ -308,6 +318,9 @@ class WorkflowImpl extends AstNodeImpl, TWorkflowNode {
308318

309319
override YamlMapping getNode() { result = n }
310320

321+
/** Gets the `on` trigger events for this workflow. */
322+
OnImpl getOn() { result.getNode() = n.lookup("on") }
323+
311324
/** Gets the 'global' `env` mapping in this workflow. */
312325
EnvImpl getEnv() { result.getNode() = n.lookup("env") }
313326

@@ -323,15 +336,8 @@ class WorkflowImpl extends AstNodeImpl, TWorkflowNode {
323336
/** Gets the permissions granted to this workflow. */
324337
PermissionsImpl getPermissions() { result.getNode() = n.lookup("permissions") }
325338

326-
/** Workflow is triggered by given trigger event */
327-
predicate hasTriggerEvent(string trigger) {
328-
exists(YamlNode y | y = n.lookup("on").(YamlMappingLikeNode).getNode(trigger))
329-
}
330-
331339
/** Gets the trigger event that starts this workflow. */
332-
string getATriggerEvent() {
333-
exists(YamlNode y | y = n.lookup("on").(YamlMappingLikeNode).getNode(result))
334-
}
340+
EventImpl getATriggerEvent() { this.getOn().getAnEvent() = result }
335341

336342
/** Gets the strategy for this workflow. */
337343
StrategyImpl getStrategy() { result.getNode() = n.lookup("strategy") }
@@ -573,6 +579,66 @@ class NeedsImpl extends AstNodeImpl, TNeedsNode {
573579
}
574580
}
575581

582+
class OnImpl extends AstNodeImpl, TOnNode {
583+
YamlMappingLikeNode n;
584+
585+
OnImpl() { this = TOnNode(n) }
586+
587+
override string toString() { result = n.toString() }
588+
589+
override AstNodeImpl getAChildNode() { result.getNode() = n.getAChildNode*() }
590+
591+
override WorkflowImpl getParentNode() { result.getAChildNode() = this }
592+
593+
override string getAPrimaryQlClass() { result = "OnImpl" }
594+
595+
override Location getLocation() { result = n.getLocation() }
596+
597+
override YamlMappingLikeNode getNode() { result = n }
598+
599+
/** Gets an event that triggers the workflow. */
600+
EventImpl getAnEvent() { result.getParentNode() = this }
601+
}
602+
603+
class EventImpl extends AstNodeImpl, TEventNode {
604+
YamlScalar e;
605+
YamlMappingLikeNode n;
606+
607+
EventImpl() { this = TEventNode(e, n) }
608+
609+
override string toString() { result = e.getValue() }
610+
611+
override AstNodeImpl getAChildNode() { result.getNode() = n.getAChildNode*() }
612+
613+
override OnImpl getParentNode() { result.getAChildNode() = this }
614+
615+
override string getAPrimaryQlClass() { result = "EventImpl" }
616+
617+
override Location getLocation() { result = e.getLocation() }
618+
619+
override YamlScalar getNode() { result = e }
620+
621+
/** Gets the name of the event that triggers the workflow. */
622+
string getName() { result = e.getValue() }
623+
624+
/** Gets the Yaml Node associated with the event if any */
625+
YamlMappingLikeNode getValueNode() { result = n }
626+
627+
/** Gets an activity type */
628+
string getAnActivityType() {
629+
result =
630+
n.(YamlMapping).lookup("types").(YamlMappingLikeNode).getNode(_).(YamlScalar).getValue()
631+
}
632+
633+
/** Gets a string value for any property (eg: branches, branches-ignore, etc.) */
634+
string getAPropertyValue(string prop) {
635+
result = n.(YamlMapping).lookup(prop).(YamlMappingLikeNode).getNode(_).(YamlScalar).getValue()
636+
}
637+
638+
/** Holds if the event has a property with the given name */
639+
predicate hasProperty(string prop) { exists(this.getAPropertyValue(prop)) }
640+
}
641+
576642
class JobImpl extends AstNodeImpl, TJobNode {
577643
YamlMapping n;
578644
string jobId;
@@ -686,7 +752,7 @@ class JobImpl extends AstNodeImpl, TJobNode {
686752
// For workflows that are triggered by the pull_request_target event, the GITHUB_TOKEN is granted read/write repository permission unless the permissions key is specified and the workflow can access secrets, even when it is triggered from a fork.
687753
// The Job is triggered by an event other than `pull_request`
688754
count(this.getATriggerEvent()) = 1 and
689-
not this.getATriggerEvent() = ["pull_request", "workflow_call"]
755+
not this.getATriggerEvent().getName() = ["pull_request", "workflow_call"]
690756
or
691757
// The Workflow is only triggered by `workflow_call` and there is
692758
// a caller workflow triggered by an event other than `pull_request`
@@ -701,16 +767,11 @@ class JobImpl extends AstNodeImpl, TJobNode {
701767
count(this.getATriggerEvent()) > 1
702768
}
703769

704-
/** Workflow is triggered by given trigger event */
705-
predicate hasTriggerEvent(string trigger) {
706-
exists(YamlNode y | y = n.lookup("on").(YamlMappingLikeNode).getNode(trigger))
707-
}
708-
709770
/** Gets the trigger event that starts this workflow. */
710-
string getATriggerEvent() { result = this.getEnclosingWorkflow().getATriggerEvent() }
771+
EventImpl getATriggerEvent() { result = this.getEnclosingWorkflow().getATriggerEvent() }
711772

712773
private predicate hasSingleTrigger(string trigger) {
713-
this.getATriggerEvent() = trigger and
774+
this.getATriggerEvent().getName() = trigger and
714775
count(this.getATriggerEvent()) = 1
715776
}
716777

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

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

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

1558
class CacheActionUsesStep extends CacheWritingStep, UsesStep {

‎ql/src/Security/CWE-284/CodeExecutionOnSelfHostedRunner.ql

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,7 @@ import codeql.actions.dataflow.ExternalFlow
2121
*/
2222
predicate staticallyIdentifiedSelfHostedRunner(Job job) {
2323
exists(string label |
24-
job.getEnclosingWorkflow().getATriggerEvent() =
24+
job.getEnclosingWorkflow().getATriggerEvent().getName() =
2525
["pull_request", "pull_request_review", "pull_request_review_comment", "pull_request_target"] and
2626
label = job.getARunsOnLabel() and
2727
// source: https://github.com/boostsecurityio/poutine/blob/main/opa/rego/poutine/utils.rego#L49C3-L49C136

‎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()

‎ql/test/library-tests/test.expected

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -98,6 +98,10 @@ runStepChildren
9898
| .github/workflows/test.yml:39:9:40:53 | Run Step: sink | .github/workflows/test.yml:40:14:40:52 | echo ${{needs.job1.outputs.job_output}} |
9999
parentNodes
100100
| .github/workflows/expression_nodes.yml:1:5:1:17 | issue_comment | .github/workflows/expression_nodes.yml:1:1:21:47 | on: issue_comment |
101+
| .github/workflows/expression_nodes.yml:1:5:1:17 | issue_comment | .github/workflows/expression_nodes.yml:1:1:21:47 | on: issue_comment |
102+
| .github/workflows/expression_nodes.yml:1:5:1:17 | issue_comment | .github/workflows/expression_nodes.yml:1:5:1:17 | issue_comment |
103+
| .github/workflows/expression_nodes.yml:1:5:1:17 | issue_comment | .github/workflows/expression_nodes.yml:1:5:1:17 | issue_comment |
104+
| .github/workflows/expression_nodes.yml:1:5:1:17 | issue_comment | .github/workflows/expression_nodes.yml:1:5:1:17 | issue_comment |
101105
| .github/workflows/expression_nodes.yml:5:5:21:47 | Job: echo-chamber | .github/workflows/expression_nodes.yml:1:1:21:47 | on: issue_comment |
102106
| .github/workflows/expression_nodes.yml:5:14:5:26 | ubuntu-latest | .github/workflows/expression_nodes.yml:1:1:21:47 | on: issue_comment |
103107
| .github/workflows/expression_nodes.yml:5:14:5:26 | ubuntu-latest | .github/workflows/expression_nodes.yml:5:5:21:47 | Job: echo-chamber |
@@ -136,8 +140,14 @@ parentNodes
136140
| .github/workflows/expression_nodes.yml:20:14:21:46 | LINE 1 echo '${{ github.event.comment.body }}' echo '${{github.event.issue.body}}' | .github/workflows/expression_nodes.yml:20:9:21:47 | Run Step |
137141
| .github/workflows/expression_nodes.yml:20:14:21:46 | github.event.comment.body | .github/workflows/expression_nodes.yml:20:14:21:46 | LINE 1 echo '${{ github.event.comment.body }}' echo '${{github.event.issue.body}}' |
138142
| .github/workflows/expression_nodes.yml:20:14:21:46 | github.event.issue.body | .github/workflows/expression_nodes.yml:20:14:21:46 | LINE 1 echo '${{ github.event.comment.body }}' echo '${{github.event.issue.body}}' |
143+
| .github/workflows/multiline.yml:2:3:2:14 | workflow_run | .github/workflows/multiline.yml:2:3:5:18 | workflow_run: |
144+
| .github/workflows/multiline.yml:2:3:5:18 | workflow_run: | .github/workflows/multiline.yml:1:1:33:14 | on: |
139145
| .github/workflows/multiline.yml:3:17:3:22 | Prev | .github/workflows/multiline.yml:1:1:33:14 | on: |
146+
| .github/workflows/multiline.yml:3:17:3:22 | Prev | .github/workflows/multiline.yml:2:3:2:14 | workflow_run |
147+
| .github/workflows/multiline.yml:3:17:3:22 | Prev | .github/workflows/multiline.yml:2:3:5:18 | workflow_run: |
140148
| .github/workflows/multiline.yml:5:9:5:17 | completed | .github/workflows/multiline.yml:1:1:33:14 | on: |
149+
| .github/workflows/multiline.yml:5:9:5:17 | completed | .github/workflows/multiline.yml:2:3:2:14 | workflow_run |
150+
| .github/workflows/multiline.yml:5:9:5:17 | completed | .github/workflows/multiline.yml:2:3:5:18 | workflow_run: |
141151
| .github/workflows/multiline.yml:9:5:33:14 | Job: Test | .github/workflows/multiline.yml:1:1:33:14 | on: |
142152
| .github/workflows/multiline.yml:9:14:9:26 | ubuntu-latest | .github/workflows/multiline.yml:1:1:33:14 | on: |
143153
| .github/workflows/multiline.yml:9:14:9:26 | ubuntu-latest | .github/workflows/multiline.yml:9:5:33:14 | Job: Test |
@@ -163,6 +173,10 @@ parentNodes
163173
| .github/workflows/multiline.yml:30:14:33:14 | cat <<-"EOF" > event.json\n ${{ toJson(github.event) }}\nEOF\n | .github/workflows/multiline.yml:30:9:33:14 | Run Step |
164174
| .github/workflows/multiline.yml:32:13:32:39 | toJson(github.event) | .github/workflows/multiline.yml:30:14:33:14 | cat <<-"EOF" > event.json\n ${{ toJson(github.event) }}\nEOF\n |
165175
| .github/workflows/test.yml:1:5:1:8 | push | .github/workflows/test.yml:1:1:40:53 | on: push |
176+
| .github/workflows/test.yml:1:5:1:8 | push | .github/workflows/test.yml:1:1:40:53 | on: push |
177+
| .github/workflows/test.yml:1:5:1:8 | push | .github/workflows/test.yml:1:5:1:8 | push |
178+
| .github/workflows/test.yml:1:5:1:8 | push | .github/workflows/test.yml:1:5:1:8 | push |
179+
| .github/workflows/test.yml:1:5:1:8 | push | .github/workflows/test.yml:1:5:1:8 | push |
166180
| .github/workflows/test.yml:5:5:31:2 | Job: job1 | .github/workflows/test.yml:1:1:40:53 | on: push |
167181
| .github/workflows/test.yml:5:14:5:26 | ubuntu-latest | .github/workflows/test.yml:1:1:40:53 | on: push |
168182
| .github/workflows/test.yml:5:14:5:26 | ubuntu-latest | .github/workflows/test.yml:5:5:31:2 | Job: job1 |

‎ql/test/query-tests/Security/CWE-349/.github/workflows/test13.yml

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,22 @@
1+
name: Cache Poisoning
2+
3+
on:
4+
pull_request_target:
5+
branches:
6+
- foo
7+
8+
permissions: read-all
9+
10+
jobs:
11+
poison:
12+
runs-on: ubuntu-latest
13+
steps:
14+
- uses: actions/checkout@v3
15+
with:
16+
ref: ${{ github.event.pull_request.head.sha }}
17+
- uses: actions/cache@v2
18+
with:
19+
path: ./poison
20+
key: poison_key
21+
- run: |
22+
cat poison

‎ql/test/query-tests/Security/CWE-349/.github/workflows/test14.yml

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,22 @@
1+
name: Cache Poisoning
2+
3+
on:
4+
pull_request_target:
5+
branches-ignore:
6+
- main
7+
8+
permissions: read-all
9+
10+
jobs:
11+
poison:
12+
runs-on: ubuntu-latest
13+
steps:
14+
- uses: actions/checkout@v3
15+
with:
16+
ref: ${{ github.event.pull_request.head.sha }}
17+
- uses: actions/cache@v2
18+
with:
19+
path: ./poison
20+
key: poison_key
21+
- run: |
22+
cat poison

‎ql/test/query-tests/Security/CWE-349/.github/workflows/test15.yml

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,22 @@
1+
name: Cache Poisoning
2+
3+
on:
4+
pull_request_target:
5+
branches:
6+
- main
7+
8+
permissions: read-all
9+
10+
jobs:
11+
poison:
12+
runs-on: ubuntu-latest
13+
steps:
14+
- uses: actions/checkout@v3
15+
with:
16+
ref: ${{ github.event.pull_request.head.sha }}
17+
- uses: actions/cache@v2
18+
with:
19+
path: ./poison
20+
key: poison_key
21+
- run: |
22+
cat poison

0 commit comments

Comments
 (0)