Merge pull request #5 from GitHubSecurityLab/env_context

Implement support for env context

Author: Alvaro Muñoz

ql/lib/codeql/actions/Ast.qll

@@ -184,26 +184,6 @@ class StepStmt extends Statement instanceof Actions::Step {
   string getId() { result = super.getId() }
 
   JobStmt getJobStmt() { result = super.getJob() }
-
-  /**
-   * Gets a environment variable expression by name in the scope of the current step.
-   */
-  Expression getEnvExpr(string name) {
-    exists(Actions::StepEnv env |
-      env.getStep() = this and
-      env.(YamlMapping).maps(any(YamlScalar s | s.getValue() = name), result)
-    )
-    or
-    exists(Actions::JobEnv env |
-      env.getJob() = this.getJobStmt() and
-      env.(YamlMapping).maps(any(YamlScalar s | s.getValue() = name), result)
-    )
-    or
-    exists(Actions::WorkflowEnv env |
-      env.getWorkflow() = this.getJobStmt().getWorkflowStmt() and
-      env.(YamlMapping).maps(any(YamlScalar s | s.getValue() = name), result)
-    )
-  }
 }
 
 /**
@@ -238,7 +218,25 @@ class StepUsesExpr extends StepStmt, UsesExpr {
     )
   }
 
-  override Expression getEnvExpr(string name) { result = this.(StepStmt).getEnvExpr(name) }
+  /**
+   * Gets a environment variable expression by name in the scope of the current step.
+   */
+  override Expression getEnvExpr(string name) {
+    exists(Actions::StepEnv env |
+      env.getStep() = this and
+      env.(YamlMapping).maps(any(YamlScalar s | s.getValue() = name), result)
+    )
+    or
+    exists(Actions::JobEnv env |
+      env.getJob() = this.getJobStmt() and
+      env.(YamlMapping).maps(any(YamlScalar s | s.getValue() = name), result)
+    )
+    or
+    exists(Actions::WorkflowEnv env |
+      env.getWorkflow() = this.getJobStmt().getWorkflowStmt() and
+      env.(YamlMapping).maps(any(YamlScalar s | s.getValue() = name), result)
+    )
+  }
 }
 
 /**
@@ -317,6 +315,26 @@ class RunExpr extends StepStmt, Expression {
   Expression getScriptExpr() { result = scriptExpr }
 
   string getScript() { result = scriptExpr.getValue() }
+
+  /**
+   * Gets a environment variable expression by name in the scope of the current node.
+   */
+  Expression getEnvExpr(string name) {
+    exists(Actions::StepEnv env |
+      env.getStep() = this and
+      env.(YamlMapping).maps(any(YamlScalar s | s.getValue() = name), result)
+    )
+    or
+    exists(Actions::JobEnv env |
+      env.getJob() = this.getJobStmt() and
+      env.(YamlMapping).maps(any(YamlScalar s | s.getValue() = name), result)
+    )
+    or
+    exists(Actions::WorkflowEnv env |
+      env.getWorkflow() = this.getJobStmt().getWorkflowStmt() and
+      env.(YamlMapping).maps(any(YamlScalar s | s.getValue() = name), result)
+    )
+  }
 }
 
 /**
@@ -330,11 +348,14 @@ class ExprAccessExpr extends Expression instanceof YamlString {
   string getExpression() { result = expr }
 
   JobStmt getJobStmt() { result.getAChildNode*() = this }
+
+  abstract Expression getRefExpr();
 }
 
 /**
- * A ExprAccessExpr where the expression evaluated is a step output read.
- * eg: `${{ steps.changed-files.outputs.all_changed_files }}`
+ * Holds for an ExprAccessExpr accesing the `steps` context.
+ * https://docs.github.com/en/actions/learn-github-actions/contexts#context-availability
+ * e.g. `${{ steps.changed-files.outputs.all_changed_files }}`
  */
 class StepOutputAccessExpr extends ExprAccessExpr {
   string stepId;
@@ -347,17 +368,16 @@ class StepOutputAccessExpr extends ExprAccessExpr {
       this.getExpression().regexpCapture("steps\\.[A-Za-z0-9_-]+\\.outputs\\.([A-Za-z0-9_-]+)", 1)
   }
 
-  string getStepId() { result = stepId }
-
-  string getVarName() { result = varName }
-
-  StepStmt getStepStmt() { result.getId() = stepId }
+  override Expression getRefExpr() {
+    this.getJobStmt() = result.(StepStmt).getJobStmt() and
+    result.(StepStmt).getId() = stepId
+  }
 }
 
 /**
- * A ExprAccessExpr where the expression evaluated is a job output read.
- * eg: `${{ needs.job1.outputs.foo}}`
- * eg: `${{ jobs.job1.outputs.foo}}` (for reusable workflows)
+ * Holds for an ExprAccessExpr accesing the `needs` or `job` contexts.
+ * https://docs.github.com/en/actions/learn-github-actions/contexts#context-availability
+ * e.g. `${{ needs.job1.outputs.foo}}` or `${{ jobs.job1.outputs.foo}}` (for reusable workflows)
  */
 class JobOutputAccessExpr extends ExprAccessExpr {
   string jobId;
@@ -372,9 +392,7 @@ class JobOutputAccessExpr extends ExprAccessExpr {
           .regexpCapture("(needs|jobs)\\.[A-Za-z0-9_-]+\\.outputs\\.([A-Za-z0-9_-]+)", 2)
   }
 
-  string getVarName() { result = varName }
-
-  Expression getOutputExpr() {
+  override Expression getRefExpr() {
     exists(JobStmt job |
       job.getId() = jobId and
       job.getLocation().getFile() = this.getLocation().getFile() and
@@ -391,8 +409,9 @@ class JobOutputAccessExpr extends ExprAccessExpr {
 }
 
 /**
- * A ExprAccessExpr where the expression evaluated is a reusable workflow input read.
- * eg: `${{ inputs.foo}}`
+ * Holds for an ExprAccessExpr accesing the `inputs` context.
+ * https://docs.github.com/en/actions/learn-github-actions/contexts#context-availability
+ * e.g. `${{ inputs.foo }}`
  */
 class ReusableWorkflowInputAccessExpr extends ExprAccessExpr {
   string paramName;
@@ -401,12 +420,29 @@ class ReusableWorkflowInputAccessExpr extends ExprAccessExpr {
     paramName = this.getExpression().regexpCapture("inputs\\.([A-Za-z0-9_-]+)", 1)
   }
 
-  string getParamName() { result = paramName }
-
-  Expression getInputExpr() {
+  override Expression getRefExpr() {
     exists(ReusableWorkflowStmt w |
       w.getLocation().getFile() = this.getLocation().getFile() and
       w.getInputsStmt().getInputExpr(paramName) = result
     )
   }
 }
+
+/**
+ * Holds for an ExprAccessExpr accesing the `env` context.
+ * https://docs.github.com/en/actions/learn-github-actions/contexts#context-availability
+ * e.g. `${{ env.foo }}`
+ */
+class EnvAccessExpr extends ExprAccessExpr {
+  string varName;
+
+  EnvAccessExpr() { varName = this.getExpression().regexpCapture("env\\.([A-Za-z0-9_-]+)", 1) }
+
+  override Expression getRefExpr() {
+    exists(JobUsesExpr s | s.getEnvExpr(varName) = result)
+    or
+    exists(StepUsesExpr s | s.getEnvExpr(varName) = result)
+    or
+    exists(RunExpr s | s.getEnvExpr(varName) = result)
+  }
+}

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

@@ -227,7 +227,8 @@ private class StepUsesTree extends StandardPreOrderTree instanceof StepUsesExpr
   override ControlFlowTree getChildNode(int i) {
     result =
       rank[i](Expression child, Location l |
-        child = super.getArgumentExpr(_) and l = child.getLocation()
+        (child = super.getArgumentExpr(_) or child = super.getEnvExpr(_)) and
+        l = child.getLocation()
       |
         child
         order by
@@ -240,7 +241,8 @@ private class JobUsesTree extends StandardPreOrderTree instanceof JobUsesExpr {
   override ControlFlowTree getChildNode(int i) {
     result =
       rank[i](Expression child, Location l |
-        child = super.getArgumentExpr(_) and l = child.getLocation()
+        (child = super.getArgumentExpr(_) or child = super.getEnvExpr(_)) and
+        l = child.getLocation()
       |
         child
         order by

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

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

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

@@ -164,41 +164,52 @@ class ArgumentPosition extends string {
  */
 predicate parameterMatch(ParameterPosition ppos, ArgumentPosition apos) { ppos = apos }
 
-predicate stepUsesOutputDefToUse(Node nodeFrom, Node nodeTo) {
-  // nodeTo is an OutputVarAccessExpr scoped with the namespace of the nodeFrom Step output
-  exists(StepUsesExpr uses, StepOutputAccessExpr outputRead |
-    uses = nodeFrom.asExpr() and
-    outputRead = nodeTo.asExpr() and
-    outputRead.getStepId() = uses.getId() and
-    uses.getJobStmt() = outputRead.getJobStmt()
+/**
+ * Holds if there is a local flow step between a ${{}} expression accesing a step output variable and the step output itself
+ * e.g. ${{ steps.step1.output.foo }}
+ */
+predicate stepsCtxLocalStep(Node nodeFrom, Node nodeTo) {
+  exists(StepStmt astFrom, StepOutputAccessExpr astTo |
+    (astFrom instanceof UsesExpr or astFrom instanceof RunExpr) and
+    astFrom = nodeFrom.asExpr() and
+    astTo = nodeTo.asExpr() and
+    astTo.getRefExpr() = astFrom
   )
 }
 
-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.getJobStmt() = outputRead.getJobStmt()
+/**
+ * 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 ${{ job.job1.output.foo }}
+ */
+predicate jobsCtxLocalStep(Node nodeFrom, Node nodeTo) {
+  exists(Expression astFrom, JobOutputAccessExpr astTo |
+    astFrom = nodeFrom.asExpr() and
+    astTo = nodeTo.asExpr() and
+    astTo.getRefExpr() = astFrom
   )
 }
 
-predicate jobOutputDefToUse(Node nodeFrom, Node nodeTo) {
-  // nodeTo is a JobOutputAccessExpr and nodeFrom is the Job output expression
-  exists(Expression astFrom, JobOutputAccessExpr astTo |
+/**
+ * Holds if there is a local flow step between a ${{}} expression accesing a reusable workflow input variable and the input itself
+ * e.g. ${{ inputs.foo }}
+ */
+predicate inputsCtxLocalStep(Node nodeFrom, Node nodeTo) {
+  exists(Expression astFrom, ReusableWorkflowInputAccessExpr astTo |
     astFrom = nodeFrom.asExpr() and
     astTo = nodeTo.asExpr() and
-    astTo.getOutputExpr() = astFrom
+    astTo.getRefExpr() = astFrom
   )
 }
 
-predicate reusableWorkflowInputDefToUse(Node nodeFrom, Node nodeTo) {
-  // nodeTo is a ReusableWorkflowInputAccessExpr and nodeFrom is the ReusableWorkflowStmt corresponding parameter expression
-  exists(Expression astFrom, ReusableWorkflowInputAccessExpr astTo |
+/**
+ * Holds if there is a local flow step between a ${{}} expression accesing an env var and the var definition itself
+ * e.g. ${{ env.foo }}
+ */
+predicate envCtxLocalStep(Node nodeFrom, Node nodeTo) {
+  exists(Expression astFrom, EnvAccessExpr astTo |
     astFrom = nodeFrom.asExpr() and
     astTo = nodeTo.asExpr() and
-    astTo.getInputExpr() = astFrom
+    astTo.getRefExpr() = astFrom
   )
 }
 
@@ -209,10 +220,10 @@ predicate reusableWorkflowInputDefToUse(Node nodeFrom, Node nodeTo) {
  */
 pragma[nomagic]
 predicate localFlowStep(Node nodeFrom, Node nodeTo) {
-  stepUsesOutputDefToUse(nodeFrom, nodeTo) or
-  runOutputDefToUse(nodeFrom, nodeTo) or
-  jobOutputDefToUse(nodeFrom, nodeTo) or
-  reusableWorkflowInputDefToUse(nodeFrom, nodeTo)
+  stepsCtxLocalStep(nodeFrom, nodeTo) or
+  jobsCtxLocalStep(nodeFrom, nodeTo) or
+  inputsCtxLocalStep(nodeFrom, nodeTo) or
+  envCtxLocalStep(nodeFrom, nodeTo)
 }
 
 /**

ql/lib/test/test.ql

@@ -31,10 +31,6 @@ query predicate runStepChildren(RunExpr run, AstNode child) { child.getParentNod
 
 query predicate varAccesses(ExprAccessExpr ea, string expr) { expr = ea.getExpression() }
 
-query predicate outputAccesses(StepOutputAccessExpr va, string id, string var) {
-  id = va.getStepId() and var = va.getVarName()
-}
-
 query predicate orphanVarAccesses(ExprAccessExpr va, string var) {
   var = va.getExpression() and
   not exists(AstNode n | n = va.getParentNode())
@@ -53,25 +49,21 @@ query predicate cfgNodes(Cfg::Node n) {
 }
 
 query predicate dfNodes(DataFlow::Node e) {
-  e.getLocation().getFile().getBaseName() = "simple1.yml"
+  e.getLocation().getFile().getBaseName() = "argus_case_study.yml"
 }
 
 query predicate exprNodes(DataFlow::ExprNode e) { any() }
 
 query predicate argumentNodes(DataFlow::ArgumentNode e) { any() }
 
-query predicate localFlow(StepUsesExpr s, StepOutputAccessExpr o) { s.getId() = o.getStepId() }
-
 query predicate usesIds(StepUsesExpr s, string a) { s.getId() = a }
 
-query predicate varIds(StepOutputAccessExpr s, string a) { s.getStepId() = a }
-
 query predicate nodeLocations(DataFlow::Node n, Location l) { n.getLocation() = l }
 
 query predicate scopes(Cfg::CfgScope c) { any() }
 
-query predicate sources(string action, string version, string output, string kind) {
-  sourceModel(action, version, output, kind)
+query predicate sources(string action, string version, string output, string trigger, string kind) {
+  sourceModel(action, version, output, trigger, kind)
 }
 
 query predicate summaries(string action, string version, string input, string output, string kind) {

ql/src/test/.github/workflows/argus_case_study.yml

@@ -0,0 +1,29 @@
+name: Issue Workflow
+
+on:
+  issues:
+    types: [opened, edited]
+
+jobs:
+  redirectIssue:
+    runs-on: ubuntu-latest
+    name: Check for issue transfer
+    env:
+      content_analysis_response: undefined
+    steps:
+      - uses: actions/checkout@v2
+      - name: Remove conflicting chars
+        env:
+          ISSUE_TITLE: ${{github.event.issue.title}}
+        uses: frabert/[email protected]
+        id: remove_quotations
+        with:
+          pattern: "\""
+          string: ${{env.ISSUE_TITLE}}          
+          replace-with: "-"
+      - name: Check info
+        id: check-info
+        run: |
+          echo "foo $(pwsh bar ${{steps.remove_quotations.outputs.replaced}}) " >> $GITHUB_ENV
+
+