Skip to content

Commit b1ddbc9

Browse files
author
Alvaro Muñoz
committed
Improve Control Checks
1 parent 153fb49 commit b1ddbc9

22 files changed

+168
-217
lines changed

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

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -79,8 +79,6 @@ class CompositeAction extends AstNode instanceof CompositeActionImpl {
7979
UsesStep getACallerStep() { result = super.getACallerStep() }
8080

8181
predicate isPrivileged() { super.isPrivileged() }
82-
83-
predicate isPrivilegedExternallyTriggerable() { super.isPrivilegedExternallyTriggerable() }
8482
}
8583

8684
/**
@@ -200,7 +198,9 @@ abstract class Job extends AstNode instanceof JobImpl {
200198

201199
predicate isPrivileged() { super.isPrivileged() }
202200

203-
predicate isPrivilegedExternallyTriggerable() { super.isPrivilegedExternallyTriggerable() }
201+
predicate isPrivilegedExternallyTriggerable(Event event) {
202+
super.isPrivilegedExternallyTriggerable(event)
203+
}
204204
}
205205

206206
abstract class StepsContainer extends AstNode instanceof StepsContainerImpl {

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

Lines changed: 3 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -238,44 +238,12 @@ predicate fileToGitHubPath(Run run, string path) {
238238
fileToFileWrite(run.getScript(), "GITHUB_PATH", path)
239239
}
240240

241-
predicate inPrivilegedCompositeAction(AstNode node) {
242-
exists(CompositeAction a |
243-
a = node.getEnclosingCompositeAction() and
244-
a.isPrivilegedExternallyTriggerable()
245-
)
246-
}
247-
248-
predicate inPrivilegedExternallyTriggerableJob(AstNode node) {
249-
exists(Job j |
250-
j = node.getEnclosingJob() and
251-
j.isPrivilegedExternallyTriggerable()
252-
)
253-
}
254-
255-
predicate inPrivilegedContext(AstNode node) {
256-
inPrivilegedCompositeAction(node)
257-
or
258-
inPrivilegedExternallyTriggerableJob(node)
259-
}
260-
261-
predicate inNonPrivilegedCompositeAction(AstNode node) {
262-
exists(CompositeAction a |
263-
a = node.getEnclosingCompositeAction() and
264-
not a.isPrivilegedExternallyTriggerable()
265-
)
266-
}
267-
268-
predicate inNonPrivilegedJob(AstNode node) {
269-
exists(Job j |
270-
j = node.getEnclosingJob() and
271-
not j.isPrivilegedExternallyTriggerable()
272-
)
241+
predicate inPrivilegedContext(AstNode node, Event event) {
242+
node.getEnclosingJob().isPrivilegedExternallyTriggerable(event)
273243
}
274244

275245
predicate inNonPrivilegedContext(AstNode node) {
276-
inNonPrivilegedCompositeAction(node)
277-
or
278-
inNonPrivilegedJob(node)
246+
not node.getEnclosingJob().isPrivilegedExternallyTriggerable(_)
279247
}
280248

281249
string partialFileContentRegexp() {

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

Lines changed: 30 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -359,18 +359,6 @@ class CompositeActionImpl extends AstNodeImpl, TCompositeAction {
359359
}
360360

361361
EventImpl getATriggerEvent() { result = this.getACallerJob().getATriggerEvent() }
362-
363-
/** Holds if the action is privileged and externally triggerable. */
364-
predicate isPrivilegedExternallyTriggerable() {
365-
// the action is externally triggerable
366-
exists(JobImpl caller, EventImpl event |
367-
caller = this.getACallerJob() and
368-
event = caller.getATriggerEvent() and
369-
event.isExternallyTriggerable() and
370-
// the action is privileged
371-
(this.isPrivileged() or caller.isPrivileged())
372-
)
373-
}
374362
}
375363

376364
class WorkflowImpl extends AstNodeImpl, TWorkflowNode {
@@ -970,31 +958,30 @@ class JobImpl extends AstNodeImpl, TJobNode {
970958
}
971959

972960
/** Holds if the action is privileged and externally triggerable. */
973-
predicate isPrivilegedExternallyTriggerable() {
974-
exists(EventImpl e | this.getATriggerEvent() = e |
975-
// job is triggereable by an external user
976-
e.isExternallyTriggerable() and
977-
// no matter if `pull_request` is granted write permissions or access to secrets
978-
// when the job is triggered by a `pull_request` event from a fork, they will get revoked
979-
not e.getName() = "pull_request" and
980-
(
981-
// job is privileged (write access or access to secrets)
982-
this.isPrivileged()
983-
or
984-
// the trigger event is __normally__ privileged
985-
e.isPrivileged() and
986-
// and we have no runtime data to prove otherwise
987-
not this.hasRuntimeData() and
988-
// and the job is not explicitly non-privileged
989-
not (
990-
(
991-
this.hasExplicitNonePermission() or
992-
this.hasImplicitNonePermission() or
993-
this.hasExplicitReadPermission() or
994-
this.hasImplicitReadPermission()
995-
) and
996-
not this.hasExplicitSecretAccess()
997-
)
961+
predicate isPrivilegedExternallyTriggerable(EventImpl event) {
962+
this.getATriggerEvent() = event and
963+
// job is triggereable by an external user
964+
event.isExternallyTriggerable() and
965+
// no matter if `pull_request` is granted write permissions or access to secrets
966+
// when the job is triggered by a `pull_request` event from a fork, they will get revoked
967+
not event.getName() = "pull_request" and
968+
(
969+
// job is privileged (write access or access to secrets)
970+
this.isPrivileged()
971+
or
972+
// the trigger event is __normally__ privileged
973+
event.isPrivileged() and
974+
// and we have no runtime data to prove otherwise
975+
not this.hasRuntimeData() and
976+
// and the job is not explicitly non-privileged
977+
not (
978+
(
979+
this.hasExplicitNonePermission() or
980+
this.hasImplicitNonePermission() or
981+
this.hasExplicitReadPermission() or
982+
this.hasImplicitReadPermission()
983+
) and
984+
not this.hasExplicitSecretAccess()
998985
)
999986
)
1000987
}
@@ -1073,6 +1060,12 @@ class StepImpl extends AstNodeImpl, TStepNode {
10731060

10741061
override YamlMapping getNode() { result = n }
10751062

1063+
override JobImpl getEnclosingJob() {
1064+
// if a step is within a composite action, we should follow the caller job
1065+
result = this.getEnclosingCompositeAction().getACallerJob() or
1066+
result = super.getEnclosingJob()
1067+
}
1068+
10761069
EnvImpl getEnv() { result.getNode() = n.lookup("env") }
10771070

10781071
/** Gets the ID of this step, if any. */

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

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -38,9 +38,12 @@ abstract class ControlCheck extends AstNode {
3838
}
3939

4040
predicate protects(Step step, Event event, string category) {
41-
event = step.getEnclosingWorkflow().getATriggerEvent() and
41+
// The check dominates the step it should protect
4242
this.dominates(step) and
43-
this.protectsCategoryAndEvent(category, event.getName())
43+
// The check is effective against the event and category
44+
this.protectsCategoryAndEvent(category, event.getName()) and
45+
// The check can be triggered by the event
46+
this.getEnclosingJob().getATriggerEvent() = event
4447
}
4548

4649
predicate dominates(Step step) {

‎ql/src/Security/CWE-074/OutputClobberingHigh.ql

Lines changed: 4 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -18,25 +18,20 @@ import codeql.actions.dataflow.ExternalFlow
1818
import OutputClobberingFlow::PathGraph
1919
import codeql.actions.security.ControlChecks
2020

21-
from OutputClobberingFlow::PathNode source, OutputClobberingFlow::PathNode sink
21+
from OutputClobberingFlow::PathNode source, OutputClobberingFlow::PathNode sink, Event event
2222
where
2323
OutputClobberingFlow::flowPath(source, sink) and
24-
inPrivilegedContext(sink.getNode().asExpr()) and
24+
inPrivilegedContext(sink.getNode().asExpr(), event) and
2525
// exclude paths to file read sinks from non-artifact sources
2626
(
2727
not source.getNode().(RemoteFlowSource).getSourceType() = "artifact" and
2828
not exists(ControlCheck check |
29-
check
30-
.protects(sink.getNode().asExpr(),
31-
source.getNode().asExpr().getEnclosingJob().getATriggerEvent(), "code-injection")
29+
check.protects(sink.getNode().asExpr(), event, "code-injection")
3230
)
3331
or
3432
source.getNode().(RemoteFlowSource).getSourceType() = "artifact" and
3533
not exists(ControlCheck check |
36-
check
37-
.protects(sink.getNode().asExpr(),
38-
source.getNode().asExpr().getEnclosingJob().getATriggerEvent(),
39-
["untrusted-checkout", "artifact-poisoning"])
34+
check.protects(sink.getNode().asExpr(), event, ["untrusted-checkout", "artifact-poisoning"])
4035
) and
4136
(
4237
sink.getNode() instanceof OutputClobberingFromFileReadSink or

‎ql/src/Security/CWE-077/EnvPathInjectionCritical.ql

Lines changed: 4 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -17,24 +17,19 @@ import codeql.actions.security.EnvPathInjectionQuery
1717
import EnvPathInjectionFlow::PathGraph
1818
import codeql.actions.security.ControlChecks
1919

20-
from EnvPathInjectionFlow::PathNode source, EnvPathInjectionFlow::PathNode sink
20+
from EnvPathInjectionFlow::PathNode source, EnvPathInjectionFlow::PathNode sink, Event event
2121
where
2222
EnvPathInjectionFlow::flowPath(source, sink) and
23-
inPrivilegedContext(sink.getNode().asExpr()) and
23+
inPrivilegedContext(sink.getNode().asExpr(), event) and
2424
(
2525
not source.getNode().(RemoteFlowSource).getSourceType() = "artifact" and
2626
not exists(ControlCheck check |
27-
check
28-
.protects(sink.getNode().asExpr(),
29-
source.getNode().asExpr().getEnclosingJob().getATriggerEvent(), "code-injection")
27+
check.protects(sink.getNode().asExpr(), event, "code-injection")
3028
)
3129
or
3230
source.getNode().(RemoteFlowSource).getSourceType() = "artifact" and
3331
not exists(ControlCheck check |
34-
check
35-
.protects(sink.getNode().asExpr(),
36-
source.getNode().asExpr().getEnclosingJob().getATriggerEvent(),
37-
["untrusted-checkout", "artifact-poisoning"])
32+
check.protects(sink.getNode().asExpr(), event, ["untrusted-checkout", "artifact-poisoning"])
3833
) and
3934
sink.getNode() instanceof EnvPathInjectionFromFileReadSink
4035
)

‎ql/src/Security/CWE-077/EnvVarInjectionCritical.ql

Lines changed: 5 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -18,30 +18,23 @@ import codeql.actions.dataflow.ExternalFlow
1818
import EnvVarInjectionFlow::PathGraph
1919
import codeql.actions.security.ControlChecks
2020

21-
from EnvVarInjectionFlow::PathNode source, EnvVarInjectionFlow::PathNode sink
21+
from EnvVarInjectionFlow::PathNode source, EnvVarInjectionFlow::PathNode sink, Event event
2222
where
2323
EnvVarInjectionFlow::flowPath(source, sink) and
24-
inPrivilegedContext(sink.getNode().asExpr()) and
24+
inPrivilegedContext(sink.getNode().asExpr(), event) and
2525
not exists(ControlCheck check |
26-
check
27-
.protects(sink.getNode().asExpr(),
28-
source.getNode().asExpr().getEnclosingJob().getATriggerEvent(), "envvar-injection")
26+
check.protects(sink.getNode().asExpr(), event, "envvar-injection")
2927
) and
3028
// exclude paths to file read sinks from non-artifact sources
3129
(
3230
not source.getNode().(RemoteFlowSource).getSourceType() = "artifact" and
3331
not exists(ControlCheck check |
34-
check
35-
.protects(sink.getNode().asExpr(),
36-
source.getNode().asExpr().getEnclosingJob().getATriggerEvent(), "code-injection")
32+
check.protects(sink.getNode().asExpr(), event, "code-injection")
3733
)
3834
or
3935
source.getNode().(RemoteFlowSource).getSourceType() = "artifact" and
4036
not exists(ControlCheck check |
41-
check
42-
.protects(sink.getNode().asExpr(),
43-
source.getNode().asExpr().getEnclosingJob().getATriggerEvent(),
44-
["untrusted-checkout", "artifact-poisoning"])
37+
check.protects(sink.getNode().asExpr(), event, ["untrusted-checkout", "artifact-poisoning"])
4538
) and
4639
(
4740
sink.getNode() instanceof EnvVarInjectionFromFileReadSink or

‎ql/src/Security/CWE-078/CommandInjectionCritical.ql

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -17,10 +17,10 @@ import actions
1717
import codeql.actions.security.CommandInjectionQuery
1818
import CommandInjectionFlow::PathGraph
1919

20-
from CommandInjectionFlow::PathNode source, CommandInjectionFlow::PathNode sink
20+
from CommandInjectionFlow::PathNode source, CommandInjectionFlow::PathNode sink, Event event
2121
where
2222
CommandInjectionFlow::flowPath(source, sink) and
23-
inPrivilegedContext(sink.getNode().asExpr())
23+
inPrivilegedContext(sink.getNode().asExpr(), event)
2424
select sink.getNode(), source, sink,
2525
"Potential command injection in $@, which may be controlled by an external user.", sink,
2626
sink.getNode().asExpr().(Expression).getRawExpression()

‎ql/src/Security/CWE-088/ArgumentInjectionCritical.ql

Lines changed: 3 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -16,14 +16,12 @@ import codeql.actions.security.ArgumentInjectionQuery
1616
import ArgumentInjectionFlow::PathGraph
1717
import codeql.actions.security.ControlChecks
1818

19-
from ArgumentInjectionFlow::PathNode source, ArgumentInjectionFlow::PathNode sink
19+
from ArgumentInjectionFlow::PathNode source, ArgumentInjectionFlow::PathNode sink, Event event
2020
where
2121
ArgumentInjectionFlow::flowPath(source, sink) and
22-
inPrivilegedContext(sink.getNode().asExpr()) and
22+
inPrivilegedContext(sink.getNode().asExpr(), event) and
2323
not exists(ControlCheck check |
24-
check
25-
.protects(sink.getNode().asExpr(),
26-
source.getNode().asExpr().getEnclosingJob().getATriggerEvent(), "argument-injection")
24+
check.protects(sink.getNode().asExpr(), event, "argument-injection")
2725
)
2826
select sink.getNode(), source, sink,
2927
"Potential argument injection in $@ command, which may be controlled by an external user.", sink,

‎ql/src/Security/CWE-094/CodeInjectionCritical.ql

Lines changed: 3 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -19,15 +19,11 @@ import codeql.actions.security.CodeInjectionQuery
1919
import CodeInjectionFlow::PathGraph
2020
import codeql.actions.security.ControlChecks
2121

22-
from CodeInjectionFlow::PathNode source, CodeInjectionFlow::PathNode sink
22+
from CodeInjectionFlow::PathNode source, CodeInjectionFlow::PathNode sink, Event event
2323
where
2424
CodeInjectionFlow::flowPath(source, sink) and
25-
inPrivilegedContext(sink.getNode().asExpr()) and
26-
not exists(ControlCheck check |
27-
check
28-
.protects(sink.getNode().asExpr(),
29-
source.getNode().asExpr().getEnclosingJob().getATriggerEvent(), "code-injection")
30-
) and
25+
inPrivilegedContext(sink.getNode().asExpr(), event) and
26+
not exists(ControlCheck check | check.protects(sink.getNode().asExpr(), event, "code-injection")) and
3127
// exclude cases where the sink is a JS script and the expression uses toJson
3228
not exists(UsesStep script |
3329
script.getCallee() = "actions/github-script" and

0 commit comments

Comments
 (0)