✨ Make Token-Permission check more granular (#773)

* draft

* add tests

* add e2e2 tests

* typos

* typo

* fixes

* linter

* use named value

* comments

* comment

checks/file_utils.go

@@ -131,6 +131,61 @@ func CheckFilesContent(shellPathFnPattern string,
 	return res, nil
 }
 
+// FileCbData is any data the caller can act upon
+// to keep state.
+type FileCbData interface{}
+
+// FileContentCb is the callback.
+// The bool returned indicates whether the CheckFilesContent2
+// should continue iterating over files or not.
+type FileContentCb func(path string, content []byte,
+	dl checker.DetailLogger, data FileCbData) (bool, error)
+
+func CheckFilesContent2(shellPathFnPattern string,
+	caseSensitive bool,
+	c *checker.CheckRequest,
+	onFileContent FileContentCb,
+	data FileCbData,
+) error {
+	predicate := func(filepath string) (bool, error) {
+		// Filter out Scorecard's own test files.
+		if isScorecardTestFile(c.Owner, c.Repo, filepath) {
+			return false, nil
+		}
+		// Filter out files based on path/names using the pattern.
+		b, err := isMatchingPath(shellPathFnPattern, filepath, caseSensitive)
+		if err != nil {
+			return false, err
+		}
+		return b, nil
+	}
+
+	matchedFiles, err := c.RepoClient.ListFiles(predicate)
+	if err != nil {
+		// nolint: wrapcheck
+		return err
+	}
+
+	for _, file := range matchedFiles {
+		content, err := c.RepoClient.GetFileContent(file)
+		if err != nil {
+			//nolint
+			return err
+		}
+
+		continueIter, err := onFileContent(file, content, c.Dlogger, data)
+		if err != nil {
+			return err
+		}
+
+		if !continueIter {
+			break
+		}
+	}
+
+	return nil
+}
+
 // CheckFileContainsCommands checks if the file content contains commands or not.
 // `comment` is the string or character that indicates a comment:
 // for example for Dockerfiles, it would be `#`.

checks/permissions.go

@@ -24,69 +24,88 @@ import (
 	sce "github.com/ossf/scorecard/v2/errors"
 )
 
-const CheckPermissions = "Token-Permissions"
+const CheckTokenPermissions = "Token-Permissions"
 
 //nolint:gochecknoinits
 func init() {
-	registerCheck(CheckPermissions, leastPrivilegedTokens)
+	registerCheck(CheckTokenPermissions, TokenPermissions)
 }
 
-func leastPrivilegedTokens(c *checker.CheckRequest) checker.CheckResult {
-	r, err := CheckFilesContent(".github/workflows/*", false, c, validateGitHubActionTokenPermissions)
-	return createResultForLeastPrivilegeTokens(r, err)
+// Holds stateful data to pass thru callbacks.
+// Each field correpsonds to a GitHub permission type, and
+// will hold true if declared non-write, false otherwise.
+type permissionCbData struct {
+	writePermissions map[string]bool
+}
+
+func TokenPermissions(c *checker.CheckRequest) checker.CheckResult {
+	data := permissionCbData{writePermissions: make(map[string]bool)}
+	err := CheckFilesContent2(".github/workflows/*", false,
+		c, validateGitHubActionTokenPermissions, &data)
+	return createResultForLeastPrivilegeTokens(data, err)
 }
 
 func validatePermission(key string, value interface{}, path string,
-	dl checker.DetailLogger) (bool, error) {
+	dl checker.DetailLogger, pdata *permissionCbData) error {
 	val, ok := value.(string)
 	if !ok {
 		//nolint
-		return false, sce.Create(sce.ErrScorecardInternal, errInvalidGitHubWorkflowFile.Error())
+		return sce.Create(sce.ErrScorecardInternal, errInvalidGitHubWorkflowFile.Error())
 	}
 
 	if strings.EqualFold(val, "write") {
-		dl.Warn("'%v' permission set to '%v' in %v", key, val, path)
-		return false, nil
+		if isPermissionOfInterest(key) {
+			dl.Warn("'%v' permission set to '%v' in %v", key, val, path)
+			recordPermissionWrite(key, pdata)
+		} else {
+			// Only log for debugging, otherwise
+			// it may confuse users.
+			dl.Debug("'%v' permission set to '%v' in %v", key, val, path)
+		}
+		return nil
 	}
 
 	dl.Info("'%v' permission set to '%v' in %v", key, val, path)
-	return true, nil
+	return nil
 }
 
 func validateMapPermissions(values map[interface{}]interface{}, path string,
-	dl checker.DetailLogger) (bool, error) {
-	permissionRead := true
-	var r bool
-	var err error
-
+	dl checker.DetailLogger, pdata *permissionCbData) error {
 	// Iterate over the permission, verify keys and values are strings.
 	for k, v := range values {
 		key, ok := k.(string)
 		if !ok {
 			//nolint
-			return false, sce.Create(sce.ErrScorecardInternal, errInvalidGitHubWorkflowFile.Error())
+			return sce.Create(sce.ErrScorecardInternal, errInvalidGitHubWorkflowFile.Error())
 		}
 
-		if r, err = validatePermission(key, v, path, dl); err != nil {
-			return false, err
-		}
-
-		if !r {
-			permissionRead = false
+		if err := validatePermission(key, v, path, dl, pdata); err != nil {
+			return err
 		}
 	}
-	return permissionRead, nil
+	return nil
+}
+
+func recordPermissionWrite(name string, pdata *permissionCbData) {
+	pdata.writePermissions[name] = true
+}
+
+func recordAllPermissionsWrite(pdata *permissionCbData) {
+	// Special case: `all` does not correspond
+	// to a GitHub permission.
+	pdata.writePermissions["all"] = true
 }
 
 func validateReadPermissions(config map[interface{}]interface{}, path string,
-	dl checker.DetailLogger) (bool, error) {
+	dl checker.DetailLogger, pdata *permissionCbData) error {
 	var permissions interface{}
 
 	// Check if permissions are set explicitly.
 	permissions, ok := config["permissions"]
 	if !ok {
 		dl.Warn("no permission defined in %v", path)
-		return false, nil
+		recordAllPermissionsWrite(pdata)
+		return nil
 	}
 
 	// Check the type of our values.
@@ -94,67 +113,153 @@ func validateReadPermissions(config map[interface{}]interface{}, path string,
 	// Empty string is nil type.
 	// It defaults to 'none'
 	case nil:
-		dl.Info("permission set to 'none' in %v", path)
+		dl.Info("permissions set to 'none' in %v", path)
 	// String type.
 	case string:
 		if !strings.EqualFold(val, "read-all") && val != "" {
-			dl.Warn("permission set to '%v' in %v", val, path)
-			return false, nil
+			dl.Warn("permissions set to '%v' in %v", val, path)
+			recordAllPermissionsWrite(pdata)
+			return nil
 		}
 		dl.Info("permission set to '%v' in %v", val, path)
 
 	// Map type.
 	case map[interface{}]interface{}:
-		if res, err := validateMapPermissions(val, path, dl); err != nil {
-			return false, err
-		} else if !res {
-			return false, nil
+		if err := validateMapPermissions(val, path, dl, pdata); err != nil {
+			return err
 		}
 
 	// Invalid type.
 	default:
 		//nolint
-		return false, sce.Create(sce.ErrScorecardInternal, errInvalidGitHubWorkflowFile.Error())
+		return sce.Create(sce.ErrScorecardInternal, errInvalidGitHubWorkflowFile.Error())
 	}
 
-	return true, nil
+	return nil
+}
+
+func isPermissionOfInterest(name string) bool {
+	return strings.EqualFold(name, "statuses") ||
+		strings.EqualFold(name, "checks") ||
+		strings.EqualFold(name, "security-events") ||
+		strings.EqualFold(name, "deployments") ||
+		strings.EqualFold(name, "contents") ||
+		strings.EqualFold(name, "packages") ||
+		strings.EqualFold(name, "options")
+}
+
+// Calculate the score.
+func calculateScore(result permissionCbData) int {
+	// See list https://github.blog/changelog/2021-04-20-github-actions-control-permissions-for-github_token/.
+	// 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 _, ok := result.writePermissions["all"]; ok {
+		return checker.MinResultScore
+	}
+
+	score := float32(checker.MaxResultScore)
+	// 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 _, ok := result.writePermissions["statuses"]; ok {
+		score -= 0.5
+	}
+
+	// checks.
+	// May allow an attacker to edit checks to remove pre-submit and introduce a bug.
+	// Low risk: -0.5.
+	if _, ok := result.writePermissions["checks"]; ok {
+		score -= 0.5
+	}
+
+	// secEvents.
+	// May allow attacker to read vuln reports before patch available.
+	// Low risk: -1
+	if _, ok := result.writePermissions["security-events"]; ok {
+		score--
+	}
+
+	// deployments: https://docs.github.com/en/rest/reference/repos#deployments.
+	// May allow 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 var unsanitized.
+	// Low risk: -1
+	if _, ok := result.writePermissions["deployments"]; ok {
+		score--
+	}
+
+	// contents.
+	// Allows attacker to commit unreviewed code.
+	// High risk: -10
+	if _, ok := result.writePermissions["contents"]; ok {
+		score -= checker.MaxResultScore
+	}
+
+	// packages.
+	// Allows attacker to publish packages.
+	// High risk: -10
+	if _, ok := result.writePermissions["packages"]; ok {
+		score -= checker.MaxResultScore
+	}
+
+	// actions.
+	// May allow an attacker to steal GitHub secrets by adding a malicious workflow/action.
+	// High risk: -10
+	if _, ok := result.writePermissions["actions"]; ok {
+		score -= checker.MaxResultScore
+	}
+
+	if score < checker.MinResultScore {
+		return checker.MinResultScore
+	}
+
+	return int(score)
 }
 
 // Create the result.
-func createResultForLeastPrivilegeTokens(r bool, err error) checker.CheckResult {
+func createResultForLeastPrivilegeTokens(result permissionCbData, err error) checker.CheckResult {
 	if err != nil {
-		return checker.CreateRuntimeErrorResult(CheckPermissions, err)
-	}
-	if !r {
-		return checker.CreateMinScoreResult(CheckPermissions,
-			"non read-only tokens detected in GitHub workflows")
+		return checker.CreateRuntimeErrorResult(CheckTokenPermissions, err)
 	}
 
-	return checker.CreateMaxScoreResult(CheckPermissions,
+	score := calculateScore(result)
+
+	if score != checker.MaxResultScore {
+		return checker.CreateResultWithScore(CheckTokenPermissions,
+			"non read-only tokens detected in GitHub workflows", score)
+	}
+
+	return checker.CreateMaxScoreResult(CheckTokenPermissions,
 		"tokens are read-only in GitHub workflows")
 }
 
 func testValidateGitHubActionTokenPermissions(pathfn string,
 	content []byte, dl checker.DetailLogger) checker.CheckResult {
-	r, err := validateGitHubActionTokenPermissions(pathfn, content, dl)
-	return createResultForLeastPrivilegeTokens(r, err)
+	data := permissionCbData{writePermissions: make(map[string]bool)}
+	_, err := validateGitHubActionTokenPermissions(pathfn, content, dl, &data)
+	return createResultForLeastPrivilegeTokens(data, err)
 }
 
 // Check file content.
 func validateGitHubActionTokenPermissions(path string, content []byte,
-	dl checker.DetailLogger) (bool, error) {
-	if len(content) == 0 {
-		//nolint
-		return false, sce.Create(sce.ErrScorecardInternal, errInternalEmptyFile.Error())
+	dl checker.DetailLogger, data FileCbData) (bool, error) {
+	// Verify the type of the data.
+	pdata, ok := data.(*permissionCbData)
+	if !ok {
+		// This never happens.
+		panic("invalid type")
+	}
+
+	if !CheckFileContainsCommands(content, "#") {
+		return true, nil
 	}
 
 	var workflow map[interface{}]interface{}
-	var r bool
-	var err error
-	err = yaml.Unmarshal(content, &workflow)
+	err := yaml.Unmarshal(content, &workflow)
 	if err != nil {
 		//nolint
-		return false, sce.Create(sce.ErrScorecardInternal, fmt.Sprintf("yaml.Unmarshal: %v", err))
+		return false,
+			sce.Create(sce.ErrScorecardInternal, fmt.Sprintf("yaml.Unmarshal: %v", err))
 	}
 
 	// 1. Check that each file uses 'content: read' only or 'none'.
@@ -162,11 +267,8 @@ func validateGitHubActionTokenPermissions(path string, content []byte,
 	// https://docs.github.com/en/actions/reference/authentication-in-a-workflow#example-1-passing-the-github_token-as-an-input,
 	// https://github.blog/changelog/2021-04-20-github-actions-control-permissions-for-github_token/,
 	// https://docs.github.com/en/actions/reference/authentication-in-a-workflow#modifying-the-permissions-for-the-github_token.
-	if r, err = validateReadPermissions(workflow, path, dl); err != nil {
-		return false, nil
-	}
-	if !r {
-		return r, nil
+	if err := validateReadPermissions(workflow, path, dl, pdata); err != nil {
+		return false, err
 	}
 
 	// TODO(laurent): 2. Identify github actions that require write and add checks.

checks/permissions_test.go

@@ -70,10 +70,10 @@ func TestGithubTokenPermissions(t *testing.T) {
 			filename: "./testdata/github-workflow-permissions-writes.yaml",
 			expected: scut.TestReturn{
 				Errors:        nil,
-				Score:         checker.MinResultScore,
-				NumberOfWarn:  1,
+				Score:         checker.MaxResultScore,
+				NumberOfWarn:  0,
 				NumberOfInfo:  1,
-				NumberOfDebug: 0,
+				NumberOfDebug: 1,
 			},
 		},
 		{
@@ -109,6 +109,61 @@ func TestGithubTokenPermissions(t *testing.T) {
 				NumberOfDebug: 0,
 			},
 		},
+		{
+			name:     "status/checks write",
+			filename: "./testdata/github-workflow-permissions-status-checks.yaml",
+			expected: scut.TestReturn{
+				Errors:        nil,
+				Score:         checker.MaxResultScore - 1,
+				NumberOfWarn:  2,
+				NumberOfInfo:  2,
+				NumberOfDebug: 1,
+			},
+		},
+		{
+			name:     "sec-events/deployments write",
+			filename: "./testdata/github-workflow-permissions-secevent-deployments.yaml",
+			expected: scut.TestReturn{
+				Errors:        nil,
+				Score:         checker.MaxResultScore - 2,
+				NumberOfWarn:  2,
+				NumberOfInfo:  3,
+				NumberOfDebug: 0,
+			},
+		},
+		{
+			name:     "contents write",
+			filename: "./testdata/github-workflow-permissions-contents.yaml",
+			expected: scut.TestReturn{
+				Errors:        nil,
+				Score:         checker.MinResultScore,
+				NumberOfWarn:  1,
+				NumberOfInfo:  2,
+				NumberOfDebug: 0,
+			},
+		},
+		{
+			name:     "actions write",
+			filename: "./testdata/github-workflow-permissions-actions.yaml",
+			expected: scut.TestReturn{
+				Errors:        nil,
+				Score:         checker.MinResultScore,
+				NumberOfWarn:  1,
+				NumberOfInfo:  2,
+				NumberOfDebug: 0,
+			},
+		},
+		{
+			name:     "packages write",
+			filename: "./testdata/github-workflow-permissions-packages.yaml",
+			expected: scut.TestReturn{
+				Errors:        nil,
+				Score:         checker.MinResultScore,
+				NumberOfWarn:  1,
+				NumberOfInfo:  1,
+				NumberOfDebug: 0,
+			},
+		},
 	}
 	for _, tt := range tests {
 		tt := tt // Re-initializing variable so it is not changed while executing the closure below

checks/testdata/github-workflow-permissions-actions.yaml

@@ -0,0 +1,25 @@
+# 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]
+permissions:
+  contents: write
+  packages: none
+  actions: read
+
+jobs:
+  Explore-GitHub-Actions:
+    runs-on: ubuntu-latest
+    steps:
+      - run: echo "write-and-read workflow"

checks/testdata/github-workflow-permissions-contents.yaml

@@ -0,0 +1,25 @@
+# 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]
+permissions:
+  contents: write
+  packages: none
+  actions: read
+
+jobs:
+  Explore-GitHub-Actions:
+    runs-on: ubuntu-latest
+    steps:
+      - run: echo "write-and-read workflow"

checks/testdata/github-workflow-permissions-packages.yaml

@@ -0,0 +1,24 @@
+# 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]
+permissions:
+  contents: write
+  actions: read
+
+jobs:
+  Explore-GitHub-Actions:
+    runs-on: ubuntu-latest
+    steps:
+      - run: echo "write-and-read workflow"

checks/testdata/github-workflow-permissions-secevent-deployments.yaml

@@ -0,0 +1,27 @@
+# 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]
+permissions:
+  security-events: write
+  pull-requests: read
+  deployments: write
+  packages: none
+  actions: none
+
+jobs:
+  Explore-GitHub-Actions:
+    runs-on: ubuntu-latest
+    steps:
+      - run: echo "write-and-read workflow"

checks/testdata/github-workflow-permissions-status-checks.yaml

@@ -0,0 +1,27 @@
+# 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]
+permissions:
+  statuses: write
+  contents: read
+  checks: write
+  packages: none
+  issues: write
+
+jobs:
+  Explore-GitHub-Actions:
+    runs-on: ubuntu-latest
+    steps:
+      - run: echo "write-and-read workflow"

e2e/permissions_test.go

@@ -0,0 +1,63 @@
+// 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.
+//nolint:dupl
+package e2e
+
+import (
+	"context"
+
+	. "github.com/onsi/ginkgo"
+	. "github.com/onsi/gomega"
+
+	"github.com/ossf/scorecard/v2/checker"
+	"github.com/ossf/scorecard/v2/checks"
+	"github.com/ossf/scorecard/v2/clients/githubrepo"
+	scut "github.com/ossf/scorecard/v2/utests"
+)
+
+var _ = Describe("E2E TEST:"+checks.CheckTokenPermissions, func() {
+	Context("E2E TEST:Validating token permission check", func() {
+		It("Should return token permission works", func() {
+			dl := scut.TestDetailLogger{}
+			repoClient := githubrepo.CreateGithubRepoClient(context.Background(), ghClient, graphClient)
+			err := repoClient.InitRepo("ossf-tests", "scorecard-check-token-permissions-e2e")
+			Expect(err).Should(BeNil())
+			req := checker.CheckRequest{
+				Ctx:         context.Background(),
+				Client:      ghClient,
+				HTTPClient:  httpClient,
+				RepoClient:  repoClient,
+				Owner:       "ossf-tests",
+				Repo:        "scorecard-check-token-permissions-e2e",
+				GraphClient: graphClient,
+				Dlogger:     &dl,
+			}
+			expected := scut.TestReturn{
+				Errors:        nil,
+				Score:         checker.MinResultScore,
+				NumberOfWarn:  1,
+				NumberOfInfo:  2,
+				NumberOfDebug: 0,
+			}
+			result := checks.TokenPermissions(&req)
+			// UPGRADEv2: to remove.
+			// Old version.
+
+			Expect(result.Error).Should(BeNil())
+			Expect(result.Pass).Should(BeFalse())
+			// New version.
+			Expect(scut.ValidateTestReturn(nil, "token permissions", &expected, &result, &dl)).Should(BeTrue())
+		})
+	})
+})