✨ Token-Permission: Allow top level permissions not defined if all run level permissions are (#1356)

* doc

* allow non defined top level

* fix

* e2e fix

* linter

checks/errors.go

@@ -20,13 +20,14 @@ import (
 
 //nolint
 var (
-	errInternalInvalidDockerFile = errors.New("invalid Dockerfile")
+	errInternalInvalidDockerFile  = errors.New("invalid Dockerfile")
-	errInternalInvalidYamlFile   = errors.New("invalid yaml file")
+	errInternalInvalidYamlFile    = errors.New("invalid yaml file")
-	errInternalFilenameMatch     = errors.New("filename match error")
+	errInternalFilenameMatch      = errors.New("filename match error")
-	errInternalEmptyFile         = errors.New("empty file")
+	errInternalEmptyFile          = errors.New("empty file")
-	errInternalCommitishNil      = errors.New("commitish is nil")
+	errInternalCommitishNil       = errors.New("commitish is nil")
-	errInternalBranchNotFound    = errors.New("branch not found")
+	errInternalBranchNotFound     = errors.New("branch not found")
-	errInvalidGitHubWorkflow     = errors.New("invalid GitHub workflow")
+	errInvalidGitHubWorkflow      = errors.New("invalid GitHub workflow")
-	errInternalNoReviews         = errors.New("no reviews found")
+	errInternalNoReviews          = errors.New("no reviews found")
-	errInternalNoCommits         = errors.New("no commits found")
+	errInternalNoCommits          = errors.New("no commits found")
+	errInternalInvalidPermissions = errors.New("invalid permissions")
 )

checks/permissions.go

@@ -26,7 +26,11 @@ import (
 )
 
 // CheckTokenPermissions is the exported name for Token-Permissions check.
-const CheckTokenPermissions = "Token-Permissions"
+const (
+	CheckTokenPermissions = "Token-Permissions"
+	runLevelPermission    = "run level"
+	topLevelPermission    = "top level"
+)
 
 //nolint:gochecknoinits
 func init() {
@@ -53,8 +57,8 @@ func TokenPermissions(c *checker.CheckRequest) checker.CheckResult {
 	return createResultForLeastPrivilegeTokens(data, err)
 }
 
-func validatePermission(permissionKey string, permissionValue *actionlint.PermissionScope, path string,
+func validatePermission(permissionKey string, permissionValue *actionlint.PermissionScope,
-	dl checker.DetailLogger, pPermissions map[string]bool,
+	permLevel, path string, dl checker.DetailLogger, pPermissions map[string]bool,
 	ignoredPermissions map[string]bool) error {
 	if permissionValue.Value == nil {
 		return sce.WithMessage(sce.ErrScorecardInternal, errInvalidGitHubWorkflow.Error())
@@ -70,7 +74,7 @@ func validatePermission(permissionKey string, permissionValue *actionlint.Permis
 				Path:   path,
 				Type:   checker.FileTypeSource,
 				Offset: lineNumber,
-				Text:   fmt.Sprintf("'%v' permission set to '%v'", permissionKey, val),
+				Text:   fmt.Sprintf("%s '%v' permission set to '%v'", permLevel, permissionKey, val),
 				// TODO: set Snippet.
 			})
 			recordPermissionWrite(permissionKey, pPermissions)
@@ -81,7 +85,7 @@ func validatePermission(permissionKey string, permissionValue *actionlint.Permis
 				Path:   path,
 				Type:   checker.FileTypeSource,
 				Offset: lineNumber,
-				Text:   fmt.Sprintf("'%v' permission set to '%v'", permissionKey, val),
+				Text:   fmt.Sprintf("%s '%v' permission set to '%v'", permLevel, permissionKey, val),
 				// TODO: set Snippet.
 			})
 		}
@@ -92,17 +96,17 @@ func validatePermission(permissionKey string, permissionValue *actionlint.Permis
 		Path:   path,
 		Type:   checker.FileTypeSource,
 		Offset: lineNumber,
-		Text:   fmt.Sprintf("'%v' permission set to '%v'", permissionKey, val),
+		Text:   fmt.Sprintf("%s '%v' permission set to '%v'", permLevel, permissionKey, val),
 		// TODO: set Snippet.
 	})
 	return nil
 }
 
-func validateMapPermissions(scopes map[string]*actionlint.PermissionScope, path string,
+func validateMapPermissions(scopes map[string]*actionlint.PermissionScope, permLevel, path string,
 	dl checker.DetailLogger, pPermissions map[string]bool,
 	ignoredPermissions map[string]bool) error {
 	for key, v := range scopes {
-		if err := validatePermission(key, v, path, dl, pPermissions, ignoredPermissions); err != nil {
+		if err := validatePermission(key, v, permLevel, path, dl, pPermissions, ignoredPermissions); err != nil {
 			return err
 		}
 	}
@@ -119,7 +123,7 @@ func recordAllPermissionsWrite(pPermissions map[string]bool) {
 	pPermissions["all"] = true
 }
 
-func validatePermissions(permissions *actionlint.Permissions, path string,
+func validatePermissions(permissions *actionlint.Permissions, permLevel, path string,
 	dl checker.DetailLogger, pPermissions map[string]bool,
 	ignoredPermissions map[string]bool) error {
 	allIsSet := permissions != nil && permissions.All != nil && permissions.All.Value != ""
@@ -129,7 +133,7 @@ func validatePermissions(permissions *actionlint.Permissions, path string,
 			Path:   path,
 			Type:   checker.FileTypeSource,
 			Offset: checker.OffsetDefault,
-			Text:   "permissions set to 'none'",
+			Text:   fmt.Sprintf("%s permissions set to 'none'", permLevel),
 		})
 	}
 	if allIsSet {
@@ -138,12 +142,13 @@ func validatePermissions(permissions *actionlint.Permissions, path string,
 		if permissions.All.Pos != nil {
 			lineNumber = permissions.All.Pos.Line
 		}
+
 		if !strings.EqualFold(val, "read-all") && val != "" {
 			dl.Warn3(&checker.LogMessage{
 				Path:   path,
 				Type:   checker.FileTypeSource,
 				Offset: lineNumber,
-				Text:   fmt.Sprintf("permissions set to '%v'", val),
+				Text:   fmt.Sprintf("%s permissions set to '%v'", permLevel, val),
 				// TODO: set Snippet.
 			})
 			recordAllPermissionsWrite(pPermissions)
@@ -154,10 +159,10 @@ func validatePermissions(permissions *actionlint.Permissions, path string,
 			Path:   path,
 			Type:   checker.FileTypeSource,
 			Offset: lineNumber,
-			Text:   fmt.Sprintf("permissions set to '%v'", val),
+			Text:   fmt.Sprintf("%s permissions set to '%v'", permLevel, val),
 			// TODO: set Snippet.
 		})
-	} else /* scopeIsSet == true */ if err := validateMapPermissions(permissions.Scopes, path, dl, pPermissions,
+	} else /* scopeIsSet == true */ if err := validateMapPermissions(permissions.Scopes, permLevel, path, dl, pPermissions,
 		ignoredPermissions); err != nil {
 		return err
 	}
@@ -172,13 +177,13 @@ func validateTopLevelPermissions(workflow *actionlint.Workflow, path string,
 			Path:   path,
 			Type:   checker.FileTypeSource,
 			Offset: checker.OffsetDefault,
-			Text:   "no permission defined",
+			Text:   fmt.Sprintf("no %s permission defined", topLevelPermission),
 		})
 		recordAllPermissionsWrite(pdata.topLevelWritePermissions)
 		return nil
 	}
 
-	return validatePermissions(workflow.Permissions, path, dl,
+	return validatePermissions(workflow.Permissions, topLevelPermission, path, dl,
 		pdata.topLevelWritePermissions, map[string]bool{})
 }
 
@@ -198,11 +203,13 @@ func validateRunLevelPermissions(workflow *actionlint.Workflow, path string,
 				Path:   path,
 				Type:   checker.FileTypeSource,
 				Offset: lineNumber,
-				Text:   "no permission defined",
+				Text:   fmt.Sprintf("no %s permission defined", runLevelPermission),
 			})
+			recordAllPermissionsWrite(pdata.runLevelWritePermissions)
 			continue
 		}
-		err := validatePermissions(job.Permissions, path, dl, pdata.runLevelWritePermissions, ignoredPermissions)
+		err := validatePermissions(job.Permissions, runLevelPermission,
+			path, dl, pdata.runLevelWritePermissions, ignoredPermissions)
 		if err != nil {
 			return err
 		}
@@ -225,9 +232,18 @@ func isPermissionOfInterest(name string, ignoredPermissions map[string]bool) boo
 }
 
 func permissionIsPresent(result permissionCbData, name string) bool {
-	_, ok1 := result.topLevelWritePermissions[name]
+	return permissionIsPresentInTopLevel(result, name) ||
-	_, ok2 := result.runLevelWritePermissions[name]
+		permissionIsPresentInRunLevel(result, name)
-	return ok1 || ok2
+}
+
+func permissionIsPresentInTopLevel(result permissionCbData, name string) bool {
+	_, ok := result.topLevelWritePermissions[name]
+	return ok
+}
+
+func permissionIsPresentInRunLevel(result permissionCbData, name string) bool {
+	_, ok := result.runLevelWritePermissions[name]
+	return ok
 }
 
 // Calculate the score.
@@ -236,13 +252,20 @@ func calculateScore(result permissionCbData) int {
 	// Note: there are legitimate reasons to use some of the permissions like checks, deployments, etc.
 	// in CI/CD systems https://docs.travis-ci.com/user/github-oauth-scopes/.
 
-	if permissionIsPresent(result, "all") {
-		return checker.MinResultScore
-	}
-
 	// Start with a perfect score.
 	score := float32(checker.MaxResultScore)
 
+	// If no top level permissions are defined, all the permissions
+	// are enabled by default, hence "all". In this case,
+	if permissionIsPresentInTopLevel(result, "all") {
+		if permissionIsPresentInRunLevel(result, "all") {
+			// ... give lowest score if no run level permissions are defined either.
+			return checker.MinResultScore
+		}
+		// ... reduce score if run level permissions are defined.
+		score--
+	}
+
 	// 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.

checks/permissions_test.go

@@ -263,6 +263,17 @@ func TestGithubTokenPermissions(t *testing.T) {
 				NumberOfDebug: 3,
 			},
 		},
+		{
+			name:     "workflow jobs only",
+			filename: "./testdata/github-workflow-permissions-jobs-only.yaml",
+			expected: scut.TestReturn{
+				Error:         nil,
+				Score:         9,
+				NumberOfWarn:  1,
+				NumberOfInfo:  3,
+				NumberOfDebug: 4,
+			},
+		},
 	}
 	for _, tt := range tests {
 		tt := tt // Re-initializing variable so it is not changed while executing the closure below

checks/testdata/github-workflow-permissions-jobs-only.yaml

@@ -0,0 +1,34 @@
+# Copyright 2021 Security 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]
+
+jobs:
+  jobOne:
+    runs-on: ubuntu-latest
+    permissions:
+    steps:
+      - run: echo "write-and-read workflow"
+  jobTwo:
+    runs-on: ubuntu-latest
+    permissions:
+      contents: read
+    steps:
+      - run: echo "write-and-read workflow"
+  jobThree:
+    runs-on: ubuntu-latest
+    permissions:
+      contents: read
+    steps:
+      - run: echo "write-and-read workflow"

docs/checks.md

@@ -528,7 +528,7 @@ information about a bug is not publicly visible.
 **Remediation steps**
 - Place a security policy file `SECURITY.md` in the root directory of your repository. This makes it easily discoverable by a vulnerability reporter.
 - The file should contain information on what constitutes a vulnerability and a way to report it securely (e.g. issue tracker with private issue support, encrypted email with a published public key).
-- For GitHub, see more information [here](https://docs.github.com/en/code-security/getting-started/adding-a-security-policy-to-your-repository).
+- For GitHub, see more information [here](https://docs.github.com/en/code-security/getting-started/adding-a-security-policy-to-your-repository.).
 
 ## Signed-Releases 
 
@@ -572,6 +572,9 @@ yaml file are set as read-only at the
 [top level](https://docs.github.com/en/actions/reference/workflow-syntax-for-github-actions#permissions)
 and the required write permissions are declared at the
 [run-level](https://docs.github.com/en/actions/reference/workflow-syntax-for-github-actions#jobsjob_idpermissions).
+One point is reduced from the score if all jobs have their permissions defined but the top level permissions are not defined. 
+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.   

docs/checks/internal/checks.yaml

@@ -632,6 +632,9 @@ checks:
       [top level](https://docs.github.com/en/actions/reference/workflow-syntax-for-github-actions#permissions)
       and the required write permissions are declared at the
       [run-level](https://docs.github.com/en/actions/reference/workflow-syntax-for-github-actions#jobsjob_idpermissions).
+      One point is reduced from the score if all jobs have their permissions defined but the top level permissions are not defined. 
+      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.