Merge pull request #42 from GitHubSecurityLab/priv_workflows

priv workflows

Author: Alvaro Muñoz

ql/lib/codeql/actions/Ast.qll

@@ -35,6 +35,8 @@ class AstNode instanceof AstNodeImpl {
 
   Workflow getEnclosingWorkflow() { result = super.getEnclosingWorkflow() }
 
+  CompositeAction getEnclosingCompositeAction() { result = super.getEnclosingCompositeAction() }
+
   Expression getInScopeEnvVarExpr(string name) { result = super.getInScopeEnvVarExpr(name) }
 }
 
@@ -123,6 +125,25 @@ class Workflow extends AstNode instanceof WorkflowImpl {
   Permissions getPermissions() { result = super.getPermissions() }
 
   Strategy getStrategy() { result = super.getStrategy() }
+
+  predicate hasSingleTrigger(string trigger) {
+    this.getATriggerEvent() = trigger and
+    count(string t | this.getATriggerEvent() = t | t) = 1
+  }
+
+  predicate isPrivileged() {
+    // The Workflow is triggered by an event other than `pull_request`
+    not this.hasSingleTrigger("pull_request")
+    or
+    // The Workflow is only triggered by `workflow_call` and there is
+    // a caller workflow triggered by an event other than `pull_request`
+    this.hasSingleTrigger("workflow_call") and
+    exists(ExternalJob call, Workflow caller |
+      call.getCallee() = this.getLocation().getFile().getRelativePath() and
+      caller = call.getWorkflow() and
+      not caller.hasSingleTrigger("pull_request")
+    )
+  }
 }
 
 class ReusableWorkflow extends Workflow instanceof ReusableWorkflowImpl {

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

@@ -95,10 +95,15 @@ abstract class AstNodeImpl extends TAstNode {
   JobImpl getEnclosingJob() { result.getAChildNode*() = this.getParentNode() }
 
   /**
-   * Gets the enclosing workflow statement.
+   * Gets the enclosing workflow if any.
    */
   WorkflowImpl getEnclosingWorkflow() { this = result.getAChildNode*() }
 
+  /**
+   * Gets the enclosing composite action if any.
+   */
+  CompositeActionImpl getEnclosingCompositeAction() { this = result.getAChildNode*() }
+
   /**
    * Gets a environment variable expression by name in the scope of the current node.
    */

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

@@ -17,7 +17,16 @@ import codeql.actions.security.EnvVarInjectionQuery
 import EnvVarInjectionFlow::PathGraph
 
 from EnvVarInjectionFlow::PathNode source, EnvVarInjectionFlow::PathNode sink
-where EnvVarInjectionFlow::flowPath(source, sink)
+where
+  EnvVarInjectionFlow::flowPath(source, sink) and
+  (
+    exists(source.getNode().asExpr().getEnclosingCompositeAction())
+    or
+    exists(Workflow w |
+      w = source.getNode().asExpr().getEnclosingWorkflow() and
+      not w.isPrivileged()
+    )
+  )
 select sink.getNode(), source, sink,
   "Potential environment variable injection in $@, which may be controlled by an external user.",
   sink, sink.getNode().asExpr().(Expression).getRawExpression()

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

@@ -16,16 +16,13 @@ import actions
 import codeql.actions.security.EnvVarInjectionQuery
 import EnvVarInjectionFlow::PathGraph
 
-predicate isSingleTriggerWorkflow(Workflow w, string trigger) {
-  w.getATriggerEvent() = trigger and
-  count(string t | w.getATriggerEvent() = t | t) = 1
-}
-
-from EnvVarInjectionFlow::PathNode source, EnvVarInjectionFlow::PathNode sink, Workflow w
+from EnvVarInjectionFlow::PathNode source, EnvVarInjectionFlow::PathNode sink
 where
   EnvVarInjectionFlow::flowPath(source, sink) and
-  w = source.getNode().asExpr().getEnclosingWorkflow() and
-  not isSingleTriggerWorkflow(w, "pull_request")
+  exists(Workflow w |
+    w = source.getNode().asExpr().getEnclosingWorkflow() and
+    w.isPrivileged()
+  )
 select sink.getNode(), source, sink,
   "Potential privileged environment variable injection in $@, which may be controlled by an external user.",
   sink, sink.getNode().asExpr().(Expression).getRawExpression()

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

@@ -17,7 +17,16 @@ import codeql.actions.security.CommandInjectionQuery
 import CommandInjectionFlow::PathGraph
 
 from CommandInjectionFlow::PathNode source, CommandInjectionFlow::PathNode sink
-where CommandInjectionFlow::flowPath(source, sink)
+where
+  CommandInjectionFlow::flowPath(source, sink) and
+  (
+    exists(source.getNode().asExpr().getEnclosingCompositeAction())
+    or
+    exists(Workflow w |
+      w = source.getNode().asExpr().getEnclosingWorkflow() and
+      not w.isPrivileged()
+    )
+  )
 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-078/PrivilegedCommandInjection.ql

@@ -16,16 +16,13 @@ import actions
 import codeql.actions.security.CommandInjectionQuery
 import CommandInjectionFlow::PathGraph
 
-predicate isSingleTriggerWorkflow(Workflow w, string trigger) {
-  w.getATriggerEvent() = trigger and
-  count(string t | w.getATriggerEvent() = t | t) = 1
-}
-
-from CommandInjectionFlow::PathNode source, CommandInjectionFlow::PathNode sink, Workflow w
+from CommandInjectionFlow::PathNode source, CommandInjectionFlow::PathNode sink
 where
   CommandInjectionFlow::flowPath(source, sink) and
-  w = source.getNode().asExpr().getEnclosingWorkflow() and
-  not isSingleTriggerWorkflow(w, "pull_request")
+  exists(Workflow w |
+    w = source.getNode().asExpr().getEnclosingWorkflow() and
+    w.isPrivileged()
+  )
 select sink.getNode(), source, sink,
   "Potential privileged command injection in $@, which may be controlled by an external user.",
   sink, sink.getNode().asExpr().(Expression).getRawExpression()

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

@@ -19,7 +19,16 @@ import codeql.actions.security.CodeInjectionQuery
 import CodeInjectionFlow::PathGraph
 
 from CodeInjectionFlow::PathNode source, CodeInjectionFlow::PathNode sink
-where CodeInjectionFlow::flowPath(source, sink)
+where
+  CodeInjectionFlow::flowPath(source, sink) and
+  (
+    exists(source.getNode().asExpr().getEnclosingCompositeAction())
+    or
+    exists(Workflow w |
+      w = source.getNode().asExpr().getEnclosingWorkflow() and
+      not w.isPrivileged()
+    )
+  )
 select sink.getNode(), source, sink,
   "Potential code injection in $@, which may be controlled by an external user.", sink,
   sink.getNode().asExpr().(Expression).getRawExpression()

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

@@ -18,16 +18,13 @@ import actions
 import codeql.actions.security.CodeInjectionQuery
 import CodeInjectionFlow::PathGraph
 
-predicate isSingleTriggerWorkflow(Workflow w, string trigger) {
-  w.getATriggerEvent() = trigger and
-  count(string t | w.getATriggerEvent() = t | t) = 1
-}
-
-from CodeInjectionFlow::PathNode source, CodeInjectionFlow::PathNode sink, Workflow w
+from CodeInjectionFlow::PathNode source, CodeInjectionFlow::PathNode sink
 where
   CodeInjectionFlow::flowPath(source, sink) and
-  w = source.getNode().asExpr().getEnclosingWorkflow() and
-  not isSingleTriggerWorkflow(w, "pull_request")
+  exists(Workflow w |
+    w = source.getNode().asExpr().getEnclosingWorkflow() and
+    w.isPrivileged()
+  )
 select sink.getNode(), source, sink,
   "Potential privileged code injection in $@, which may be controlled by an external user.", sink,
   sink.getNode().asExpr().(Expression).getRawExpression()

ql/src/Security/CWE-829/ArtifactPoisoning.ql

@@ -14,27 +14,9 @@
 import actions
 import codeql.actions.security.ArtifactPoisoningQuery
 
-predicate isSingleTriggerWorkflow(Workflow w, string trigger) {
-  w.getATriggerEvent() = trigger and
-  count(string t | w.getATriggerEvent() = t | t) = 1
-}
-
-from Workflow w, LocalJob job, ArtifactDownloadStep download, Step run
+from LocalJob job, ArtifactDownloadStep download, Step run
 where
-  w = job.getWorkflow() and
-  (
-    // The Workflow is triggered by an event other than `pull_request`
-    not isSingleTriggerWorkflow(w, "pull_request")
-    or
-    // The Workflow is only triggered by `workflow_call` and there is
-    // a caller workflow triggered by an event other than `pull_request`
-    isSingleTriggerWorkflow(w, "workflow_call") and
-    exists(ExternalJob call, Workflow caller |
-      call.getCallee() = w.getLocation().getFile().getRelativePath() and
-      caller = call.getWorkflow() and
-      not isSingleTriggerWorkflow(caller, "pull_request")
-    )
-  ) and
+  job.getWorkflow().isPrivileged() and
   (run instanceof Run or run instanceof UsesStep) and
   exists(int i, int j |
     job.getStep(i) = download and

ql/src/Security/CWE-829/UntrustedCheckout.ql

@@ -121,26 +121,9 @@ class GitCheckout extends PRHeadCheckoutStep instanceof Run {
   }
 }
 
-predicate isSingleTriggerWorkflow(Workflow w, string trigger) {
-  w.getATriggerEvent() = trigger and
-  count(string t | w.getATriggerEvent() = t | t) = 1
-}
-
 from Workflow w, PRHeadCheckoutStep checkout
 where
-  (
-    // The Workflow is triggered by an event other than `pull_request`
-    not isSingleTriggerWorkflow(w, "pull_request")
-    or
-    // The Workflow is only triggered by `workflow_call` and there is
-    // a caller workflow triggered by an event other than `pull_request`
-    isSingleTriggerWorkflow(w, "workflow_call") and
-    exists(ExternalJob call, Workflow caller |
-      call.getCallee() = w.getLocation().getFile().getRelativePath() and
-      caller = call.getWorkflow() and
-      not isSingleTriggerWorkflow(caller, "pull_request")
-    )
-  ) and
+  w.isPrivileged() and
   w.getAJob().(LocalJob).getAStep() = checkout and
   not exists(ControlCheck check |
     checkout.getIf() = check or checkout.getEnclosingJob().getIf() = check