Improve Control Checks

Author: Alvaro Muñoz

ql/lib/codeql/actions/Ast.qll

@@ -79,8 +79,6 @@ class CompositeAction extends AstNode instanceof CompositeActionImpl {
   UsesStep getACallerStep() { result = super.getACallerStep() }
 
   predicate isPrivileged() { super.isPrivileged() }
-
-  predicate isPrivilegedExternallyTriggerable() { super.isPrivilegedExternallyTriggerable() }
 }
 
 /**
@@ -200,7 +198,9 @@ abstract class Job extends AstNode instanceof JobImpl {
 
   predicate isPrivileged() { super.isPrivileged() }
 
-  predicate isPrivilegedExternallyTriggerable() { super.isPrivilegedExternallyTriggerable() }
+  predicate isPrivilegedExternallyTriggerable(Event event) {
+    super.isPrivilegedExternallyTriggerable(event)
+  }
 }
 
 abstract class StepsContainer extends AstNode instanceof StepsContainerImpl {

ql/lib/codeql/actions/Helper.qll

@@ -238,44 +238,12 @@ predicate fileToGitHubPath(Run run, string path) {
   fileToFileWrite(run.getScript(), "GITHUB_PATH", path)
 }
 
-predicate inPrivilegedCompositeAction(AstNode node) {
-  exists(CompositeAction a |
-    a = node.getEnclosingCompositeAction() and
-    a.isPrivilegedExternallyTriggerable()
-  )
-}
-
-predicate inPrivilegedExternallyTriggerableJob(AstNode node) {
-  exists(Job j |
-    j = node.getEnclosingJob() and
-    j.isPrivilegedExternallyTriggerable()
-  )
-}
-
-predicate inPrivilegedContext(AstNode node) {
-  inPrivilegedCompositeAction(node)
-  or
-  inPrivilegedExternallyTriggerableJob(node)
-}
-
-predicate inNonPrivilegedCompositeAction(AstNode node) {
-  exists(CompositeAction a |
-    a = node.getEnclosingCompositeAction() and
-    not a.isPrivilegedExternallyTriggerable()
-  )
-}
-
-predicate inNonPrivilegedJob(AstNode node) {
-  exists(Job j |
-    j = node.getEnclosingJob() and
-    not j.isPrivilegedExternallyTriggerable()
-  )
+predicate inPrivilegedContext(AstNode node, Event event) {
+  node.getEnclosingJob().isPrivilegedExternallyTriggerable(event)
 }
 
 predicate inNonPrivilegedContext(AstNode node) {
-  inNonPrivilegedCompositeAction(node)
-  or
-  inNonPrivilegedJob(node)
+  not node.getEnclosingJob().isPrivilegedExternallyTriggerable(_)
 }
 
 string partialFileContentRegexp() {

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

@@ -359,18 +359,6 @@ class CompositeActionImpl extends AstNodeImpl, TCompositeAction {
   }
 
   EventImpl getATriggerEvent() { result = this.getACallerJob().getATriggerEvent() }
-
-  /** Holds if the action is privileged and externally triggerable. */
-  predicate isPrivilegedExternallyTriggerable() {
-    // the action is externally triggerable
-    exists(JobImpl caller, EventImpl event |
-      caller = this.getACallerJob() and
-      event = caller.getATriggerEvent() and
-      event.isExternallyTriggerable() and
-      // the action is privileged
-      (this.isPrivileged() or caller.isPrivileged())
-    )
-  }
 }
 
 class WorkflowImpl extends AstNodeImpl, TWorkflowNode {
@@ -970,31 +958,30 @@ class JobImpl extends AstNodeImpl, TJobNode {
   }
 
   /** Holds if the action is privileged and externally triggerable. */
-  predicate isPrivilegedExternallyTriggerable() {
-    exists(EventImpl e | this.getATriggerEvent() = e |
-      // job is triggereable by an external user
-      e.isExternallyTriggerable() and
-      // no matter if `pull_request` is granted write permissions or access to secrets
-      // when the job is triggered by a `pull_request` event from a fork, they will get revoked
-      not e.getName() = "pull_request" and
-      (
-        // job is privileged (write access or access to secrets)
-        this.isPrivileged()
-        or
-        // the trigger event is __normally__ privileged
-        e.isPrivileged() and
-        // and we have no runtime data to prove otherwise
-        not this.hasRuntimeData() and
-        // and the job is not explicitly non-privileged
-        not (
-          (
-            this.hasExplicitNonePermission() or
-            this.hasImplicitNonePermission() or
-            this.hasExplicitReadPermission() or
-            this.hasImplicitReadPermission()
-          ) and
-          not this.hasExplicitSecretAccess()
-        )
+  predicate isPrivilegedExternallyTriggerable(EventImpl event) {
+    this.getATriggerEvent() = event and
+    // job is triggereable by an external user
+    event.isExternallyTriggerable() and
+    // no matter if `pull_request` is granted write permissions or access to secrets
+    // when the job is triggered by a `pull_request` event from a fork, they will get revoked
+    not event.getName() = "pull_request" and
+    (
+      // job is privileged (write access or access to secrets)
+      this.isPrivileged()
+      or
+      // the trigger event is __normally__ privileged
+      event.isPrivileged() and
+      // and we have no runtime data to prove otherwise
+      not this.hasRuntimeData() and
+      // and the job is not explicitly non-privileged
+      not (
+        (
+          this.hasExplicitNonePermission() or
+          this.hasImplicitNonePermission() or
+          this.hasExplicitReadPermission() or
+          this.hasImplicitReadPermission()
+        ) and
+        not this.hasExplicitSecretAccess()
       )
     )
   }
@@ -1073,6 +1060,12 @@ class StepImpl extends AstNodeImpl, TStepNode {
 
   override YamlMapping getNode() { result = n }
 
+  override JobImpl getEnclosingJob() {
+    // if a step is within a composite action, we should follow the caller job
+    result = this.getEnclosingCompositeAction().getACallerJob() or
+    result = super.getEnclosingJob()
+  }
+
   EnvImpl getEnv() { result.getNode() = n.lookup("env") }
 
   /** Gets the ID of this step, if any. */

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

@@ -38,9 +38,12 @@ abstract class ControlCheck extends AstNode {
   }
 
   predicate protects(Step step, Event event, string category) {
-    event = step.getEnclosingWorkflow().getATriggerEvent() and
+    // The check dominates the step it should protect
     this.dominates(step) and
-    this.protectsCategoryAndEvent(category, event.getName())
+    // The check is effective against the event and category
+    this.protectsCategoryAndEvent(category, event.getName()) and
+    // The check can be triggered by the event
+    this.getEnclosingJob().getATriggerEvent() = event
   }
 
   predicate dominates(Step step) {

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

@@ -18,25 +18,20 @@ import codeql.actions.dataflow.ExternalFlow
 import OutputClobberingFlow::PathGraph
 import codeql.actions.security.ControlChecks
 
-from OutputClobberingFlow::PathNode source, OutputClobberingFlow::PathNode sink
+from OutputClobberingFlow::PathNode source, OutputClobberingFlow::PathNode sink, Event event
 where
   OutputClobberingFlow::flowPath(source, sink) and
-  inPrivilegedContext(sink.getNode().asExpr()) and
+  inPrivilegedContext(sink.getNode().asExpr(), event) and
   // exclude paths to file read sinks from non-artifact sources
   (
     not source.getNode().(RemoteFlowSource).getSourceType() = "artifact" and
     not exists(ControlCheck check |
-      check
-          .protects(sink.getNode().asExpr(),
-            source.getNode().asExpr().getEnclosingJob().getATriggerEvent(), "code-injection")
+      check.protects(sink.getNode().asExpr(), event, "code-injection")
     )
     or
     source.getNode().(RemoteFlowSource).getSourceType() = "artifact" and
     not exists(ControlCheck check |
-      check
-          .protects(sink.getNode().asExpr(),
-            source.getNode().asExpr().getEnclosingJob().getATriggerEvent(),
-            ["untrusted-checkout", "artifact-poisoning"])
+      check.protects(sink.getNode().asExpr(), event, ["untrusted-checkout", "artifact-poisoning"])
     ) and
     (
       sink.getNode() instanceof OutputClobberingFromFileReadSink or

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

@@ -17,24 +17,19 @@ import codeql.actions.security.EnvPathInjectionQuery
 import EnvPathInjectionFlow::PathGraph
 import codeql.actions.security.ControlChecks
 
-from EnvPathInjectionFlow::PathNode source, EnvPathInjectionFlow::PathNode sink
+from EnvPathInjectionFlow::PathNode source, EnvPathInjectionFlow::PathNode sink, Event event
 where
   EnvPathInjectionFlow::flowPath(source, sink) and
-  inPrivilegedContext(sink.getNode().asExpr()) and
+  inPrivilegedContext(sink.getNode().asExpr(), event) and
   (
     not source.getNode().(RemoteFlowSource).getSourceType() = "artifact" and
     not exists(ControlCheck check |
-      check
-          .protects(sink.getNode().asExpr(),
-            source.getNode().asExpr().getEnclosingJob().getATriggerEvent(), "code-injection")
+      check.protects(sink.getNode().asExpr(), event, "code-injection")
     )
     or
     source.getNode().(RemoteFlowSource).getSourceType() = "artifact" and
     not exists(ControlCheck check |
-      check
-          .protects(sink.getNode().asExpr(),
-            source.getNode().asExpr().getEnclosingJob().getATriggerEvent(),
-            ["untrusted-checkout", "artifact-poisoning"])
+      check.protects(sink.getNode().asExpr(), event, ["untrusted-checkout", "artifact-poisoning"])
     ) and
     sink.getNode() instanceof EnvPathInjectionFromFileReadSink
   )

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

@@ -18,30 +18,23 @@ import codeql.actions.dataflow.ExternalFlow
 import EnvVarInjectionFlow::PathGraph
 import codeql.actions.security.ControlChecks
 
-from EnvVarInjectionFlow::PathNode source, EnvVarInjectionFlow::PathNode sink
+from EnvVarInjectionFlow::PathNode source, EnvVarInjectionFlow::PathNode sink, Event event
 where
   EnvVarInjectionFlow::flowPath(source, sink) and
-  inPrivilegedContext(sink.getNode().asExpr()) and
+  inPrivilegedContext(sink.getNode().asExpr(), event) and
   not exists(ControlCheck check |
-    check
-        .protects(sink.getNode().asExpr(),
-          source.getNode().asExpr().getEnclosingJob().getATriggerEvent(), "envvar-injection")
+    check.protects(sink.getNode().asExpr(), event, "envvar-injection")
   ) and
   // exclude paths to file read sinks from non-artifact sources
   (
     not source.getNode().(RemoteFlowSource).getSourceType() = "artifact" and
     not exists(ControlCheck check |
-      check
-          .protects(sink.getNode().asExpr(),
-            source.getNode().asExpr().getEnclosingJob().getATriggerEvent(), "code-injection")
+      check.protects(sink.getNode().asExpr(), event, "code-injection")
     )
     or
     source.getNode().(RemoteFlowSource).getSourceType() = "artifact" and
     not exists(ControlCheck check |
-      check
-          .protects(sink.getNode().asExpr(),
-            source.getNode().asExpr().getEnclosingJob().getATriggerEvent(),
-            ["untrusted-checkout", "artifact-poisoning"])
+      check.protects(sink.getNode().asExpr(), event, ["untrusted-checkout", "artifact-poisoning"])
     ) and
     (
       sink.getNode() instanceof EnvVarInjectionFromFileReadSink or

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

@@ -17,10 +17,10 @@ import actions
 import codeql.actions.security.CommandInjectionQuery
 import CommandInjectionFlow::PathGraph
 
-from CommandInjectionFlow::PathNode source, CommandInjectionFlow::PathNode sink
+from CommandInjectionFlow::PathNode source, CommandInjectionFlow::PathNode sink, Event event
 where
   CommandInjectionFlow::flowPath(source, sink) and
-  inPrivilegedContext(sink.getNode().asExpr())
+  inPrivilegedContext(sink.getNode().asExpr(), event)
 select sink.getNode(), source, sink,
   "Potential command injection in $@, which may be controlled by an external user.", sink,
   sink.getNode().asExpr().(Expression).getRawExpression()

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

@@ -16,14 +16,12 @@ import codeql.actions.security.ArgumentInjectionQuery
 import ArgumentInjectionFlow::PathGraph
 import codeql.actions.security.ControlChecks
 
-from ArgumentInjectionFlow::PathNode source, ArgumentInjectionFlow::PathNode sink
+from ArgumentInjectionFlow::PathNode source, ArgumentInjectionFlow::PathNode sink, Event event
 where
   ArgumentInjectionFlow::flowPath(source, sink) and
-  inPrivilegedContext(sink.getNode().asExpr()) and
+  inPrivilegedContext(sink.getNode().asExpr(), event) and
   not exists(ControlCheck check |
-    check
-        .protects(sink.getNode().asExpr(),
-          source.getNode().asExpr().getEnclosingJob().getATriggerEvent(), "argument-injection")
+    check.protects(sink.getNode().asExpr(), event, "argument-injection")
   )
 select sink.getNode(), source, sink,
   "Potential argument injection in $@ command, which may be controlled by an external user.", sink,

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

@@ -19,15 +19,11 @@ import codeql.actions.security.CodeInjectionQuery
 import CodeInjectionFlow::PathGraph
 import codeql.actions.security.ControlChecks
 
-from CodeInjectionFlow::PathNode source, CodeInjectionFlow::PathNode sink
+from CodeInjectionFlow::PathNode source, CodeInjectionFlow::PathNode sink, Event event
 where
   CodeInjectionFlow::flowPath(source, sink) and
-  inPrivilegedContext(sink.getNode().asExpr()) and
-  not exists(ControlCheck check |
-    check
-        .protects(sink.getNode().asExpr(),
-          source.getNode().asExpr().getEnclosingJob().getATriggerEvent(), "code-injection")
-  ) and
+  inPrivilegedContext(sink.getNode().asExpr(), event) and
+  not exists(ControlCheck check | check.protects(sink.getNode().asExpr(), event, "code-injection")) and
   // exclude cases where the sink is a JS script and the expression uses toJson
   not exists(UsesStep script |
     script.getCallee() = "actions/github-script" and