From ddf4bb194ef1562a45ad17d5a1066f88021f23c3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alvaro=20Mu=C3=B1oz?= Date: Mon, 6 May 2024 23:32:06 +0200 Subject: [PATCH 1/7] Fix incorrect source for dorny path filters --- .../codeql/actions/dataflow/FlowSources.qll | 17 ++++- ql/lib/codeql/actions/dataflow/FlowSteps.qll | 20 ++++++ ql/lib/ext/dorny_paths-filter.model.yml | 6 -- ql/test/library-tests/test.expected | 1 - .../CWE-094/.github/workflows/test2.yml | 64 +++++++++++++++++++ .../Security/CWE-094/CodeInjection.expected | 6 ++ .../CWE-094/PrivilegedCodeInjection.expected | 8 +++ 7 files changed, 114 insertions(+), 8 deletions(-) delete mode 100644 ql/lib/ext/dorny_paths-filter.model.yml create mode 100644 ql/test/query-tests/Security/CWE-094/.github/workflows/test2.yml diff --git a/ql/lib/codeql/actions/dataflow/FlowSources.qll b/ql/lib/codeql/actions/dataflow/FlowSources.qll index 580fb1d25ab..db111b9e190 100644 --- a/ql/lib/codeql/actions/dataflow/FlowSources.qll +++ b/ql/lib/codeql/actions/dataflow/FlowSources.qll @@ -252,10 +252,25 @@ class CompositeActionInputSource extends RemoteFlowSource { } /** - * A downloadeded artifact. + * A downloaded artifact. */ private class ArtifactSource extends RemoteFlowSource { ArtifactSource() { this.asExpr() instanceof UntrustedArtifactDownloadStep } override string getSourceType() { result = "artifact" } } + +/** + * A list of file names returned by dorny/paths-filter. + */ +private class DornyPathsFilterSource extends RemoteFlowSource { + DornyPathsFilterSource() { + exists(UsesStep u | + u.getCallee() = "dorny/paths-filter" and + u.getArgument("list-files") = ["csv", "json"] and + this.asExpr() = u + ) + } + + override string getSourceType() { result = "filename" } +} diff --git a/ql/lib/codeql/actions/dataflow/FlowSteps.qll b/ql/lib/codeql/actions/dataflow/FlowSteps.qll index b24f9484a80..32c329d8c67 100644 --- a/ql/lib/codeql/actions/dataflow/FlowSteps.qll +++ b/ql/lib/codeql/actions/dataflow/FlowSteps.qll @@ -124,3 +124,23 @@ class ArtifactDownloadToUseTaintStep extends AdditionalTaintStep { artifactDownloadToUseStep(node1, node2) } } + +/** + * A read of the _files field of the dorny/paths-filter action. + */ +predicate dornyPathsFilterTaintStep(DataFlow::Node pred, DataFlow::Node succ) { + exists(UsesStep u, StepsExpression o | + u.getCallee() = "dorny/paths-filter" and + u.getArgument("list-files") = ["csv", "json"] and + u = pred.asExpr() and + o.getStepId() = u.getId() and + o.getFieldName().matches("%_files") and + succ.asExpr() = o + ) +} + +class DornyPathsFilterTaintStep extends AdditionalTaintStep { + override predicate step(DataFlow::Node node1, DataFlow::Node node2) { + dornyPathsFilterTaintStep(node1, node2) + } +} diff --git a/ql/lib/ext/dorny_paths-filter.model.yml b/ql/lib/ext/dorny_paths-filter.model.yml deleted file mode 100644 index 79621a6a30c..00000000000 --- a/ql/lib/ext/dorny_paths-filter.model.yml +++ /dev/null @@ -1,6 +0,0 @@ -extensions: - - addsTo: - pack: githubsecuritylab/actions-all - extensible: sourceModel - data: - - ["dorny/paths-filter", "*", "output.changes", "filename", "manual"] diff --git a/ql/test/library-tests/test.expected b/ql/test/library-tests/test.expected index 5bd009b31b0..d7f944c5a3d 100644 --- a/ql/test/library-tests/test.expected +++ b/ql/test/library-tests/test.expected @@ -438,7 +438,6 @@ sources | amannn/action-semantic-pull-request | * | output.error_message | text | manual | | cypress-io/github-action | * | env.GH_BRANCH | branch | manual | | dawidd6/action-download-artifact | * | output.artifacts | artifact | manual | -| dorny/paths-filter | * | output.changes | filename | manual | | franzdiebold/github-env-vars-action | * | output.CI_PR_DESCRIPTION | text | manual | | franzdiebold/github-env-vars-action | * | output.CI_PR_TITLE | title | manual | | googlecloudplatform/magic-modules | * | output.changed-files | filename | manual | diff --git a/ql/test/query-tests/Security/CWE-094/.github/workflows/test2.yml b/ql/test/query-tests/Security/CWE-094/.github/workflows/test2.yml new file mode 100644 index 00000000000..03ee63fe9cf --- /dev/null +++ b/ql/test/query-tests/Security/CWE-094/.github/workflows/test2.yml @@ -0,0 +1,64 @@ +name: List files + +on: + pull_request_target: + types: [ opened, synchronize, workflow_dispatch] + +permissions: {} +jobs: + test: + permissions: + contents: write + pull-requests: write + runs-on: ubuntu-latest + env: + GITHUB_TOKEN: "${{ secrets.GITHUB_TOKEN }}" + steps: + - name: Check for relevant changes + uses: dorny/paths-filter@v3 + id: changed + with: + list-files: json + filters: | + locale: + - '*.xml' + - name: Changed files 1 + run: | + echo changed: ${{ steps.changed.outputs.locale_files }} + echo changed: ${{ steps.changed.outputs.changes }} + - name: Check for relevant changes + uses: dorny/paths-filter@v3 + id: changed2 + with: + list-files: csv + filters: | + locale: + - '*.xml' + - name: Changed files 2 + run: | + echo changed:${{ steps.changed2.outputs.locale_files }} + echo changed: ${{ steps.changed2.outputs.changes }} + - name: Check for relevant changes + uses: dorny/paths-filter@v3 + id: changed3 + with: + list-files: shell + filters: | + locale: + - '*.xml' + - name: Changed files 3 + run: | + echo changed:${{ steps.changed3.outputs.locale_files }} + echo changed: ${{ steps.changed3.outputs.changes }} + - name: Check for relevant changes + uses: dorny/paths-filter@v3 + id: changed4 + with: + list-files: escape + filters: | + locale: + - '*.xml' + - name: Changed files 4 + run: | + echo changed:${{ steps.changed4.outputs.locale_files }} + echo changed: ${{ steps.changed4.outputs.changes }} diff --git a/ql/test/query-tests/Security/CWE-094/CodeInjection.expected b/ql/test/query-tests/Security/CWE-094/CodeInjection.expected index 50cb0c40d24..e220d368b20 100644 --- a/ql/test/query-tests/Security/CWE-094/CodeInjection.expected +++ b/ql/test/query-tests/Security/CWE-094/CodeInjection.expected @@ -55,6 +55,8 @@ edges | .github/workflows/simple2.yml:18:9:26:6 | Uses Step: step [value] | .github/workflows/simple2.yml:29:24:29:54 | steps.step.outputs.value | | .github/workflows/simple2.yml:22:20:22:64 | steps.source.outputs.all_changed_files | .github/workflows/simple2.yml:18:9:26:6 | Uses Step: step [value] | | .github/workflows/test1.yml:22:38:22:75 | github.event.pull_request.title | .github/workflows/test1.yml:25:20:25:39 | env.ISSUE_KEY | +| .github/workflows/test2.yml:17:9:25:6 | Uses Step: changed | .github/workflows/test2.yml:27:26:27:66 | steps.changed.outputs.locale_files | +| .github/workflows/test2.yml:29:9:37:6 | Uses Step: changed2 | .github/workflows/test2.yml:39:25:39:66 | steps.changed2.outputs.locale_files | | .github/workflows/test.yml:8:7:10:4 | Job outputs node [job_output] | .github/workflows/test.yml:49:20:49:56 | needs.job1.outputs['job_output'] | | .github/workflows/test.yml:8:20:8:50 | steps.step5.outputs.MSG5 | .github/workflows/test.yml:8:7:10:4 | Job outputs node [job_output] | | .github/workflows/test.yml:12:9:18:6 | Uses Step: step0 [value] | .github/workflows/test.yml:20:18:20:48 | steps.step0.outputs.value | @@ -210,6 +212,10 @@ nodes | .github/workflows/test1.yml:22:38:22:75 | github.event.pull_request.title | semmle.label | github.event.pull_request.title | | .github/workflows/test1.yml:22:38:22:75 | github.event.pull_request.title | semmle.label | github.event.pull_request.title | | .github/workflows/test1.yml:25:20:25:39 | env.ISSUE_KEY | semmle.label | env.ISSUE_KEY | +| .github/workflows/test2.yml:17:9:25:6 | Uses Step: changed | semmle.label | Uses Step: changed | +| .github/workflows/test2.yml:27:26:27:66 | steps.changed.outputs.locale_files | semmle.label | steps.changed.outputs.locale_files | +| .github/workflows/test2.yml:29:9:37:6 | Uses Step: changed2 | semmle.label | Uses Step: changed2 | +| .github/workflows/test2.yml:39:25:39:66 | steps.changed2.outputs.locale_files | semmle.label | steps.changed2.outputs.locale_files | | .github/workflows/test.yml:8:7:10:4 | Job outputs node [job_output] | semmle.label | Job outputs node [job_output] | | .github/workflows/test.yml:8:20:8:50 | steps.step5.outputs.MSG5 | semmle.label | steps.step5.outputs.MSG5 | | .github/workflows/test.yml:12:9:18:6 | Uses Step: step0 [value] | semmle.label | Uses Step: step0 [value] | diff --git a/ql/test/query-tests/Security/CWE-094/PrivilegedCodeInjection.expected b/ql/test/query-tests/Security/CWE-094/PrivilegedCodeInjection.expected index 9068ef92715..1c4ab8a61cf 100644 --- a/ql/test/query-tests/Security/CWE-094/PrivilegedCodeInjection.expected +++ b/ql/test/query-tests/Security/CWE-094/PrivilegedCodeInjection.expected @@ -55,6 +55,8 @@ edges | .github/workflows/simple2.yml:18:9:26:6 | Uses Step: step [value] | .github/workflows/simple2.yml:29:24:29:54 | steps.step.outputs.value | | .github/workflows/simple2.yml:22:20:22:64 | steps.source.outputs.all_changed_files | .github/workflows/simple2.yml:18:9:26:6 | Uses Step: step [value] | | .github/workflows/test1.yml:22:38:22:75 | github.event.pull_request.title | .github/workflows/test1.yml:25:20:25:39 | env.ISSUE_KEY | +| .github/workflows/test2.yml:17:9:25:6 | Uses Step: changed | .github/workflows/test2.yml:27:26:27:66 | steps.changed.outputs.locale_files | +| .github/workflows/test2.yml:29:9:37:6 | Uses Step: changed2 | .github/workflows/test2.yml:39:25:39:66 | steps.changed2.outputs.locale_files | | .github/workflows/test.yml:8:7:10:4 | Job outputs node [job_output] | .github/workflows/test.yml:49:20:49:56 | needs.job1.outputs['job_output'] | | .github/workflows/test.yml:8:20:8:50 | steps.step5.outputs.MSG5 | .github/workflows/test.yml:8:7:10:4 | Job outputs node [job_output] | | .github/workflows/test.yml:12:9:18:6 | Uses Step: step0 [value] | .github/workflows/test.yml:20:18:20:48 | steps.step0.outputs.value | @@ -210,6 +212,10 @@ nodes | .github/workflows/test1.yml:22:38:22:75 | github.event.pull_request.title | semmle.label | github.event.pull_request.title | | .github/workflows/test1.yml:22:38:22:75 | github.event.pull_request.title | semmle.label | github.event.pull_request.title | | .github/workflows/test1.yml:25:20:25:39 | env.ISSUE_KEY | semmle.label | env.ISSUE_KEY | +| .github/workflows/test2.yml:17:9:25:6 | Uses Step: changed | semmle.label | Uses Step: changed | +| .github/workflows/test2.yml:27:26:27:66 | steps.changed.outputs.locale_files | semmle.label | steps.changed.outputs.locale_files | +| .github/workflows/test2.yml:29:9:37:6 | Uses Step: changed2 | semmle.label | Uses Step: changed2 | +| .github/workflows/test2.yml:39:25:39:66 | steps.changed2.outputs.locale_files | semmle.label | steps.changed2.outputs.locale_files | | .github/workflows/test.yml:8:7:10:4 | Job outputs node [job_output] | semmle.label | Job outputs node [job_output] | | .github/workflows/test.yml:8:20:8:50 | steps.step5.outputs.MSG5 | semmle.label | steps.step5.outputs.MSG5 | | .github/workflows/test.yml:12:9:18:6 | Uses Step: step0 [value] | semmle.label | Uses Step: step0 [value] | @@ -319,6 +325,8 @@ subpaths | .github/workflows/simple3.yml:22:11:22:37 | toJSON(github.event) | .github/workflows/simple3.yml:22:11:22:37 | toJSON(github.event) | .github/workflows/simple3.yml:22:11:22:37 | toJSON(github.event) | Potential privileged code injection in $@, which may be controlled by an external user. | .github/workflows/simple3.yml:22:11:22:37 | toJSON(github.event) | ${{ toJSON(github.event) }} | | .github/workflows/test1.yml:22:38:22:75 | github.event.pull_request.title | .github/workflows/test1.yml:22:38:22:75 | github.event.pull_request.title | .github/workflows/test1.yml:22:38:22:75 | github.event.pull_request.title | Potential privileged code injection in $@, which may be controlled by an external user. | .github/workflows/test1.yml:22:38:22:75 | github.event.pull_request.title | ${{ github.event.pull_request.title }} | | .github/workflows/test1.yml:25:20:25:39 | env.ISSUE_KEY | .github/workflows/test1.yml:22:38:22:75 | github.event.pull_request.title | .github/workflows/test1.yml:25:20:25:39 | env.ISSUE_KEY | Potential privileged code injection in $@, which may be controlled by an external user. | .github/workflows/test1.yml:25:20:25:39 | env.ISSUE_KEY | ${{ env.ISSUE_KEY }} | +| .github/workflows/test2.yml:27:26:27:66 | steps.changed.outputs.locale_files | .github/workflows/test2.yml:17:9:25:6 | Uses Step: changed | .github/workflows/test2.yml:27:26:27:66 | steps.changed.outputs.locale_files | Potential privileged code injection in $@, which may be controlled by an external user. | .github/workflows/test2.yml:27:26:27:66 | steps.changed.outputs.locale_files | ${{ steps.changed.outputs.locale_files }} | +| .github/workflows/test2.yml:39:25:39:66 | steps.changed2.outputs.locale_files | .github/workflows/test2.yml:29:9:37:6 | Uses Step: changed2 | .github/workflows/test2.yml:39:25:39:66 | steps.changed2.outputs.locale_files | Potential privileged code injection in $@, which may be controlled by an external user. | .github/workflows/test2.yml:39:25:39:66 | steps.changed2.outputs.locale_files | ${{ steps.changed2.outputs.locale_files }} | | .github/workflows/test.yml:49:20:49:56 | needs.job1.outputs['job_output'] | .github/workflows/test.yml:15:20:15:64 | github.event['head_commit']['message'] | .github/workflows/test.yml:49:20:49:56 | needs.job1.outputs['job_output'] | Potential privileged code injection in $@, which may be controlled by an external user. | .github/workflows/test.yml:49:20:49:56 | needs.job1.outputs['job_output'] | ${{needs.job1.outputs['job_output']}} | | .github/workflows/workflow_run.yml:9:19:9:64 | github.event.workflow_run.display_title | .github/workflows/workflow_run.yml:9:19:9:64 | github.event.workflow_run.display_title | .github/workflows/workflow_run.yml:9:19:9:64 | github.event.workflow_run.display_title | Potential privileged code injection in $@, which may be controlled by an external user. | .github/workflows/workflow_run.yml:9:19:9:64 | github.event.workflow_run.display_title | ${{ github.event.workflow_run.display_title }} | | .github/workflows/workflow_run.yml:10:19:10:70 | github.event.workflow_run.head_commit.message | .github/workflows/workflow_run.yml:10:19:10:70 | github.event.workflow_run.head_commit.message | .github/workflows/workflow_run.yml:10:19:10:70 | github.event.workflow_run.head_commit.message | Potential privileged code injection in $@, which may be controlled by an external user. | .github/workflows/workflow_run.yml:10:19:10:70 | github.event.workflow_run.head_commit.message | ${{ github.event.workflow_run.head_commit.message }} | From 5d6a3c4900d427da833674a18796601c8e202a12 Mon Sep 17 00:00:00 2001 From: Jorge <46056498+jorgectf@users.noreply.github.com> Date: Tue, 7 May 2024 09:45:12 +0200 Subject: [PATCH 2/7] Copy master branch only --- .github/workflows/copy-to-bughalla.yml | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/.github/workflows/copy-to-bughalla.yml b/.github/workflows/copy-to-bughalla.yml index 572d987ce37..a6b568f2bfb 100644 --- a/.github/workflows/copy-to-bughalla.yml +++ b/.github/workflows/copy-to-bughalla.yml @@ -1,6 +1,9 @@ name: Copy to Bughalla -on: push +on: + push: + branches: + - 'master' permissions: contents: read From 778c6ad923c5164cb86ff53f0e3ac461830d5f77 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alvaro=20Mu=C3=B1oz?= Date: Tue, 7 May 2024 10:41:42 +0200 Subject: [PATCH 3/7] Fix tj-actions/changed-files sources --- .../codeql/actions/dataflow/FlowSources.qll | 38 +++++++++++- ql/lib/codeql/actions/dataflow/FlowSteps.qll | 60 +++++++++++++------ ql/lib/ext/tj-actions_changed-files.model.yml | 22 ------- .../tj-actions_verify-changed-files.model.yml | 6 -- .../.github/workflows/changed-files.yml | 31 +++++++--- .../Security/CWE-094/CodeInjection.expected | 12 ++-- .../CWE-094/PrivilegedCodeInjection.expected | 9 ++- 7 files changed, 118 insertions(+), 60 deletions(-) delete mode 100644 ql/lib/ext/tj-actions_changed-files.model.yml delete mode 100644 ql/lib/ext/tj-actions_verify-changed-files.model.yml diff --git a/ql/lib/codeql/actions/dataflow/FlowSources.qll b/ql/lib/codeql/actions/dataflow/FlowSources.qll index db111b9e190..b4cf1f70315 100644 --- a/ql/lib/codeql/actions/dataflow/FlowSources.qll +++ b/ql/lib/codeql/actions/dataflow/FlowSources.qll @@ -263,7 +263,7 @@ private class ArtifactSource extends RemoteFlowSource { /** * A list of file names returned by dorny/paths-filter. */ -private class DornyPathsFilterSource extends RemoteFlowSource { +class DornyPathsFilterSource extends RemoteFlowSource { DornyPathsFilterSource() { exists(UsesStep u | u.getCallee() = "dorny/paths-filter" and @@ -274,3 +274,39 @@ private class DornyPathsFilterSource extends RemoteFlowSource { override string getSourceType() { result = "filename" } } + +/** + * A list of file names returned by tj-actions/changed-files. + */ +class TJActionsChangedFilesSource extends RemoteFlowSource { + TJActionsChangedFilesSource() { + exists(UsesStep u | + u.getCallee() = "tj-actions/changed-files" and + ( + u.getArgument("safe_output") = "false" or + u.getVersion().regexpReplaceAll("^v", "").regexpReplaceAll("\\..*", "").toInt() < 41 + ) and + this.asExpr() = u + ) + } + + override string getSourceType() { result = "filename" } +} + +/** + * A list of file names returned by tj-actions/verify-changed-files. + */ +class TJActionsVerifyChangedFilesSource extends RemoteFlowSource { + TJActionsVerifyChangedFilesSource() { + exists(UsesStep u | + u.getCallee() = "tj-actions/verify-changed-files" and + ( + u.getArgument("safe_output") = "false" or + u.getVersion().regexpReplaceAll("^v", "").regexpReplaceAll("\\..*", "").toInt() < 17 + ) and + this.asExpr() = u + ) + } + + override string getSourceType() { result = "filename" } +} diff --git a/ql/lib/codeql/actions/dataflow/FlowSteps.qll b/ql/lib/codeql/actions/dataflow/FlowSteps.qll index 32c329d8c67..cb391f2a262 100644 --- a/ql/lib/codeql/actions/dataflow/FlowSteps.qll +++ b/ql/lib/codeql/actions/dataflow/FlowSteps.qll @@ -5,6 +5,7 @@ private import actions private import codeql.util.Unit private import codeql.actions.DataFlow +private import codeql.actions.dataflow.FlowSources private import codeql.actions.dataflow.ExternalFlow private import codeql.actions.security.ArtifactPoisoningQuery @@ -43,10 +44,6 @@ predicate envToRunStep(DataFlow::Node pred, DataFlow::Node succ) { ) } -class EnvToRunTaintStep extends AdditionalTaintStep { - override predicate step(DataFlow::Node node1, DataFlow::Node node2) { envToRunStep(node1, node2) } -} - /** * Holds if a Run step declares an environment variable, uses it in its script and sets an output in its script. * e.g. @@ -119,28 +116,57 @@ predicate artifactDownloadToUseStep(DataFlow::Node pred, DataFlow::Node succ) { ) } -class ArtifactDownloadToUseTaintStep extends AdditionalTaintStep { - override predicate step(DataFlow::Node node1, DataFlow::Node node2) { - artifactDownloadToUseStep(node1, node2) - } -} - /** * A read of the _files field of the dorny/paths-filter action. */ predicate dornyPathsFilterTaintStep(DataFlow::Node pred, DataFlow::Node succ) { - exists(UsesStep u, StepsExpression o | - u.getCallee() = "dorny/paths-filter" and - u.getArgument("list-files") = ["csv", "json"] and - u = pred.asExpr() and - o.getStepId() = u.getId() and + exists(StepsExpression o | + pred instanceof DornyPathsFilterSource and + o.getStepId() = pred.asExpr().(UsesStep).getId() and o.getFieldName().matches("%_files") and succ.asExpr() = o ) } -class DornyPathsFilterTaintStep extends AdditionalTaintStep { +/** + * A read of user-controlled field of the tj-actions/changed-files action. + */ +predicate tjActionsChangedFilesTaintStep(DataFlow::Node pred, DataFlow::Node succ) { + exists(StepsExpression o | + pred instanceof TJActionsChangedFilesSource and + o.getTarget() = pred.asExpr() and + o.getStepId() = pred.asExpr().(UsesStep).getId() and + o.getFieldName() = + [ + "added_files", "copied_files", "deleted_files", "modified_files", "renamed_files", + "all_old_new_renamed_files", "type_changed_files", "unmerged_files", "unknown_files", + "all_changed_and_modified_files", "all_changed_files", "other_changed_files", + "all_modified_files", "other_modified_files", "other_deleted_files", "modified_keys", + "changed_keys" + ] and + succ.asExpr() = o + ) +} + +/** + * A read of user-controlled field of the tj-actions/verify-changed-files action. + */ +predicate tjActionsVerifyChangedFilesTaintStep(DataFlow::Node pred, DataFlow::Node succ) { + exists(StepsExpression o | + pred instanceof TJActionsChangedFilesSource and + o.getTarget() = pred.asExpr() and + o.getStepId() = pred.asExpr().(UsesStep).getId() and + o.getFieldName() = "changed_files" and + succ.asExpr() = o + ) +} + +class TaintSteps extends AdditionalTaintStep { override predicate step(DataFlow::Node node1, DataFlow::Node node2) { - dornyPathsFilterTaintStep(node1, node2) + envToRunStep(node1, node2) or + artifactDownloadToUseStep(node1, node2) or + dornyPathsFilterTaintStep(node1, node2) or + tjActionsChangedFilesTaintStep(node1, node2) or + tjActionsVerifyChangedFilesTaintStep(node1, node2) } } diff --git a/ql/lib/ext/tj-actions_changed-files.model.yml b/ql/lib/ext/tj-actions_changed-files.model.yml deleted file mode 100644 index 60fa0149573..00000000000 --- a/ql/lib/ext/tj-actions_changed-files.model.yml +++ /dev/null @@ -1,22 +0,0 @@ -extensions: - - addsTo: - pack: githubsecuritylab/actions-all - extensible: sourceModel - data: - - ["tj-actions/changed-files", "*", "output.added_files", "filename", "manual"] - - ["tj-actions/changed-files", "*", "output.copied_files", "filename", "manual"] - - ["tj-actions/changed-files", "*", "output.deleted_files", "filename", "manual"] - - ["tj-actions/changed-files", "*", "output.modified_files", "filename", "manual"] - - ["tj-actions/changed-files", "*", "output.renamed_files", "filename", "manual"] - - ["tj-actions/changed-files", "*", "output.all_old_new_renamed_files", "filename", "manual"] - - ["tj-actions/changed-files", "*", "output.type_changed_files", "filename", "manual"] - - ["tj-actions/changed-files", "*", "output.unmerged_files", "filename", "manual"] - - ["tj-actions/changed-files", "*", "output.unknown_files", "filename", "manual"] - - ["tj-actions/changed-files", "*", "output.all_changed_and_modified_files", "filename", "manual"] - - ["tj-actions/changed-files", "*", "output.all_changed_files", "filename", "manual"] - - ["tj-actions/changed-files", "*", "output.other_changed_files", "filename", "manual"] - - ["tj-actions/changed-files", "*", "output.all_modified_files", "filename", "manual"] - - ["tj-actions/changed-files", "*", "output.other_modified_files", "filename", "manual"] - - ["tj-actions/changed-files", "*", "output.other_deleted_files", "filename", "manual"] - - ["tj-actions/changed-files", "*", "output.modified_keys", "filename", "manual"] - - ["tj-actions/changed-files", "*", "output.changed_keys", "filename", "manual"] diff --git a/ql/lib/ext/tj-actions_verify-changed-files.model.yml b/ql/lib/ext/tj-actions_verify-changed-files.model.yml deleted file mode 100644 index 9dccf6d5e6c..00000000000 --- a/ql/lib/ext/tj-actions_verify-changed-files.model.yml +++ /dev/null @@ -1,6 +0,0 @@ -extensions: - - addsTo: - pack: githubsecuritylab/actions-all - extensible: sourceModel - data: - - ["tj-actions/verify-changed-files", "*", "output.changed-files", "filename", "manual"] diff --git a/ql/test/query-tests/Security/CWE-094/.github/workflows/changed-files.yml b/ql/test/query-tests/Security/CWE-094/.github/workflows/changed-files.yml index 12bade510ba..85f59f6fa26 100644 --- a/ql/test/query-tests/Security/CWE-094/.github/workflows/changed-files.yml +++ b/ql/test/query-tests/Security/CWE-094/.github/workflows/changed-files.yml @@ -2,8 +2,6 @@ name: CI on: pull_request: - branches: - - main jobs: changed_files: @@ -13,13 +11,32 @@ jobs: - uses: actions/checkout@v4 with: fetch-depth: 0 - - name: Get changed files - id: changed-files - uses: tj-actions/changed-files@v40 - - name: List all changed files + - name: Get changed files 1 + id: changed-files1 + uses: tj-actions/changed-files@v40 + - name: List all changed files 1 run: | - for file in ${{ steps.changed-files.outputs.all_changed_files }}; do + for file in ${{ steps.changed-files1.outputs.all_changed_files }}; do echo "$file was changed" done + - name: Get changed files 2 + id: changed-files2 + uses: tj-actions/changed-files@v41 + - name: List all changed files 2 + run: | + for file in ${{ steps.changed-files2.outputs.all_changed_files }}; do + echo "$file was changed" + done + + - name: Get changed files 3 + id: changed-files3 + uses: tj-actions/changed-files@v41 + with: + safe_output: false + - name: List all changed files 3 + run: | + for file in ${{ steps.changed-files3.outputs.all_changed_files }}; do + echo "$file was changed" + done diff --git a/ql/test/query-tests/Security/CWE-094/CodeInjection.expected b/ql/test/query-tests/Security/CWE-094/CodeInjection.expected index e220d368b20..e9738fa9458 100644 --- a/ql/test/query-tests/Security/CWE-094/CodeInjection.expected +++ b/ql/test/query-tests/Security/CWE-094/CodeInjection.expected @@ -6,7 +6,8 @@ edges | .github/workflows/artifactpoisoning1.yml:20:9:24:6 | Run Step: pr [id] | .github/workflows/artifactpoisoning1.yml:34:67:34:92 | steps.pr.outputs.id | | .github/workflows/artifactpoisoning1.yml:22:14:22:55 | echo "::set-output name=id::$( Date: Tue, 7 May 2024 11:01:14 +0200 Subject: [PATCH 4/7] Update --- ql/test/library-tests/test.expected | 18 ------------------ .../PrivilegedUntrustedCheckoutHigh.expected | 2 +- 2 files changed, 1 insertion(+), 19 deletions(-) diff --git a/ql/test/library-tests/test.expected b/ql/test/library-tests/test.expected index d7f944c5a3d..c735596ae05 100644 --- a/ql/test/library-tests/test.expected +++ b/ql/test/library-tests/test.expected @@ -455,24 +455,6 @@ sources | tj-actions/branch-names | * | output.current_branch | branch | manual | | tj-actions/branch-names | * | output.head_ref_branch | branch | manual | | tj-actions/branch-names | * | output.ref_branch | branch | manual | -| tj-actions/changed-files | * | output.added_files | filename | manual | -| tj-actions/changed-files | * | output.all_changed_and_modified_files | filename | manual | -| tj-actions/changed-files | * | output.all_changed_files | filename | manual | -| tj-actions/changed-files | * | output.all_modified_files | filename | manual | -| tj-actions/changed-files | * | output.all_old_new_renamed_files | filename | manual | -| tj-actions/changed-files | * | output.changed_keys | filename | manual | -| tj-actions/changed-files | * | output.copied_files | filename | manual | -| tj-actions/changed-files | * | output.deleted_files | filename | manual | -| tj-actions/changed-files | * | output.modified_files | filename | manual | -| tj-actions/changed-files | * | output.modified_keys | filename | manual | -| tj-actions/changed-files | * | output.other_changed_files | filename | manual | -| tj-actions/changed-files | * | output.other_deleted_files | filename | manual | -| tj-actions/changed-files | * | output.other_modified_files | filename | manual | -| tj-actions/changed-files | * | output.renamed_files | filename | manual | -| tj-actions/changed-files | * | output.type_changed_files | filename | manual | -| tj-actions/changed-files | * | output.unknown_files | filename | manual | -| tj-actions/changed-files | * | output.unmerged_files | filename | manual | -| tj-actions/verify-changed-files | * | output.changed-files | filename | manual | | trilom/file-changes-action | * | output.files | filename | manual | | trilom/file-changes-action | * | output.files_added | filename | manual | | trilom/file-changes-action | * | output.files_modified | filename | manual | diff --git a/ql/test/query-tests/Security/CWE-829/PrivilegedUntrustedCheckoutHigh.expected b/ql/test/query-tests/Security/CWE-829/PrivilegedUntrustedCheckoutHigh.expected index dc5a6bc915f..628234f7e8b 100644 --- a/ql/test/query-tests/Security/CWE-829/PrivilegedUntrustedCheckoutHigh.expected +++ b/ql/test/query-tests/Security/CWE-829/PrivilegedUntrustedCheckoutHigh.expected @@ -1,4 +1,4 @@ -j .github/workflows/auto_ci.yml:20:9:27:6 | Uses Step | Potential unsafe checkout of untrusted pull request on privileged workflow. | +| .github/workflows/auto_ci.yml:20:9:27:6 | Uses Step | Potential unsafe checkout of untrusted pull request on privileged workflow. | | .github/workflows/issue_comment_3rd_party_action.yml:16:9:22:2 | Uses Step | Potential unsafe checkout of untrusted pull request on privileged workflow. | | .github/workflows/issue_comment_3rd_party_action.yml:30:9:36:2 | Uses Step | Potential unsafe checkout of untrusted pull request on privileged workflow. | | .github/workflows/issue_comment_3rd_party_action.yml:45:9:49:6 | Uses Step | Potential unsafe checkout of untrusted pull request on privileged workflow. | From 6a87192f6491a36d107c49e871228cf6488721a1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alvaro=20Mu=C3=B1oz?= Date: Wed, 8 May 2024 09:42:56 +0200 Subject: [PATCH 5/7] Account for insecure action versions --- ql/lib/codeql/actions/ast/internal/Ast.qll | 12 +++-- .../codeql/actions/dataflow/FlowSources.qll | 51 ++++++++++++++++++- .../.github/workflows/changed-files.yml | 18 +++++++ .../Security/CWE-094/CodeInjection.expected | 4 ++ .../CWE-094/PrivilegedCodeInjection.expected | 3 ++ .../CWE-829/UnpinnedActionsTag.expected | 32 ++++++------ 6 files changed, 98 insertions(+), 22 deletions(-) diff --git a/ql/lib/codeql/actions/ast/internal/Ast.qll b/ql/lib/codeql/actions/ast/internal/Ast.qll index 0cbb8ab10ed..83787882d6f 100644 --- a/ql/lib/codeql/actions/ast/internal/Ast.qll +++ b/ql/lib/codeql/actions/ast/internal/Ast.qll @@ -791,6 +791,8 @@ abstract class UsesImpl extends AstNodeImpl { abstract string getVersion(); + int getMajorVersion() { result = this.getVersion().regexpReplaceAll("\\..*", "").toInt() } + /** Gets the argument expression for the given key. */ string getArgument(string key) { exists(ScalarValueImpl scalar | @@ -832,8 +834,10 @@ class UsesStepImpl extends StepImpl, UsesImpl { ).toLowerCase() } - /** Gets the version reference used when checking out the Action, e.g. `v2` in `actions/checkout@v2`. */ - override string getVersion() { result = u.getValue().regexpCapture(usesParser(), 3) } + /** Gets the version reference used when checking out the Action, e.g. `2` in `actions/checkout@v2`. */ + override string getVersion() { + result = u.getValue().regexpCapture(usesParser(), 3).regexpReplaceAll("^v", "") + } override string toString() { if exists(this.getId()) then result = "Uses Step: " + this.getId() else result = "Uses Step" @@ -865,12 +869,12 @@ class ExternalJobImpl extends JobImpl, UsesImpl { u.getValue().regexpCapture(repoUsesParser(), 3) } - /** Gets the version reference used when checking out the Action, e.g. `v2` in `actions/checkout@v2`. */ + /** Gets the version reference used when checking out the Action, e.g. `2` in `actions/checkout@v2`. */ override string getVersion() { exists(YamlString name | n.lookup("uses") = name and if not name.getValue().matches("\\.%") - then result = name.getValue().regexpCapture(repoUsesParser(), 4) + then result = name.getValue().regexpCapture(repoUsesParser(), 4).regexpReplaceAll("^v", "") else none() ) } diff --git a/ql/lib/codeql/actions/dataflow/FlowSources.qll b/ql/lib/codeql/actions/dataflow/FlowSources.qll index b4cf1f70315..bfe85dbdbe6 100644 --- a/ql/lib/codeql/actions/dataflow/FlowSources.qll +++ b/ql/lib/codeql/actions/dataflow/FlowSources.qll @@ -284,7 +284,54 @@ class TJActionsChangedFilesSource extends RemoteFlowSource { u.getCallee() = "tj-actions/changed-files" and ( u.getArgument("safe_output") = "false" or - u.getVersion().regexpReplaceAll("^v", "").regexpReplaceAll("\\..*", "").toInt() < 41 + u.getMajorVersion() < 41 or + u.getVersion() + .matches([ + "56284d8", "9454999", "1c93849", "da093c1", "25ef392", "18c8a4e", "4052680", + "bfc49f4", "af292f1", "56284d8", "fea790c", "95690f9", "408093d", "db153ba", + "8238a41", "4196030", "a21a533", "8e79ba7", "76c4d81", "6ee9cdc", "246636f", + "48566bb", "fea790c", "1aee362", "2f7246c", "0fc9663", "c860b5c", "2f8b802", + "b7f1b73", "1c26215", "17f3fec", "1aee362", "a0585ff", "87697c0", "85c8b82", + "a96679d", "920e7b9", "de0eba3", "3928317", "68b429d", "2a968ff", "1f20fb8", + "87e23c4", "54849de", "bb33761", "ec1e14c", "2106eb4", "e5efec4", "5817a9e", + "a0585ff", "54479c3", "e1754a4", "9bf0914", "c912451", "174a2a6", "fb20f4d", + "07e0177", "b137868", "1aae160", "5d2fcdb", "9ecc6e7", "8c9ee56", "5978e5a", + "17c3e9e", "3f7b5c9", "cf4fe87", "043929e", "4e2535f", "652648a", "9ad1a5b", + "c798a4e", "25eaddf", "abef388", "1c2673b", "53c377a", "54479c3", "039afcd", + "b2d17f5", "4a0aac0", "ce810b2", "7ecfc67", "b109d83", "79adacd", "6e426e6", + "5e2d64b", "e9b5807", "db5dd7c", "07f86bc", "3a3ec49", "ee13744", "cda2902", + "9328bab", "4e680e1", "bd376fb", "84ed30e", "74b06ca", "5ce975c", "04124ef", + "3ee6abf", "23e3c43", "5a331a4", "7433886", "d5414fd", "7f2aa19", "210cc83", + "db3ea27", "57d9664", "0953088", "0562b9f", "487675b", "9a6dabf", "7839ede", + "c2296c1", "ea251d4", "1d1287f", "392359f", "7f33882", "1d8a2f9", "0626c3f", + "a2b1e5d", "110b9ba", "039afcd", "ce4b8e3", "3b6c057", "4f64429", "3f1e44a", + "74dc2e8", "8356a01", "baaf598", "8a4cc4f", "8a7336f", "3996bc3", "ef0a290", + "3ebdc42", "94e6fba", "3dbb79f", "991e8b3", "72d3bb8", "72d3bb8", "5f89dc7", + "734bb16", "d2e030b", "6ba3c59", "d0e4477", "b91acef", "1263363", "7184077", + "cbfb0fd", "932dad3", "9f28968", "c4d29bf", "ce4b8e3", "aa52cfc", "aa52cfc", + "1d6e210", "8953e85", "8de562e", "7c640bd", "2706452", "1d6e210", "dd7c814", + "528984a", "75af1a4", "5184a75", "dd7c814", "402f382", "402f382", "f7a5640", + "df4daca", "602081b", "6e12407", "c5c9b6f", "c41b715", "60f4aab", "82edb42", + "18edda7", "bec82eb", "f7a5640", "28ac672", "602cf94", "5e56dca", "58ae566", + "7394701", "36e65a1", "bf6ddb7", "6c44eb8", "b2ee165", "34a865a", "fb1fe28", + "ae90a0b", "bc1dc8f", "3de1f9a", "0edfedf", "2054502", "944a8b8", "581eef0", + "e55f7fb", "07b38ce", "d262520", "a6d456f", "a59f800", "a2f1692", "72aab29", + "e35d0af", "081ee9c", "1f30bd2", "227e314", "ffd30e8", "f5a8de7", "0bc7d40", + "a53d74f", "9335416", "4daffba", "4b1f26a", "09441d3", "e44053b", "c0dba81", + "fd2e991", "2a8a501", "a8ea720", "88edda5", "be68c10", "b59431b", "68bd279", + "2c85495", "f276697", "00f80ef", "f56e736", "019a09d", "3b638a9", "b42f932", + "8dfe0ee", "aae164d", "09a8797", "b54a7ae", "902e607", "2b51570", "040111b", + "3b638a9", "1d34e69", "b86b537", "2a771ad", "75933dc", "2c0d12b", "7abdbc9", + "675ab58", "8c6f276", "d825b1f", "0bd70b7", "0fe67a1", "7bfa539", "d679de9", + "1e10ed4", "0754fda", "d290bdd", "15b1769", "2ecd06d", "5fe8e4d", "7c66aa2", + "2ecd06d", "e95bba8", "7852058", "81f32e2", "450eadf", "0e956bb", "300e935", + "fcb2ab8", "271bbd6", "e8ace01", "473984b", "032f37f", "3a35bdf", "c2216f6", + "0f16c26", "271468e", "fb063fc", "a05436f", "c061ef1", "489e2d5", "8d5a33c", + "fbfaba5", "1980f55", "a86b560", "f917cc3", "e18ccae", "e1d275d", "00f80ef", + "9c1a181", "5eaa2d8", "188487d", "3098891", "467d26c", "d9eb683", "09a8797", + "8e7cc77", "81ad4b8", "5e2a2f1", "1af9ab3", "55a857d", "62a9200", "b915d09", + "f0751de", "eef9423" + ] + "%") ) and this.asExpr() = u ) @@ -302,7 +349,7 @@ class TJActionsVerifyChangedFilesSource extends RemoteFlowSource { u.getCallee() = "tj-actions/verify-changed-files" and ( u.getArgument("safe_output") = "false" or - u.getVersion().regexpReplaceAll("^v", "").regexpReplaceAll("\\..*", "").toInt() < 17 + u.getMajorVersion() < 17 ) and this.asExpr() = u ) diff --git a/ql/test/query-tests/Security/CWE-094/.github/workflows/changed-files.yml b/ql/test/query-tests/Security/CWE-094/.github/workflows/changed-files.yml index 85f59f6fa26..6d506e65a13 100644 --- a/ql/test/query-tests/Security/CWE-094/.github/workflows/changed-files.yml +++ b/ql/test/query-tests/Security/CWE-094/.github/workflows/changed-files.yml @@ -40,3 +40,21 @@ jobs: for file in ${{ steps.changed-files3.outputs.all_changed_files }}; do echo "$file was changed" done + + - name: Get changed files 4 + id: changed-files4 + uses: tj-actions/changed-files@0874344d6ebbaa00a27da73276ae7162fadcaf69 # v44.3.0 + - name: List all changed files 4 + run: | + for file in ${{ steps.changed-files4.outputs.all_changed_files }}; do + echo "$file was changed" + done + + - name: Get changed files 5 + id: changed-files5 + uses: tj-actions/changed-files@95690f9ece77c1740f4a55b7f1de9023ed6b1f87 # v39.2.3 + - name: List all changed files 5 + run: | + for file in ${{ steps.changed-files5.outputs.all_changed_files }}; do + echo "$file was changed" + done diff --git a/ql/test/query-tests/Security/CWE-094/CodeInjection.expected b/ql/test/query-tests/Security/CWE-094/CodeInjection.expected index e9738fa9458..9e479f9eaf4 100644 --- a/ql/test/query-tests/Security/CWE-094/CodeInjection.expected +++ b/ql/test/query-tests/Security/CWE-094/CodeInjection.expected @@ -8,6 +8,7 @@ edges | .github/workflows/artifactpoisoning2.yml:13:9:19:6 | Uses Step: pr | .github/workflows/artifactpoisoning2.yml:22:17:22:42 | steps.pr.outputs.id | | .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 | | .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 | +| .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 | | .github/workflows/changelog.yml:49:19:49:56 | github.event.pull_request.title | .github/workflows/changelog.yml:58:26:58:39 | env.log | | .github/workflows/changelog_from_prt.yml:49:19:49:56 | github.event.pull_request.title | .github/workflows/changelog_from_prt.yml:58:26:58:39 | env.log | | .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 | @@ -87,6 +88,8 @@ nodes | .github/workflows/changed-files.yml:20:24:20:76 | steps.changed-files1.outputs.all_changed_files | semmle.label | steps.changed-files1.outputs.all_changed_files | | .github/workflows/changed-files.yml:33:9:38:6 | Uses Step: changed-files3 | semmle.label | Uses Step: changed-files3 | | .github/workflows/changed-files.yml:40:24:40:76 | steps.changed-files3.outputs.all_changed_files | semmle.label | steps.changed-files3.outputs.all_changed_files | +| .github/workflows/changed-files.yml:53:9:56:6 | Uses Step: changed-files5 | semmle.label | Uses Step: changed-files5 | +| .github/workflows/changed-files.yml:58:24:58:76 | steps.changed-files5.outputs.all_changed_files | semmle.label | steps.changed-files5.outputs.all_changed_files | | .github/workflows/changelog.yml:49:19:49:56 | github.event.pull_request.title | semmle.label | github.event.pull_request.title | | .github/workflows/changelog.yml:58:26:58:39 | env.log | semmle.label | env.log | | .github/workflows/changelog_from_prt.yml:49:19:49:56 | github.event.pull_request.title | semmle.label | github.event.pull_request.title | @@ -247,6 +250,7 @@ subpaths #select | .github/workflows/changed-files.yml:20:24:20:76 | steps.changed-files1.outputs.all_changed_files | .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 | Potential code injection in $@, which may be controlled by an external user. | .github/workflows/changed-files.yml:20:24:20:76 | steps.changed-files1.outputs.all_changed_files | ${{ steps.changed-files1.outputs.all_changed_files }} | | .github/workflows/changed-files.yml:40:24:40:76 | steps.changed-files3.outputs.all_changed_files | .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 | Potential code injection in $@, which may be controlled by an external user. | .github/workflows/changed-files.yml:40:24:40:76 | steps.changed-files3.outputs.all_changed_files | ${{ steps.changed-files3.outputs.all_changed_files }} | +| .github/workflows/changed-files.yml:58:24:58:76 | steps.changed-files5.outputs.all_changed_files | .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 | Potential code injection in $@, which may be controlled by an external user. | .github/workflows/changed-files.yml:58:24:58:76 | steps.changed-files5.outputs.all_changed_files | ${{ steps.changed-files5.outputs.all_changed_files }} | | .github/workflows/changelog.yml:58:26:58:39 | env.log | .github/workflows/changelog.yml:49:19:49:56 | github.event.pull_request.title | .github/workflows/changelog.yml:58:26:58:39 | env.log | Potential code injection in $@, which may be controlled by an external user. | .github/workflows/changelog.yml:58:26:58:39 | env.log | ${{ env.log }} | | .github/workflows/changelog_from_prt.yml:58:26:58:39 | env.log | .github/workflows/changelog_from_prt.yml:49:19:49:56 | github.event.pull_request.title | .github/workflows/changelog_from_prt.yml:58:26:58:39 | env.log | Potential code injection in $@, which may be controlled by an external user. | .github/workflows/changelog_from_prt.yml:58:26:58:39 | env.log | ${{ env.log }} | | action1/action.yml:14:19:14:50 | github.event.comment.body | action1/action.yml:14:19:14:50 | github.event.comment.body | action1/action.yml:14:19:14:50 | github.event.comment.body | Potential code injection in $@, which may be controlled by an external user. | action1/action.yml:14:19:14:50 | github.event.comment.body | ${{ github.event.comment.body }} | diff --git a/ql/test/query-tests/Security/CWE-094/PrivilegedCodeInjection.expected b/ql/test/query-tests/Security/CWE-094/PrivilegedCodeInjection.expected index 048b0446f5f..738270e3ccd 100644 --- a/ql/test/query-tests/Security/CWE-094/PrivilegedCodeInjection.expected +++ b/ql/test/query-tests/Security/CWE-094/PrivilegedCodeInjection.expected @@ -8,6 +8,7 @@ edges | .github/workflows/artifactpoisoning2.yml:13:9:19:6 | Uses Step: pr | .github/workflows/artifactpoisoning2.yml:22:17:22:42 | steps.pr.outputs.id | | .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 | | .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 | +| .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 | | .github/workflows/changelog.yml:49:19:49:56 | github.event.pull_request.title | .github/workflows/changelog.yml:58:26:58:39 | env.log | | .github/workflows/changelog_from_prt.yml:49:19:49:56 | github.event.pull_request.title | .github/workflows/changelog_from_prt.yml:58:26:58:39 | env.log | | .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 | @@ -87,6 +88,8 @@ nodes | .github/workflows/changed-files.yml:20:24:20:76 | steps.changed-files1.outputs.all_changed_files | semmle.label | steps.changed-files1.outputs.all_changed_files | | .github/workflows/changed-files.yml:33:9:38:6 | Uses Step: changed-files3 | semmle.label | Uses Step: changed-files3 | | .github/workflows/changed-files.yml:40:24:40:76 | steps.changed-files3.outputs.all_changed_files | semmle.label | steps.changed-files3.outputs.all_changed_files | +| .github/workflows/changed-files.yml:53:9:56:6 | Uses Step: changed-files5 | semmle.label | Uses Step: changed-files5 | +| .github/workflows/changed-files.yml:58:24:58:76 | steps.changed-files5.outputs.all_changed_files | semmle.label | steps.changed-files5.outputs.all_changed_files | | .github/workflows/changelog.yml:49:19:49:56 | github.event.pull_request.title | semmle.label | github.event.pull_request.title | | .github/workflows/changelog.yml:58:26:58:39 | env.log | semmle.label | env.log | | .github/workflows/changelog_from_prt.yml:49:19:49:56 | github.event.pull_request.title | semmle.label | github.event.pull_request.title | diff --git a/ql/test/query-tests/Security/CWE-829/UnpinnedActionsTag.expected b/ql/test/query-tests/Security/CWE-829/UnpinnedActionsTag.expected index 0ba7832e8e8..dbbfba0a557 100644 --- a/ql/test/query-tests/Security/CWE-829/UnpinnedActionsTag.expected +++ b/ql/test/query-tests/Security/CWE-829/UnpinnedActionsTag.expected @@ -1,17 +1,17 @@ -| .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/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 | -| .github/workflows/issue_comment_3rd_party_action.yml:12:9:16:6 | Uses Step: comment-branch | Unpinned 3rd party Action 'PR head from 3rd party action' step $@ uses 'xt0rted/pull-request-comment-branch' with ref 'v2', not a pinned commit hash | .github/workflows/issue_comment_3rd_party_action.yml:12:9:16:6 | Uses Step: comment-branch | Uses Step: comment-branch | -| .github/workflows/issue_comment_3rd_party_action.yml:25:9:30:6 | Uses Step: comment-branch | Unpinned 3rd party Action 'PR head from 3rd party action' step $@ uses 'xt0rted/pull-request-comment-branch' with ref 'v2', not a pinned commit hash | .github/workflows/issue_comment_3rd_party_action.yml:25:9:30:6 | Uses Step: comment-branch | Uses Step: comment-branch | +| .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 '2', 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 '1', not a pinned commit hash | .github/workflows/actor_trusted_checkout.yml:23:7:26:21 | 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 '2', 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 '2', 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 '3', 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 '5', 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 '2', not a pinned commit hash | .github/workflows/auto_ci.yml:125:9:133:6 | Uses Step | Uses Step | +| .github/workflows/issue_comment_3rd_party_action.yml:12:9:16:6 | Uses Step: comment-branch | Unpinned 3rd party Action 'PR head from 3rd party action' step $@ uses 'xt0rted/pull-request-comment-branch' with ref '2', not a pinned commit hash | .github/workflows/issue_comment_3rd_party_action.yml:12:9:16:6 | Uses Step: comment-branch | Uses Step: comment-branch | +| .github/workflows/issue_comment_3rd_party_action.yml:25:9:30:6 | Uses Step: comment-branch | Unpinned 3rd party Action 'PR head from 3rd party action' step $@ uses 'xt0rted/pull-request-comment-branch' with ref '2', not a pinned commit hash | .github/workflows/issue_comment_3rd_party_action.yml:25:9:30:6 | Uses Step: comment-branch | Uses Step: comment-branch | | .github/workflows/issue_comment_3rd_party_action.yml:39:9:45:6 | Uses Step: refs | Unpinned 3rd party Action 'PR head from 3rd party action' step $@ uses 'eficode/resolve-pr-refs' with ref 'main', not a pinned commit hash | .github/workflows/issue_comment_3rd_party_action.yml:39:9:45:6 | Uses Step: refs | Uses Step: refs | -| .github/workflows/issue_comment_octokit.yml:12:9:19:6 | Uses Step: fetch_issue | Unpinned 3rd party Action 'Octokit (heuristics)' step $@ uses 'octokit/request-action' with ref 'v2.x', not a pinned commit hash | .github/workflows/issue_comment_octokit.yml:12:9:19:6 | Uses Step: fetch_issue | Uses Step: fetch_issue | -| .github/workflows/issue_comment_octokit.yml:19:9:26:6 | Uses Step: fetch_pr | Unpinned 3rd party Action 'Octokit (heuristics)' step $@ uses 'octokit/request-action' with ref 'v2.x', not a pinned commit hash | .github/workflows/issue_comment_octokit.yml:19:9:26:6 | Uses Step: fetch_pr | Uses Step: fetch_pr | -| .github/workflows/issue_comment_octokit.yml:103:9:109:6 | Uses Step: request | Unpinned 3rd party Action 'Octokit (heuristics)' step $@ uses 'octokit/request-action' with ref 'v2.0.2', not a pinned commit hash | .github/workflows/issue_comment_octokit.yml:103:9:109:6 | Uses Step: request | Uses Step: request | -| .github/workflows/label_trusted_checkout.yml:20:7:24:4 | Uses Step | Unpinned 3rd party Action 'label_trusted_checkout.yml' step $@ uses 'completely/fakeaction' with ref 'v2', not a pinned commit hash | .github/workflows/label_trusted_checkout.yml:20:7:24:4 | Uses Step | Uses Step | -| .github/workflows/label_trusted_checkout.yml:24:7:27:21 | Uses Step | Unpinned 3rd party Action 'label_trusted_checkout.yml' step $@ uses 'fakerepo/comment-on-pr' with ref 'v1', not a pinned commit hash | .github/workflows/label_trusted_checkout.yml:24:7:27:21 | Uses Step | Uses Step | -| .github/workflows/level0.yml:36:9:39:6 | Uses Step | Unpinned 3rd party Action 'Poutine Level 0' step $@ uses 'rlespinasse/github-slug-action' with ref 'v4', not a pinned commit hash | .github/workflows/level0.yml:36:9:39:6 | Uses Step | Uses Step | -| .github/workflows/unpinned_tags.yml:10:7:11:4 | Uses Step | Unpinned 3rd party Action 'unpinned_tags.yml' step $@ uses 'foo/bar' with ref 'v1', not a pinned commit hash | .github/workflows/unpinned_tags.yml:10:7:11:4 | Uses Step | Uses Step | +| .github/workflows/issue_comment_octokit.yml:12:9:19:6 | Uses Step: fetch_issue | Unpinned 3rd party Action 'Octokit (heuristics)' step $@ uses 'octokit/request-action' with ref '2.x', not a pinned commit hash | .github/workflows/issue_comment_octokit.yml:12:9:19:6 | Uses Step: fetch_issue | Uses Step: fetch_issue | +| .github/workflows/issue_comment_octokit.yml:19:9:26:6 | Uses Step: fetch_pr | Unpinned 3rd party Action 'Octokit (heuristics)' step $@ uses 'octokit/request-action' with ref '2.x', not a pinned commit hash | .github/workflows/issue_comment_octokit.yml:19:9:26:6 | Uses Step: fetch_pr | Uses Step: fetch_pr | +| .github/workflows/issue_comment_octokit.yml:103:9:109:6 | Uses Step: request | Unpinned 3rd party Action 'Octokit (heuristics)' step $@ uses 'octokit/request-action' with ref '2.0.2', not a pinned commit hash | .github/workflows/issue_comment_octokit.yml:103:9:109:6 | Uses Step: request | Uses Step: request | +| .github/workflows/label_trusted_checkout.yml:20:7:24:4 | Uses Step | Unpinned 3rd party Action 'label_trusted_checkout.yml' step $@ uses 'completely/fakeaction' with ref '2', not a pinned commit hash | .github/workflows/label_trusted_checkout.yml:20:7:24:4 | Uses Step | Uses Step | +| .github/workflows/label_trusted_checkout.yml:24:7:27:21 | Uses Step | Unpinned 3rd party Action 'label_trusted_checkout.yml' step $@ uses 'fakerepo/comment-on-pr' with ref '1', not a pinned commit hash | .github/workflows/label_trusted_checkout.yml:24:7:27:21 | Uses Step | Uses Step | +| .github/workflows/level0.yml:36:9:39:6 | Uses Step | Unpinned 3rd party Action 'Poutine Level 0' step $@ uses 'rlespinasse/github-slug-action' with ref '4', not a pinned commit hash | .github/workflows/level0.yml:36:9:39:6 | Uses Step | Uses Step | +| .github/workflows/unpinned_tags.yml:10:7:11:4 | Uses Step | Unpinned 3rd party Action 'unpinned_tags.yml' step $@ uses 'foo/bar' with ref '1', not a pinned commit hash | .github/workflows/unpinned_tags.yml:10:7:11:4 | Uses Step | Uses Step | From d3bb6668f6fca4eb93a8f909bb1f49f5e7f5550d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alvaro=20Mu=C3=B1oz?= Date: Wed, 8 May 2024 09:44:48 +0200 Subject: [PATCH 6/7] Missing getMajorVersion predicate --- ql/lib/codeql/actions/Ast.qll | 2 ++ 1 file changed, 2 insertions(+) diff --git a/ql/lib/codeql/actions/Ast.qll b/ql/lib/codeql/actions/Ast.qll index bfbc990d671..6d80c67f7fd 100644 --- a/ql/lib/codeql/actions/Ast.qll +++ b/ql/lib/codeql/actions/Ast.qll @@ -372,6 +372,8 @@ abstract class Uses extends AstNode instanceof UsesImpl { string getVersion() { result = super.getVersion() } + int getMajorVersion() { result = super.getMajorVersion() } + string getArgument(string argName) { result = super.getArgument(argName) } Expression getArgumentExpr(string argName) { result = super.getArgumentExpr(argName) } From c39e802c1729d0a9a41e00246686d9395d3bec07 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alvaro=20Mu=C3=B1oz?= Date: Wed, 8 May 2024 13:56:49 +0200 Subject: [PATCH 7/7] Fix sources for tj-actions/verify-changed-files --- ql/lib/codeql/actions/dataflow/FlowSources.qll | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/ql/lib/codeql/actions/dataflow/FlowSources.qll b/ql/lib/codeql/actions/dataflow/FlowSources.qll index bfe85dbdbe6..9e4c258e39a 100644 --- a/ql/lib/codeql/actions/dataflow/FlowSources.qll +++ b/ql/lib/codeql/actions/dataflow/FlowSources.qll @@ -349,7 +349,20 @@ class TJActionsVerifyChangedFilesSource extends RemoteFlowSource { u.getCallee() = "tj-actions/verify-changed-files" and ( u.getArgument("safe_output") = "false" or - u.getMajorVersion() < 17 + u.getMajorVersion() < 17 or + u.getVersion() + .matches([ + "54e20d3", "a9b6fd3", "30aa174", "7f1b21c", "54e20d3", "0409e18", "7da22d0", + "7016858", "0409e18", "7517b83", "bad2f5d", "3b573ac", "7517b83", "f557547", + "9ed3155", "f557547", "a3391b5", "a3391b5", "1d7ee97", "c432297", "6e986df", + "fa6ea30", "6f40ee1", "1b13d25", "c09bcad", "fda469d", "bd1e271", "367ba21", + "9dea97e", "c154cc6", "527ff75", "e8756d5", "bcb4e76", "25267f5", "ea24bfd", + "f2a40ba", "197e121", "a8f1b11", "95c26dd", "97ba4cc", "68310bb", "720ba6a", + "cedd709", "d68d3d2", "2e1153b", "c3dd635", "81bd1de", "31a9c74", "e981d37", + "e7f801c", "e86d0b9", "ad255a4", "3a8aed1", "de910b5", "d31b2a1", "e61c6fc", + "380890d", "873cfd6", "b0c60c8", "7183183", "6555389", "9828a95", "8150cee", + "48ddf88" + ] + "%") ) and this.asExpr() = u )