Improve Artifact Poisoning query

ql/lib/codeql/actions/Ast.qll

@@ -225,6 +225,8 @@ class Step extends AstNode instanceof StepImpl {
   Env getEnv() { result = super.getEnv() }
 
   If getIf() { result = super.getIf() }
+
+  Step getAFollowingStep() { result = super.getAFollowingStep() }
 }
 
 /**

ql/lib/codeql/actions/ast/internal/Ast.qll

@@ -629,6 +629,15 @@ class StepImpl extends AstNodeImpl, TStepNode {
 
   /** Gets the value of the `if` field in this step, if any. */
   IfImpl getIf() { result.getNode() = n.lookup("if") }
+
+  /** Gets a step that follows this step. */
+  StepImpl getAFollowingStep() {
+    exists(LocalJobImpl job, int i, int j |
+      job.getStep(i) = this and
+      result = job.getStep(j) and
+      i < j
+    )
+  }
 }
 
 class IfImpl extends AstNodeImpl, TIfNode {

ql/lib/codeql/actions/security/ArtifactPoisoningQuery.qll

@@ -1,13 +1,36 @@
 import actions
 
-class ArtifactDownloadStep extends Step {
-  ArtifactDownloadStep() {
+string unzipRegexp() { result = ".*(unzip|tar)\\s+.*" }
+
+string unzipDirArgRegexp() {
+  result = "-d\\s+\"([^ ]+)\".*" or
+  result = "-d\\s+'([^ ]+)'.*"
+}
+
+abstract class ArtifactDownloadStep extends Step {
+  abstract string getPath();
+}
+
+class Dawidd6ActionDownloadArtifactDownloadStep extends ArtifactDownloadStep, UsesStep {
+  Dawidd6ActionDownloadArtifactDownloadStep() {
     // eg: - uses: dawidd6/action-download-artifact@v2
-    this.(UsesStep).getCallee() = "dawidd6/action-download-artifact" and
-    // exclude downloads outside the current directory
-    // TODO: add more checks to make sure the artifacts can be controlled
-    not exists(this.(UsesStep).getArgumentExpr("path"))
-    or
+    this.getCallee() = "dawidd6/action-download-artifact" and
+    // An attacker should not be able to push to local branches which `branch` normally is used for.
+    (
+      not exists(this.getArgument("branch")) or
+      not this.getArgument("branch") = ["main", "master"]
+    )
+  }
+
+  override string getPath() {
+    if exists(this.getArgument("path")) then result = this.getArgument("path") else result = ""
+  }
+}
+
+class ActionsGitHubScriptDownloadStep extends ArtifactDownloadStep, UsesStep {
+  string script;
+
+  ActionsGitHubScriptDownloadStep() {
     // eg:
     // - uses: actions/github-script@v6
     //   with:
@@ -26,16 +49,79 @@ class ArtifactDownloadStep extends Step {
     //           artifact_id: matchArtifact.id,
     //           archive_format: 'zip',
     //       });
-    this.(UsesStep).getCallee() = "actions/github-script" and
-    exists(string script |
-      this.(UsesStep).getArgument("script") = script and
-      script.matches("%listWorkflowRunArtifacts(%") and
-      script.matches("%downloadArtifact(%")
+    //      var fs = require('fs');
+    //      fs.writeFileSync('${{github.workspace}}/test-results.zip', Buffer.from(download.data));
+    this.getCallee() = "actions/github-script" and
+    this.getArgument("script") = script and
+    script.matches("%listWorkflowRunArtifacts(%") and
+    script.matches("%downloadArtifact(%") and
+    script.matches("%writeFileSync%")
+  }
+
+  override string getPath() {
+    if
+      this.getAFollowingStep()
+          .(Run)
+          .getScript()
+          .splitAt("\n")
+          .regexpMatch(unzipRegexp() + unzipDirArgRegexp())
+    then
+      result =
+        this.getAFollowingStep()
+            .(Run)
+            .getScript()
+            .splitAt("\n")
+            .regexpCapture(unzipRegexp() + unzipDirArgRegexp(), 2)
+    else
+      if this.getAFollowingStep().(Run).getScript().splitAt("\n").regexpMatch(unzipRegexp())
+      then result = ""
+      else none()
+  }
+}
+
+class GHRunArtifactDownloadStep extends ArtifactDownloadStep, Run {
+  string script;
+
+  GHRunArtifactDownloadStep() {
+    // eg: - run: gh run download ${{ github.event.workflow_run.id }} --repo "${GITHUB_REPOSITORY}" --name "artifact_name"
+    this.getScript() = script and
+    script.splitAt("\n").regexpMatch(".*gh\\s+run\\s+download.*") and
+    script.splitAt("\n").matches("%github.event.workflow_run.id%") and
+    (
+      script.splitAt("\n").regexpMatch(unzipRegexp()) or
+      this.getAFollowingStep().(Run).getScript().splitAt("\n").regexpMatch(unzipRegexp())
     )
-    or
-    // eg: - run: gh run download "${WORKFLOW_RUN_ID}" --repo "${GITHUB_REPOSITORY}" --name "artifact_name"
-    this.(Run).getScript().splitAt("\n").regexpMatch(".*gh\\s+run\\s+download.*")
-    or
+  }
+
+  override string getPath() {
+    if
+      this.getAFollowingStep()
+          .(Run)
+          .getScript()
+          .splitAt("\n")
+          .regexpMatch(unzipRegexp() + unzipDirArgRegexp()) or
+      script.splitAt("\n").regexpMatch(unzipRegexp() + unzipDirArgRegexp())
+    then
+      result = script.splitAt("\n").regexpCapture(unzipRegexp() + unzipDirArgRegexp(), 2) or
+      result =
+        this.getAFollowingStep()
+            .(Run)
+            .getScript()
+            .splitAt("\n")
+            .regexpCapture(unzipRegexp() + unzipDirArgRegexp(), 2)
+    else
+      if
+        this.getAFollowingStep().(Run).getScript().splitAt("\n").regexpMatch(unzipRegexp()) or
+        script.splitAt("\n").regexpMatch(unzipRegexp())
+      then result = ""
+      else none()
+  }
+}
+
+class DirectArtifactDownloadStep extends ArtifactDownloadStep, Run {
+  string script;
+
+  DirectArtifactDownloadStep() {
     // eg:
     // run: |
     //   artifacts_url=${{ github.event.workflow_run.artifacts_url }}
@@ -45,6 +131,69 @@ class ArtifactDownloadStep extends Step {
     //     gh api $url > "$name.zip"
     //     unzip -d "$name" "$name.zip"
     //   done
-    this.(Run).getScript().splitAt("\n").matches("%github.event.workflow_run.artifacts_url%")
+    this.getScript() = script and
+    script.splitAt("\n").matches("%github.event.workflow_run.artifacts_url%") and
+    (
+      script.splitAt("\n").regexpMatch(unzipRegexp()) or
+      this.getAFollowingStep().(Run).getScript().splitAt("\n").regexpMatch(unzipRegexp())
+    )
+  }
+
+  override string getPath() {
+    if
+      script.splitAt("\n").regexpMatch(unzipRegexp() + unzipDirArgRegexp()) or
+      this.getAFollowingStep()
+          .(Run)
+          .getScript()
+          .splitAt("\n")
+          .regexpMatch(unzipRegexp() + unzipDirArgRegexp())
+    then
+      result = script.splitAt("\n").regexpCapture(unzipRegexp() + unzipDirArgRegexp(), 2) or
+      result =
+        this.getAFollowingStep()
+            .(Run)
+            .getScript()
+            .splitAt("\n")
+            .regexpCapture(unzipRegexp() + unzipDirArgRegexp(), 2)
+    else result = ""
   }
 }
+
+abstract class PoisonableStep extends Step { }
+
+class CommandExecutionRunStep extends PoisonableStep, Run {
+  CommandExecutionRunStep() {
+    exists(ArtifactDownloadStep step |
+      step.getAFollowingStep() = this and
+      // Heuristic:
+      // Run step with a command starting with `./xxxx`, `sh xxxx`, `node xxxx`, ...
+      // eg: `./test.sh`, `sh test.sh`, `node test.js`, ...
+      this.getScript()
+          .trim()
+          .regexpMatch(".*(./|(node|python|ruby|sh)\\s+)" + step.getPath() + ".*")
+    )
+  }
+}
+
+predicate writeToGithubEnv(Run run, string key, string value) {
+  exists(string script, string line |
+    script = run.getScript() and
+    line = script.splitAt("\n") and
+    key = line.regexpCapture("echo\\s+(\")?([^=]+)\\s*=(.*)(\")?\\s*>>\\s*\\$GITHUB_ENV", 2) and
+    value = line.regexpCapture("echo\\s+(\")?([^=]+)\\s*=(.*)(\")?\\s*>>\\s*\\$GITHUB_ENV", 3)
+  )
+}
+
+class EnvVarInjectionRunStep extends PoisonableStep, Run {
+  EnvVarInjectionRunStep() {
+    exists(ArtifactDownloadStep step, string value |
+      step.getAFollowingStep() = this and
+      // Heuristic:
+      // Run step with env var definition based on file content.
+      // eg: `echo "sha=$(cat test-results/sha-number)" >> $GITHUB_ENV`
+      writeToGithubEnv(this, _, value) and
+      value.regexpMatch(".*cat\\s+.*")
+    )
+  }
+}
+// TODO: Taint Step for output var definition based on file content. eg: `echo "sha=$(cat test-results/sha-number)" >> $GITHUB_OUTPUT`

ql/lib/codeql/actions/security/EnvVarInjectionQuery.qll

@@ -8,8 +8,8 @@ predicate writeToGithubEnvSink(DataFlow::Node exprNode, string key, string value
   exists(Expression expr, Run run, string script, string line |
     script = run.getScript() and
     line = script.splitAt("\n") and
-    key = line.regexpCapture("echo\\s+([^=]+)\\s*=(.*)>>\\s*\\$GITHUB_ENV", 1) and
-    value = line.regexpCapture("echo\\s+([^=]+)\\s*=(.*)>>\\s*\\$GITHUB_ENV", 2) and
+    key = line.regexpCapture("echo\\s+(\")?([^=]+)\\s*=(.*)(\")?\\s*>>\\s*\\$GITHUB_ENV", 2) and
+    value = line.regexpCapture("echo\\s+(\")?([^=]+)\\s*=(.*)(\")?\\s*>>\\s*\\$GITHUB_ENV", 3) and
     expr = exprNode.asExpr() and
     run.getAnScriptExpr() = expr and
     value.indexOf(expr.getRawExpression()) > 0

ql/src/Security/CWE-829/ArtifactPoisoning.ql

@@ -14,13 +14,10 @@
 import actions
 import codeql.actions.security.ArtifactPoisoningQuery
 
-from LocalJob job, ArtifactDownloadStep download, Step run
+from LocalJob job, ArtifactDownloadStep downloadStep, PoisonableStep step
 where
+  // Workflow is privileged
   job.getWorkflow().isPrivileged() and
-  (run instanceof Run or run instanceof UsesStep) and
-  exists(int i, int j |
-    job.getStep(i) = download and
-    job.getStep(j) = run and
-    i < j
-  )
-select download, "Potential artifact poisoning."
+  // Download step is followed by a step that may be poisoned by the download
+  downloadStep.getAFollowingStep() = step
+select downloadStep, "Potential artifact poisoning."

ql/test/library-tests/test.expected

@@ -351,6 +351,10 @@ sources
 | tj-actions/changed-files | * | output.unknown_files | PR changed files |
 | tj-actions/changed-files | * | output.unmerged_files | PR changed files |
 | tj-actions/verify-changed-files | * | output.changed-files | PR changed files |
+| trilom/file-changes-action | * | output.files | PR changed files |
+| trilom/file-changes-action | * | output.files_added | PR changed files |
+| trilom/file-changes-action | * | output.files_modified | PR changed files |
+| trilom/file-changes-action | * | output.files_removed | PR changed files |
 | tzkhan/pr-update-action | * | output.headMatch |  |
 | xt0rted/slash-command-action | * | output.command-arguments |  |
 summaries

ql/test/query-tests/Security/CWE-829/.github/workflows/artifactpoisoning11.yml

@@ -0,0 +1,41 @@
+name: Pull Request Open
+
+on: 
+  workflow_run:
+    workflows: ["Prev"]
+    types:
+      - completed
+
+jobs:
+  Download:
+    runs-on: ubuntu-latest
+    steps:
+      - uses: actions/github-script@v6
+        with:
+          script: |
+            let allArtifacts = await github.rest.actions.listWorkflowRunArtifacts({
+                owner: context.repo.owner,
+                repo: context.repo.repo,
+                run_id: context.payload.workflow_run.id,
+            });
+            let matchArtifact = allArtifacts.data.artifacts.filter((artifact) => {
+                return artifact.name == "<ARTEFACT_NAME>"
+            })[0];
+            let download = await github.rest.actions.downloadArtifact({
+                owner: context.repo.owner,
+                repo: context.repo.repo,
+                artifact_id: matchArtifact.id,
+                archive_format: 'zip',
+            });
+            let fs = require('fs');
+            fs.writeFileSync(`${process.env.GITHUB_WORKSPACE}/sonarcloud-data.zip`, Buffer.from(download.data)); 
+      - name: Unzip
+        run: |
+          unzip sonarcloud-data.zip -d sonarcloud-data
+          ls -a sonarcloud-data
+      - name: Run command
+        run: 
+          ./sonarcloud-data/x.py build -j$(nproc) --compiler gcc --skip-build
+
+
+

ql/test/query-tests/Security/CWE-829/.github/workflows/artifactpoisoning12.yml

@@ -27,8 +27,14 @@ jobs:
                 artifact_id: matchArtifact.id,
                 archive_format: 'zip',
             });
+            let fs = require('fs');
+            fs.writeFileSync(`${process.env.GITHUB_WORKSPACE}/sonarcloud-data.zip`, Buffer.from(download.data)); 
+      - name: Unzip
+        run: |
+          unzip sonarcloud-data.zip
+          ls -a sonarcloud-data
       - name: Run command
-        run: cmd
-
+        run: 
+          ./x.py build -j$(nproc) --compiler gcc --skip-build
 
 

ql/test/query-tests/Security/CWE-829/.github/workflows/artifactpoisoning21.yml

@@ -10,10 +10,14 @@ jobs:
   Download:
     runs-on: ubuntu-latest
     steps:
-      - run: |
-          gh run download "${WORKFLOW_RUN_ID}" --repo "${GITHUB_REPOSITORY}" --name "artifact_name"
+      - uses: dawidd6/action-download-artifact@v2
+        with:
+          name: artifact_name
+          workflow: wf.yml
+          path: foo
       - name: Run command
-        run: cmd
+        run: |
+          ./foo/cmd
 
 
 

ql/test/query-tests/Security/CWE-829/.github/workflows/artifactpoisoning22.yml

@@ -15,7 +15,7 @@ jobs:
           name: artifact_name
           workflow: wf.yml
       - name: Run command
-        run: cmd
+        run: ./cmd
 
 
 

ql/test/query-tests/Security/CWE-829/.github/workflows/artifactpoisoning31.yml

@@ -0,0 +1,22 @@
+name: Pull Request Open
+
+on: 
+  workflow_run:
+    workflows: ["Prev"]
+    types:
+      - completed
+
+jobs:
+  Download:
+    runs-on: ubuntu-latest
+    steps:
+      - run: |
+          gh run download "${{github.event.workflow_run.id}}" --repo "${GITHUB_REPOSITORY}" --name "artifact_name"
+      - name: Unzip
+        run: |
+          unzip artifact_name.zip -d foo
+      - name: Run command
+        run: ./foo/cmd
+
+
+

ql/test/query-tests/Security/CWE-829/.github/workflows/artifactpoisoning32.yml

@@ -0,0 +1,21 @@
+name: Pull Request Open
+
+on: 
+  workflow_run:
+    workflows: ["Prev"]
+    types:
+      - completed
+
+jobs:
+  Download:
+    runs-on: ubuntu-latest
+    steps:
+      - run: |
+          gh run download "${{github.event.workflow_run.id}}" --repo "${GITHUB_REPOSITORY}" --name "artifact_name" --dir foo
+          unzip artifact_name.zip -d bar
+      - name: Run command
+        run: |
+          ./bar/cmd
+
+
+

ql/test/query-tests/Security/CWE-829/.github/workflows/artifactpoisoning33.yml

@@ -0,0 +1,21 @@
+name: Pull Request Open
+
+on: 
+  workflow_run:
+    workflows: ["Prev"]
+    types:
+      - completed
+
+jobs:
+  Download:
+    runs-on: ubuntu-latest
+    steps:
+      - run: |
+          gh run download "${{github.event.workflow_run.id}}" --repo "${GITHUB_REPOSITORY}" --name "artifact_name" --dir foo
+          unzip foo/artifact_name.zip
+      - name: Run command
+        run: |
+          ./bar/cmd
+
+
+

ql/test/query-tests/Security/CWE-829/.github/workflows/artifactpoisoning41.yml

@@ -0,0 +1,25 @@
+name: Pull Request Open
+
+on: 
+  workflow_run:
+    workflows: ["Prev"]
+    types:
+      - completed
+
+jobs:
+  Download:
+    runs-on: ubuntu-latest
+    steps:
+      - run: |
+          artifacts_url=${{ github.event.workflow_run.artifacts_url }}
+          gh api "$artifacts_url" -q '.artifacts[] | [.name, .archive_download_url] | @tsv' | while read artifact
+          do
+            IFS=$'\t' read name url <<< "$artifact"
+            gh api $url > "$name.zip"
+            unzip -d "foo" "$name.zip"
+          done
+      - name: Run command
+        run: ./foo/cmd
+
+
+

ql/test/query-tests/Security/CWE-829/.github/workflows/artifactpoisoning42.yml

@@ -16,10 +16,10 @@ jobs:
           do
             IFS=$'\t' read name url <<< "$artifact"
             gh api $url > "$name.zip"
-            unzip -d "$name" "$name.zip"
+            unzip "$name.zip"
           done
       - name: Run command
-        run: cmd
+        run: ./cmd
 
 
 

ql/test/query-tests/Security/CWE-829/.github/workflows/artifactpoisoning51.yml

@@ -0,0 +1,24 @@
+name: Pull Request Open
+
+on: 
+  workflow_run:
+    workflows: ["Prev"]
+    types:
+      - completed
+
+jobs:
+  Download:
+    runs-on: ubuntu-latest
+    steps:
+      - run: |
+          gh run download "${{github.event.workflow_run.id}}" --repo "${GITHUB_REPOSITORY}" --name "artifact_name"
+      - name: Unzip
+        run: |
+          unzip artifact_name.zip -d foo
+      - name: Env Var Injection
+        run: |
+          echo "pr_number=$(cat foo/bar)" >> $GITHUB_ENV
+
+
+
+

ql/test/query-tests/Security/CWE-829/ArtifactPoisoning.expected

@@ -1,4 +1,10 @@
-| .github/workflows/artifactpoisoning1.yml:13:9:30:6 | Uses Step | Potential artifact poisoning. |
-| .github/workflows/artifactpoisoning2.yml:13:9:17:6 | Uses Step | Potential artifact poisoning. |
-| .github/workflows/artifactpoisoning3.yml:13:9:15:6 | Run Step | Potential artifact poisoning. |
-| .github/workflows/artifactpoisoning4.yml:13:9:21:6 | Run Step | Potential artifact poisoning. |
+| .github/workflows/artifactpoisoning11.yml:13:9:32:6 | Uses Step | Potential artifact poisoning. |
+| .github/workflows/artifactpoisoning12.yml:13:9:32:6 | Uses Step | Potential artifact poisoning. |
+| .github/workflows/artifactpoisoning21.yml:13:9:18:6 | Uses Step | Potential artifact poisoning. |
+| .github/workflows/artifactpoisoning22.yml:13:9:17:6 | Uses Step | Potential artifact poisoning. |
+| .github/workflows/artifactpoisoning31.yml:13:9:15:6 | Run Step | Potential artifact poisoning. |
+| .github/workflows/artifactpoisoning32.yml:13:9:16:6 | Run Step | Potential artifact poisoning. |
+| .github/workflows/artifactpoisoning33.yml:13:9:16:6 | Run Step | Potential artifact poisoning. |
+| .github/workflows/artifactpoisoning41.yml:13:9:21:6 | Run Step | Potential artifact poisoning. |
+| .github/workflows/artifactpoisoning42.yml:13:9:21:6 | Run Step | Potential artifact poisoning. |
+| .github/workflows/artifactpoisoning51.yml:13:9:15:6 | Run Step | Potential artifact poisoning. |

ql/test/query-tests/Security/CWE-829/UnpinnedActionsTag.expected

@@ -1,6 +1,7 @@
 | .github/workflows/actor_trusted_checkout.yml:19:7:23:4 | Uses Step | Unpinned 3rd party Action 'actor_trusted_checkout.yml' step $@ uses 'completely/fakeaction' with ref 'v2', not a pinned commit hash | .github/workflows/actor_trusted_checkout.yml:19:7:23:4 | Uses Step | Uses Step |
 | .github/workflows/actor_trusted_checkout.yml:23:7:26:21 | Uses Step | Unpinned 3rd party Action 'actor_trusted_checkout.yml' step $@ uses 'fakerepo/comment-on-pr' with ref 'v1', not a pinned commit hash | .github/workflows/actor_trusted_checkout.yml:23:7:26:21 | Uses Step | Uses Step |
-| .github/workflows/artifactpoisoning2.yml:13:9:17:6 | Uses Step | Unpinned 3rd party Action 'Pull Request Open' step $@ uses 'dawidd6/action-download-artifact' with ref 'v2', not a pinned commit hash | .github/workflows/artifactpoisoning2.yml:13:9:17:6 | Uses Step | Uses Step |
+| .github/workflows/artifactpoisoning21.yml:13:9:18:6 | Uses Step | Unpinned 3rd party Action 'Pull Request Open' step $@ uses 'dawidd6/action-download-artifact' with ref 'v2', not a pinned commit hash | .github/workflows/artifactpoisoning21.yml:13:9:18:6 | Uses Step | Uses Step |
+| .github/workflows/artifactpoisoning22.yml:13:9:17:6 | Uses Step | Unpinned 3rd party Action 'Pull Request Open' step $@ uses 'dawidd6/action-download-artifact' with ref 'v2', not a pinned commit hash | .github/workflows/artifactpoisoning22.yml:13:9:17:6 | Uses Step | Uses Step |
 | .github/workflows/auto_ci.yml:93:9:96:6 | Uses Step | Unpinned 3rd party Action 'Python CI' step $@ uses 'codecov/codecov-action' with ref 'v3', not a pinned commit hash | .github/workflows/auto_ci.yml:93:9:96:6 | Uses Step | Uses Step |
 | .github/workflows/auto_ci.yml:108:9:119:6 | Uses Step: create_pr | Unpinned 3rd party Action 'Python CI' step $@ uses 'peter-evans/create-pull-request' with ref 'v5', not a pinned commit hash | .github/workflows/auto_ci.yml:108:9:119:6 | Uses Step: create_pr | Uses Step: create_pr |
 | .github/workflows/auto_ci.yml:125:9:133:6 | Uses Step | Unpinned 3rd party Action 'Python CI' step $@ uses 'thollander/actions-comment-pull-request' with ref 'v2', not a pinned commit hash | .github/workflows/auto_ci.yml:125:9:133:6 | Uses Step | Uses Step |