From 8b9fec33205d0e5bba25b82458f4d6b4ed5346ec Mon Sep 17 00:00:00 2001 From: Pedro Kaj Kjellerup Nacht Date: Tue, 13 Jun 2023 13:41:58 +0000 Subject: [PATCH 01/10] Forgive all job-level permissions Signed-off-by: Pedro Kaj Kjellerup Nacht --- checks/evaluation/permissions/permissions.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/checks/evaluation/permissions/permissions.go b/checks/evaluation/permissions/permissions.go index ed8b0cbc21d..9e8c444e10a 100644 --- a/checks/evaluation/permissions/permissions.go +++ b/checks/evaluation/permissions/permissions.go @@ -325,21 +325,21 @@ func calculateScore(result map[string]permissions) int { // status: https://docs.github.com/en/rest/reference/repos#statuses. // May allow an attacker to change the result of pre-submit and get a PR merged. // Low risk: -0.5. - if permissionIsPresent(perms, "statuses") { + if permissionIsPresentInTopLevel(perms, "statuses") { score -= 0.5 } // checks. // May allow an attacker to edit checks to remove pre-submit and introduce a bug. // Low risk: -0.5. - if permissionIsPresent(perms, "checks") { + if permissionIsPresentInTopLevel(perms, "checks") { score -= 0.5 } // secEvents. // May allow attacker to read vuln reports before patch available. // Low risk: -1 - if permissionIsPresent(perms, "security-events") { + if permissionIsPresentInTopLevel(perms, "security-events") { score-- } @@ -348,7 +348,7 @@ func calculateScore(result map[string]permissions) int { // and tiny chance an attacker can trigger a remote // service with code they own if server accepts code/location var unsanitized. // Low risk: -1 - if permissionIsPresent(perms, "deployments") { + if permissionIsPresentInTopLevel(perms, "deployments") { score-- } From 082803bdbf62ab6a4ce82dd823add2ec0def34bc Mon Sep 17 00:00:00 2001 From: Pedro Kaj Kjellerup Nacht Date: Tue, 13 Jun 2023 13:42:10 +0000 Subject: [PATCH 02/10] Update tests Signed-off-by: Pedro Kaj Kjellerup Nacht --- checks/permissions_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/checks/permissions_test.go b/checks/permissions_test.go index 41f42f105bf..e7edc423aa3 100644 --- a/checks/permissions_test.go +++ b/checks/permissions_test.go @@ -53,7 +53,7 @@ func TestGithubTokenPermissions(t *testing.T) { filenames: []string{"./testdata/.github/workflows/github-workflow-permissions-run-no-codeql-write.yaml"}, expected: scut.TestReturn{ Error: nil, - Score: checker.MaxResultScore - 1, + Score: checker.MaxResultScore, NumberOfWarn: 1, NumberOfInfo: 1, NumberOfDebug: 4, @@ -317,7 +317,7 @@ func TestGithubTokenPermissions(t *testing.T) { filenames: []string{"./testdata/.github/workflows/github-workflow-permissions-run-write-codeql-comment.yaml"}, expected: scut.TestReturn{ Error: nil, - Score: checker.MaxResultScore - 1, + Score: checker.MaxResultScore, NumberOfWarn: 1, NumberOfInfo: 1, NumberOfDebug: 4, From 39d138b783ded9451e92e5e4d0baa596d66ec3c5 Mon Sep 17 00:00:00 2001 From: Pedro Kaj Kjellerup Nacht Date: Tue, 13 Jun 2023 13:43:51 +0000 Subject: [PATCH 03/10] Replace magic number Signed-off-by: Pedro Kaj Kjellerup Nacht --- checks/permissions_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/checks/permissions_test.go b/checks/permissions_test.go index e7edc423aa3..0bcef19a92f 100644 --- a/checks/permissions_test.go +++ b/checks/permissions_test.go @@ -306,7 +306,7 @@ func TestGithubTokenPermissions(t *testing.T) { filenames: []string{"./testdata/.github/workflows/github-workflow-permissions-jobs-only.yaml"}, expected: scut.TestReturn{ Error: nil, - Score: 9, + Score: checker.MaxResultScore - 1, NumberOfWarn: 1, NumberOfInfo: 4, NumberOfDebug: 4, From dd75c17e54ce0824d3b7566fc209f8575c80cd08 Mon Sep 17 00:00:00 2001 From: Pedro Kaj Kjellerup Nacht Date: Wed, 14 Jun 2023 20:51:43 +0000 Subject: [PATCH 04/10] Rename test Signed-off-by: Pedro Kaj Kjellerup Nacht --- checks/permissions_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/checks/permissions_test.go b/checks/permissions_test.go index 0bcef19a92f..0bf220edbc7 100644 --- a/checks/permissions_test.go +++ b/checks/permissions_test.go @@ -302,7 +302,7 @@ func TestGithubTokenPermissions(t *testing.T) { }, }, { - name: "workflow jobs only", + name: "penalize job-level read without top level permissions", filenames: []string{"./testdata/.github/workflows/github-workflow-permissions-jobs-only.yaml"}, expected: scut.TestReturn{ Error: nil, From 8e75f9f7e0522f10a05543a8681246519d952a61 Mon Sep 17 00:00:00 2001 From: Pedro Kaj Kjellerup Nacht Date: Wed, 14 Jun 2023 20:53:12 +0000 Subject: [PATCH 05/10] Test that multiple job-level permissions are forgiven Signed-off-by: Pedro Kaj Kjellerup Nacht --- checks/permissions_test.go | 13 ++++++++ ...kflow-permissions-run-multiple-writes.yaml | 30 +++++++++++++++++++ 2 files changed, 43 insertions(+) create mode 100644 checks/testdata/.github/workflows/github-workflow-permissions-run-multiple-writes.yaml diff --git a/checks/permissions_test.go b/checks/permissions_test.go index 0bf220edbc7..0b06e0628f8 100644 --- a/checks/permissions_test.go +++ b/checks/permissions_test.go @@ -389,6 +389,19 @@ func TestGithubTokenPermissions(t *testing.T) { NumberOfDebug: 5, }, }, + { + name: "don't penalize job-level writes", + filenames: []string{ + "./testdata/.github/workflows/github-workflow-permissions-run-multiple-writes.yaml", + }, + expected: scut.TestReturn{ + Error: nil, + Score: checker.MaxResultScore, + NumberOfWarn: 7, // number of job-level write permissions + NumberOfInfo: 1, // read-only top-level permissions + NumberOfDebug: 4, // This is 4 + (number of actions = 0) + }, + }, } for _, tt := range tests { tt := tt // Re-initializing variable so it is not changed while executing the closure below diff --git a/checks/testdata/.github/workflows/github-workflow-permissions-run-multiple-writes.yaml b/checks/testdata/.github/workflows/github-workflow-permissions-run-multiple-writes.yaml new file mode 100644 index 00000000000..427579485fa --- /dev/null +++ b/checks/testdata/.github/workflows/github-workflow-permissions-run-multiple-writes.yaml @@ -0,0 +1,30 @@ +# Copyright 2021 OpenSSF Scorecard Authors +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. +name: write-and-read workflow +on: [push] +permissions: read-all + +jobs: + Explore-GitHub-Actions: + runs-on: ubuntu-latest + permissions: + statuses: write + checks: write + security-events: write + deployments: write + contents: write + packages: write + actions: write + steps: + - run: echo "write-and-read workflow" From 57efdf490383e449a15b45d33f8dbb1ffee02f63 Mon Sep 17 00:00:00 2001 From: Pedro Kaj Kjellerup Nacht Date: Wed, 14 Jun 2023 20:53:41 +0000 Subject: [PATCH 06/10] Drop unused permissionIsPresent Signed-off-by: Pedro Kaj Kjellerup Nacht --- checks/evaluation/permissions/permissions.go | 5 ----- 1 file changed, 5 deletions(-) diff --git a/checks/evaluation/permissions/permissions.go b/checks/evaluation/permissions/permissions.go index 9e8c444e10a..ba4f32703f5 100644 --- a/checks/evaluation/permissions/permissions.go +++ b/checks/evaluation/permissions/permissions.go @@ -386,11 +386,6 @@ func calculateScore(result map[string]permissions) int { return int(score) } -func permissionIsPresent(perms permissions, name string) bool { - return permissionIsPresentInTopLevel(perms, name) || - permissionIsPresentInRunLevel(perms, name) -} - func permissionIsPresentInTopLevel(perms permissions, name string) bool { _, ok := perms.topLevelWritePermissions[name] return ok From 066a4f5848cfdf0680879be027f8879b0ea93e22 Mon Sep 17 00:00:00 2001 From: Pedro Kaj Kjellerup Nacht Date: Wed, 14 Jun 2023 20:55:46 +0000 Subject: [PATCH 07/10] Update documentation Signed-off-by: Pedro Kaj Kjellerup Nacht --- docs/checks.md | 28 ++++++--------------- docs/checks/internal/checks.yaml | 42 ++++++++++++-------------------- 2 files changed, 24 insertions(+), 46 deletions(-) diff --git a/docs/checks.md b/docs/checks.md index 406bb213f1b..3872f8f87a2 100644 --- a/docs/checks.md +++ b/docs/checks.md @@ -613,13 +613,13 @@ Note: The check does not verify the signatures. Risk: `High` (vulnerable to malicious code additions) -This check determines whether the project's automated workflows tokens are set -to read-only by default. It is currently limited to repositories hosted on -GitHub, and does not support other source hosting repositories (i.e., Forges). +This check determines whether the project's automated workflows tokens follow the +principle of least privilege. This is important because attackers may use a +compromised token with write access to, for example, push malicious code into the +project. -Setting token permissions to read-only follows the principle of least privilege. -This is important because attackers may use a compromised token with write -access to push malicious code into the project. +It is currently limited to repositories hosted on GitHub, and does not support +other source hosting repositories (i.e., Forges). The highest score is awarded when the permissions definitions in each workflow's yaml file are set as read-only at the @@ -632,23 +632,11 @@ left undefined because of human error. The check cannot detect if the "read-only" GitHub permission setting is enabled, as there is no API available. - -Additionally, points are reduced if certain write permissions are defined for a job. - -### Write permissions causing a small reduction -* `statuses` - May allow an attacker to change the result of pre-submit checks and get a PR merged. -* `checks` - May allow an attacker to remove pre-submit checks and introduce a bug. -* `security-events` - May allow an attacker to read vulnerability reports before a patch is available. However, points are not reduced if the job utilizes a recognized action for uploading SARIF results. -* `deployments` - May allow an attacker to charge repo owner by triggering VM runs, and tiny chance an attacker can trigger a remote service with code they own if server accepts code/location variables unsanitized. - -### Write permissions causing a large reduction -* `contents` - Allows an attacker to commit unreviewed code. However, points are not reduced if the job utilizes a recognized packaging action or command. -* `packages` - Allows an attacker to publish packages. However, points are not reduced if the job utilizes a recognized packaging action or command. -* `actions` - May allow an attacker to steal GitHub secrets by approving to run an action that needs approval. **Remediation steps** -- Set permissions as `read-all` or `contents: read` as described in GitHub's [documentation](https://docs.github.com/en/actions/reference/workflow-syntax-for-github-actions#permissions). +- Set top-level permissions as `read-all` or `contents: read` as described in GitHub's [documentation](https://docs.github.com/en/actions/reference/workflow-syntax-for-github-actions#permissions). +- Set any required write permissions at the job-level. Only set the permissions required for that job; do not set `permissions: write-all` at the job level. - To help determine the permissions needed for your workflows, you may use [StepSecurity's online tool](https://app.stepsecurity.io/secureworkflow/) by ticking the "Restrict permissions for GITHUB_TOKEN". You may also tick the "Pin actions to a full length commit SHA" to fix issues found by the Pinned-dependencies check. ## Vulnerabilities diff --git a/docs/checks/internal/checks.yaml b/docs/checks/internal/checks.yaml index 406d9871b60..133d1f82d7a 100644 --- a/docs/checks/internal/checks.yaml +++ b/docs/checks/internal/checks.yaml @@ -653,13 +653,13 @@ checks: description: | Risk: `High` (vulnerable to malicious code additions) - This check determines whether the project's automated workflows tokens are set - to read-only by default. It is currently limited to repositories hosted on - GitHub, and does not support other source hosting repositories (i.e., Forges). + This check determines whether the project's automated workflows tokens follow the + principle of least privilege. This is important because attackers may use a + compromised token with write access to, for example, push malicious code into the + project. - Setting token permissions to read-only follows the principle of least privilege. - This is important because attackers may use a compromised token with write - access to push malicious code into the project. + It is currently limited to repositories hosted on GitHub, and does not support + other source hosting repositories (i.e., Forges). The highest score is awarded when the permissions definitions in each workflow's yaml file are set as read-only at the @@ -673,23 +673,13 @@ checks: The check cannot detect if the "read-only" GitHub permission setting is enabled, as there is no API available. - Additionally, points are reduced if certain write permissions are defined for a job. - - ### Write permissions causing a small reduction - * `statuses` - May allow an attacker to change the result of pre-submit checks and get a PR merged. - * `checks` - May allow an attacker to remove pre-submit checks and introduce a bug. - * `security-events` - May allow an attacker to read vulnerability reports before a patch is available. However, points are not reduced if the job utilizes a recognized action for uploading SARIF results. - * `deployments` - May allow an attacker to charge repo owner by triggering VM runs, and tiny chance an attacker can trigger a remote service with code they own if server accepts code/location variables unsanitized. - - ### Write permissions causing a large reduction - * `contents` - Allows an attacker to commit unreviewed code. However, points are not reduced if the job utilizes a recognized packaging action or command. - * `packages` - Allows an attacker to publish packages. However, points are not reduced if the job utilizes a recognized packaging action or command. - * `actions` - May allow an attacker to steal GitHub secrets by approving to run an action that needs approval. - remediation: - >- - Set permissions as `read-all` or `contents: read` as described in + Set top-level permissions as `read-all` or `contents: read` as described in GitHub's [documentation](https://docs.github.com/en/actions/reference/workflow-syntax-for-github-actions#permissions). + - >- + Set any required write permissions at the job-level. Only set the permissions + required for that job; do not set `permissions: write-all` at the job level. - >- To help determine the permissions needed for your workflows, you may use [StepSecurity's online tool](https://app.stepsecurity.io/secureworkflow/) by ticking the "Restrict permissions for GITHUB_TOKEN". You may also tick the "Pin actions to a full length commit SHA" to fix issues found @@ -819,9 +809,9 @@ checks: This check determines whether the webhook defined in the repository has a token configured to authenticate the origins of requests. remediation: - - >- - Check whether your service supports token authentication. - - >- - If there is support for token authentication, set the secret in the webhook configuration. See [Setting up a webhook](https://docs.github.com/en/developers/webhooks-and-events/webhooks/creating-webhooks#setting-up-a-webhook) - - >- - If there is no support for token authentication, consider implementing it by following [these directions](https://docs.github.com/en/developers/webhooks-and-events/webhooks/securing-your-webhooks). + - >- + Check whether your service supports token authentication. + - >- + If there is support for token authentication, set the secret in the webhook configuration. See [Setting up a webhook](https://docs.github.com/en/developers/webhooks-and-events/webhooks/creating-webhooks#setting-up-a-webhook) + - >- + If there is no support for token authentication, consider implementing it by following [these directions](https://docs.github.com/en/developers/webhooks-and-events/webhooks/securing-your-webhooks). From 5d115403cf8a806ecef3971a840ce4e24451e4f2 Mon Sep 17 00:00:00 2001 From: Pedro Kaj Kjellerup Nacht Date: Thu, 6 Jul 2023 15:47:25 +0000 Subject: [PATCH 08/10] Modify score descriptions Signed-off-by: Pedro Kaj Kjellerup Nacht --- checks/evaluation/permissions/permissions.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/checks/evaluation/permissions/permissions.go b/checks/evaluation/permissions/permissions.go index ba4f32703f5..8f0c1be5d2b 100644 --- a/checks/evaluation/permissions/permissions.go +++ b/checks/evaluation/permissions/permissions.go @@ -56,11 +56,11 @@ func TokenPermissions(name string, c *checker.CheckRequest, r *checker.TokenPerm if score != checker.MaxResultScore { return checker.CreateResultWithScore(name, - "non read-only tokens detected in GitHub workflows", score) + "detected GitHub workflow tokens with excessive permissions", score) } return checker.CreateMaxScoreResult(name, - "tokens are read-only in GitHub workflows") + "GitHub workflow tokens follow principle of least privilege") } func applyScorePolicy(results *checker.TokenPermissionsData, c *checker.CheckRequest) (int, error) { From 72406532d6da44b3926340e79b7e2dabfd49f11f Mon Sep 17 00:00:00 2001 From: Pedro Kaj Kjellerup Nacht Date: Mon, 10 Jul 2023 18:23:52 +0000 Subject: [PATCH 09/10] Document warning for job-level permissions Signed-off-by: Pedro Kaj Kjellerup Nacht --- docs/checks.md | 5 +++++ docs/checks/internal/checks.yaml | 5 +++++ 2 files changed, 10 insertions(+) diff --git a/docs/checks.md b/docs/checks.md index 3872f8f87a2..03409003495 100644 --- a/docs/checks.md +++ b/docs/checks.md @@ -630,6 +630,11 @@ One point is reduced from the score if all jobs have their permissions defined b This configuration is secure, but there is a chance that when a new job is added to the workflow, its job permissions could be left undefined because of human error. +Though a project's score won't be penalized, the check's details will include +warnings for all run-level permissions. This compromise makes it clear the +maintainer has done what's possible to use those permissions safety, but allows +users to identify that the permissions are used. + The check cannot detect if the "read-only" GitHub permission setting is enabled, as there is no API available. diff --git a/docs/checks/internal/checks.yaml b/docs/checks/internal/checks.yaml index 133d1f82d7a..2bbcf52e932 100644 --- a/docs/checks/internal/checks.yaml +++ b/docs/checks/internal/checks.yaml @@ -670,6 +670,11 @@ checks: This configuration is secure, but there is a chance that when a new job is added to the workflow, its job permissions could be left undefined because of human error. + Though a project's score won't be penalized, the check's details will include + warnings for all run-level permissions. This compromise makes it clear the + maintainer has done what's possible to use those permissions safety, but allows + users to identify that the permissions are used. + The check cannot detect if the "read-only" GitHub permission setting is enabled, as there is no API available. From a68162e21ba93f41ad60850387b1a8e791cc3d0b Mon Sep 17 00:00:00 2001 From: Pedro Kaj Kjellerup Nacht Date: Thu, 13 Jul 2023 19:30:04 +0000 Subject: [PATCH 10/10] List job-level permissions that get WARNed Signed-off-by: Pedro Kaj Kjellerup Nacht --- docs/checks.md | 15 ++++++++++++--- docs/checks/internal/checks.yaml | 15 ++++++++++++--- 2 files changed, 24 insertions(+), 6 deletions(-) diff --git a/docs/checks.md b/docs/checks.md index 03409003495..a8aafd8a937 100644 --- a/docs/checks.md +++ b/docs/checks.md @@ -631,9 +631,18 @@ This configuration is secure, but there is a chance that when a new job is added left undefined because of human error. Though a project's score won't be penalized, the check's details will include -warnings for all run-level permissions. This compromise makes it clear the -maintainer has done what's possible to use those permissions safety, but allows -users to identify that the permissions are used. +warnings for more sensitive run-level permissions, listed below: + +* `actions` - May allow an attacker to steal GitHub secrets by approving to run an action that needs approval. +* `checks` - May allow an attacker to remove pre-submit checks and introduce a bug. +* `contents` - Allows an attacker to commit unreviewed code. However, points are not reduced if the job utilizes a recognized packaging action or command. +* `deployments` - May allow an attacker to charge repo owner by triggering VM runs, and tiny chance an attacker can trigger a remote service with code they own if server accepts code/location variables unsanitized. +* `packages` - Allows an attacker to publish packages. However, points are not reduced if the job utilizes a recognized packaging action or command. +* `security-events` - May allow an attacker to read vulnerability reports before a patch is available. However, points are not reduced if the job utilizes a recognized action for uploading SARIF results. +* `statuses` - May allow an attacker to change the result of pre-submit checks and get a PR merged. + +This compromise makes it clear the maintainer has done what's possible to use those permissions safety, +but allows users to identify that the permissions are used. The check cannot detect if the "read-only" GitHub permission setting is enabled, as there is no API available. diff --git a/docs/checks/internal/checks.yaml b/docs/checks/internal/checks.yaml index 2bbcf52e932..3f5018e432b 100644 --- a/docs/checks/internal/checks.yaml +++ b/docs/checks/internal/checks.yaml @@ -671,9 +671,18 @@ checks: left undefined because of human error. Though a project's score won't be penalized, the check's details will include - warnings for all run-level permissions. This compromise makes it clear the - maintainer has done what's possible to use those permissions safety, but allows - users to identify that the permissions are used. + warnings for more sensitive run-level permissions, listed below: + + * `actions` - May allow an attacker to steal GitHub secrets by approving to run an action that needs approval. + * `checks` - May allow an attacker to remove pre-submit checks and introduce a bug. + * `contents` - Allows an attacker to commit unreviewed code. However, points are not reduced if the job utilizes a recognized packaging action or command. + * `deployments` - May allow an attacker to charge repo owner by triggering VM runs, and tiny chance an attacker can trigger a remote service with code they own if server accepts code/location variables unsanitized. + * `packages` - Allows an attacker to publish packages. However, points are not reduced if the job utilizes a recognized packaging action or command. + * `security-events` - May allow an attacker to read vulnerability reports before a patch is available. However, points are not reduced if the job utilizes a recognized action for uploading SARIF results. + * `statuses` - May allow an attacker to change the result of pre-submit checks and get a PR merged. + + This compromise makes it clear the maintainer has done what's possible to use those permissions safety, + but allows users to identify that the permissions are used. The check cannot detect if the "read-only" GitHub permission setting is enabled, as there is no API available.