🐛 Forgive job-level permissions (#3162)

* Forgive all job-level permissions Signed-off-by: Pedro Kaj Kjellerup Nacht <[email protected]> * Update tests Signed-off-by: Pedro Kaj Kjellerup Nacht <[email protected]> * Replace magic number Signed-off-by: Pedro Kaj Kjellerup Nacht <[email protected]> * Rename test Signed-off-by: Pedro Kaj Kjellerup Nacht <[email protected]> * Test that multiple job-level permissions are forgiven Signed-off-by: Pedro Kaj Kjellerup Nacht <[email protected]> * Drop unused permissionIsPresent Signed-off-by: Pedro Kaj Kjellerup Nacht <[email protected]> * Update documentation Signed-off-by: Pedro Kaj Kjellerup Nacht <[email protected]> * Modify score descriptions Signed-off-by: Pedro Kaj Kjellerup Nacht <[email protected]> * Document warning for job-level permissions Signed-off-by: Pedro Kaj Kjellerup Nacht <[email protected]> * List job-level permissions that get WARNed Signed-off-by: Pedro Kaj Kjellerup Nacht <[email protected]> --------- Signed-off-by: Pedro Kaj Kjellerup Nacht <[email protected]>
ossf · Jul 14, 2023 · fc87616 · fc87616
1 parent 96b2169
commit fc87616
Show file tree

Hide file tree

Showing 5 changed files with 101 additions and 57 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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).