From 83ca36bc76ff38f99be4c14a2f07a69b0ab021b5 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Alvaro=20Mu=C3=B1oz?= <pwntester@github.com>
Date: Thu, 8 Feb 2024 11:56:22 +0100
Subject: [PATCH] Support RunExpr's env vars

---
 ql/lib/codeql/actions/Ast.qll                 |  8 +++++
 .../actions/controlflow/internal/Cfg.qll      | 14 ++++++--
 ql/lib/codeql/actions/dataflow/FlowSteps.qll  | 32 +++++++++++++++++++
 .../dataflow/internal/DataFlowPrivate.qll     | 15 +++++++--
 4 files changed, 65 insertions(+), 4 deletions(-)

diff --git a/ql/lib/codeql/actions/Ast.qll b/ql/lib/codeql/actions/Ast.qll
index d2c7fdd4501..d9306b53815 100644
--- a/ql/lib/codeql/actions/Ast.qll
+++ b/ql/lib/codeql/actions/Ast.qll
@@ -169,6 +169,13 @@ class RunExpr extends StepStmt, Expression {
 
   Expression getScriptExpr() { result = scriptExpr }
 
+  Expression getEnvExpr(string name) {
+    exists(Actions::StepEnv env |
+      env.getStep() = this and
+      env.(YamlMapping).maps(any(YamlScalar s | s.getValue() = name), result)
+    )
+  }
+
   string getScript() { result = scriptExpr.getValue() }
 }
 
@@ -183,6 +190,7 @@ class ExprAccessExpr extends Expression instanceof YamlString {
   string getExpression() { result = expr }
 
   JobStmt getJob() { result.getAChildNode*() = this }
+  //override string toString() { result = expr }
 }
 
 /**
diff --git a/ql/lib/codeql/actions/controlflow/internal/Cfg.qll b/ql/lib/codeql/actions/controlflow/internal/Cfg.qll
index c549eb40198..a2ebb10219e 100644
--- a/ql/lib/codeql/actions/controlflow/internal/Cfg.qll
+++ b/ql/lib/codeql/actions/controlflow/internal/Cfg.qll
@@ -171,8 +171,18 @@ private class UsesTree extends StandardPreOrderTree instanceof UsesExpr {
 }
 
 private class RunTree extends StandardPreOrderTree instanceof RunExpr {
-  override ControlFlowTree getChildNode(int i) { result = super.getScriptExpr() and i = 0 }
+  //override ControlFlowTree getChildNode(int i) { result = super.getScriptExpr() and i = 0 }
+  override ControlFlowTree getChildNode(int i) {
+    result =
+      rank[i](Expression child, Location l |
+        (child = super.getEnvExpr(_) or child = super.getScriptExpr()) and
+        l = child.getLocation()
+      |
+        child
+        order by
+          l.getStartLine(), l.getStartColumn(), l.getEndColumn(), l.getEndLine(), child.toString()
+      )
+  }
 }
 
 private class ExprAccessTree extends LeafTree instanceof ExprAccessExpr { }
-
diff --git a/ql/lib/codeql/actions/dataflow/FlowSteps.qll b/ql/lib/codeql/actions/dataflow/FlowSteps.qll
index 528f9e54832..223ff305ba4 100644
--- a/ql/lib/codeql/actions/dataflow/FlowSteps.qll
+++ b/ql/lib/codeql/actions/dataflow/FlowSteps.qll
@@ -20,6 +20,9 @@ class AdditionalTaintStep extends Unit {
   abstract predicate step(DataFlow::Node node1, DataFlow::Node node2);
 }
 
+/**
+ * Holds if actions-find-and-replace-string step is used.
+ */
 private class ActionsFindAndReplaceStringStep extends AdditionalTaintStep {
   override predicate step(DataFlow::Node pred, DataFlow::Node succ) {
     exists(UsesExpr u |
@@ -29,3 +32,32 @@ private class ActionsFindAndReplaceStringStep extends AdditionalTaintStep {
     )
   }
 }
+
+/**
+ * Holds if a Run step declares an environment variable, uses it in its script and sets an output in its script.
+ * e.g.
+ *  - name: Extract and Clean Initial URL
+ *    id: extract-url
+ *    env:
+ *      BODY: ${{ github.event.comment.body }}
+ *    run: |
+ *      INITIAL_URL=$(echo "$BODY" | grep -o 'https://github.com/github/release-assets/assets/[^ >]*')
+ *      echo "Cleaned Initial URL: $INITIAL_URL"
+ *      echo "::set-output name=initial_url::$INITIAL_URL"
+ */
+private class RunEnvToScriptStep extends AdditionalTaintStep {
+  override predicate step(DataFlow::Node pred, DataFlow::Node succ) { test(pred, succ) }
+}
+
+predicate test(DataFlow::Node pred, DataFlow::Node succ) {
+  exists(RunExpr r, string varName |
+    r.getEnvExpr(varName) = pred.asExpr() and
+    exists(string script, string line |
+      script = r.getScript() and
+      line = script.splitAt("\n") and
+      line.regexpMatch(".*::set-output\\s+name.*") and
+      script.indexOf("$" + ["", "{", "ENV{"] + varName) > 0
+    ) and
+    succ.asExpr() = r
+  )
+}
diff --git a/ql/lib/codeql/actions/dataflow/internal/DataFlowPrivate.qll b/ql/lib/codeql/actions/dataflow/internal/DataFlowPrivate.qll
index 9f028623848..534eb4fe657 100644
--- a/ql/lib/codeql/actions/dataflow/internal/DataFlowPrivate.qll
+++ b/ql/lib/codeql/actions/dataflow/internal/DataFlowPrivate.qll
@@ -197,7 +197,7 @@ predicate parameterMatch(ParameterPosition ppos, ArgumentPosition apos) { ppos =
  */
 predicate simpleLocalFlowStep(Node nodeFrom, Node nodeTo) { localFlowStep(nodeFrom, nodeTo) }
 
-predicate stepOutputDefToUse(Node nodeFrom, Node nodeTo) {
+predicate usesOutputDefToUse(Node nodeFrom, Node nodeTo) {
   // nodeTo is an OutputVarAccessExpr scoped with the namespace of the nodeFrom Step output
   exists(UsesExpr uses, StepOutputAccessExpr outputRead |
     uses = nodeFrom.asExpr() and
@@ -207,6 +207,16 @@ predicate stepOutputDefToUse(Node nodeFrom, Node nodeTo) {
   )
 }
 
+predicate runOutputDefToUse(Node nodeFrom, Node nodeTo) {
+  // nodeTo is an OutputVarAccessExpr scoped with the namespace of the nodeFrom Step output
+  exists(RunExpr uses, StepOutputAccessExpr outputRead |
+    uses = nodeFrom.asExpr() and
+    outputRead = nodeTo.asExpr() and
+    outputRead.getStepId() = uses.getId() and
+    uses.getJob() = outputRead.getJob()
+  )
+}
+
 predicate jobOutputDefToUse(Node nodeFrom, Node nodeTo) {
   // nodeTo is a JobOutputAccessExpr and nodeFrom is the Job output expression
   exists(Expression astFrom, JobOutputAccessExpr astTo |
@@ -223,7 +233,8 @@ predicate jobOutputDefToUse(Node nodeFrom, Node nodeTo) {
  */
 pragma[nomagic]
 predicate localFlowStep(Node nodeFrom, Node nodeTo) {
-  stepOutputDefToUse(nodeFrom, nodeTo) or
+  usesOutputDefToUse(nodeFrom, nodeTo) or
+  runOutputDefToUse(nodeFrom, nodeTo) or
   jobOutputDefToUse(nodeFrom, nodeTo)
 }