Merge pull request #86 from github/analyze_reusable_workflows

Cross remote Reusable Workflow analysis
github · Sep 22, 2024 · b685a8d · b685a8d
2 parents a1e44bc + d44e7ae
commit b685a8d
Show file tree

Hide file tree

Showing 10 changed files with 147 additions and 23 deletions.
diff --git a/ql/lib/codeql/actions/Helper.qll b/ql/lib/codeql/actions/Helper.qll
@@ -252,10 +252,26 @@ predicate inPrivilegedExternallyTriggerableJob(AstNode node) {
   )
 }
 
+predicate calledByPrivilegedExternallyTriggerableJob(AstNode node) {
+  exists(ReusableWorkflow rw, ExternalJob caller, Job callee |
+    callee = node.getEnclosingJob() and
+    rw.getACaller() = caller and
+    rw.getAJob() = callee and
+    caller.isPrivilegedExternallyTriggerable()
+  )
+  or
+  exists(LocalJob caller |
+    caller = node.getEnclosingCompositeAction().getACallerJob() and
+    caller.isPrivilegedExternallyTriggerable()
+  )
+}
+
 predicate inPrivilegedContext(AstNode node) {
   inPrivilegedCompositeAction(node)
   or
   inPrivilegedExternallyTriggerableJob(node)
+  or
+  calledByPrivilegedExternallyTriggerableJob(node)
 }
 
 predicate inNonPrivilegedCompositeAction(AstNode node) {

diff --git a/ql/lib/codeql/actions/dataflow/internal/DataFlowPrivate.qll b/ql/lib/codeql/actions/dataflow/internal/DataFlowPrivate.qll
@@ -89,6 +89,23 @@ class DataFlowCall instanceof Cfg::Node {
   Location getLocation() { result = this.(Cfg::Node).getLocation() }
 }
 
+string getRepoRoot() {
+  exists(Workflow w |
+    w.getLocation().getFile().getRelativePath().indexOf("/.github/workflows") > 0 and
+    result =
+      w.getLocation()
+          .getFile()
+          .getRelativePath()
+          .prefix(w.getLocation().getFile().getRelativePath().indexOf("/.github/workflows") + 1) and
+    // exclude workflow_enum reusable workflows directory root
+    not result.indexOf(".github/reusable_workflows/") > -1
+    or
+    not w.getLocation().getFile().getRelativePath().indexOf("/.github/workflows") > 0 and
+    not w.getLocation().getFile().getRelativePath().indexOf(".github/reusable_workflows") > -1 and
+    result = ""
+  )
+}
+
 /**
  * A Cfg scope that can be called
  */
@@ -97,28 +114,7 @@ class DataFlowCallable instanceof Cfg::CfgScope {
 
   string getName() {
     if this instanceof ReusableWorkflow
-    then
-      //result = this.(ReusableWorkflow).getLocation().getFile().getRelativePath()
-      result =
-        this.(ReusableWorkflow)
-            .getLocation()
-            .getFile()
-            .getRelativePath()
-            .suffix(this.(ReusableWorkflow)
-                    .getLocation()
-                    .getFile()
-                    .getRelativePath()
-                    .indexOf("/.github/workflows") + 1) or
-      result =
-        this.(ReusableWorkflow)
-            .getLocation()
-            .getFile()
-            .getRelativePath()
-            .suffix(this.(ReusableWorkflow)
-                  .getLocation()
-                  .getFile()
-                  .getRelativePath()
-                  .indexOf(".github/workflows"))
+    then result = this.(ReusableWorkflow).getLocation().getFile().getRelativePath() // or
     else
       if this instanceof CompositeAction
       then
@@ -154,7 +150,13 @@ class NormalReturn extends ReturnKind, TNormalReturn {
 }
 
 /** Gets a viable implementation of the target of the given `Call`. */
-DataFlowCallable viableCallable(DataFlowCall c) { c.getName() = result.getName() }
+DataFlowCallable viableCallable(DataFlowCall c) {
+  c.getName() = result.getName() or
+  c.getName() = result.getName().replaceAll(getRepoRoot(), "") or
+  // special case for reusable workflows downloaded by the workflow_enum action
+  c.getName() =
+    result.getName().replaceAll(getRepoRoot(), "").replaceAll(".github/reusable_workflows/", "")
+}
 
 /**
  * Gets a node that can read the value returned from `call` with return kind

diff --git a/ql/test/query-tests/Security/CWE-094/CodeInjectionCritical.expected b/ql/test/query-tests/Security/CWE-094/CodeInjectionCritical.expected
@@ -1,4 +1,5 @@
 edges
+| .github/actions/action5/action.yml:4:3:4:7 | input taint | .github/actions/action5/action.yml:26:19:26:37 | inputs.taint | provenance |  |
 | .github/workflows/argus_case_study.yml:15:9:24:6 | Uses Step: remove_quotations [replaced] | .github/workflows/argus_case_study.yml:27:33:27:77 | steps.remove_quotations.outputs.replaced | provenance |  |
 | .github/workflows/argus_case_study.yml:17:25:17:53 | github.event.issue.title | .github/workflows/argus_case_study.yml:22:20:22:39 | env.ISSUE_TITLE | provenance |  |
 | .github/workflows/argus_case_study.yml:22:20:22:39 | env.ISSUE_TITLE | .github/workflows/argus_case_study.yml:15:9:24:6 | Uses Step: remove_quotations [replaced] | provenance |  |
@@ -29,6 +30,7 @@ edges
 | .github/workflows/changed-files.yml:15:9:18:6 | Uses Step: changed-files1 | .github/workflows/changed-files.yml:20:24:20:76 | steps.changed-files1.outputs.all_changed_files | provenance |  |
 | .github/workflows/changed-files.yml:33:9:38:6 | Uses Step: changed-files3 | .github/workflows/changed-files.yml:40:24:40:76 | steps.changed-files3.outputs.all_changed_files | provenance |  |
 | .github/workflows/changed-files.yml:53:9:56:6 | Uses Step: changed-files5 | .github/workflows/changed-files.yml:58:24:58:76 | steps.changed-files5.outputs.all_changed_files | provenance |  |
+| .github/workflows/composite-action-caller-3.yml:12:19:12:50 | github.event.comment.body | .github/actions/action5/action.yml:4:3:4:7 | input taint | provenance |  |
 | .github/workflows/cross3.yml:27:7:37:4 | Uses Step: remove_quotations [replaced] | .github/workflows/cross3.yml:39:31:39:75 | steps.remove_quotations.outputs.replaced | provenance |  |
 | .github/workflows/cross3.yml:27:7:37:4 | Uses Step: remove_quotations [replaced] | .github/workflows/cross3.yml:57:29:57:73 | steps.remove_quotations.outputs.replaced | provenance |  |
 | .github/workflows/cross3.yml:32:18:32:53 | github.event.commits[0].message | .github/workflows/cross3.yml:27:7:37:4 | Uses Step: remove_quotations [replaced] | provenance |  |
@@ -126,7 +128,9 @@ nodes
 | .github/actions/action1/action.yml:7:19:7:55 | github.event.pull_request.body | semmle.label | github.event.pull_request.body |
 | .github/actions/action3/action.yml:9:19:9:55 | github.event.pull_request.body | semmle.label | github.event.pull_request.body |
 | .github/actions/action4/action.yml:7:19:7:55 | github.event.pull_request.body | semmle.label | github.event.pull_request.body |
+| .github/actions/action5/action.yml:4:3:4:7 | input taint | semmle.label | input taint |
 | .github/actions/action5/action.yml:16:19:16:55 | github.event.pull_request.body | semmle.label | github.event.pull_request.body |
+| .github/actions/action5/action.yml:26:19:26:37 | inputs.taint | semmle.label | inputs.taint |
 | .github/workflows/argus_case_study.yml:15:9:24:6 | Uses Step: remove_quotations [replaced] | semmle.label | Uses Step: remove_quotations [replaced] |
 | .github/workflows/argus_case_study.yml:17:25:17:53 | github.event.issue.title | semmle.label | github.event.issue.title |
 | .github/workflows/argus_case_study.yml:22:20:22:39 | env.ISSUE_TITLE | semmle.label | env.ISSUE_TITLE |
@@ -179,6 +183,7 @@ nodes
 | .github/workflows/comment_issue_newline.yml:10:25:10:56 | github.event.comment.body | semmle.label | github.event.comment.body |
 | .github/workflows/comment_issue_newline.yml:11:24:11:51 | github.event.issue.body | semmle.label | github.event.issue.body |
 | .github/workflows/comment_issue_newline.yml:12:24:12:55 | github.event.comment.body | semmle.label | github.event.comment.body |
+| .github/workflows/composite-action-caller-3.yml:12:19:12:50 | github.event.comment.body | semmle.label | github.event.comment.body |
 | .github/workflows/cross3.yml:27:7:37:4 | Uses Step: remove_quotations [replaced] | semmle.label | Uses Step: remove_quotations [replaced] |
 | .github/workflows/cross3.yml:32:18:32:53 | github.event.commits[0].message | semmle.label | github.event.commits[0].message |
 | .github/workflows/cross3.yml:39:31:39:75 | steps.remove_quotations.outputs.replaced | semmle.label | steps.remove_quotations.outputs.replaced |
@@ -385,6 +390,7 @@ subpaths
 #select
 | .github/actions/action1/action.yml:7:19:7:55 | github.event.pull_request.body | .github/actions/action1/action.yml:7:19:7:55 | github.event.pull_request.body | .github/actions/action1/action.yml:7:19:7:55 | github.event.pull_request.body | Potential code injection in $@, which may be controlled by an external user. | .github/actions/action1/action.yml:7:19:7:55 | github.event.pull_request.body | ${{ github.event.pull_request.body }} |
 | .github/actions/action5/action.yml:16:19:16:55 | github.event.pull_request.body | .github/actions/action5/action.yml:16:19:16:55 | github.event.pull_request.body | .github/actions/action5/action.yml:16:19:16:55 | github.event.pull_request.body | Potential code injection in $@, which may be controlled by an external user. | .github/actions/action5/action.yml:16:19:16:55 | github.event.pull_request.body | ${{ github.event.pull_request.body }} |
+| .github/actions/action5/action.yml:26:19:26:37 | inputs.taint | .github/workflows/composite-action-caller-3.yml:12:19:12:50 | github.event.comment.body | .github/actions/action5/action.yml:26:19:26:37 | inputs.taint | Potential code injection in $@, which may be controlled by an external user. | .github/actions/action5/action.yml:26:19:26:37 | inputs.taint | ${{ inputs.taint }} |
 | .github/workflows/argus_case_study.yml:27:33:27:77 | steps.remove_quotations.outputs.replaced | .github/workflows/argus_case_study.yml:17:25:17:53 | github.event.issue.title | .github/workflows/argus_case_study.yml:27:33:27:77 | steps.remove_quotations.outputs.replaced | Potential code injection in $@, which may be controlled by an external user. | .github/workflows/argus_case_study.yml:27:33:27:77 | steps.remove_quotations.outputs.replaced | ${{steps.remove_quotations.outputs.replaced}} |
 | .github/workflows/artifactpoisoning1.yml:27:67:27:92 | steps.pr.outputs.id | .github/workflows/artifactpoisoning1.yml:14:9:20:6 | Uses Step | .github/workflows/artifactpoisoning1.yml:27:67:27:92 | steps.pr.outputs.id | Potential code injection in $@, which may be controlled by an external user. | .github/workflows/artifactpoisoning1.yml:27:67:27:92 | steps.pr.outputs.id | ${{ steps.pr.outputs.id }} |
 | .github/workflows/artifactpoisoning2.yml:22:17:22:42 | steps.pr.outputs.id | .github/workflows/artifactpoisoning2.yml:13:9:19:6 | Uses Step: pr | .github/workflows/artifactpoisoning2.yml:22:17:22:42 | steps.pr.outputs.id | Potential code injection in $@, which may be controlled by an external user. | .github/workflows/artifactpoisoning2.yml:22:17:22:42 | steps.pr.outputs.id | ${{ steps.pr.outputs.id }} |

diff --git a/ql/test/query-tests/Security/CWE-094/CodeInjectionMedium.expected b/ql/test/query-tests/Security/CWE-094/CodeInjectionMedium.expected
@@ -1,4 +1,5 @@
 edges
+| .github/actions/action5/action.yml:4:3:4:7 | input taint | .github/actions/action5/action.yml:26:19:26:37 | inputs.taint | provenance |  |
 | .github/workflows/argus_case_study.yml:15:9:24:6 | Uses Step: remove_quotations [replaced] | .github/workflows/argus_case_study.yml:27:33:27:77 | steps.remove_quotations.outputs.replaced | provenance |  |
 | .github/workflows/argus_case_study.yml:17:25:17:53 | github.event.issue.title | .github/workflows/argus_case_study.yml:22:20:22:39 | env.ISSUE_TITLE | provenance |  |
 | .github/workflows/argus_case_study.yml:22:20:22:39 | env.ISSUE_TITLE | .github/workflows/argus_case_study.yml:15:9:24:6 | Uses Step: remove_quotations [replaced] | provenance |  |
@@ -29,6 +30,7 @@ edges
 | .github/workflows/changed-files.yml:15:9:18:6 | Uses Step: changed-files1 | .github/workflows/changed-files.yml:20:24:20:76 | steps.changed-files1.outputs.all_changed_files | provenance |  |
 | .github/workflows/changed-files.yml:33:9:38:6 | Uses Step: changed-files3 | .github/workflows/changed-files.yml:40:24:40:76 | steps.changed-files3.outputs.all_changed_files | provenance |  |
 | .github/workflows/changed-files.yml:53:9:56:6 | Uses Step: changed-files5 | .github/workflows/changed-files.yml:58:24:58:76 | steps.changed-files5.outputs.all_changed_files | provenance |  |
+| .github/workflows/composite-action-caller-3.yml:12:19:12:50 | github.event.comment.body | .github/actions/action5/action.yml:4:3:4:7 | input taint | provenance |  |
 | .github/workflows/cross3.yml:27:7:37:4 | Uses Step: remove_quotations [replaced] | .github/workflows/cross3.yml:39:31:39:75 | steps.remove_quotations.outputs.replaced | provenance |  |
 | .github/workflows/cross3.yml:27:7:37:4 | Uses Step: remove_quotations [replaced] | .github/workflows/cross3.yml:57:29:57:73 | steps.remove_quotations.outputs.replaced | provenance |  |
 | .github/workflows/cross3.yml:32:18:32:53 | github.event.commits[0].message | .github/workflows/cross3.yml:27:7:37:4 | Uses Step: remove_quotations [replaced] | provenance |  |
@@ -126,7 +128,9 @@ nodes
 | .github/actions/action1/action.yml:7:19:7:55 | github.event.pull_request.body | semmle.label | github.event.pull_request.body |
 | .github/actions/action3/action.yml:9:19:9:55 | github.event.pull_request.body | semmle.label | github.event.pull_request.body |
 | .github/actions/action4/action.yml:7:19:7:55 | github.event.pull_request.body | semmle.label | github.event.pull_request.body |
+| .github/actions/action5/action.yml:4:3:4:7 | input taint | semmle.label | input taint |
 | .github/actions/action5/action.yml:16:19:16:55 | github.event.pull_request.body | semmle.label | github.event.pull_request.body |
+| .github/actions/action5/action.yml:26:19:26:37 | inputs.taint | semmle.label | inputs.taint |
 | .github/workflows/argus_case_study.yml:15:9:24:6 | Uses Step: remove_quotations [replaced] | semmle.label | Uses Step: remove_quotations [replaced] |
 | .github/workflows/argus_case_study.yml:17:25:17:53 | github.event.issue.title | semmle.label | github.event.issue.title |
 | .github/workflows/argus_case_study.yml:22:20:22:39 | env.ISSUE_TITLE | semmle.label | env.ISSUE_TITLE |
@@ -179,6 +183,7 @@ nodes
 | .github/workflows/comment_issue_newline.yml:10:25:10:56 | github.event.comment.body | semmle.label | github.event.comment.body |
 | .github/workflows/comment_issue_newline.yml:11:24:11:51 | github.event.issue.body | semmle.label | github.event.issue.body |
 | .github/workflows/comment_issue_newline.yml:12:24:12:55 | github.event.comment.body | semmle.label | github.event.comment.body |
+| .github/workflows/composite-action-caller-3.yml:12:19:12:50 | github.event.comment.body | semmle.label | github.event.comment.body |
 | .github/workflows/cross3.yml:27:7:37:4 | Uses Step: remove_quotations [replaced] | semmle.label | Uses Step: remove_quotations [replaced] |
 | .github/workflows/cross3.yml:32:18:32:53 | github.event.commits[0].message | semmle.label | github.event.commits[0].message |
 | .github/workflows/cross3.yml:39:31:39:75 | steps.remove_quotations.outputs.replaced | semmle.label | steps.remove_quotations.outputs.replaced |

diff --git a/...curity/CWE-829/.github/reusable_workflows/TestOrg/TestRepo/.github/workflows/reusable.yml b/...curity/CWE-829/.github/reusable_workflows/TestOrg/TestRepo/.github/workflows/reusable.yml
@@ -0,0 +1,29 @@
+name: Test
+
+on:
+  workflow_call:
+    inputs:
+      branch:
+        type: string
+        default: "**"
+
+defaults:
+  run:
+    shell: bash
+
+jobs:
+  test:
+    name: Checkout
+    runs-on: ubuntu-latest
+
+    permissions:
+      contents: write
+      pull-requests: write
+    steps:
+      - uses: actions/checkout@v2
+        with:
+          ref: ${{ inputs.branch }}
+      - run: |
+          npm install
+          npm run lint
+      
diff --git a/ql/test/query-tests/Security/CWE-829/.github/workflows/reusable_caller1.yaml b/ql/test/query-tests/Security/CWE-829/.github/workflows/reusable_caller1.yaml
@@ -0,0 +1,11 @@
+name: assets-test
+
+on:
+  pull_request_target:
+
+jobs:
+  check-execution-context:
+    uses: TestOrg/TestRepo/.github/workflows/reusable.yml@main
+    with:
+      branch: ${{ github.event.pull_request.head.ref }}
+
diff --git a/ql/test/query-tests/Security/CWE-829/.github/workflows/reusable_caller2.yaml b/ql/test/query-tests/Security/CWE-829/.github/workflows/reusable_caller2.yaml
@@ -0,0 +1,11 @@
+name: assets-test
+
+on:
+  pull_request:
+
+jobs:
+  check-execution-context:
+    uses: TestOrg/TestRepo/.github/workflows/reusable.yml@main
+    with:
+      branch: ${{ github.event.pull_request.head.ref }}
+
diff --git a/ql/test/query-tests/Security/CWE-829/.github/workflows/reusable_caller3.yaml b/ql/test/query-tests/Security/CWE-829/.github/workflows/reusable_caller3.yaml
@@ -0,0 +1,11 @@
+name: assets-test
+
+on:
+  pull_request:
+
+jobs:
+  check-execution-context:
+    uses: ./.github/workflows/reusable_local.yml
+    with:
+      branch: ${{ github.event.pull_request.head.ref }}
+
diff --git a/ql/test/query-tests/Security/CWE-829/.github/workflows/reusable_local.yml b/ql/test/query-tests/Security/CWE-829/.github/workflows/reusable_local.yml
@@ -0,0 +1,29 @@
+name: Test
+
+on:
+  workflow_call:
+    inputs:
+      branch:
+        type: string
+        default: "**"
+
+defaults:
+  run:
+    shell: bash
+
+jobs:
+  test:
+    name: Checkout
+    runs-on: ubuntu-latest
+
+    permissions:
+      contents: write
+      pull-requests: write
+    steps:
+      - uses: actions/checkout@v2
+        with:
+          ref: ${{ inputs.branch }}
+      - run: |
+          npm install
+          npm run lint
+