Merge pull request github#12748 from JarLob/yi

JS: Add more sources, more unit tests, fixes to the GitHub Actions injection query

Author: asgerf

javascript/ql/lib/change-notes/2023-04-03-gh-injection.md

@@ -0,0 +1,4 @@
+---
+category: minorAnalysis
+---
+* Improved the queries for injection vulnerabilities in GitHub Actions workflows (`js/actions/command-injection` and `js/actions/pull-request-target`) and the associated library `semmle.javascript.Actions`. These now support steps defined in composite actions, in addition to steps defined in Actions workflow files. It supports more potentially untrusted input values. Additionally to the shell injections it now also detects injections in `actions/github-script`. It also detects simple injections from user controlled `${{ env.name }}`. Additionally to the `yml` extension now it also supports workflows with the `yaml` extension.

javascript/ql/lib/semmle/javascript/Actions.qll

@@ -10,16 +10,70 @@ import javascript
  * See https://docs.github.com/en/actions/reference/workflow-syntax-for-github-actions.
  */
 module Actions {
-  /** A YAML node in a GitHub Actions workflow file. */
+  /** A YAML node in a GitHub Actions workflow or a custom composite action file. */
   private class Node extends YamlNode {
     Node() {
-      this.getLocation()
-          .getFile()
-          .getRelativePath()
-          .regexpMatch("(^|.*/)\\.github/workflows/.*\\.yml$")
+      exists(File f |
+        f = this.getLocation().getFile() and
+        (
+          f.getRelativePath().regexpMatch("(^|.*/)\\.github/workflows/.*\\.ya?ml$")
+          or
+          f.getBaseName() = ["action.yml", "action.yaml"]
+        )
+      )
     }
   }
 
+  /**
+   * A custom composite action. This is a mapping at the top level of an Actions YAML action file.
+   * See https://docs.github.com/en/actions/creating-actions/metadata-syntax-for-github-actions.
+   */
+  class CompositeAction extends Node, YamlDocument, YamlMapping {
+    CompositeAction() {
+      this.getFile().getBaseName() = ["action.yml", "action.yaml"] and
+      this.lookup("runs").(YamlMapping).lookup("using").(YamlScalar).getValue() = "composite"
+    }
+
+    /** Gets the `runs` mapping. */
+    Runs getRuns() { result = this.lookup("runs") }
+  }
+
+  /**
+   * An `runs` mapping in a custom composite action YAML.
+   * See https://docs.github.com/en/actions/creating-actions/metadata-syntax-for-github-actions#runs
+   */
+  class Runs extends StepsContainer {
+    CompositeAction action;
+
+    Runs() { action.lookup("runs") = this }
+
+    /** Gets the action that this `runs` mapping is in. */
+    CompositeAction getAction() { result = action }
+
+    /** Gets the `using` mapping. */
+    Using getUsing() { result = this.lookup("using") }
+  }
+
+  /**
+   * The parent class of the class that can contain `steps` mappings. (`Job` or `Runs` currently.)
+   */
+  abstract class StepsContainer extends YamlNode, YamlMapping {
+    /** Gets the sequence of `steps` within this YAML node. */
+    YamlSequence getSteps() { result = this.lookup("steps") }
+  }
+
+  /**
+   * A `using` mapping in a custom composite action YAML.
+   */
+  class Using extends YamlNode, YamlScalar {
+    Runs runs;
+
+    Using() { runs.lookup("using") = this }
+
+    /** Gets the `runs` mapping that this `using` mapping is in. */
+    Runs getRuns() { result = runs }
+  }
+
   /**
    * An Actions workflow. This is a mapping at the top level of an Actions YAML workflow file.
    * See https://docs.github.com/en/actions/reference/workflow-syntax-for-github-actions.
@@ -28,6 +82,9 @@ module Actions {
     /** Gets the `jobs` mapping from job IDs to job definitions in this workflow. */
     YamlMapping getJobs() { result = this.lookup("jobs") }
 
+    /** Gets the 'global' `env` mapping in this workflow. */
+    WorkflowEnv getEnv() { result = this.lookup("env") }
+
     /** Gets the name of the workflow. */
     string getName() { result = this.lookup("name").(YamlString).getValue() }
 
@@ -54,11 +111,44 @@ module Actions {
     Workflow getWorkflow() { result = workflow }
   }
 
+  /** A common class for `env` in workflow, job or step. */
+  abstract class Env extends YamlNode, YamlMapping { }
+
+  /** A workflow level `env` mapping. */
+  class WorkflowEnv extends Env {
+    Workflow workflow;
+
+    WorkflowEnv() { workflow.lookup("env") = this }
+
+    /** Gets the workflow this field belongs to. */
+    Workflow getWorkflow() { result = workflow }
+  }
+
+  /** A job level `env` mapping. */
+  class JobEnv extends Env {
+    Job job;
+
+    JobEnv() { job.lookup("env") = this }
+
+    /** Gets the job this field belongs to. */
+    Job getJob() { result = job }
+  }
+
+  /** A step level `env` mapping. */
+  class StepEnv extends Env {
+    Step step;
+
+    StepEnv() { step.lookup("env") = this }
+
+    /** Gets the step this field belongs to. */
+    Step getStep() { result = step }
+  }
+
   /**
    * An Actions job within a workflow.
    * See https://docs.github.com/en/actions/reference/workflow-syntax-for-github-actions#jobs.
    */
-  class Job extends YamlNode, YamlMapping {
+  class Job extends StepsContainer {
     string jobId;
     Workflow workflow;
 
@@ -85,8 +175,8 @@ module Actions {
     /** Gets the step at the given index within this job. */
     Step getStep(int index) { result.getJob() = this and result.getIndex() = index }
 
-    /** Gets the sequence of `steps` within this job. */
-    YamlSequence getSteps() { result = this.lookup("steps") }
+    /** Gets the `env` mapping in this job. */
+    JobEnv getEnv() { result = this.lookup("env") }
 
     /** Gets the workflow this job belongs to. */
     Workflow getWorkflow() { result = workflow }
@@ -130,15 +220,18 @@ module Actions {
    */
   class Step extends YamlNode, YamlMapping {
     int index;
-    Job job;
+    StepsContainer parent;
 
-    Step() { this = job.getSteps().getElement(index) }
+    Step() { this = parent.getSteps().getElement(index) }
 
     /** Gets the 0-based position of this step within the sequence of `steps`. */
     int getIndex() { result = index }
 
-    /** Gets the job this step belongs to. */
-    Job getJob() { result = job }
+    /** Gets the `job` this step belongs to, if the step belongs to a `job` in a workflow. Has no result if the step belongs to `runs` in a custom composite action. */
+    Job getJob() { result = parent }
+
+    /** Gets the `runs` this step belongs to, if the step belongs to a `runs` in a custom composite action. Has no result if the step belongs to a `job` in a workflow. */
+    Runs getRuns() { result = parent }
 
     /** Gets the value of the `uses` field in this step, if any. */
     Uses getUses() { result.getStep() = this }
@@ -149,6 +242,9 @@ module Actions {
     /** Gets the value of the `if` field in this step, if any. */
     StepIf getIf() { result.getStep() = this }
 
+    /** Gets the value of the `env` field in this step, if any. */
+    StepEnv getEnv() { result = this.lookup("env") }
+
     /** Gets the ID of this step, if any. */
     string getId() { result = this.lookup("id").(YamlString).getValue() }
   }
@@ -244,6 +340,25 @@ module Actions {
     With getWith() { result = with }
   }
 
+  /**
+   * Holds if `${{ e }}` is a GitHub Actions expression evaluated within this YAML string.
+   * See https://docs.github.com/en/free-pro-team@latest/actions/reference/context-and-expression-syntax-for-github-actions.
+   * Only finds simple expressions like `${{ github.event.comment.body }}`, where the expression contains only alphanumeric characters, underscores, dots, or dashes.
+   * Does not identify more complicated expressions like `${{ fromJSON(env.time) }}`, or ${{ format('{{Hello {0}!}}', github.event.head_commit.author.name) }}
+   */
+  string getASimpleReferenceExpression(YamlString node) {
+    // We use `regexpFind` to obtain *all* matches of `${{...}}`,
+    // not just the last (greedy match) or first (reluctant match).
+    result =
+      node.getValue()
+          .regexpFind("\\$\\{\\{\\s*[A-Za-z0-9_\\[\\]\\*\\(\\)\\.\\-]+\\s*\\}\\}", _, _)
+          .regexpCapture("\\$\\{\\{\\s*([A-Za-z0-9_\\[\\]\\*\\((\\)\\.\\-]+)\\s*\\}\\}", 1)
+  }
+
+  /** Extracts the 'name' part from env.name */
+  bindingset[name]
+  string getEnvName(string name) { result = name.regexpCapture("env\\.([A-Za-z0-9_]+)", 1) }
+
   /**
    * A `run` field within an Actions job step, which runs command-line programs using an operating system shell.
    * See https://docs.github.com/en/free-pro-team@latest/actions/reference/workflow-syntax-for-github-actions#jobsjob_idstepsrun.
@@ -255,20 +370,5 @@ module Actions {
 
     /** Gets the step that executes this `run` command. */
     Step getStep() { result = step }
-
-    /**
-     * Holds if `${{ e }}` is a GitHub Actions expression evaluated within this `run` command.
-     * See https://docs.github.com/en/free-pro-team@latest/actions/reference/context-and-expression-syntax-for-github-actions.
-     * Only finds simple expressions like `${{ github.event.comment.body }}`, where the expression contains only alphanumeric characters, underscores, dots, or dashes.
-     * Does not identify more complicated expressions like `${{ fromJSON(env.time) }}`, or ${{ format('{{Hello {0}!}}', github.event.head_commit.author.name) }}
-     */
-    string getASimpleReferenceExpression() {
-      // We use `regexpFind` to obtain *all* matches of `${{...}}`,
-      // not just the last (greedy match) or first (reluctant match).
-      result =
-        this.getValue()
-            .regexpFind("\\$\\{\\{\\s*[A-Za-z0-9_\\.\\-]+\\s*\\}\\}", _, _)
-            .regexpCapture("\\$\\{\\{\\s*([A-Za-z0-9_\\.\\-]+)\\s*\\}\\}", 1)
-    }
   }
 }

javascript/ql/src/Security/CWE-094/ExpressionInjection.qhelp

@@ -8,18 +8,20 @@
 			code injection in contexts like <i>run:</i> or <i>script:</i>.
 		</p>
         <p>
-            Code injection in GitHub Actions may allow an attacker to 
-            exfiltrate the temporary GitHub repository authorization token. 
+            Code injection in GitHub Actions may allow an attacker to
+            exfiltrate any secrets used in the workflow and
+            the temporary GitHub repository authorization token.
             The token might have write access to the repository, allowing an attacker
-             to use the token to make changes to the repository.
+            to use the token to make changes to the repository.
         </p>
 	</overview>
 
 	<recommendation>
 		<p>
 			The best practice to avoid code injection vulnerabilities
 			in GitHub workflows is to set the untrusted input value of the expression
-			to an intermediate environment variable.
+			to an intermediate environment variable and then use the environment variable
+			using the native syntax of the shell/script interpreter (that is, not <i>${{ env.VAR }}</i>).
 		</p>
         <p>
             It is also recommended to limit the permissions of any tokens used 
@@ -33,6 +35,12 @@
 		</p>
 		<sample src="examples/comment_issue_bad.yml" />
 
+		<p>
+			The following example uses an environment variable, but
+			<b>still allows the injection</b> because of the use of expression syntax:
+		</p>
+		<sample src="examples/comment_issue_bad_env.yml" />
+
 		<p>
 			The following example uses shell syntax to read
 			the environment variable and will prevent the attack: