From 5837c806a3b9ce7a85f0adeb9dfe24da109472cb Mon Sep 17 00:00:00 2001 From: Pedro Nacht Date: Thu, 13 Jul 2023 21:33:21 -0300 Subject: [PATCH] =?UTF-8?q?=F0=9F=90=9B=20Forgive=20job-level=20permission?= =?UTF-8?q?s=20(#3162)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Forgive all job-level permissions Signed-off-by: Pedro Kaj Kjellerup Nacht * Update tests Signed-off-by: Pedro Kaj Kjellerup Nacht * Replace magic number Signed-off-by: Pedro Kaj Kjellerup Nacht * Rename test Signed-off-by: Pedro Kaj Kjellerup Nacht * Test that multiple job-level permissions are forgiven Signed-off-by: Pedro Kaj Kjellerup Nacht * Drop unused permissionIsPresent Signed-off-by: Pedro Kaj Kjellerup Nacht * Update documentation Signed-off-by: Pedro Kaj Kjellerup Nacht * Modify score descriptions Signed-off-by: Pedro Kaj Kjellerup Nacht * Document warning for job-level permissions Signed-off-by: Pedro Kaj Kjellerup Nacht * List job-level permissions that get WARNed Signed-off-by: Pedro Kaj Kjellerup Nacht --------- Signed-off-by: Pedro Kaj Kjellerup Nacht Signed-off-by: Allen Shearin --- checks/evaluation/permissions/permissions.go | 17 +++--- checks/permissions_test.go | 21 ++++++-- ...kflow-permissions-run-multiple-writes.yaml | 30 +++++++++++ docs/checks.md | 38 +++++++------- docs/checks/internal/checks.yaml | 52 ++++++++++--------- 5 files changed, 101 insertions(+), 57 deletions(-) create mode 100644 checks/testdata/.github/workflows/github-workflow-permissions-run-multiple-writes.yaml diff --git a/checks/evaluation/permissions/permissions.go b/checks/evaluation/permissions/permissions.go index ed8b0cbc21d..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) { @@ -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-- } @@ -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 diff --git a/checks/permissions_test.go b/checks/permissions_test.go index 41f42f105bf..0b06e0628f8 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, @@ -302,11 +302,11 @@ 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, - Score: 9, + Score: checker.MaxResultScore - 1, NumberOfWarn: 1, NumberOfInfo: 4, 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, @@ -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" diff --git a/docs/checks.md b/docs/checks.md index 406bb213f1b..a8aafd8a937 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 @@ -630,25 +630,27 @@ 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. -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. +Though a project's score won't be penalized, the check's details will include +warnings for more sensitive run-level permissions, listed below: -### Write permissions causing a small reduction -* `statuses` - May allow an attacker to change the result of pre-submit checks and get a PR merged. +* `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. -* `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. +* `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. -* `actions` - May allow an attacker to steal GitHub secrets by approving to run an action that needs approval. +* `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. **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 51601c05a55..9233da7a4df 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 @@ -670,26 +670,30 @@ 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. - 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. + Though a project's score won't be penalized, the check's details will include + warnings for more sensitive run-level permissions, listed below: - ### Write permissions causing a small reduction - * `statuses` - May allow an attacker to change the result of pre-submit checks and get a PR merged. + * `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. - * `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. + * `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. - * `actions` - May allow an attacker to steal GitHub secrets by approving to run an action that needs approval. + * `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. 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 +823,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).