Merge pull request #10 from GitHubSecurityLab/job_outputs

feat(field-flow): Refactor flow through job outputs

Author: Alvaro Muñoz

ql/lib/codeql/actions/Ast.qll

@@ -149,7 +149,7 @@ class JobStmt extends Statement instanceof Actions::Job {
    *   out1: ${steps.foo.bar}
    *   out2: ${steps.foo.baz}
    */
-  JobOutputStmt getOutputStmt() { result = this.(Actions::Job).lookup("outputs") }
+  OutputsStmt getOutputsStmt() { result = this.(Actions::Job).lookup("outputs") }
 
   /**
    * Reusable workflow jobs may have Uses children
@@ -166,28 +166,6 @@ class JobStmt extends Statement instanceof Actions::Job {
   }
 }
 
-/**
- * Declaration of the outputs for the job.
- * eg:
- *   out1: ${steps.foo.bar}
- *   out2: ${steps.foo.baz}
- */
-class JobOutputStmt extends Statement instanceof YamlMapping {
-  JobStmt job;
-
-  JobOutputStmt() { job.(YamlMapping).lookup("outputs") = this }
-
-  YamlMapping asYamlMapping() { result = this }
-
-  /**
-   * Gets a specific value expression
-   * eg: ${steps.foo.bar}
-   */
-  Expression getOutputExpr(string id) {
-    this.(YamlMapping).maps(any(YamlScalar s | s.getValue() = id), result)
-  }
-}
-
 /**
  * A Step is a single task that can be executed as part of a job.
  */
@@ -435,9 +413,9 @@ class NeedsCtxAccessExpr extends CtxAccessExpr {
     job.getLocation().getFile() = this.getLocation().getFile() and
     (
       // regular jobs
-      job.getOutputStmt().getOutputExpr(fieldName) = result
+      job.getOutputsStmt() = result
       or
-      // jobs calling reusable workflows
+      // reusable workflow calling jobs
       job.getUsesExpr() = result
     )
   }
@@ -464,7 +442,7 @@ class JobsCtxAccessExpr extends CtxAccessExpr {
     exists(JobStmt job |
       job.getId() = jobId and
       job.getLocation().getFile() = this.getLocation().getFile() and
-      job.getOutputStmt().getOutputExpr(fieldName) = result
+      job.getOutputsStmt() = result
     )
   }
 }

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

@@ -231,7 +231,7 @@ private class JobTree extends StandardPreOrderTree instanceof JobStmt {
       rank[i](Expression child, Location l |
         (
           child = super.getAStepStmt() or
-          child = super.getOutputStmt() or
+          child = super.getOutputsStmt() or
           child = super.getUsesExpr()
         ) and
         l = child.getLocation()
@@ -243,10 +243,6 @@ private class JobTree extends StandardPreOrderTree instanceof JobStmt {
   }
 }
 
-private class JobOutputTree extends StandardPreOrderTree instanceof JobOutputStmt {
-  override ControlFlowTree getChildNode(int i) { result = super.asYamlMapping().getValueNode(i) }
-}
-
 private class StepUsesTree extends StandardPreOrderTree instanceof StepUsesExpr {
   override ControlFlowTree getChildNode(int i) {
     result =

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

@@ -50,22 +50,22 @@ predicate externallyDefinedSource(DataFlow::Node source, string sourceType, stri
     ) and
     (
       if fieldName.trim().matches("env.%")
-      then source.asExpr() = uses.getEnvExpr(fieldName.trim().replaceAll("env\\.", ""))
+      then source.asExpr() = uses.getEnvExpr(fieldName.trim().replaceAll("env.", ""))
       else
         if fieldName.trim().matches("output.%")
-        then
-          // 'output.' is the default qualifier
-          source.asExpr() = uses
+        then source.asExpr() = uses
         else none()
     ) and
     sourceType = kind
   )
 }
 
-predicate externallyDefinedSummary(DataFlow::Node pred, DataFlow::Node succ, DataFlow::ContentSet c) {
+predicate externallyDefinedStoreStep(
+  DataFlow::Node pred, DataFlow::Node succ, DataFlow::ContentSet c
+) {
   exists(UsesExpr uses, string action, string version, string input, string output |
-    c = any(DataFlow::FieldContent ct | ct.getName() = output.replaceAll("output\\.", "")) and
     summaryModel(action, version, input, output, "taint") and
+    c = any(DataFlow::FieldContent ct | ct.getName() = output.replaceAll("output.", "")) and
     uses.getCallee() = action.toLowerCase() and
     (
       if version.trim() = "*"
@@ -74,10 +74,11 @@ predicate externallyDefinedSummary(DataFlow::Node pred, DataFlow::Node succ, Dat
     ) and
     (
       if input.trim().matches("env.%")
-      then pred.asExpr() = uses.getEnvExpr(input.trim().replaceAll("env\\.", ""))
+      then pred.asExpr() = uses.getEnvExpr(input.trim().replaceAll("env.", ""))
       else
-        // 'input.' is the default qualifier
-        pred.asExpr() = uses.getArgumentExpr(input.trim().replaceAll("input\\.", ""))
+        if input.trim().matches("input.%")
+        then pred.asExpr() = uses.getArgumentExpr(input.trim().replaceAll("input.", ""))
+        else none()
     ) and
     succ.asExpr() = uses
   )
@@ -87,8 +88,11 @@ predicate externallyDefinedSink(DataFlow::ExprNode sink, string kind) {
   exists(UsesExpr uses, string action, string version, string input |
     (
       if input.trim().matches("env.%")
-      then sink.asExpr() = uses.getEnvExpr(input.trim().replaceAll("input\\.", ""))
-      else sink.asExpr() = uses.getArgumentExpr(input.trim())
+      then sink.asExpr() = uses.getEnvExpr(input.trim().replaceAll("env.", ""))
+      else
+        if input.trim().matches("input.%")
+        then sink.asExpr() = uses.getArgumentExpr(input.trim().replaceAll("input.", ""))
+        else none()
     ) and
     sinkModel(action, version, input, kind) and
     uses.getCallee() = action.toLowerCase() and

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

@@ -40,7 +40,7 @@ class AdditionalTaintStep extends Unit {
  *      echo "foo=$(echo $TAINTED)" >> $GITHUB_OUTPUT
  *      echo "test=${{steps.step1.outputs.MSG}}" >> "$GITHUB_OUTPUT"
  */
-predicate runEnvToScriptstep(DataFlow::Node pred, DataFlow::Node succ, DataFlow::ContentSet c) {
+predicate runEnvToScriptStoreStep(DataFlow::Node pred, DataFlow::Node succ, DataFlow::ContentSet c) {
   exists(RunExpr r, string varName, string output |
     c = any(DataFlow::FieldContent ct | ct.getName() = output.replaceAll("output\\.", "")) and
     r.getEnvExpr(varName) = pred.asExpr() and

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

@@ -133,6 +133,7 @@ predicate typeStrongerThan(DataFlowType t1, DataFlowType t2) { none() }
 
 newtype TContent =
   TFieldContent(string name) {
+    // We only use field flow for steps and jobs outputs, not for accessing other context fields such as env or inputs
     name = any(StepsCtxAccessExpr a).getFieldName() or
     name = any(NeedsCtxAccessExpr a).getFieldName() or
     name = any(JobsCtxAccessExpr a).getFieldName()
@@ -188,35 +189,22 @@ class ArgumentPosition extends string {
 predicate parameterMatch(ParameterPosition ppos, ArgumentPosition apos) { ppos = apos }
 
 /**
- * Holds if there is a local flow step between a ${{}} expression accesing a step output variable and the step output itself
- * But only for those cases where the step output is defined externally in a MaD specification.
- * The reason for this is that we don't currently have a way to specify that a source starts with a non-empty access
- * path so  the easiest thing is to add the corresponding read steps of that field as local flow steps as well.
- * e.g. ${{ steps.step1.output.foo }}
+ * Holds if there is a local flow step between a ${{ steps.xxx.outputs.yyy }} expression accesing a step output field
+ * and the step output itself. But only for those cases where the step output is defined externally in a MaD Source
+ * specification. The reason for this is that we don't currently have a way to specify that a source starts with a
+ * non-empty access path so we cannot write a Source that stores the taint in a Content, we can only do that for steps
+ * (storeStep). The easiest thing is to add this local flow step that simulates a read step from the source node for a specific
+ * field name.
  */
 predicate stepsCtxLocalStep(Node nodeFrom, Node nodeTo) {
-  exists(StepStmt astFrom, StepsCtxAccessExpr astTo |
+  exists(UsesExpr astFrom, StepsCtxAccessExpr astTo |
     externallyDefinedSource(nodeFrom, _, "output." + astTo.getFieldName()) and
-    astFrom instanceof UsesExpr and
     astFrom = nodeFrom.asExpr() and
     astTo = nodeTo.asExpr() and
     astTo.getRefExpr() = astFrom
   )
 }
 
-/**
- * Holds if there is a local flow step between a ${{}} expression accesing a job output variable and the job output itself
- * e.g. ${{ needs.job1.output.foo }} or ${{ jobs.job1.output.foo }}
- */
-predicate jobsCtxLocalStep(Node nodeFrom, Node nodeTo) {
-  exists(Expression astFrom, CtxAccessExpr astTo |
-    astFrom = nodeFrom.asExpr() and
-    astTo = nodeTo.asExpr() and
-    astTo.getRefExpr() = astFrom and
-    (astTo instanceof NeedsCtxAccessExpr or astTo instanceof JobsCtxAccessExpr)
-  )
-}
-
 /**
  * Holds if there is a local flow step between a ${{}} expression accesing an input variable and the input itself
  * e.g. ${{ inputs.foo }}
@@ -252,7 +240,6 @@ predicate envCtxLocalStep(Node nodeFrom, Node nodeTo) {
 pragma[nomagic]
 predicate localFlowStep(Node nodeFrom, Node nodeTo) {
   stepsCtxLocalStep(nodeFrom, nodeTo) or
-  jobsCtxLocalStep(nodeFrom, nodeTo) or
   inputsCtxLocalStep(nodeFrom, nodeTo) or
   envCtxLocalStep(nodeFrom, nodeTo)
 }
@@ -273,42 +260,35 @@ predicate simpleLocalFlowStep(Node nodeFrom, Node nodeTo) { localFlowStep(nodeFr
 predicate jumpStep(Node nodeFrom, Node nodeTo) { none() }
 
 /**
- * A read step to read the value of a ReusableWork uses step  and connect it to its
- * corresponding JobOutputAccessExpr
+ * Holds if a CtxAccessExpr reads a field from a job (needs/jobs), step (steps) output via a read of `c` (fieldname)
  */
-predicate reusableWorkflowReturnReadStep(Node node1, Node node2, ContentSet c) {
-  exists(NeedsCtxAccessExpr expr, string fieldName |
-    expr.usesReusableWorkflow() and
-    expr.getRefExpr() = node1.asExpr() and
-    expr.getFieldName() = fieldName and
-    expr = node2.asExpr() and
-    c = any(FieldContent ct | ct.getName() = fieldName)
+predicate ctxFieldReadStep(Node node1, Node node2, ContentSet c) {
+  exists(CtxAccessExpr access |
+    (
+      access instanceof NeedsCtxAccessExpr or
+      access instanceof StepsCtxAccessExpr or
+      access instanceof JobsCtxAccessExpr
+    ) and
+    c = any(FieldContent ct | ct.getName() = access.getFieldName()) and
+    node1.asExpr() = access.getRefExpr() and
+    node2.asExpr() = access
   )
 }
 
 /**
  * Holds if data can flow from `node1` to `node2` via a read of `c`.  Thus,
  * `node1` references an object with a content `c.getAReadContent()` whose
  * value ends up in `node2`.
+ * Store steps without corresponding reads are pruned aggressively very early, since they can never contribute to a complete path.
  */
-predicate readStep(Node node1, ContentSet c, Node node2) {
-  // TODO: Extract to its own predicate
-  exists(StepsCtxAccessExpr access |
-    c = any(FieldContent ct | ct.getName() = access.getFieldName()) and
-    node1.asExpr() = access.getRefExpr() and
-    node2.asExpr() = access
-  )
-  or
-  reusableWorkflowReturnReadStep(node1, node2, c)
-}
+predicate readStep(Node node1, ContentSet c, Node node2) { ctxFieldReadStep(node1, node2, c) }
 
 /**
- * A store step to store the value of a ReusableWorkflowStmt output expr into the return node (node2)
- * with a given access path (fieldName)
+ * Stores an output expression (node1) into its OutputsStm node (node2)
+ * using the output variable name as the access path
  */
-predicate reusableWorkflowReturnStoreStep(Node node1, Node node2, ContentSet c) {
-  exists(ReusableWorkflowStmt stmt, OutputsStmt out, string fieldName |
-    out = stmt.getOutputsStmt() and
+predicate fieldStoreStep(Node node1, Node node2, ContentSet c) {
+  exists(OutputsStmt out, string fieldName |
     node1.asExpr() = out.getOutputExpr(fieldName) and
     node2.asExpr() = out and
     c = any(FieldContent ct | ct.getName() = fieldName)
@@ -319,15 +299,12 @@ predicate reusableWorkflowReturnStoreStep(Node node1, Node node2, ContentSet c)
  * Holds if data can flow from `node1` to `node2` via a store into `c`.  Thus,
  * `node2` references an object with a content `c.getAStoreContent()` that
  * contains the value of `node1`.
+ * Store steps without corresponding reads are pruned aggressively very early, since they can never contribute to a complete path.
  */
 predicate storeStep(Node node1, ContentSet c, Node node2) {
-  reusableWorkflowReturnStoreStep(node1, node2, c)
-  or
-  // TODO: rename to xxxxStoreStep
-  externallyDefinedSummary(node1, node2, c)
-  or
-  // TODO: rename to xxxxStoreStep
-  runEnvToScriptstep(node1, node2, c)
+  fieldStoreStep(node1, node2, c) or
+  externallyDefinedStoreStep(node1, node2, c) or
+  runEnvToScriptStoreStep(node1, node2, c)
 }
 
 /**

ql/lib/ext/PLACEHOLDER.model.yml

@@ -0,0 +1,7 @@
+extensions:
+  - addsTo:
+      pack: codeql/actions-all
+      extensible: sinkModel
+    data:
+      - ["","","",""]
+

ql/lib/ext/frabert_replace-string-action.model.yml

@@ -3,5 +3,5 @@ extensions:
       pack: codeql/actions-all
       extensible: summaryModel
     data:
-      - ["frabert/replace-string-action", "*", "string", "replaced", "taint"]
-      - ["frabert/replace-string-action", "*", "replace-with", "replaced", "taint"]
+      - ["frabert/replace-string-action", "*", "input.string", "output.replaced", "taint"]
+      - ["frabert/replace-string-action", "*", "input.replace-with", "output.replaced", "taint"]

ql/lib/ext/mad9000_actions-find-and-replace-string.model.yml

@@ -3,5 +3,5 @@ extensions:
       pack: codeql/actions-all
       extensible: summaryModel
     data:
-      - ["mad9000/actions-find-and-replace-string", "*", "source", "value", "taint"]
-      - ["mad9000/actions-find-and-replace-string", "*", "replace", "value", "taint"]
+      - ["mad9000/actions-find-and-replace-string", "*", "input.source", "output.value", "taint"]
+      - ["mad9000/actions-find-and-replace-string", "*", "input.replace", "output.value", "taint"]