hohn/codeql
1
0
Fork 0
mirror of https://github.com/github/codeql.git synced 2026-05-02 12:15:17 +02:00
Code Issues Packages Projects Releases Wiki Activity

Improve privleged detection

Browse Source
This commit is contained in:
Alvaro Muñoz
2024-06-03 11:26:51 +02:00
parent 844b6e014b
commit 88465bd0e3
7 changed files with 141 additions and 138 deletions
Download Patch File Download Diff File Expand all files Collapse all files

12
ql/lib/codeql/actions/Ast.qll
View File

@@ -77,6 +77,8 @@ class CompositeAction extends AstNode instanceof CompositeActionImpl {
LocalJob getACaller() { result = super.getACaller() }
predicate isPrivileged() { super.isPrivileged() }
predicate isPrivilegedExternallyTriggerable() { super.isPrivilegedExternallyTriggerable() }
}
/**
@@ -169,6 +171,10 @@ class Event extends AstNode instanceof EventImpl {
string getAPropertyValue(string prop) { result = super.getAPropertyValue(prop) }
predicate hasProperty(string prop) { super.hasProperty(prop) }
predicate isExternallyTriggerable() { super.isExternallyTriggerable() }
predicate isPrivileged() { super.isPrivileged() }
}
/**
@@ -198,11 +204,11 @@ abstract class Job extends AstNode instanceof JobImpl {
Strategy getStrategy() { result = super.getStrategy() }
string getARunsOnLabel() { result = super.getARunsOnLabel() }
predicate isPrivileged() { super.isPrivileged() }
predicate isExternallyTriggerable() { super.isExternallyTriggerable() }
string getARunsOnLabel() { result = super.getARunsOnLabel() }
predicate isPrivilegedExternallyTriggerable() { super.isPrivilegedExternallyTriggerable() }
}
class LocalJob extends Job instanceof LocalJobImpl {

46
ql/lib/codeql/actions/Helper.qll
View File

@@ -210,54 +210,28 @@ predicate writeToGitHubPath(Run run, string content) {
predicate inPrivilegedCompositeAction(AstNode node) {
exists(CompositeAction a |
// node is in a privileged composite action
a = node.getEnclosingCompositeAction() and
(
a.isPrivileged()
or
exists(Job caller |
caller = a.getACaller() and
caller.isPrivileged() and
caller.isExternallyTriggerable()
)
)
)
}
predicate inPrivilegedExternallyTriggerableJob(AstNode node) {
exists(Job j |
// node is in a privileged and externally triggereable job
j = node.getEnclosingJob() and
// job is privileged (write access or access to secrets)
j.isPrivileged() and
// job is triggereable by an external user
j.isExternallyTriggerable()
a.isPrivilegedExternallyTriggerable()
)
}
predicate inNonPrivilegedCompositeAction(AstNode node) {
exists(CompositeAction a |
// node is in a non-privileged composite action
a = node.getEnclosingCompositeAction() and
not a.isPrivileged() and
not exists(LocalJob caller |
caller = a.getACaller() and
caller.isPrivileged() and
caller.isExternallyTriggerable()
)
not a.isPrivilegedExternallyTriggerable()
)
}
predicate inPrivilegedExternallyTriggerableJob(AstNode node) {
exists(Job j |
j = node.getEnclosingJob() and
j.isPrivilegedExternallyTriggerable()
)
}
predicate inNonPrivilegedJob(AstNode node) {
exists(Job j |
// node is in a non-privileged or not externally triggereable job
j = node.getEnclosingJob() and
(
// job is non-privileged (no write access and no access to secrets)
not j.isPrivileged()
or
// job is triggereable by an external user
not j.isExternallyTriggerable()
)
not j.isPrivilegedExternallyTriggerable()
)
}

215
ql/lib/codeql/actions/ast/internal/Ast.qll
View File

@@ -317,18 +317,6 @@ class CompositeActionImpl extends AstNodeImpl, TCompositeAction {
)
}
/** Holds if the action is privileged. */
predicate isPrivileged() {
// there is a calling job that defines explicit write permissions
this.hasExplicitWritePermission()
or
// the actions has an explicit secret accesses
this.hasExplicitSecretAccess()
or
// there is a privileged caller job
this.getACaller().isPrivileged()
}
private predicate hasExplicitSecretAccess() {
// the job accesses a secret other than GITHUB_TOKEN
exists(SecretsExpressionImpl expr |
@@ -340,6 +328,35 @@ class CompositeActionImpl extends AstNodeImpl, TCompositeAction {
// a calling job has an explicit write permission
this.getACaller().getPermissions().getAPermission().matches("%write")
}
/** Holds if the action is privileged. */
predicate isPrivileged() {
// there is a calling job that defines explicit write permissions
this.hasExplicitWritePermission()
or
// the actions has an explicit secret accesses
this.hasExplicitSecretAccess()
or
// there is a privileged caller job
(
this.getACaller().isPrivileged()
or
not this.getACaller().isPrivileged() and
this.getACaller().getATriggerEvent().isPrivileged()
)
}
/** Holds if the action is privileged and externally triggerable. */
predicate isPrivilegedExternallyTriggerable() {
// the action is externally triggerable
exists(JobImpl caller, EventImpl event |
caller = this.getACaller() and
event = caller.getATriggerEvent() and
event.isExternallyTriggerable() and
// the action is privileged
(this.isPrivileged() or caller.isPrivileged())
)
}
}
class WorkflowImpl extends AstNodeImpl, TWorkflowNode {
@@ -688,6 +705,42 @@ class EventImpl extends AstNodeImpl, TEventNode {
/** Holds if the event has a property with the given name */
predicate hasProperty(string prop) { exists(this.getAPropertyValue(prop)) }
/** Holds if the event can be triggered by an external actor. */
predicate isExternallyTriggerable() {
// the job is triggered by an event that can be triggered externally
externallyTriggerableEventsDataModel(this.getName())
or
// the event is `workflow_call` and there is a caller workflow that can be triggered externally
this.getName() = "workflow_call" and
(
// there are hints that this workflow is meant to be called by external triggers
exists(ExpressionImpl expr, string external_trigger |
expr.getEnclosingWorkflow() = this.getEnclosingWorkflow() and
expr.getExpression().matches("%github.event" + external_trigger + "%") and
externallyTriggerableEventsDataModel(external_trigger)
)
or
this.getEnclosingWorkflow()
.(ReusableWorkflowImpl)
.getACaller()
.getATriggerEvent()
.isExternallyTriggerable()
)
}
predicate isPrivileged() {
// the Job is triggered by an event other than `pull_request`, or `workflow_call`
not this.getName() = "pull_request" and
not this.getName() = "workflow_call"
or
// Reusable Workflow with a privileged caller or we cant find a caller
this.getName() = "workflow_call" and
(
this.getEnclosingWorkflow().(ReusableWorkflowImpl).getACaller().isPrivileged() or
not exists(this.getEnclosingWorkflow().(ReusableWorkflowImpl).getACaller())
)
}
}
class JobImpl extends AstNodeImpl, TJobNode {
@@ -746,43 +799,39 @@ class JobImpl extends AstNodeImpl, TJobNode {
/** Gets the strategy for this job. */
StrategyImpl getStrategy() { result.getNode() = n.lookup("strategy") }
/** Holds if the job can be triggered by an external actor. */
predicate isExternallyTriggerable() {
// the job is triggered by an event that can be triggered externally
externallyTriggerableEventsDataModel(this.getATriggerEvent().getName())
or
// the job is triggered by a workflow_call event that can be triggered externally
this.getATriggerEvent().getName() = "workflow_call" and
(
exists(ExpressionImpl e, string external_trigger |
e.getEnclosingJob() = this and
e.getExpression().matches("%github.event" + external_trigger + "%") and
externallyTriggerableEventsDataModel(external_trigger)
)
or
this.getEnclosingWorkflow().(ReusableWorkflowImpl).getACaller().isExternallyTriggerable()
)
}
/** Gets the trigger event that starts this workflow. */
EventImpl getATriggerEvent() { result = this.getEnclosingWorkflow().getATriggerEvent() }
/** Holds if the job is privileged. */
predicate isPrivileged() {
// the job has privileged runtime permissions
this.hasRuntimeWritePermissions()
or
// the job has an explicit secret accesses
this.hasExplicitSecretAccess()
or
// the job has an explicit write permission
this.hasExplicitWritePermission()
or
// the job has no explicit permissions but the workflow has write permissions
not exists(this.getPermissions()) and
this.hasImplicitWritePermission()
or
// neither the job nor the workflow have permissions but the job has a privileged trigger
not exists(this.getPermissions()) and
not exists(this.getEnclosingWorkflow().getPermissions()) and
this.hasPrivilegedTrigger()
// private predicate hasSingleTrigger(string trigger) {
// this.getATriggerEvent().getName() = trigger and
// count(this.getATriggerEvent()) = 1
// }
/** Gets the runs-on field of the job. */
string getARunsOnLabel() {
exists(ScalarValueImpl lbl, YamlMappingLikeNode runson |
runson = n.lookup("runs-on").(YamlMappingLikeNode)
|
(
lbl.getNode() = runson.getNode(_) and
not lbl.getNode() = runson.getNode("group")
or
lbl.getNode() = runson.getNode("labels").(YamlMappingLikeNode).getNode(_)
) and
(
not exists(MatrixExpressionImpl e | e.getParentNode() = lbl) and
result =
lbl.getValue()
.trim()
.regexpReplaceAll("^('|\")", "")
.regexpReplaceAll("('|\")$", "")
.trim()
or
exists(MatrixExpressionImpl e |
e.getParentNode() = lbl and
result = e.getLiteralValues()
)
)
)
}
private predicate hasExplicitSecretAccess() {
@@ -817,60 +866,34 @@ class JobImpl extends AstNodeImpl, TJobNode {
)
}
private predicate hasPrivilegedTrigger() {
// the Job is triggered by an event other than `pull_request`, `push`, or `workflow_call`
count(this.getATriggerEvent()) = 1 and
not this.getATriggerEvent().getName() = "push" and
not this.getATriggerEvent().getName() = "pull_request" and
not this.getATriggerEvent().getName() = "workflow_call"
/** Holds if the job is privileged. */
predicate isPrivileged() {
// the job has privileged runtime permissions
this.hasRuntimeWritePermissions()
or
// the Workflow is a Reusable Workflow only and there is
// a privileged caller workflow or we cant find a caller
count(this.getATriggerEvent()) = 1 and
this.getATriggerEvent().getName() = "workflow_call" and
(
this.getEnclosingWorkflow().(ReusableWorkflowImpl).getACaller().isPrivileged() or
not exists(this.getEnclosingWorkflow().(ReusableWorkflowImpl).getACaller())
)
// the job has an explicit secret accesses
this.hasExplicitSecretAccess()
or
// the Job is triggered by an event other than `push`, `pull_request`, or `workflow_call`
exists(string event |
this.getATriggerEvent().getName() = event and
not event = ["push", "pull_request", "workflow_call"]
)
// the job has an explicit write permission
this.hasExplicitWritePermission()
or
// the job has no explicit permissions but the workflow has write permissions
not exists(this.getPermissions()) and
this.hasImplicitWritePermission()
}
/** Gets the trigger event that starts this workflow. */
EventImpl getATriggerEvent() { result = this.getEnclosingWorkflow().getATriggerEvent() }
// private predicate hasSingleTrigger(string trigger) {
// this.getATriggerEvent().getName() = trigger and
// count(this.getATriggerEvent()) = 1
// }
/** Gets the runs-on field of the job. */
string getARunsOnLabel() {
exists(ScalarValueImpl lbl, YamlMappingLikeNode runson |
runson = n.lookup("runs-on").(YamlMappingLikeNode)
|
/** Holds if the action is privileged and externally triggerable. */
predicate isPrivilegedExternallyTriggerable() {
exists(EventImpl e |
// job is triggereable by an external user
this.getATriggerEvent() = e and
e.isExternallyTriggerable() and
// job is privileged (write access or access to secrets)
(
lbl.getNode() = runson.getNode(_) and
not lbl.getNode() = runson.getNode("group")
this.isPrivileged()
or
lbl.getNode() = runson.getNode("labels").(YamlMappingLikeNode).getNode(_)
) and
(
not exists(MatrixExpressionImpl e | e.getParentNode() = lbl) and
result =
lbl.getValue()
.trim()
.regexpReplaceAll("^('|\")", "")
.regexpReplaceAll("('|\")$", "")
.trim()
or
exists(MatrixExpressionImpl e |
e.getParentNode() = lbl and
result = e.getLiteralValues()
)
not this.isPrivileged() and
e.isPrivileged()
)
)
}

2
ql/src/Security/CWE-349/CachePoisoning.ql
View File

@@ -24,7 +24,7 @@ where
// TODO: Consider adding artifact downloads as a potential source of cache poisoning
j.getAStep() = checkout and
// job can be triggered by an external user
j.isExternallyTriggerable() and
j.getATriggerEvent().isExternallyTriggerable() and
(
// the job writes to the cache
// (No need to follow the checkout step as the cache writing is normally done after the job completes)

2
ql/src/Security/CWE-349/CachePoisoningByCodeInjection.ql
View File

@@ -22,7 +22,7 @@ where
CodeInjectionFlow::flowPath(source, sink) and
j = sink.getNode().asExpr().getEnclosingJob() and
// job can be triggered by an external user
j.isExternallyTriggerable() and
j.getATriggerEvent().isExternallyTriggerable() and
// excluding privileged workflows since they can be easily exploited in similar circumstances
not j.isPrivileged() and
// The workflow runs in the context of the default branch

1
ql/test/query-tests/Security/CWE-094/CodeInjectionCritical.expected
View File

@@ -261,7 +261,6 @@ nodes
subpaths
#select
| .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 }} |
| .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 }} |
| .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 }} |
| .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}} |
| .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 }} |

1
ql/test/query-tests/Security/CWE-094/CodeInjectionMedium.expected
View File

@@ -260,6 +260,7 @@ nodes
| .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 |
subpaths
#select
| .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 }} |
| .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 }} |
| .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 }} |
| .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 }} |