Skip to content

Commit fe06c9e

Browse files
author
Alvaro Muñoz
committed
d /Users/pwntester/src/github.com/github/codeql-actions/ql
1 parent 2bfb156 commit fe06c9e

15 files changed

+102
-81
lines changed

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

Lines changed: 57 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -417,8 +417,10 @@ class ReusableWorkflowImpl extends AstNodeImpl, WorkflowImpl {
417417
override AstNodeImpl getAChildNode() { result.getNode() = n.getAChildNode*() }
418418

419419
override EventImpl getATriggerEvent() {
420+
// The trigger event for a reusable workflow is the trigger event of the caller workflow
420421
this.getACaller().getEnclosingWorkflow().getOn().getAnEvent() = result
421422
or
423+
// or the trigger event of the workflow if it has any other than workflow_call
422424
this.getOn().getAnEvent() = result and not result.getName() = "workflow_call"
423425
}
424426

@@ -803,8 +805,13 @@ class JobImpl extends AstNodeImpl, TJobNode {
803805

804806
/** Gets the trigger event that starts this workflow. */
805807
EventImpl getATriggerEvent() {
806-
result = this.getEnclosingWorkflow().getATriggerEvent() or
807-
result = this.getEnclosingWorkflow().(ReusableWorkflowImpl).getACaller().getATriggerEvent()
808+
if this.getEnclosingWorkflow() instanceof ReusableWorkflowImpl
809+
then
810+
result = this.getEnclosingWorkflow().(ReusableWorkflowImpl).getACaller().getATriggerEvent()
811+
or
812+
result = this.getEnclosingWorkflow().getATriggerEvent() and
813+
not result.getName() = "workflow_call"
814+
else result = this.getEnclosingWorkflow().getATriggerEvent()
808815
}
809816

810817
/** Gets the runs-on field of the job. */
@@ -844,9 +851,8 @@ class JobImpl extends AstNodeImpl, TJobNode {
844851
)
845852
}
846853

847-
private predicate hasExplicitWritePermission() {
848-
// the job has an explicit write permission
849-
this.getPermissions().getAPermission().matches("%write")
854+
private predicate hasExplicitNonePermission() {
855+
exists(this.getPermissions()) and not exists(this.getPermissions().getAPermission())
850856
}
851857

852858
private predicate hasExplicitReadPermission() {
@@ -855,15 +861,57 @@ class JobImpl extends AstNodeImpl, TJobNode {
855861
not this.getPermissions().getAPermission().matches("%write")
856862
}
857863

858-
private predicate hasImplicitWritePermission() {
864+
private predicate hasExplicitWritePermission() {
859865
// the job has an explicit write permission
860-
this.getEnclosingWorkflow().getPermissions().getAPermission().matches("%write")
866+
this.getPermissions().getAPermission().matches("%write")
867+
}
868+
869+
private predicate hasImplicitNonePermission() {
870+
not exists(this.getPermissions()) and
871+
exists(this.getEnclosingWorkflow().getPermissions()) and
872+
not exists(this.getEnclosingWorkflow().getPermissions().getAPermission())
873+
or
874+
not exists(this.getPermissions()) and
875+
not exists(this.getEnclosingWorkflow().getPermissions()) and
876+
exists(this.getEnclosingWorkflow().(ReusableWorkflowImpl).getACaller().getPermissions()) and
877+
not exists(
878+
this.getEnclosingWorkflow()
879+
.(ReusableWorkflowImpl)
880+
.getACaller()
881+
.getPermissions()
882+
.getAPermission()
883+
)
861884
}
862885

863886
private predicate hasImplicitReadPermission() {
864887
// the job has not an explicit write permission
888+
not exists(this.getPermissions()) and
865889
exists(this.getEnclosingWorkflow().getPermissions().getAPermission()) and
866890
not this.getEnclosingWorkflow().getPermissions().getAPermission().matches("%write")
891+
or
892+
not exists(this.getPermissions()) and
893+
not exists(this.getEnclosingWorkflow().getPermissions()) and
894+
this.getEnclosingWorkflow()
895+
.(ReusableWorkflowImpl)
896+
.getACaller()
897+
.getPermissions()
898+
.getAPermission()
899+
.matches("%read")
900+
}
901+
902+
private predicate hasImplicitWritePermission() {
903+
// the job has an explicit write permission
904+
not exists(this.getPermissions()) and
905+
this.getEnclosingWorkflow().getPermissions().getAPermission().matches("%write")
906+
or
907+
not exists(this.getPermissions()) and
908+
not exists(this.getEnclosingWorkflow().getPermissions()) and
909+
this.getEnclosingWorkflow()
910+
.(ReusableWorkflowImpl)
911+
.getACaller()
912+
.getPermissions()
913+
.getAPermission()
914+
.matches("%write")
867915
}
868916

869917
private predicate hasRuntimeData() {
@@ -922,6 +970,8 @@ class JobImpl extends AstNodeImpl, TJobNode {
922970
// and the job is not explicitly non-privileged
923971
not (
924972
(
973+
this.hasExplicitNonePermission() or
974+
this.hasImplicitNonePermission() or
925975
this.hasExplicitReadPermission() or
926976
this.hasImplicitReadPermission()
927977
) and

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -38,7 +38,7 @@ abstract class ControlCheck extends AstNode {
3838
}
3939

4040
predicate protects(Step step, Event event, string category) {
41-
event.getEnclosingWorkflow() = step.getEnclosingWorkflow() and
41+
event = step.getEnclosingWorkflow().getATriggerEvent() and
4242
this.dominates(step) and
4343
this.protectsCategoryAndEvent(category, event.getName())
4444
}

‎ql/src/Security/CWE-829/UntrustedCheckoutCritical.ql

Lines changed: 11 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -20,39 +20,28 @@ import codeql.actions.security.ControlChecks
2020

2121
query predicate edges(Step a, Step b) { a.getNextStep() = b }
2222

23-
from PRHeadCheckoutStep checkout, PoisonableStep step
23+
from PRHeadCheckoutStep checkout, PoisonableStep step, Event event
2424
where
2525
// the checkout is followed by a known poisonable step
2626
checkout.getAFollowingStep() = step and
2727
// the checkout occurs in a privileged context
2828
inPrivilegedContext(checkout) and
29+
event = checkout.getEnclosingJob().getATriggerEvent() and
2930
(
3031
// issue_comment: check for date comparison checks and actor/access control checks
31-
exists(Event event |
32-
event = checkout.getEnclosingJob().getATriggerEvent() and
32+
event.getName() = "issue_comment" and
33+
not exists(ControlCheck check, CommentVsHeadDateCheck date_check |
3334
(
34-
event.getName() = "issue_comment"
35-
or
36-
event.getName() = "workflow_call" and
37-
checkout.getEnclosingWorkflow().(ReusableWorkflow).getACaller().getATriggerEvent().getName() =
38-
"issue_comment"
35+
check instanceof ActorCheck or
36+
check instanceof AssociationCheck or
37+
check instanceof PermissionCheck
3938
) and
40-
not exists(ControlCheck check, CommentVsHeadDateCheck date_check |
41-
(
42-
check instanceof ActorCheck or
43-
check instanceof AssociationCheck or
44-
check instanceof PermissionCheck
45-
) and
46-
check.dominates(checkout) and
47-
date_check.dominates(checkout)
48-
)
39+
check.dominates(checkout) and
40+
date_check.dominates(checkout)
4941
)
5042
or
5143
// not issue_comment triggered workflows
52-
exists(Event event |
53-
not event.getName() = "issue_comment" and
54-
event = checkout.getEnclosingJob().getATriggerEvent() and
55-
not exists(ControlCheck check | check.protects(checkout, event, "untrusted-checkout"))
56-
)
44+
not event.getName() = "issue_comment" and
45+
not exists(ControlCheck check | check.protects(checkout, event, "untrusted-checkout"))
5746
)
5847
select step, checkout, step, "Execution of untrusted code on a privileged workflow."

‎ql/src/Security/CWE-829/UntrustedCheckoutHigh.ql

Lines changed: 11 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -18,39 +18,28 @@ import codeql.actions.security.UntrustedCheckoutQuery
1818
import codeql.actions.security.PoisonableSteps
1919
import codeql.actions.security.ControlChecks
2020

21-
from PRHeadCheckoutStep checkout
21+
from PRHeadCheckoutStep checkout, Event event
2222
where
2323
// the checkout is NOT followed by a known poisonable step
2424
not checkout.getAFollowingStep() instanceof PoisonableStep and
2525
// the checkout occurs in a privileged context
2626
inPrivilegedContext(checkout) and
27+
event = checkout.getEnclosingJob().getATriggerEvent() and
2728
(
2829
// issue_comment: check for date comparison checks and actor/access control checks
29-
exists(Event event |
30-
event = checkout.getEnclosingJob().getATriggerEvent() and
30+
event.getName() = "issue_comment" and
31+
not exists(ControlCheck check, CommentVsHeadDateCheck date_check |
3132
(
32-
event.getName() = "issue_comment"
33-
or
34-
event.getName() = "workflow_call" and
35-
checkout.getEnclosingWorkflow().(ReusableWorkflow).getACaller().getATriggerEvent().getName() =
36-
"issue_comment"
33+
check instanceof ActorCheck or
34+
check instanceof AssociationCheck or
35+
check instanceof PermissionCheck
3736
) and
38-
not exists(ControlCheck check, CommentVsHeadDateCheck date_check |
39-
(
40-
check instanceof ActorCheck or
41-
check instanceof AssociationCheck or
42-
check instanceof PermissionCheck
43-
) and
44-
check.dominates(checkout) and
45-
date_check.dominates(checkout)
46-
)
37+
check.dominates(checkout) and
38+
date_check.dominates(checkout)
4739
)
4840
or
4941
// not issue_comment triggered workflows
50-
exists(Event event |
51-
not event.getName() = "issue_comment" and
52-
event = checkout.getEnclosingJob().getATriggerEvent() and
53-
not exists(ControlCheck check | check.protects(checkout, event, "untrusted-checkout"))
54-
)
42+
not event.getName() = "issue_comment" and
43+
not exists(ControlCheck check | check.protects(checkout, event, "untrusted-checkout"))
5544
)
5645
select checkout, "Potential execution of untrusted code on a privileged workflow."

‎ql/test/query-tests/Security/CWE-078/.github/workflows/documentation.yml

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@ name: Documentation
22

33
on:
44
workflow_dispatch:
5-
workflow_call:
5+
pull_request:
66

77
jobs:
88
parse_commit_info:

‎ql/test/query-tests/Security/CWE-078/CommandInjectionCritical.expected

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,6 @@
11
edges
22
nodes
33
| .github/workflows/comment_issue.yml:9:26:9:57 | github.event.comment.body | semmle.label | github.event.comment.body |
4-
| .github/workflows/documentation.yml:87:28:87:66 | github.event.head_commit.message | semmle.label | github.event.head_commit.message |
54
subpaths
65
#select
76
| .github/workflows/comment_issue.yml:9:26:9:57 | github.event.comment.body | .github/workflows/comment_issue.yml:9:26:9:57 | github.event.comment.body | .github/workflows/comment_issue.yml:9:26:9:57 | github.event.comment.body | Potential command injection in $@, which may be controlled by an external user. | .github/workflows/comment_issue.yml:9:26:9:57 | github.event.comment.body | ${{ github.event.comment.body }} |
8-
| .github/workflows/documentation.yml:87:28:87:66 | github.event.head_commit.message | .github/workflows/documentation.yml:87:28:87:66 | github.event.head_commit.message | .github/workflows/documentation.yml:87:28:87:66 | github.event.head_commit.message | Potential command injection in $@, which may be controlled by an external user. | .github/workflows/documentation.yml:87:28:87:66 | github.event.head_commit.message | ${{ github.event.head_commit.message }} |

‎ql/test/query-tests/Security/CWE-078/CommandInjectionMedium.expected

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,5 @@
11
edges
22
nodes
33
| .github/workflows/comment_issue.yml:9:26:9:57 | github.event.comment.body | semmle.label | github.event.comment.body |
4-
| .github/workflows/documentation.yml:87:28:87:66 | github.event.head_commit.message | semmle.label | github.event.head_commit.message |
54
subpaths
65
#select

‎ql/test/query-tests/Security/CWE-094/.github/workflows/reusable-workflow-caller-1.yml

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,11 @@
11
name: Caller
22

33
on:
4-
issue_comment:
4+
pull_request_target:
55

66
jobs:
77
test:
88
permissions: {}
99
uses: ./.github/workflows/reusable-workflow-1.yml
1010
with:
11-
taint: ${{ github.event.comment.body }}
11+
taint: ${{ github.event.pull_request.title }}

‎ql/test/query-tests/Security/CWE-094/.github/workflows/reusable-workflow-caller-2.yml

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,10 @@
11
name: Caller
22

33
on:
4-
issue_comment:
4+
pull_request_target:
55

66
jobs:
77
test:
88
uses: ./.github/workflows/reusable-workflow-2.yml
99
with:
10-
taint: ${{ github.event.comment.body }}
10+
taint: ${{ github.event.pull_request.title }}

‎ql/test/query-tests/Security/CWE-094/CodeInjectionCritical.expected

Lines changed: 5 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -70,8 +70,8 @@ edges
7070
| .github/workflows/reusable-workflow-1.yml:44:19:44:56 | github.event.pull_request.title | .github/workflows/reusable-workflow-1.yml:53:26:53:39 | env.log | provenance | |
7171
| .github/workflows/reusable-workflow-2.yml:6:7:6:11 | input taint | .github/workflows/reusable-workflow-2.yml:36:21:36:39 | inputs.taint | provenance | |
7272
| .github/workflows/reusable-workflow-2.yml:44:19:44:56 | github.event.pull_request.title | .github/workflows/reusable-workflow-2.yml:53:26:53:39 | env.log | provenance | |
73-
| .github/workflows/reusable-workflow-caller-1.yml:11:15:11:46 | github.event.comment.body | .github/workflows/reusable-workflow-1.yml:6:7:6:11 | input taint | provenance | |
74-
| .github/workflows/reusable-workflow-caller-2.yml:10:15:10:46 | github.event.comment.body | .github/workflows/reusable-workflow-2.yml:6:7:6:11 | input taint | provenance | |
73+
| .github/workflows/reusable-workflow-caller-1.yml:11:15:11:52 | github.event.pull_request.title | .github/workflows/reusable-workflow-1.yml:6:7:6:11 | input taint | provenance | |
74+
| .github/workflows/reusable-workflow-caller-2.yml:10:15:10:52 | github.event.pull_request.title | .github/workflows/reusable-workflow-2.yml:6:7:6:11 | input taint | provenance | |
7575
| .github/workflows/self_needs.yml:11:7:12:4 | Job outputs node [job_output] | .github/workflows/self_needs.yml:20:15:20:51 | needs.test1.outputs.job_output | provenance | |
7676
| .github/workflows/self_needs.yml:11:20:11:52 | steps.source.outputs.value | .github/workflows/self_needs.yml:11:7:12:4 | Job outputs node [job_output] | provenance | |
7777
| .github/workflows/self_needs.yml:13:9:19:6 | Uses Step: source [value] | .github/workflows/self_needs.yml:11:20:11:52 | steps.source.outputs.value | provenance | |
@@ -287,8 +287,8 @@ nodes
287287
| .github/workflows/reusable-workflow-2.yml:36:21:36:39 | inputs.taint | semmle.label | inputs.taint |
288288
| .github/workflows/reusable-workflow-2.yml:44:19:44:56 | github.event.pull_request.title | semmle.label | github.event.pull_request.title |
289289
| .github/workflows/reusable-workflow-2.yml:53:26:53:39 | env.log | semmle.label | env.log |
290-
| .github/workflows/reusable-workflow-caller-1.yml:11:15:11:46 | github.event.comment.body | semmle.label | github.event.comment.body |
291-
| .github/workflows/reusable-workflow-caller-2.yml:10:15:10:46 | github.event.comment.body | semmle.label | github.event.comment.body |
290+
| .github/workflows/reusable-workflow-caller-1.yml:11:15:11:52 | github.event.pull_request.title | semmle.label | github.event.pull_request.title |
291+
| .github/workflows/reusable-workflow-caller-2.yml:10:15:10:52 | github.event.pull_request.title | semmle.label | github.event.pull_request.title |
292292
| .github/workflows/self_needs.yml:11:7:12:4 | Job outputs node [job_output] | semmle.label | Job outputs node [job_output] |
293293
| .github/workflows/self_needs.yml:11:20:11:52 | steps.source.outputs.value | semmle.label | steps.source.outputs.value |
294294
| .github/workflows/self_needs.yml:13:9:19:6 | Uses Step: source [value] | semmle.label | Uses Step: source [value] |
@@ -451,9 +451,7 @@ subpaths
451451
| .github/workflows/pull_request_target.yml:14:19:14:69 | github.event.pull_request.head.repo.homepage | .github/workflows/pull_request_target.yml:14:19:14:69 | github.event.pull_request.head.repo.homepage | .github/workflows/pull_request_target.yml:14:19:14:69 | github.event.pull_request.head.repo.homepage | Potential code injection in $@, which may be controlled by an external user. | .github/workflows/pull_request_target.yml:14:19:14:69 | github.event.pull_request.head.repo.homepage | ${{ github.event.pull_request.head.repo.homepage }} |
452452
| .github/workflows/pull_request_target.yml:15:19:15:59 | github.event.pull_request.head.ref | .github/workflows/pull_request_target.yml:15:19:15:59 | github.event.pull_request.head.ref | .github/workflows/pull_request_target.yml:15:19:15:59 | github.event.pull_request.head.ref | Potential code injection in $@, which may be controlled by an external user. | .github/workflows/pull_request_target.yml:15:19:15:59 | github.event.pull_request.head.ref | ${{ github.event.pull_request.head.ref }} |
453453
| .github/workflows/pull_request_target.yml:16:19:16:40 | github.head_ref | .github/workflows/pull_request_target.yml:16:19:16:40 | github.head_ref | .github/workflows/pull_request_target.yml:16:19:16:40 | github.head_ref | Potential code injection in $@, which may be controlled by an external user. | .github/workflows/pull_request_target.yml:16:19:16:40 | github.head_ref | ${{ github.head_ref }} |
454-
| .github/workflows/reusable-workflow-1.yml:36:21:36:39 | inputs.taint | .github/workflows/reusable-workflow-caller-1.yml:11:15:11:46 | github.event.comment.body | .github/workflows/reusable-workflow-1.yml:36:21:36:39 | inputs.taint | Potential code injection in $@, which may be controlled by an external user. | .github/workflows/reusable-workflow-1.yml:36:21:36:39 | inputs.taint | ${{ inputs.taint }} |
455-
| .github/workflows/reusable-workflow-1.yml:53:26:53:39 | env.log | .github/workflows/reusable-workflow-1.yml:44:19:44:56 | github.event.pull_request.title | .github/workflows/reusable-workflow-1.yml:53:26:53:39 | env.log | Potential code injection in $@, which may be controlled by an external user. | .github/workflows/reusable-workflow-1.yml:53:26:53:39 | env.log | ${{ env.log }} |
456-
| .github/workflows/reusable-workflow-2.yml:36:21:36:39 | inputs.taint | .github/workflows/reusable-workflow-caller-2.yml:10:15:10:46 | github.event.comment.body | .github/workflows/reusable-workflow-2.yml:36:21:36:39 | inputs.taint | Potential code injection in $@, which may be controlled by an external user. | .github/workflows/reusable-workflow-2.yml:36:21:36:39 | inputs.taint | ${{ inputs.taint }} |
454+
| .github/workflows/reusable-workflow-2.yml:36:21:36:39 | inputs.taint | .github/workflows/reusable-workflow-caller-2.yml:10:15:10:52 | github.event.pull_request.title | .github/workflows/reusable-workflow-2.yml:36:21:36:39 | inputs.taint | Potential code injection in $@, which may be controlled by an external user. | .github/workflows/reusable-workflow-2.yml:36:21:36:39 | inputs.taint | ${{ inputs.taint }} |
457455
| .github/workflows/reusable-workflow-2.yml:53:26:53:39 | env.log | .github/workflows/reusable-workflow-2.yml:44:19:44:56 | github.event.pull_request.title | .github/workflows/reusable-workflow-2.yml:53:26:53:39 | env.log | Potential code injection in $@, which may be controlled by an external user. | .github/workflows/reusable-workflow-2.yml:53:26:53:39 | env.log | ${{ env.log }} |
458456
| .github/workflows/self_needs.yml:19:15:19:47 | steps.source.outputs.value | .github/workflows/self_needs.yml:16:20:16:57 | github.event['comment']['body'] | .github/workflows/self_needs.yml:19:15:19:47 | steps.source.outputs.value | Potential code injection in $@, which may be controlled by an external user. | .github/workflows/self_needs.yml:19:15:19:47 | steps.source.outputs.value | ${{ steps.source.outputs.value }} |
459457
| .github/workflows/self_needs.yml:20:15:20:51 | needs.test1.outputs.job_output | .github/workflows/self_needs.yml:16:20:16:57 | github.event['comment']['body'] | .github/workflows/self_needs.yml:20:15:20:51 | needs.test1.outputs.job_output | Potential code injection in $@, which may be controlled by an external user. | .github/workflows/self_needs.yml:20:15:20:51 | needs.test1.outputs.job_output | ${{ needs.test1.outputs.job_output }} |

0 commit comments

Comments
 (0)