Merge pull request #6 from GitHubSecurityLab/composite_actions

feat: support for composite action's analysis

Author: Alvaro Muñoz

ql/lib/codeql/actions/Ast.qll

@@ -27,6 +27,25 @@ class Statement extends AstNode { }
  */
 class Expression extends Statement { }
 
+/**
+ * A composite action
+ */
+class CompositeActionStmt extends Statement instanceof Actions::CompositeAction {
+  RunsStmt getRunsStmt() { result = super.getRuns() }
+
+  InputsStmt getInputsStmt() { result = this.(YamlMapping).lookup("inputs") }
+
+  OutputsStmt getOutputsStmt() { result = this.(YamlMapping).lookup("outputs") }
+
+  string getName() { result = this.getLocation().getFile().getRelativePath() }
+}
+
+class RunsStmt extends Statement instanceof Actions::Runs {
+  StepStmt getAStepStmt() { result = super.getSteps().getElementNode(_) }
+
+  StepStmt getStepStmt(int i) { result = super.getSteps().getElementNode(i) }
+}
+
 /**
  * A Github Actions Workflow
  */
@@ -43,67 +62,45 @@ class ReusableWorkflowStmt extends WorkflowStmt {
     this.(Actions::Workflow).getOn().getNode("workflow_call") = workflow_call
   }
 
-  ReusableWorkflowInputsStmt getInputsStmt() {
-    result = workflow_call.(YamlMapping).lookup("inputs")
-  }
+  InputsStmt getInputsStmt() { result = workflow_call.(YamlMapping).lookup("inputs") }
 
-  ReusableWorkflowOutputsStmt getOutputsStmt() {
-    result = workflow_call.(YamlMapping).lookup("outputs")
-  }
+  OutputsStmt getOutputsStmt() { result = workflow_call.(YamlMapping).lookup("outputs") }
 
   string getName() { result = this.getLocation().getFile().getRelativePath() }
 }
 
-class ReusableWorkflowInputsStmt extends Statement instanceof YamlMapping {
-  ReusableWorkflowInputsStmt() {
-    exists(Actions::On on | on.getNode("workflow_call").(YamlMapping).lookup("inputs") = this)
-  }
+class InputsStmt extends Statement instanceof YamlMapping {
+  YamlMapping parent;
+
+  InputsStmt() { parent.lookup("inputs") = this }
 
   /**
-   * Gets a specific parameter expression (YamlMapping) by name.
-   * eg:
-   * on:
-   *   workflow_call:
-   *     inputs:
-   *       config-path:
-   *         required: true
-   *         type: string
-   *     secrets:
-   *       token:
-   *         required: true
+   * Gets a specific input expression (YamlMapping) by name.
    */
-  ReusableWorkflowInputExpr getInputExpr(string name) {
+  InputExpr getInputExpr(string name) {
     result.(YamlString).getValue() = name and
     this.(YamlMapping).maps(result, _)
   }
 }
 
-class ReusableWorkflowInputExpr extends Expression instanceof YamlString { }
+class OutputsStmt extends Statement instanceof YamlMapping {
+  YamlMapping parent;
 
-class ReusableWorkflowOutputsStmt extends Statement instanceof YamlMapping {
-  ReusableWorkflowOutputsStmt() {
-    exists(Actions::On on | on.getNode("workflow_call").(YamlMapping).lookup("outputs") = this)
-  }
+  OutputsStmt() { parent.lookup("outputs") = this }
 
   /**
-   * Gets a specific parameter expression (YamlMapping) by name.
-   * eg:
-   * on:
-   *   workflow_call:
-   *     outputs:
-   *       firstword:
-   *         description: "The first output string"
-   *         value: ${{ jobs.example_job.outputs.output1 }}
-   *       secondword:
-   *         description: "The second output string"
-   *         value: ${{ jobs.example_job.outputs.output2 }}
+   * Gets a specific output expression (YamlMapping) by name.
    */
-  ReusableWorkflowOutputExpr getOutputExpr(string name) {
+  OutputExpr getOutputExpr(string name) {
     this.(YamlMapping).lookup(name).(YamlMapping).lookup("value") = result
   }
 }
 
-class ReusableWorkflowOutputExpr extends Expression instanceof YamlString { }
+// TODO: Needs a characteristic predicate otherwise anything is an output expression
+class InputExpr extends Expression instanceof YamlString { }
+
+// TODO: Needs a characteristic predicate otherwise anything is an output expression
+class OutputExpr extends Expression instanceof YamlString { }
 
 /**
  * A Job is a collection of steps that run in an execution environment.
@@ -369,7 +366,7 @@ class StepOutputAccessExpr extends ExprAccessExpr {
   }
 
   override Expression getRefExpr() {
-    this.getJobStmt() = result.(StepStmt).getJobStmt() and
+    this.getLocation().getFile() = result.getLocation().getFile() and
     result.(StepStmt).getId() = stepId
   }
 }
@@ -413,10 +410,10 @@ class JobOutputAccessExpr extends ExprAccessExpr {
  * https://docs.github.com/en/actions/learn-github-actions/contexts#context-availability
  * e.g. `${{ inputs.foo }}`
  */
-class ReusableWorkflowInputAccessExpr extends ExprAccessExpr {
+class InputAccessExpr extends ExprAccessExpr {
   string paramName;
 
-  ReusableWorkflowInputAccessExpr() {
+  InputAccessExpr() {
     paramName = this.getExpression().regexpCapture("inputs\\.([A-Za-z0-9_-]+)", 1)
   }
 
@@ -425,6 +422,11 @@ class ReusableWorkflowInputAccessExpr extends ExprAccessExpr {
       w.getLocation().getFile() = this.getLocation().getFile() and
       w.getInputsStmt().getInputExpr(paramName) = result
     )
+    or
+    exists(CompositeActionStmt a |
+      a.getLocation().getFile() = this.getLocation().getFile() and
+      a.getInputsStmt().getInputExpr(paramName) = result
+    )
   }
 }
 

ql/lib/codeql/actions/controlflow/internal/Cfg.qll

@@ -88,6 +88,8 @@ module CfgScope {
   abstract class CfgScope extends AstNode { }
 
   class WorkflowScope extends CfgScope instanceof WorkflowStmt { }
+
+  class CompositeActionScope extends CfgScope instanceof CompositeActionStmt { }
 }
 
 private module Implementation implements CfgShared::InputSig<Location> {
@@ -120,9 +122,15 @@ private module Implementation implements CfgShared::InputSig<Location> {
 
   int maxSplits() { result = 0 }
 
-  predicate scopeFirst(CfgScope scope, AstNode e) { first(scope.(WorkflowStmt), e) }
+  predicate scopeFirst(CfgScope scope, AstNode e) {
+    first(scope.(WorkflowStmt), e) or
+    first(scope.(CompositeActionStmt), e)
+  }
 
-  predicate scopeLast(CfgScope scope, AstNode e, Completion c) { last(scope.(WorkflowStmt), e, c) }
+  predicate scopeLast(CfgScope scope, AstNode e, Completion c) {
+    last(scope.(WorkflowStmt), e, c) or
+    last(scope.(CompositeActionStmt), e, c)
+  }
 
   predicate successorTypeIsSimple(SuccessorType t) { t instanceof NormalSuccessor }
 
@@ -139,6 +147,28 @@ private import CfgImpl
 private import Completion
 private import CfgScope
 
+private class CompositeActionTree extends StandardPreOrderTree instanceof CompositeActionStmt {
+  override ControlFlowTree getChildNode(int i) {
+    result =
+      rank[i](Expression child, Location l |
+        (
+          child = this.(CompositeActionStmt).getInputsStmt() or
+          child = this.(CompositeActionStmt).getOutputsStmt() or
+          child = this.(CompositeActionStmt).getRunsStmt()
+        ) and
+        l = child.getLocation()
+      |
+        child
+        order by
+          l.getStartLine(), l.getStartColumn(), l.getEndColumn(), l.getEndLine(), child.toString()
+      )
+  }
+}
+
+private class RunsTree extends StandardPreOrderTree instanceof RunsStmt {
+  override ControlFlowTree getChildNode(int i) { result = super.getStepStmt(i) }
+}
+
 private class WorkflowTree extends StandardPreOrderTree instanceof WorkflowStmt {
   override ControlFlowTree getChildNode(int i) {
     if this instanceof ReusableWorkflowStmt
@@ -169,8 +199,7 @@ private class WorkflowTree extends StandardPreOrderTree instanceof WorkflowStmt
   }
 }
 
-private class ReusableWorkflowInputsTree extends StandardPreOrderTree instanceof ReusableWorkflowInputsStmt
-{
+private class InputsTree extends StandardPreOrderTree instanceof InputsStmt {
   override ControlFlowTree getChildNode(int i) {
     result =
       rank[i](Expression child, Location l |
@@ -183,10 +212,9 @@ private class ReusableWorkflowInputsTree extends StandardPreOrderTree instanceof
   }
 }
 
-private class InputExprTree extends LeafTree instanceof ReusableWorkflowInputExpr { }
+private class InputExprTree extends LeafTree instanceof InputExpr { }
 
-private class ReusableWorkflowOutputsTree extends StandardPreOrderTree instanceof ReusableWorkflowOutputsStmt
-{
+private class OutputsTree extends StandardPreOrderTree instanceof OutputsStmt {
   override ControlFlowTree getChildNode(int i) {
     result =
       rank[i](Expression child, Location l |
@@ -199,7 +227,7 @@ private class ReusableWorkflowOutputsTree extends StandardPreOrderTree instanceo
   }
 }
 
-private class OutputExprTree extends LeafTree instanceof ReusableWorkflowOutputExpr { }
+private class OutputExprTree extends LeafTree instanceof OutputExpr { }
 
 private class JobTree extends StandardPreOrderTree instanceof JobStmt {
   override ControlFlowTree getChildNode(int i) {

ql/lib/codeql/actions/dataflow/FlowSources.qll

@@ -161,3 +161,14 @@ private class ExternallyDefinedSource extends RemoteFlowSource {
 
   override string getSourceType() { result = soutceType }
 }
+
+/**
+ * Composite action input sources
+ */
+private class CompositeActionInputSource extends RemoteFlowSource {
+  CompositeActionStmt c;
+
+  CompositeActionInputSource() { c.getInputsStmt().getInputExpr(_) = this.asExpr() }
+
+  override string getSourceType() { result = "Composite action input" }
+}

ql/lib/codeql/actions/dataflow/FlowSteps.qll

@@ -83,7 +83,7 @@ predicate runEnvToScriptstep(DataFlow::Node pred, DataFlow::Node succ) {
       line = script.splitAt("\n") and
       (
         line.regexpMatch(".*::set-output\\s+name.*") or
-        line.regexpMatch(".*>>\\s*$GITHUB_ENV.*")
+        line.regexpMatch(".*>>\\s*\\$GITHUB_OUTPUT.*")
       ) and
       script.indexOf("$" + ["", "{", "ENV{"] + varName) > 0
     ) and

ql/lib/codeql/actions/dataflow/internal/DataFlowPrivate.qll

@@ -82,7 +82,10 @@ class DataFlowCallable instanceof Cfg::CfgScope {
   string getName() {
     if this instanceof ReusableWorkflowStmt
     then result = this.(ReusableWorkflowStmt).getName()
-    else none()
+    else
+      if this instanceof CompositeActionStmt
+      then result = this.(CompositeActionStmt).getName()
+      else none()
   }
 }
 
@@ -190,11 +193,11 @@ predicate jobsCtxLocalStep(Node nodeFrom, Node nodeTo) {
 }
 
 /**
- * Holds if there is a local flow step between a ${{}} expression accesing a reusable workflow input variable and the input itself
+ * Holds if there is a local flow step between a ${{}} expression accesing an input variable and the input itself
  * e.g. ${{ inputs.foo }}
  */
 predicate inputsCtxLocalStep(Node nodeFrom, Node nodeTo) {
-  exists(Expression astFrom, ReusableWorkflowInputAccessExpr astTo |
+  exists(Expression astFrom, InputAccessExpr astTo |
     astFrom = nodeFrom.asExpr() and
     astTo = nodeTo.asExpr() and
     astTo.getRefExpr() = astFrom

ql/lib/codeql/actions/dataflow/internal/DataFlowPublic.qll

@@ -48,7 +48,7 @@ class ExprNode extends Node, TExprNode {
  * Reusable workflow input nodes
  */
 class ParameterNode extends ExprNode {
-  private ReusableWorkflowInputExpr parameter;
+  private InputExpr parameter;
 
   ParameterNode() {
     this.asExpr() = parameter and
@@ -63,7 +63,7 @@ class ParameterNode extends ExprNode {
 
   override Location getLocation() { result = parameter.getLocation() }
 
-  ReusableWorkflowInputExpr getInputExpr() { result = parameter }
+  InputExpr getInputExpr() { result = parameter } 
 }
 
 /**
@@ -87,7 +87,8 @@ class ReturnNode extends ExprNode {
 
   ReturnNode() {
     this.getCfgNode() = node and
-    node.getAstNode() = any(ReusableWorkflowStmt w).getOutputsStmt().getOutputExpr(_)
+    (node.getAstNode() = any(ReusableWorkflowStmt w).getOutputsStmt().getOutputExpr(_) or
+    node.getAstNode() = any(CompositeActionStmt a).getOutputsStmt().getOutputExpr(_))
   }
 
   ReturnKind getKind() { result = TNormalReturn() }

ql/lib/test/test.ql

@@ -45,7 +45,7 @@ query predicate parentNodes(AstNode child, AstNode parent) { child.getParentNode
 
 query predicate cfgNodes(Cfg::Node n) {
   //any()
-  n.getAstNode() instanceof ReusableWorkflowOutputsStmt
+  n.getAstNode() instanceof OutputsStmt
 }
 
 query predicate dfNodes(DataFlow::Node e) {

ql/src/Security/CWE-020/CompositeActionSummaries.ql

@@ -0,0 +1,36 @@
+/**
+ * @name Composite Action Summaries
+ * @description Actions that pass user-controlled data to their output variables.
+ * @kind path-problem
+ * @problem.severity warning
+ * @security-severity 9.3
+ * @precision high
+ * @id actions/composite-action-summaries
+ * @tags actions
+ *       external/cwe/cwe-020
+ */
+
+import actions
+import codeql.actions.TaintTracking
+import codeql.actions.dataflow.FlowSources
+import codeql.actions.dataflow.ExternalFlow
+
+private class OutputVariableSink extends DataFlow::Node {
+  OutputVariableSink() { exists(OutputsStmt s | s.getOutputExpr(_) = this.asExpr()) }
+}
+
+private module MyConfig implements DataFlow::ConfigSig {
+  predicate isSource(DataFlow::Node source) {
+    exists(CompositeActionStmt c | c.getInputsStmt().getInputExpr(_) = source.asExpr())
+  }
+
+  predicate isSink(DataFlow::Node sink) { sink instanceof OutputVariableSink }
+}
+
+module MyFlow = TaintTracking::Global<MyConfig>;
+
+import MyFlow::PathGraph
+
+from MyFlow::PathNode source, MyFlow::PathNode sink
+where MyFlow::flowPath(source, sink)
+select sink.getNode(), source, sink, "Summary"

ql/src/Security/CWE-020/CompositeActionsSources.ql

@@ -0,0 +1,38 @@
+/**
+ * @name Composite Action Sources
+ * @description Actions that pass user-controlled data to their output variables.
+ * @kind path-problem
+ * @problem.severity warning
+ * @security-severity 9.3
+ * @precision high
+ * @id actions/composite-action-sources
+ * @tags actions
+ *       external/cwe/cwe-020
+ */
+
+import actions
+import codeql.actions.TaintTracking
+import codeql.actions.dataflow.FlowSources
+import codeql.actions.dataflow.ExternalFlow
+
+private class OutputVariableSink extends DataFlow::Node {
+  OutputVariableSink() { exists(OutputsStmt s | s.getOutputExpr(_) = this.asExpr()) }
+}
+
+private module MyConfig implements DataFlow::ConfigSig {
+  predicate isSource(DataFlow::Node source) {
+    source instanceof RemoteFlowSource and
+    exists(CompositeActionStmt c | c.getAChildNode*() = source.asExpr()) and
+    not exists(CompositeActionStmt c | c.getInputsStmt().getInputExpr(_) = source.asExpr())
+  }
+
+  predicate isSink(DataFlow::Node sink) { sink instanceof OutputVariableSink }
+}
+
+module MyFlow = TaintTracking::Global<MyConfig>;
+
+import MyFlow::PathGraph
+
+from MyFlow::PathNode source, MyFlow::PathNode sink
+where MyFlow::flowPath(source, sink)
+select sink.getNode(), source, sink, "Source"