Merge pull request #12748 from JarLob/yi

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

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

@@ -8,10 +8,11 @@
 			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>
 
@@ -19,7 +20,8 @@
 		<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:

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

@@ -15,6 +15,44 @@
 import javascript
 import semmle.javascript.Actions
 
+/**
+ * A `script:` field within an Actions `with:` specific to `actions/github-script` action.
+ *
+ * For example:
+ * ```
+ * uses: actions/github-script@v3
+ * with:
+ *   script: console.log('${{ github.event.pull_request.head.sha }}')
+ * ```
+ */
+class GitHubScript extends YamlNode, YamlString {
+  GitHubScriptWith with;
+
+  GitHubScript() { with.lookup("script") = this }
+
+  /** Gets the `with` field this field belongs to. */
+  GitHubScriptWith getWith() { result = with }
+}
+
+/**
+ * A step that uses `actions/github-script` action.
+ */
+class GitHubScriptStep extends Actions::Step {
+  GitHubScriptStep() { this.getUses().getGitHubRepository() = "actions/github-script" }
+}
+
+/**
+ * A `with:` field sibling to `uses: actions/github-script`.
+ */
+class GitHubScriptWith extends YamlNode, YamlMapping {
+  GitHubScriptStep step;
+
+  GitHubScriptWith() { step.lookup("with") = this }
+
+  /** Gets the step this field belongs to. */
+  GitHubScriptStep getStep() { result = step }
+}
+
 bindingset[context]
 private predicate isExternalUserControlledIssue(string context) {
   context.regexpMatch("\\bgithub\\s*\\.\\s*event\\s*\\.\\s*issue\\s*\\.\\s*title\\b") or
@@ -30,7 +68,10 @@ private predicate isExternalUserControlledPullRequest(string context) {
         "\\bgithub\\s*\\.\\s*event\\s*\\.\\s*pull_request\\s*\\.\\s*body\\b",
         "\\bgithub\\s*\\.\\s*event\\s*\\.\\s*pull_request\\s*\\.\\s*head\\s*\\.\\s*label\\b",
         "\\bgithub\\s*\\.\\s*event\\s*\\.\\s*pull_request\\s*\\.\\s*head\\s*\\.\\s*repo\\s*\\.\\s*default_branch\\b",
+        "\\bgithub\\s*\\.\\s*event\\s*\\.\\s*pull_request\\s*\\.\\s*head\\s*\\.\\s*repo\\s*\\.\\s*description\\b",
+        "\\bgithub\\s*\\.\\s*event\\s*\\.\\s*pull_request\\s*\\.\\s*head\\s*\\.\\s*repo\\s*\\.\\s*homepage\\b",
         "\\bgithub\\s*\\.\\s*event\\s*\\.\\s*pull_request\\s*\\.\\s*head\\s*\\.\\s*ref\\b",
+        "\\bgithub\\s*\\.\\s*head_ref\\b"
       ]
   |
     context.regexpMatch(reg)
@@ -39,8 +80,7 @@ private predicate isExternalUserControlledPullRequest(string context) {
 
 bindingset[context]
 private predicate isExternalUserControlledReview(string context) {
-  context.regexpMatch("\\bgithub\\s*\\.\\s*event\\s*\\.\\s*review\\s*\\.\\s*body\\b") or
-  context.regexpMatch("\\bgithub\\s*\\.\\s*event\\s*\\.\\s*review_comment\\s*\\.\\s*body\\b")
+  context.regexpMatch("\\bgithub\\s*\\.\\s*event\\s*\\.\\s*review\\s*\\.\\s*body\\b")
 }
 
 bindingset[context]
@@ -51,7 +91,8 @@ private predicate isExternalUserControlledComment(string context) {
 bindingset[context]
 private predicate isExternalUserControlledGollum(string context) {
   context
-      .regexpMatch("\\bgithub\\s*\\.\\s*event\\s*\\.\\s*pages(?:\\[[0-9]\\]|\\s*\\.\\s*\\*)+\\s*\\.\\s*page_name\\b")
+      .regexpMatch("\\bgithub\\s*\\.\\s*event\\s*\\.\\s*pages\\[[0-9]+\\]\\s*\\.\\s*page_name\\b") or
+  context.regexpMatch("\\bgithub\\s*\\.\\s*event\\s*\\.\\s*pages\\[[0-9]+\\]\\s*\\.\\s*title\\b")
 }
 
 bindingset[context]
@@ -59,13 +100,16 @@ private predicate isExternalUserControlledCommit(string context) {
   exists(string reg |
     reg =
       [
-        "\\bgithub\\s*\\.\\s*event\\s*\\.\\s*commits(?:\\[[0-9]\\]|\\s*\\.\\s*\\*)+\\s*\\.\\s*message\\b",
+        "\\bgithub\\s*\\.\\s*event\\s*\\.\\s*commits\\[[0-9]+\\]\\s*\\.\\s*message\\b",
         "\\bgithub\\s*\\.\\s*event\\s*\\.\\s*head_commit\\s*\\.\\s*message\\b",
         "\\bgithub\\s*\\.\\s*event\\s*\\.\\s*head_commit\\s*\\.\\s*author\\s*\\.\\s*email\\b",
         "\\bgithub\\s*\\.\\s*event\\s*\\.\\s*head_commit\\s*\\.\\s*author\\s*\\.\\s*name\\b",
-        "\\bgithub\\s*\\.\\s*event\\s*\\.\\s*commits(?:\\[[0-9]\\]|\\s*\\.\\s*\\*)+\\s*\\.\\s*author\\s*\\.\\s*email\\b",
-        "\\bgithub\\s*\\.\\s*event\\s*\\.\\s*commits(?:\\[[0-9]\\]|\\s*\\.\\s*\\*)+\\s*\\.\\s*author\\s*\\.\\s*name\\b",
-        "\\bgithub\\s*\\.\\s*head_ref\\b"
+        "\\bgithub\\s*\\.\\s*event\\s*\\.\\s*head_commit\\s*\\.\\s*committer\\s*\\.\\s*email\\b",
+        "\\bgithub\\s*\\.\\s*event\\s*\\.\\s*head_commit\\s*\\.\\s*committer\\s*\\.\\s*name\\b",
+        "\\bgithub\\s*\\.\\s*event\\s*\\.\\s*commits\\[[0-9]+\\]\\s*\\.\\s*author\\s*\\.\\s*email\\b",
+        "\\bgithub\\s*\\.\\s*event\\s*\\.\\s*commits\\[[0-9]+\\]\\s*\\.\\s*author\\s*\\.\\s*name\\b",
+        "\\bgithub\\s*\\.\\s*event\\s*\\.\\s*commits\\[[0-9]+\\]\\s*\\.\\s*committer\\s*\\.\\s*email\\b",
+        "\\bgithub\\s*\\.\\s*event\\s*\\.\\s*commits\\[[0-9]+\\]\\s*\\.\\s*committer\\s*\\.\\s*name\\b",
       ]
   |
     context.regexpMatch(reg)
@@ -78,32 +122,149 @@ private predicate isExternalUserControlledDiscussion(string context) {
   context.regexpMatch("\\bgithub\\s*\\.\\s*event\\s*\\.\\s*discussion\\s*\\.\\s*body\\b")
 }
 
-from Actions::Run run, string context, Actions::On on
-where
-  run.getASimpleReferenceExpression() = context and
-  run.getStep().getJob().getWorkflow().getOn() = on and
-  (
-    exists(on.getNode("issues")) and
-    isExternalUserControlledIssue(context)
-    or
-    exists(on.getNode("pull_request_target")) and
-    isExternalUserControlledPullRequest(context)
-    or
-    (exists(on.getNode("pull_request_review_comment")) or exists(on.getNode("pull_request_review"))) and
-    isExternalUserControlledReview(context)
-    or
-    (exists(on.getNode("issue_comment")) or exists(on.getNode("pull_request_target"))) and
-    isExternalUserControlledComment(context)
-    or
-    exists(on.getNode("gollum")) and
-    isExternalUserControlledGollum(context)
-    or
-    exists(on.getNode("pull_request_target")) and
-    isExternalUserControlledCommit(context)
-    or
-    (exists(on.getNode("discussion")) or exists(on.getNode("discussion_comment"))) and
-    isExternalUserControlledDiscussion(context)
+bindingset[context]
+private predicate isExternalUserControlledWorkflowRun(string context) {
+  exists(string reg |
+    reg =
+      [
+        "\\bgithub\\s*\\.\\s*event\\s*\\.\\s*workflow_run\\s*\\.\\s*head_branch\\b",
+        "\\bgithub\\s*\\.\\s*event\\s*\\.\\s*workflow_run\\s*\\.\\s*display_title\\b",
+        "\\bgithub\\s*\\.\\s*event\\s*\\.\\s*workflow_run\\s*\\.\\s*head_repository\\b\\s*\\.\\s*description\\b",
+        "\\bgithub\\s*\\.\\s*event\\s*\\.\\s*workflow_run\\s*\\.\\s*head_commit\\b\\s*\\.\\s*message\\b",
+        "\\bgithub\\s*\\.\\s*event\\s*\\.\\s*workflow_run\\s*\\.\\s*head_commit\\b\\s*\\.\\s*author\\b\\s*\\.\\s*email\\b",
+        "\\bgithub\\s*\\.\\s*event\\s*\\.\\s*workflow_run\\s*\\.\\s*head_commit\\b\\s*\\.\\s*author\\b\\s*\\.\\s*name\\b",
+        "\\bgithub\\s*\\.\\s*event\\s*\\.\\s*workflow_run\\s*\\.\\s*head_commit\\b\\s*\\.\\s*committer\\b\\s*\\.\\s*email\\b",
+        "\\bgithub\\s*\\.\\s*event\\s*\\.\\s*workflow_run\\s*\\.\\s*head_commit\\b\\s*\\.\\s*committer\\b\\s*\\.\\s*name\\b",
+      ]
+  |
+    context.regexpMatch(reg)
   )
-select run,
-  "Potential injection from the " + context +
-    " context, which may be controlled by an external user."
+}
+
+/**
+ * Holds if environment name in the `injection` (in a form of `env.name`)
+ * is tainted by the `context` (in a form of `github.event.xxx.xxx`).
+ */
+bindingset[injection]
+predicate isEnvInterpolationTainted(string injection, string context) {
+  exists(Actions::Env env, string envName, YamlString envValue |
+    envValue = env.lookup(envName) and
+    Actions::getEnvName(injection) = envName and
+    Actions::getASimpleReferenceExpression(envValue) = context
+  )
+}
+
+/**
+ * Holds if the `run` contains any expression interpolation `${{ e }}`.
+ * Sets `context` to the initial untrusted value assignment in case of `${{ env... }}` interpolation
+ */
+predicate isRunInjectable(Actions::Run run, string injection, string context) {
+  Actions::getASimpleReferenceExpression(run) = injection and
+  (
+    injection = context
+    or
+    isEnvInterpolationTainted(injection, context)
+  )
+}
+
+/**
+ * Holds if the `actions/github-script` contains any expression interpolation `${{ e }}`.
+ * Sets `context` to the initial untrusted value assignment in case of `${{ env... }}` interpolation
+ */
+predicate isScriptInjectable(GitHubScript script, string injection, string context) {
+  Actions::getASimpleReferenceExpression(script) = injection and
+  (
+    injection = context
+    or
+    isEnvInterpolationTainted(injection, context)
+  )
+}
+
+/**
+ * Holds if the composite action contains untrusted expression interpolation `${{ e }}`.
+ */
+YamlNode getInjectableCompositeActionNode(Actions::Runs runs, string injection, string context) {
+  exists(Actions::Run run |
+    isRunInjectable(run, injection, context) and
+    result = run and
+    run.getStep().getRuns() = runs
+  )
+  or
+  exists(GitHubScript script |
+    isScriptInjectable(script, injection, context) and
+    result = script and
+    script.getWith().getStep().getRuns() = runs
+  )
+}
+
+/**
+ * Holds if the workflow contains untrusted expression interpolation `${{ e }}`.
+ */
+YamlNode getInjectableWorkflowNode(Actions::On on, string injection, string context) {
+  exists(Actions::Run run |
+    isRunInjectable(run, injection, context) and
+    result = run and
+    run.getStep().getJob().getWorkflow().getOn() = on
+  )
+  or
+  exists(GitHubScript script |
+    isScriptInjectable(script, injection, context) and
+    result = script and
+    script.getWith().getStep().getJob().getWorkflow().getOn() = on
+  )
+}
+
+from YamlNode node, string injection, string context
+where
+  exists(Actions::CompositeAction action, Actions::Runs runs |
+    action.getRuns() = runs and
+    node = getInjectableCompositeActionNode(runs, injection, context) and
+    (
+      isExternalUserControlledIssue(context) or
+      isExternalUserControlledPullRequest(context) or
+      isExternalUserControlledReview(context) or
+      isExternalUserControlledComment(context) or
+      isExternalUserControlledGollum(context) or
+      isExternalUserControlledCommit(context) or
+      isExternalUserControlledDiscussion(context) or
+      isExternalUserControlledWorkflowRun(context)
+    )
+  )
+  or
+  exists(Actions::On on |
+    node = getInjectableWorkflowNode(on, injection, context) and
+    (
+      exists(on.getNode("issues")) and
+      isExternalUserControlledIssue(context)
+      or
+      exists(on.getNode("pull_request_target")) and
+      isExternalUserControlledPullRequest(context)
+      or
+      exists(on.getNode("pull_request_review")) and
+      (isExternalUserControlledReview(context) or isExternalUserControlledPullRequest(context))
+      or
+      exists(on.getNode("pull_request_review_comment")) and
+      (isExternalUserControlledComment(context) or isExternalUserControlledPullRequest(context))
+      or
+      exists(on.getNode("issue_comment")) and
+      (isExternalUserControlledComment(context) or isExternalUserControlledIssue(context))
+      or
+      exists(on.getNode("gollum")) and
+      isExternalUserControlledGollum(context)
+      or
+      exists(on.getNode("push")) and
+      isExternalUserControlledCommit(context)
+      or
+      exists(on.getNode("discussion")) and
+      isExternalUserControlledDiscussion(context)
+      or
+      exists(on.getNode("discussion_comment")) and
+      (isExternalUserControlledDiscussion(context) or isExternalUserControlledComment(context))
+      or
+      exists(on.getNode("workflow_run")) and
+      isExternalUserControlledWorkflowRun(context)
+    )
+  )
+select node,
+  "Potential injection from the ${{ " + injection +
+    " }}, which may be controlled by an external user."

javascript/ql/src/Security/CWE-094/examples/comment_issue_bad_env.yml

@@ -0,0 +1,10 @@
+on: issue_comment
+
+jobs:
+  echo-body:
+    runs-on: ubuntu-latest
+    steps:
+    -  env:
+        BODY: ${{ github.event.issue.body }}
+      run: |
+        echo '${{ env.BODY }}'

javascript/ql/src/experimental/Security/CWE-094/UntrustedCheckout.ql

@@ -17,7 +17,7 @@ import javascript
 import semmle.javascript.Actions
 
 /**
- * An action step that doesn't contain `actor` or `label` check in `if:` or
+ * An action step that doesn't contain `actor` check in `if:` or
  * the check requires manual analysis.
  */
 class ProbableStep extends Actions::Step {
@@ -29,25 +29,13 @@ class ProbableStep extends Actions::Step {
     // needs manual analysis if there is OR
     this.getIf().getValue().matches("%||%")
     or
-    // labels can be assigned by owners only
-    not exists(
-      this.getIf()
-          .getValue()
-          .regexpFind("\\bcontains\\s*\\(\\s*github\\s*\\.\\s*event\\s*\\.\\s*(?:issue|pull_request)\\s*\\.\\s*labels\\b",
-            _, _)
-    ) and
-    not exists(
-      this.getIf()
-          .getValue()
-          .regexpFind("\\bgithub\\s*\\.\\s*event\\s*\\.\\s*label\\s*\\.\\s*name\\s*==", _, _)
-    ) and
     // actor check means only the user is able to run it
     not exists(this.getIf().getValue().regexpFind("\\bgithub\\s*\\.\\s*actor\\s*==", _, _))
   }
 }
 
 /**
- * An action job that doesn't contain `actor` or `label` check in `if:` or
+ * An action job that doesn't contain `actor` check in `if:` or
  * the check requires manual analysis.
  */
 class ProbableJob extends Actions::Job {
@@ -59,46 +47,18 @@ class ProbableJob extends Actions::Job {
     // needs manual analysis if there is OR
     this.getIf().getValue().matches("%||%")
     or
-    // labels can be assigned by owners only
-    not exists(
-      this.getIf()
-          .getValue()
-          .regexpFind("\\bcontains\\s*\\(\\s*github\\s*\\.\\s*event\\s*\\.\\s*(?:issue|pull_request)\\s*\\.\\s*labels\\b",
-            _, _)
-    ) and
-    not exists(
-      this.getIf()
-          .getValue()
-          .regexpFind("\\bgithub\\s*\\.\\s*event\\s*\\.\\s*label\\s*\\.\\s*name\\s*==", _, _)
-    ) and
     // actor check means only the user is able to run it
     not exists(this.getIf().getValue().regexpFind("\\bgithub\\s*\\.\\s*actor\\s*==", _, _))
   }
 }
 
 /**
- * An action step that doesn't contain `actor` or `label` check in `if:` or
+ * The `on: pull_request_target`.
  */
 class ProbablePullRequestTarget extends Actions::On, YamlMappingLikeNode {
   ProbablePullRequestTarget() {
-    exists(YamlNode prtNode |
-      // The `on:` is triggered on `pull_request_target`
-      this.getNode("pull_request_target") = prtNode and
-      (
-        // and either doesn't contain `types` filter
-        not exists(prtNode.getAChild())
-        or
-        // or has the filter, that is something else than just [labeled]
-        exists(YamlMappingLikeNode prt, YamlMappingLikeNode types |
-          types = prt.getNode("types") and
-          prtNode = prt and
-          (
-            not types.getElementCount() = 1 or
-            not exists(types.getNode("labeled"))
-          )
-        )
-      )
-    )
+    // The `on:` is triggered on `pull_request_target`
+    exists(this.getNode("pull_request_target"))
   }
 }