From c48fe4f9edfabf31244472bd6e6371b8e74c9fa1 Mon Sep 17 00:00:00 2001 From: laurentsimon <64505099+laurentsimon@users.noreply.github.com> Date: Thu, 29 Jul 2021 17:13:01 -0700 Subject: [PATCH] =?UTF-8?q?=E2=9C=A8=20=20Make=20Token-Permission=20check?= =?UTF-8?q?=20more=20granular=20(#773)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * draft * add tests * add e2e2 tests * typos * typo * fixes * linter * use named value * comments * comment --- checks/file_utils.go | 55 +++++ checks/permissions.go | 214 +++++++++++++----- checks/permissions_test.go | 61 ++++- .../github-workflow-permissions-actions.yaml | 25 ++ .../github-workflow-permissions-contents.yaml | 25 ++ .../github-workflow-permissions-packages.yaml | 24 ++ ...flow-permissions-secevent-deployments.yaml | 27 +++ ...ub-workflow-permissions-status-checks.yaml | 27 +++ e2e/permissions_test.go | 63 ++++++ 9 files changed, 462 insertions(+), 59 deletions(-) create mode 100644 checks/testdata/github-workflow-permissions-actions.yaml create mode 100644 checks/testdata/github-workflow-permissions-contents.yaml create mode 100644 checks/testdata/github-workflow-permissions-packages.yaml create mode 100644 checks/testdata/github-workflow-permissions-secevent-deployments.yaml create mode 100644 checks/testdata/github-workflow-permissions-status-checks.yaml create mode 100644 e2e/permissions_test.go diff --git a/checks/file_utils.go b/checks/file_utils.go index c4d540bf..522b40e5 100644 --- a/checks/file_utils.go +++ b/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 `#`. diff --git a/checks/permissions.go b/checks/permissions.go index a87ad7fa..b65ff9ca 100644 --- a/checks/permissions.go +++ b/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. diff --git a/checks/permissions_test.go b/checks/permissions_test.go index 34081157..5d148b8e 100644 --- a/checks/permissions_test.go +++ b/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 diff --git a/checks/testdata/github-workflow-permissions-actions.yaml b/checks/testdata/github-workflow-permissions-actions.yaml new file mode 100644 index 00000000..6703492e --- /dev/null +++ b/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" \ No newline at end of file diff --git a/checks/testdata/github-workflow-permissions-contents.yaml b/checks/testdata/github-workflow-permissions-contents.yaml new file mode 100644 index 00000000..6703492e --- /dev/null +++ b/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" \ No newline at end of file diff --git a/checks/testdata/github-workflow-permissions-packages.yaml b/checks/testdata/github-workflow-permissions-packages.yaml new file mode 100644 index 00000000..cf1e5507 --- /dev/null +++ b/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" \ No newline at end of file diff --git a/checks/testdata/github-workflow-permissions-secevent-deployments.yaml b/checks/testdata/github-workflow-permissions-secevent-deployments.yaml new file mode 100644 index 00000000..33de433a --- /dev/null +++ b/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" \ No newline at end of file diff --git a/checks/testdata/github-workflow-permissions-status-checks.yaml b/checks/testdata/github-workflow-permissions-status-checks.yaml new file mode 100644 index 00000000..1b199f3f --- /dev/null +++ b/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" \ No newline at end of file diff --git a/e2e/permissions_test.go b/e2e/permissions_test.go new file mode 100644 index 00000000..61715438 --- /dev/null +++ b/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()) + }) + }) +})