From 113ab755d95468766e3539c0b6da7e7ecd57780a Mon Sep 17 00:00:00 2001 From: Owen Mansel-Chan <62447351+owen-mc@users.noreply.github.com> Date: Wed, 21 Aug 2024 14:18:11 +0100 Subject: [PATCH 1/7] Give clearer example of multiple query predicates in one ql file The new names aren't great, so feel free to change them, but I think we do need an explicit example of updating two relations using one ql file. --- docs/prepare-db-upgrade.md | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/docs/prepare-db-upgrade.md b/docs/prepare-db-upgrade.md index 7b0a1c6fe48..58806ecebeb 100644 --- a/docs/prepare-db-upgrade.md +++ b/docs/prepare-db-upgrade.md @@ -59,14 +59,16 @@ extended.rel: reorder input.rel (int id, string name, int parent) id name parent // QLL library, and will run in the context of the *old* dbscheme. relationname.rel: run relationname.qlo -// Create relationname.rel by running the query predicate 'predicatename' in -// relationname.qlo and writing the query results as a .rel file. This command +// Create relation1.rel by running the query predicate 'predicate1' in upgrade.qlo +// and writing the query results as a .rel file, and running 'predicate2' in +// upgrade.qlo and writing the query results as a .rel file. This command // expects the upgrade relation to be a query predicate, which has the advantage // of allowing multiple upgrade relations to appear in the same .ql file as -// multiple query predicates. The query file should be named relationname.ql and +// multiple query predicates. The query file should be named upgrade.ql and // should be placed in the upgrade directory. It should avoid using the default // QLL library, and will run in the context of the *old* dbscheme. -relationname.rel: run relationname.qlo predicatename +relation1.rel: run upgrade.qlo predicate1 +relation2.rel: run upgrade.qlo predicate2 ``` ### Testing your scripts From d9e8792d33bd2093658953b4fcc3c8f08a767462 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alvaro=20Mu=C3=B1oz?= Date: Fri, 6 Sep 2024 22:55:58 +0200 Subject: [PATCH 2/7] [javascript] Query to detect GITHUB_TOKEN leaked in artifacts --- .../CWE-312/ActionsArtifactLeak.qhelp | 30 +++++ .../Security/CWE-312/ActionsArtifactLeak.ql | 112 ++++++++++++++++++ .../examples/actions-artifact-leak-fixed.yml | 14 +++ .../examples/actions-artifact-leak.yml | 13 ++ .../CWE-312/.github/workflows/test.yml | 65 ++++++++++ .../CWE-312/ActionsArtifactLeak.expected | 4 + .../CWE-312/ActionsArtifactLeak.qlref | 1 + 7 files changed, 239 insertions(+) create mode 100644 javascript/ql/src/Security/CWE-312/ActionsArtifactLeak.qhelp create mode 100644 javascript/ql/src/Security/CWE-312/ActionsArtifactLeak.ql create mode 100644 javascript/ql/src/Security/CWE-312/examples/actions-artifact-leak-fixed.yml create mode 100644 javascript/ql/src/Security/CWE-312/examples/actions-artifact-leak.yml create mode 100644 javascript/ql/test/query-tests/Security/CWE-312/.github/workflows/test.yml create mode 100644 javascript/ql/test/query-tests/Security/CWE-312/ActionsArtifactLeak.expected create mode 100644 javascript/ql/test/query-tests/Security/CWE-312/ActionsArtifactLeak.qlref diff --git a/javascript/ql/src/Security/CWE-312/ActionsArtifactLeak.qhelp b/javascript/ql/src/Security/CWE-312/ActionsArtifactLeak.qhelp new file mode 100644 index 00000000000..7ec9c1fe777 --- /dev/null +++ b/javascript/ql/src/Security/CWE-312/ActionsArtifactLeak.qhelp @@ -0,0 +1,30 @@ + + + +

+ Sensitive information included in a GitHub Actions artifact can allow an attacker to access + the sensitive information if the artifact is published. +

+ + + +

+ Only store information that is meant to be publicly available in a GitHub Actions artifact. +

+ + + +

+ The following example uses actions/checkout to checkout code which stores the GITHUB_TOKEN in the `.git/config` file + and then stores the contents of the `.git` repository into the artifact: +

+ +

+ The issue has been fixed below, where the actions/upload-artifact uses a version (v4+) which does not include hidden files or + directories into the artifact. +

+ + + diff --git a/javascript/ql/src/Security/CWE-312/ActionsArtifactLeak.ql b/javascript/ql/src/Security/CWE-312/ActionsArtifactLeak.ql new file mode 100644 index 00000000000..96ac361968d --- /dev/null +++ b/javascript/ql/src/Security/CWE-312/ActionsArtifactLeak.ql @@ -0,0 +1,112 @@ +/** + * @name Storage of sensitive information in GitHub Actions artifact + * @description Including sensitive information in a GitHub Actions artifact can + * expose it to an attacker. + * @kind problem + * @problem.severity error + * @security-severity 7.5 + * @precision high + * @id js/actions/actions-artifact-leak + * @tags security + * external/cwe/cwe-312 + * external/cwe/cwe-315 + * external/cwe/cwe-359 + */ + +import javascript +import semmle.javascript.Actions + +/** + * A step that uses `actions/checkout` action. + */ +class ActionsCheckoutStep extends Actions::Step { + ActionsCheckoutStep() { this.getUses().getGitHubRepository() = "actions/checkout" } +} + +/** + * A `with:`/`persist-credentials` field sibling to `uses: actions/checkout`. + */ +class ActionsCheckoutWithPersistCredentials extends YamlNode, YamlString { + ActionsCheckoutStep step; + + ActionsCheckoutWithPersistCredentials() { + step.lookup("with").(YamlMapping).lookup("persist-credentials") = this + } + + /** Gets the step this field belongs to. */ + ActionsCheckoutStep getStep() { result = step } +} + +/** + * A `with:`/`path` field sibling to `uses: actions/checkout`. + */ +class ActionsCheckoutWithPath extends YamlNode, YamlString { + ActionsCheckoutStep step; + + ActionsCheckoutWithPath() { step.lookup("with").(YamlMapping).lookup("path") = this } + + /** Gets the step this field belongs to. */ + ActionsCheckoutStep getStep() { result = step } +} + +/** + * A step that uses `actions/upload-artifact` action. + */ +class ActionsUploadArtifactStep extends Actions::Step { + ActionsUploadArtifactStep() { this.getUses().getGitHubRepository() = "actions/upload-artifact" } +} + +/** + * A `with:`/`path` field sibling to `uses: actions/upload-artifact`. + */ +class ActionsUploadArtifactWithPath extends YamlNode, YamlString { + ActionsUploadArtifactStep step; + + ActionsUploadArtifactWithPath() { step.lookup("with").(YamlMapping).lookup("path") = this } + + /** Gets the step this field belongs to. */ + ActionsUploadArtifactStep getStep() { result = step } +} + +from ActionsCheckoutStep checkout, ActionsUploadArtifactStep upload, Actions::Job job, int i, int j +where + checkout.getJob() = job and + upload.getJob() = job and + job.getStep(i) = checkout and + job.getStep(j) = upload and + j = i + 1 and + upload.getUses().getVersion() = + [ + "v4.3.6", "834a144ee995460fba8ed112a2fc961b36a5ec5a", // + "v4.3.5", "89ef406dd8d7e03cfd12d9e0a4a378f454709029", // + "v4.3.4", "0b2256b8c012f0828dc542b3febcab082c67f72b", // + "v4.3.3", "65462800fd760344b1a7b4382951275a0abb4808", // + "v4.3.2", "1746f4ab65b179e0ea60a494b83293b640dd5bba", // + "v4.3.1", "5d5d22a31266ced268874388b861e4b58bb5c2f3", // + "v4.3.0", "26f96dfa697d77e81fd5907df203aa23a56210a8", // + "v4.2.0", "694cdabd8bdb0f10b2cea11669e1bf5453eed0a6", // + "v4.1.0", "1eb3cb2b3e0f29609092a73eb033bb759a334595", // + "v4.0.0", "c7d193f32edcb7bfad88892161225aeda64e9392", // + ] and + ( + not exists(ActionsCheckoutWithPersistCredentials persist | persist.getStep() = checkout) + or + exists(ActionsCheckoutWithPersistCredentials persist | + persist.getStep() = checkout and + persist.getValue() = "true" + ) + ) and + ( + not exists(ActionsCheckoutWithPath path | path.getStep() = checkout) and + exists(ActionsUploadArtifactWithPath path | + path.getStep() = upload and path.getValue() = [".", "*"] + ) + or + exists(ActionsCheckoutWithPath checkout_path, ActionsUploadArtifactWithPath upload_path | + checkout_path.getValue() + ["", "/*"] = upload_path.getValue() and + checkout_path.getStep() = checkout and + upload_path.getStep() = upload + ) + ) +select upload, "A secret is exposed in an artifact uploaded by $@", upload, + "actions/upload-artifact" diff --git a/javascript/ql/src/Security/CWE-312/examples/actions-artifact-leak-fixed.yml b/javascript/ql/src/Security/CWE-312/examples/actions-artifact-leak-fixed.yml new file mode 100644 index 00000000000..9f716584a9b --- /dev/null +++ b/javascript/ql/src/Security/CWE-312/examples/actions-artifact-leak-fixed.yml @@ -0,0 +1,14 @@ +name: secrets-in-artifacts +on: + pull_request: +jobs: + a-job: # NOT VULNERABLE + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + - name: "Upload artifact" + uses: actions/upload-artifact@v4 + with: + name: file + path: . + diff --git a/javascript/ql/src/Security/CWE-312/examples/actions-artifact-leak.yml b/javascript/ql/src/Security/CWE-312/examples/actions-artifact-leak.yml new file mode 100644 index 00000000000..9006855e997 --- /dev/null +++ b/javascript/ql/src/Security/CWE-312/examples/actions-artifact-leak.yml @@ -0,0 +1,13 @@ +name: secrets-in-artifacts +on: + pull_request: +jobs: + a-job: # VULNERABLE + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + - name: "Upload artifact" + uses: actions/upload-artifact@1746f4ab65b179e0ea60a494b83293b640dd5bba # v4.3.2 + with: + name: file + path: . diff --git a/javascript/ql/test/query-tests/Security/CWE-312/.github/workflows/test.yml b/javascript/ql/test/query-tests/Security/CWE-312/.github/workflows/test.yml new file mode 100644 index 00000000000..34a8c1497c2 --- /dev/null +++ b/javascript/ql/test/query-tests/Security/CWE-312/.github/workflows/test.yml @@ -0,0 +1,65 @@ +name: secrets-in-artifacts +on: + pull_request: +jobs: + test1: # VULNERABLE + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + - name: "Upload artifact" + uses: actions/upload-artifact@1746f4ab65b179e0ea60a494b83293b640dd5bba # v4.3.2 + with: + name: file + path: . + test2: # NOT VULNERABLE + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + - name: "Upload artifact" + uses: actions/upload-artifact@v4 + with: + name: file + path: . + test3: # VULNERABLE + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + - name: "Upload artifact" + uses: actions/upload-artifact@1746f4ab65b179e0ea60a494b83293b640dd5bba # v4.3.2 + with: + name: file + path: "*" + test4: # VULNERABLE + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + with: + path: foo + - name: "Upload artifact" + uses: actions/upload-artifact@1746f4ab65b179e0ea60a494b83293b640dd5bba # v4.3.2 + with: + name: file + path: foo + test5: # VULNERABLE + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + with: + path: foo + - name: "Upload artifact" + uses: actions/upload-artifact@1746f4ab65b179e0ea60a494b83293b640dd5bba # v4.3.2 + with: + name: file + path: foo/* + test6: # NOT VULNERABLE + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + with: + path: pr + - name: "Upload artifact" + uses: actions/upload-artifact@1746f4ab65b179e0ea60a494b83293b640dd5bba # v4.3.2 + with: + name: file + path: foo + diff --git a/javascript/ql/test/query-tests/Security/CWE-312/ActionsArtifactLeak.expected b/javascript/ql/test/query-tests/Security/CWE-312/ActionsArtifactLeak.expected new file mode 100644 index 00000000000..eb2fccea26b --- /dev/null +++ b/javascript/ql/test/query-tests/Security/CWE-312/ActionsArtifactLeak.expected @@ -0,0 +1,4 @@ +| .github/workflows/test.yml:9:9:14:2 | name: " ... tifact" | A secret is exposed in an artifact uploaded by $@ | .github/workflows/test.yml:9:9:14:2 | name: " ... tifact" | actions/upload-artifact | +| .github/workflows/test.yml:27:9:32:2 | name: " ... tifact" | A secret is exposed in an artifact uploaded by $@ | .github/workflows/test.yml:27:9:32:2 | name: " ... tifact" | actions/upload-artifact | +| .github/workflows/test.yml:38:9:43:2 | name: " ... tifact" | A secret is exposed in an artifact uploaded by $@ | .github/workflows/test.yml:38:9:43:2 | name: " ... tifact" | actions/upload-artifact | +| .github/workflows/test.yml:49:9:54:2 | name: " ... tifact" | A secret is exposed in an artifact uploaded by $@ | .github/workflows/test.yml:49:9:54:2 | name: " ... tifact" | actions/upload-artifact | diff --git a/javascript/ql/test/query-tests/Security/CWE-312/ActionsArtifactLeak.qlref b/javascript/ql/test/query-tests/Security/CWE-312/ActionsArtifactLeak.qlref new file mode 100644 index 00000000000..e133c99ba5e --- /dev/null +++ b/javascript/ql/test/query-tests/Security/CWE-312/ActionsArtifactLeak.qlref @@ -0,0 +1 @@ +Security/CWE-312/ActionsArtifactLeak.ql From 5df3af22721948c693fedf3bc628419e72479a67 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alvaro=20Mu=C3=B1oz?= Date: Fri, 6 Sep 2024 23:06:57 +0200 Subject: [PATCH 3/7] Fix alert message --- javascript/ql/src/Security/CWE-312/ActionsArtifactLeak.ql | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/javascript/ql/src/Security/CWE-312/ActionsArtifactLeak.ql b/javascript/ql/src/Security/CWE-312/ActionsArtifactLeak.ql index 96ac361968d..4ef5536f1d5 100644 --- a/javascript/ql/src/Security/CWE-312/ActionsArtifactLeak.ql +++ b/javascript/ql/src/Security/CWE-312/ActionsArtifactLeak.ql @@ -108,5 +108,4 @@ where upload_path.getStep() = upload ) ) -select upload, "A secret is exposed in an artifact uploaded by $@", upload, - "actions/upload-artifact" +select upload, "A secret may be exposed in an artifact." From 5d1da861a246aee07514119742ee6a5f7625d4f4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alvaro=20Mu=C3=B1oz?= Date: Fri, 6 Sep 2024 23:21:41 +0200 Subject: [PATCH 4/7] fix: Use YamlScalar for booleans --- .../Security/CWE-312/ActionsArtifactLeak.ql | 2 +- .../CWE-312/.github/workflows/test.yml | 22 +++++++++++++++++++ .../CWE-312/ActionsArtifactLeak.expected | 9 ++++---- 3 files changed, 28 insertions(+), 5 deletions(-) diff --git a/javascript/ql/src/Security/CWE-312/ActionsArtifactLeak.ql b/javascript/ql/src/Security/CWE-312/ActionsArtifactLeak.ql index 4ef5536f1d5..0b869d5d283 100644 --- a/javascript/ql/src/Security/CWE-312/ActionsArtifactLeak.ql +++ b/javascript/ql/src/Security/CWE-312/ActionsArtifactLeak.ql @@ -26,7 +26,7 @@ class ActionsCheckoutStep extends Actions::Step { /** * A `with:`/`persist-credentials` field sibling to `uses: actions/checkout`. */ -class ActionsCheckoutWithPersistCredentials extends YamlNode, YamlString { +class ActionsCheckoutWithPersistCredentials extends YamlNode, YamlScalar { ActionsCheckoutStep step; ActionsCheckoutWithPersistCredentials() { diff --git a/javascript/ql/test/query-tests/Security/CWE-312/.github/workflows/test.yml b/javascript/ql/test/query-tests/Security/CWE-312/.github/workflows/test.yml index 34a8c1497c2..473d5998695 100644 --- a/javascript/ql/test/query-tests/Security/CWE-312/.github/workflows/test.yml +++ b/javascript/ql/test/query-tests/Security/CWE-312/.github/workflows/test.yml @@ -62,4 +62,26 @@ jobs: with: name: file path: foo + test7: # NOT VULNERABLE + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + with: + persist-credentials: false + - name: "Upload artifact" + uses: actions/upload-artifact@1746f4ab65b179e0ea60a494b83293b640dd5bba # v4.3.2 + with: + name: file + path: . + test8: # VULNERABLE + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + with: + persist-credentials: true + - name: "Upload artifact" + uses: actions/upload-artifact@1746f4ab65b179e0ea60a494b83293b640dd5bba # v4.3.2 + with: + name: file + path: . diff --git a/javascript/ql/test/query-tests/Security/CWE-312/ActionsArtifactLeak.expected b/javascript/ql/test/query-tests/Security/CWE-312/ActionsArtifactLeak.expected index eb2fccea26b..575ddda89a4 100644 --- a/javascript/ql/test/query-tests/Security/CWE-312/ActionsArtifactLeak.expected +++ b/javascript/ql/test/query-tests/Security/CWE-312/ActionsArtifactLeak.expected @@ -1,4 +1,5 @@ -| .github/workflows/test.yml:9:9:14:2 | name: " ... tifact" | A secret is exposed in an artifact uploaded by $@ | .github/workflows/test.yml:9:9:14:2 | name: " ... tifact" | actions/upload-artifact | -| .github/workflows/test.yml:27:9:32:2 | name: " ... tifact" | A secret is exposed in an artifact uploaded by $@ | .github/workflows/test.yml:27:9:32:2 | name: " ... tifact" | actions/upload-artifact | -| .github/workflows/test.yml:38:9:43:2 | name: " ... tifact" | A secret is exposed in an artifact uploaded by $@ | .github/workflows/test.yml:38:9:43:2 | name: " ... tifact" | actions/upload-artifact | -| .github/workflows/test.yml:49:9:54:2 | name: " ... tifact" | A secret is exposed in an artifact uploaded by $@ | .github/workflows/test.yml:49:9:54:2 | name: " ... tifact" | actions/upload-artifact | +| .github/workflows/test.yml:9:9:14:2 | name: " ... tifact" | A secret may be exposed in an artifact. | +| .github/workflows/test.yml:27:9:32:2 | name: " ... tifact" | A secret may be exposed in an artifact. | +| .github/workflows/test.yml:38:9:43:2 | name: " ... tifact" | A secret may be exposed in an artifact. | +| .github/workflows/test.yml:49:9:54:2 | name: " ... tifact" | A secret may be exposed in an artifact. | +| .github/workflows/test.yml:82:9:86:18 | name: " ... tifact" | A secret may be exposed in an artifact. | From d34a0ba3060dd5e72798cb6f33e46eb9ff17187e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alvaro=20Mu=C3=B1oz?= Date: Fri, 6 Sep 2024 23:28:57 +0200 Subject: [PATCH 5/7] Add change note --- .../2024-09-06-new-actions-artifact-leak-query.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 javascript/ql/src/change-notes/2024-09-06-new-actions-artifact-leak-query.md diff --git a/javascript/ql/src/change-notes/2024-09-06-new-actions-artifact-leak-query.md b/javascript/ql/src/change-notes/2024-09-06-new-actions-artifact-leak-query.md new file mode 100644 index 00000000000..c73bbc7e135 --- /dev/null +++ b/javascript/ql/src/change-notes/2024-09-06-new-actions-artifact-leak-query.md @@ -0,0 +1,5 @@ +--- +category: majorAnalysis +--- + +- New query to detect GitHub Actions artifacts that may leak the GITHUB_TOKEN token. From 969c57c1c86c6a8e8a2abe3bde2a801753fbbd49 Mon Sep 17 00:00:00 2001 From: Andrew Eisenberg Date: Tue, 10 Sep 2024 11:42:14 -0700 Subject: [PATCH 6/7] Update pull_request_template.md Include a reminder about adding a query to autofix, --- .github/pull_request_template.md | 1 + 1 file changed, 1 insertion(+) diff --git a/.github/pull_request_template.md b/.github/pull_request_template.md index 89d6d3d66e2..3b31894b0f3 100644 --- a/.github/pull_request_template.md +++ b/.github/pull_request_template.md @@ -11,3 +11,4 @@ - [ ] Autofixes generated based on these changes are valid, only needed if this PR makes significant changes to `.ql`, `.qll`, or `.qhelp` files. See [the documentation](https://github.com/github/codeql-team/blob/main/docs/best-practices/validating-autofix-for-query-changes.md) (internal access required). - [ ] Changes are validated [at scale](https://github.com/github/codeql-dca/) (internal access required). +- [ ] Adding a new query? Consider also [adding the query to autofix](https://github.com/github/codeml-autofix/blob/main/docs/updating-query-support.md#adding-a-new-query-to-the-query-suite). From 061d58ae4a8b528352e29f6603b9882ad5f82d94 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alvaro=20Mu=C3=B1oz?= Date: Tue, 10 Sep 2024 22:18:04 +0200 Subject: [PATCH 7/7] Update javascript/ql/src/change-notes/2024-09-06-new-actions-artifact-leak-query.md Co-authored-by: Asger F --- .../change-notes/2024-09-06-new-actions-artifact-leak-query.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/javascript/ql/src/change-notes/2024-09-06-new-actions-artifact-leak-query.md b/javascript/ql/src/change-notes/2024-09-06-new-actions-artifact-leak-query.md index c73bbc7e135..316e89aa636 100644 --- a/javascript/ql/src/change-notes/2024-09-06-new-actions-artifact-leak-query.md +++ b/javascript/ql/src/change-notes/2024-09-06-new-actions-artifact-leak-query.md @@ -2,4 +2,4 @@ category: majorAnalysis --- -- New query to detect GitHub Actions artifacts that may leak the GITHUB_TOKEN token. +- Added a new query (`js/actions/actions-artifact-leak`) to detect GitHub Actions artifacts that may leak the GITHUB_TOKEN token.