Skip to content

Commit 88465bd

Browse files
author
Alvaro Muñoz
committed
Improve privleged detection
1 parent 844b6e0 commit 88465bd

File tree

7 files changed

+138
-135
lines changed

7 files changed

+138
-135
lines changed

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

Lines changed: 9 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -77,6 +77,8 @@ class CompositeAction extends AstNode instanceof CompositeActionImpl {
7777
LocalJob getACaller() { result = super.getACaller() }
7878

7979
predicate isPrivileged() { super.isPrivileged() }
80+
81+
predicate isPrivilegedExternallyTriggerable() { super.isPrivilegedExternallyTriggerable() }
8082
}
8183

8284
/**
@@ -169,6 +171,10 @@ class Event extends AstNode instanceof EventImpl {
169171
string getAPropertyValue(string prop) { result = super.getAPropertyValue(prop) }
170172

171173
predicate hasProperty(string prop) { super.hasProperty(prop) }
174+
175+
predicate isExternallyTriggerable() { super.isExternallyTriggerable() }
176+
177+
predicate isPrivileged() { super.isPrivileged() }
172178
}
173179

174180
/**
@@ -198,11 +204,11 @@ abstract class Job extends AstNode instanceof JobImpl {
198204

199205
Strategy getStrategy() { result = super.getStrategy() }
200206

201-
predicate isPrivileged() { super.isPrivileged() }
207+
string getARunsOnLabel() { result = super.getARunsOnLabel() }
202208

203-
predicate isExternallyTriggerable() { super.isExternallyTriggerable() }
209+
predicate isPrivileged() { super.isPrivileged() }
204210

205-
string getARunsOnLabel() { result = super.getARunsOnLabel() }
211+
predicate isPrivilegedExternallyTriggerable() { super.isPrivilegedExternallyTriggerable() }
206212
}
207213

208214
class LocalJob extends Job instanceof LocalJobImpl {

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

Lines changed: 10 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -210,54 +210,28 @@ predicate writeToGitHubPath(Run run, string content) {
210210

211211
predicate inPrivilegedCompositeAction(AstNode node) {
212212
exists(CompositeAction a |
213-
// node is in a privileged composite action
214213
a = node.getEnclosingCompositeAction() and
215-
(
216-
a.isPrivileged()
217-
or
218-
exists(Job caller |
219-
caller = a.getACaller() and
220-
caller.isPrivileged() and
221-
caller.isExternallyTriggerable()
222-
)
223-
)
224-
)
225-
}
226-
227-
predicate inPrivilegedExternallyTriggerableJob(AstNode node) {
228-
exists(Job j |
229-
// node is in a privileged and externally triggereable job
230-
j = node.getEnclosingJob() and
231-
// job is privileged (write access or access to secrets)
232-
j.isPrivileged() and
233-
// job is triggereable by an external user
234-
j.isExternallyTriggerable()
214+
a.isPrivilegedExternallyTriggerable()
235215
)
236216
}
237217

238218
predicate inNonPrivilegedCompositeAction(AstNode node) {
239219
exists(CompositeAction a |
240-
// node is in a non-privileged composite action
241220
a = node.getEnclosingCompositeAction() and
242-
not a.isPrivileged() and
243-
not exists(LocalJob caller |
244-
caller = a.getACaller() and
245-
caller.isPrivileged() and
246-
caller.isExternallyTriggerable()
247-
)
221+
not a.isPrivilegedExternallyTriggerable()
222+
)
223+
}
224+
225+
predicate inPrivilegedExternallyTriggerableJob(AstNode node) {
226+
exists(Job j |
227+
j = node.getEnclosingJob() and
228+
j.isPrivilegedExternallyTriggerable()
248229
)
249230
}
250231

251232
predicate inNonPrivilegedJob(AstNode node) {
252233
exists(Job j |
253-
// node is in a non-privileged or not externally triggereable job
254234
j = node.getEnclosingJob() and
255-
(
256-
// job is non-privileged (no write access and no access to secrets)
257-
not j.isPrivileged()
258-
or
259-
// job is triggereable by an external user
260-
not j.isExternallyTriggerable()
261-
)
235+
not j.isPrivilegedExternallyTriggerable()
262236
)
263237
}

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

Lines changed: 116 additions & 93 deletions
Original file line numberDiff line numberDiff line change
@@ -317,6 +317,18 @@ class CompositeActionImpl extends AstNodeImpl, TCompositeAction {
317317
)
318318
}
319319

320+
private predicate hasExplicitSecretAccess() {
321+
// the job accesses a secret other than GITHUB_TOKEN
322+
exists(SecretsExpressionImpl expr |
323+
expr.getEnclosingCompositeAction() = this and not expr.getFieldName() = "GITHUB_TOKEN"
324+
)
325+
}
326+
327+
private predicate hasExplicitWritePermission() {
328+
// a calling job has an explicit write permission
329+
this.getACaller().getPermissions().getAPermission().matches("%write")
330+
}
331+
320332
/** Holds if the action is privileged. */
321333
predicate isPrivileged() {
322334
// there is a calling job that defines explicit write permissions
@@ -326,19 +338,24 @@ class CompositeActionImpl extends AstNodeImpl, TCompositeAction {
326338
this.hasExplicitSecretAccess()
327339
or
328340
// there is a privileged caller job
329-
this.getACaller().isPrivileged()
330-
}
331-
332-
private predicate hasExplicitSecretAccess() {
333-
// the job accesses a secret other than GITHUB_TOKEN
334-
exists(SecretsExpressionImpl expr |
335-
expr.getEnclosingCompositeAction() = this and not expr.getFieldName() = "GITHUB_TOKEN"
341+
(
342+
this.getACaller().isPrivileged()
343+
or
344+
not this.getACaller().isPrivileged() and
345+
this.getACaller().getATriggerEvent().isPrivileged()
336346
)
337347
}
338348

339-
private predicate hasExplicitWritePermission() {
340-
// a calling job has an explicit write permission
341-
this.getACaller().getPermissions().getAPermission().matches("%write")
349+
/** Holds if the action is privileged and externally triggerable. */
350+
predicate isPrivilegedExternallyTriggerable() {
351+
// the action is externally triggerable
352+
exists(JobImpl caller, EventImpl event |
353+
caller = this.getACaller() and
354+
event = caller.getATriggerEvent() and
355+
event.isExternallyTriggerable() and
356+
// the action is privileged
357+
(this.isPrivileged() or caller.isPrivileged())
358+
)
342359
}
343360
}
344361

@@ -688,6 +705,42 @@ class EventImpl extends AstNodeImpl, TEventNode {
688705

689706
/** Holds if the event has a property with the given name */
690707
predicate hasProperty(string prop) { exists(this.getAPropertyValue(prop)) }
708+
709+
/** Holds if the event can be triggered by an external actor. */
710+
predicate isExternallyTriggerable() {
711+
// the job is triggered by an event that can be triggered externally
712+
externallyTriggerableEventsDataModel(this.getName())
713+
or
714+
// the event is `workflow_call` and there is a caller workflow that can be triggered externally
715+
this.getName() = "workflow_call" and
716+
(
717+
// there are hints that this workflow is meant to be called by external triggers
718+
exists(ExpressionImpl expr, string external_trigger |
719+
expr.getEnclosingWorkflow() = this.getEnclosingWorkflow() and
720+
expr.getExpression().matches("%github.event" + external_trigger + "%") and
721+
externallyTriggerableEventsDataModel(external_trigger)
722+
)
723+
or
724+
this.getEnclosingWorkflow()
725+
.(ReusableWorkflowImpl)
726+
.getACaller()
727+
.getATriggerEvent()
728+
.isExternallyTriggerable()
729+
)
730+
}
731+
732+
predicate isPrivileged() {
733+
// the Job is triggered by an event other than `pull_request`, or `workflow_call`
734+
not this.getName() = "pull_request" and
735+
not this.getName() = "workflow_call"
736+
or
737+
// Reusable Workflow with a privileged caller or we cant find a caller
738+
this.getName() = "workflow_call" and
739+
(
740+
this.getEnclosingWorkflow().(ReusableWorkflowImpl).getACaller().isPrivileged() or
741+
not exists(this.getEnclosingWorkflow().(ReusableWorkflowImpl).getACaller())
742+
)
743+
}
691744
}
692745

693746
class JobImpl extends AstNodeImpl, TJobNode {
@@ -746,45 +799,41 @@ class JobImpl extends AstNodeImpl, TJobNode {
746799
/** Gets the strategy for this job. */
747800
StrategyImpl getStrategy() { result.getNode() = n.lookup("strategy") }
748801

749-
/** Holds if the job can be triggered by an external actor. */
750-
predicate isExternallyTriggerable() {
751-
// the job is triggered by an event that can be triggered externally
752-
externallyTriggerableEventsDataModel(this.getATriggerEvent().getName())
753-
or
754-
// the job is triggered by a workflow_call event that can be triggered externally
755-
this.getATriggerEvent().getName() = "workflow_call" and
756-
(
757-
exists(ExpressionImpl e, string external_trigger |
758-
e.getEnclosingJob() = this and
759-
e.getExpression().matches("%github.event" + external_trigger + "%") and
760-
externallyTriggerableEventsDataModel(external_trigger)
802+
/** Gets the trigger event that starts this workflow. */
803+
EventImpl getATriggerEvent() { result = this.getEnclosingWorkflow().getATriggerEvent() }
804+
805+
// private predicate hasSingleTrigger(string trigger) {
806+
// this.getATriggerEvent().getName() = trigger and
807+
// count(this.getATriggerEvent()) = 1
808+
// }
809+
/** Gets the runs-on field of the job. */
810+
string getARunsOnLabel() {
811+
exists(ScalarValueImpl lbl, YamlMappingLikeNode runson |
812+
runson = n.lookup("runs-on").(YamlMappingLikeNode)
813+
|
814+
(
815+
lbl.getNode() = runson.getNode(_) and
816+
not lbl.getNode() = runson.getNode("group")
817+
or
818+
lbl.getNode() = runson.getNode("labels").(YamlMappingLikeNode).getNode(_)
819+
) and
820+
(
821+
not exists(MatrixExpressionImpl e | e.getParentNode() = lbl) and
822+
result =
823+
lbl.getValue()
824+
.trim()
825+
.regexpReplaceAll("^('|\")", "")
826+
.regexpReplaceAll("('|\")$", "")
827+
.trim()
828+
or
829+
exists(MatrixExpressionImpl e |
830+
e.getParentNode() = lbl and
831+
result = e.getLiteralValues()
832+
)
761833
)
762-
or
763-
this.getEnclosingWorkflow().(ReusableWorkflowImpl).getACaller().isExternallyTriggerable()
764834
)
765835
}
766836

767-
/** Holds if the job is privileged. */
768-
predicate isPrivileged() {
769-
// the job has privileged runtime permissions
770-
this.hasRuntimeWritePermissions()
771-
or
772-
// the job has an explicit secret accesses
773-
this.hasExplicitSecretAccess()
774-
or
775-
// the job has an explicit write permission
776-
this.hasExplicitWritePermission()
777-
or
778-
// the job has no explicit permissions but the workflow has write permissions
779-
not exists(this.getPermissions()) and
780-
this.hasImplicitWritePermission()
781-
or
782-
// neither the job nor the workflow have permissions but the job has a privileged trigger
783-
not exists(this.getPermissions()) and
784-
not exists(this.getEnclosingWorkflow().getPermissions()) and
785-
this.hasPrivilegedTrigger()
786-
}
787-
788837
private predicate hasExplicitSecretAccess() {
789838
// the job accesses a secret other than GITHUB_TOKEN
790839
exists(SecretsExpressionImpl expr |
@@ -817,60 +866,34 @@ class JobImpl extends AstNodeImpl, TJobNode {
817866
)
818867
}
819868

820-
private predicate hasPrivilegedTrigger() {
821-
// the Job is triggered by an event other than `pull_request`, `push`, or `workflow_call`
822-
count(this.getATriggerEvent()) = 1 and
823-
not this.getATriggerEvent().getName() = "push" and
824-
not this.getATriggerEvent().getName() = "pull_request" and
825-
not this.getATriggerEvent().getName() = "workflow_call"
869+
/** Holds if the job is privileged. */
870+
predicate isPrivileged() {
871+
// the job has privileged runtime permissions
872+
this.hasRuntimeWritePermissions()
826873
or
827-
// the Workflow is a Reusable Workflow only and there is
828-
// a privileged caller workflow or we cant find a caller
829-
count(this.getATriggerEvent()) = 1 and
830-
this.getATriggerEvent().getName() = "workflow_call" and
831-
(
832-
this.getEnclosingWorkflow().(ReusableWorkflowImpl).getACaller().isPrivileged() or
833-
not exists(this.getEnclosingWorkflow().(ReusableWorkflowImpl).getACaller())
834-
)
874+
// the job has an explicit secret accesses
875+
this.hasExplicitSecretAccess()
835876
or
836-
// the Job is triggered by an event other than `push`, `pull_request`, or `workflow_call`
837-
exists(string event |
838-
this.getATriggerEvent().getName() = event and
839-
not event = ["push", "pull_request", "workflow_call"]
840-
)
877+
// the job has an explicit write permission
878+
this.hasExplicitWritePermission()
879+
or
880+
// the job has no explicit permissions but the workflow has write permissions
881+
not exists(this.getPermissions()) and
882+
this.hasImplicitWritePermission()
841883
}
842884

843-
/** Gets the trigger event that starts this workflow. */
844-
EventImpl getATriggerEvent() { result = this.getEnclosingWorkflow().getATriggerEvent() }
845-
846-
// private predicate hasSingleTrigger(string trigger) {
847-
// this.getATriggerEvent().getName() = trigger and
848-
// count(this.getATriggerEvent()) = 1
849-
// }
850-
/** Gets the runs-on field of the job. */
851-
string getARunsOnLabel() {
852-
exists(ScalarValueImpl lbl, YamlMappingLikeNode runson |
853-
runson = n.lookup("runs-on").(YamlMappingLikeNode)
854-
|
885+
/** Holds if the action is privileged and externally triggerable. */
886+
predicate isPrivilegedExternallyTriggerable() {
887+
exists(EventImpl e |
888+
// job is triggereable by an external user
889+
this.getATriggerEvent() = e and
890+
e.isExternallyTriggerable() and
891+
// job is privileged (write access or access to secrets)
855892
(
856-
lbl.getNode() = runson.getNode(_) and
857-
not lbl.getNode() = runson.getNode("group")
893+
this.isPrivileged()
858894
or
859-
lbl.getNode() = runson.getNode("labels").(YamlMappingLikeNode).getNode(_)
860-
) and
861-
(
862-
not exists(MatrixExpressionImpl e | e.getParentNode() = lbl) and
863-
result =
864-
lbl.getValue()
865-
.trim()
866-
.regexpReplaceAll("^('|\")", "")
867-
.regexpReplaceAll("('|\")$", "")
868-
.trim()
869-
or
870-
exists(MatrixExpressionImpl e |
871-
e.getParentNode() = lbl and
872-
result = e.getLiteralValues()
873-
)
895+
not this.isPrivileged() and
896+
e.isPrivileged()
874897
)
875898
)
876899
}

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,7 @@ where
2424
// TODO: Consider adding artifact downloads as a potential source of cache poisoning
2525
j.getAStep() = checkout and
2626
// job can be triggered by an external user
27-
j.isExternallyTriggerable() and
27+
j.getATriggerEvent().isExternallyTriggerable() and
2828
(
2929
// the job writes to the cache
3030
// (No need to follow the checkout step as the cache writing is normally done after the job completes)

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,7 @@ where
2222
CodeInjectionFlow::flowPath(source, sink) and
2323
j = sink.getNode().asExpr().getEnclosingJob() and
2424
// job can be triggered by an external user
25-
j.isExternallyTriggerable() and
25+
j.getATriggerEvent().isExternallyTriggerable() and
2626
// excluding privileged workflows since they can be easily exploited in similar circumstances
2727
not j.isPrivileged() and
2828
// The workflow runs in the context of the default branch

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

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -261,7 +261,6 @@ nodes
261261
subpaths
262262
#select
263263
| .github/actions/action1/action.yml:7:19:7:55 | github.event.pull_request.body | .github/actions/action1/action.yml:7:19:7:55 | github.event.pull_request.body | .github/actions/action1/action.yml:7:19:7:55 | github.event.pull_request.body | Potential code injection in $@, which may be controlled by an external user. | .github/actions/action1/action.yml:7:19:7:55 | github.event.pull_request.body | ${{ github.event.pull_request.body }} |
264-
| .github/actions/action3/action.yml:9:19:9:55 | github.event.pull_request.body | .github/actions/action3/action.yml:9:19:9:55 | github.event.pull_request.body | .github/actions/action3/action.yml:9:19:9:55 | github.event.pull_request.body | Potential code injection in $@, which may be controlled by an external user. | .github/actions/action3/action.yml:9:19:9:55 | github.event.pull_request.body | ${{ github.event.pull_request.body }} |
265264
| .github/actions/action5/action.yml:16:19:16:55 | github.event.pull_request.body | .github/actions/action5/action.yml:16:19:16:55 | github.event.pull_request.body | .github/actions/action5/action.yml:16:19:16:55 | github.event.pull_request.body | Potential code injection in $@, which may be controlled by an external user. | .github/actions/action5/action.yml:16:19:16:55 | github.event.pull_request.body | ${{ github.event.pull_request.body }} |
266265
| .github/workflows/argus_case_study.yml:27:33:27:77 | steps.remove_quotations.outputs.replaced | .github/workflows/argus_case_study.yml:17:25:17:53 | github.event.issue.title | .github/workflows/argus_case_study.yml:27:33:27:77 | steps.remove_quotations.outputs.replaced | Potential code injection in $@, which may be controlled by an external user. | .github/workflows/argus_case_study.yml:27:33:27:77 | steps.remove_quotations.outputs.replaced | ${{steps.remove_quotations.outputs.replaced}} |
267266
| .github/workflows/artifactpoisoning1.yml:34:67:34:92 | steps.pr.outputs.id | .github/workflows/artifactpoisoning1.yml:14:9:20:6 | Uses Step | .github/workflows/artifactpoisoning1.yml:34:67:34:92 | steps.pr.outputs.id | Potential code injection in $@, which may be controlled by an external user. | .github/workflows/artifactpoisoning1.yml:34:67:34:92 | steps.pr.outputs.id | ${{ steps.pr.outputs.id }} |

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

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -260,6 +260,7 @@ nodes
260260
| .github/workflows/workflow_run.yml:16:19:16:78 | github.event.workflow_run.head_repository.description | semmle.label | github.event.workflow_run.head_repository.description |
261261
subpaths
262262
#select
263+
| .github/actions/action3/action.yml:9:19:9:55 | github.event.pull_request.body | .github/actions/action3/action.yml:9:19:9:55 | github.event.pull_request.body | .github/actions/action3/action.yml:9:19:9:55 | github.event.pull_request.body | Potential code injection in $@, which may be controlled by an external user. | .github/actions/action3/action.yml:9:19:9:55 | github.event.pull_request.body | ${{ github.event.pull_request.body }} |
263264
| .github/actions/action4/action.yml:7:19:7:55 | github.event.pull_request.body | .github/actions/action4/action.yml:7:19:7:55 | github.event.pull_request.body | .github/actions/action4/action.yml:7:19:7:55 | github.event.pull_request.body | Potential code injection in $@, which may be controlled by an external user. | .github/actions/action4/action.yml:7:19:7:55 | github.event.pull_request.body | ${{ github.event.pull_request.body }} |
264265
| .github/workflows/changed-files.yml:20:24:20:76 | steps.changed-files1.outputs.all_changed_files | .github/workflows/changed-files.yml:15:9:18:6 | Uses Step: changed-files1 | .github/workflows/changed-files.yml:20:24:20:76 | steps.changed-files1.outputs.all_changed_files | Potential code injection in $@, which may be controlled by an external user. | .github/workflows/changed-files.yml:20:24:20:76 | steps.changed-files1.outputs.all_changed_files | ${{ steps.changed-files1.outputs.all_changed_files }} |
265266
| .github/workflows/changed-files.yml:40:24:40:76 | steps.changed-files3.outputs.all_changed_files | .github/workflows/changed-files.yml:33:9:38:6 | Uses Step: changed-files3 | .github/workflows/changed-files.yml:40:24:40:76 | steps.changed-files3.outputs.all_changed_files | Potential code injection in $@, which may be controlled by an external user. | .github/workflows/changed-files.yml:40:24:40:76 | steps.changed-files3.outputs.all_changed_files | ${{ steps.changed-files3.outputs.all_changed_files }} |

0 commit comments

Comments
 (0)