diff --git a/checks/errors.go b/checks/errors.go index d84aabeff8b..8d7085c05f8 100644 --- a/checks/errors.go +++ b/checks/errors.go @@ -20,13 +20,14 @@ import ( //nolint var ( - errInternalInvalidDockerFile = errors.New("invalid Dockerfile") - errInternalInvalidYamlFile = errors.New("invalid yaml file") - errInternalFilenameMatch = errors.New("filename match error") - errInternalEmptyFile = errors.New("empty file") - errInternalCommitishNil = errors.New("commitish is nil") - errInternalBranchNotFound = errors.New("branch not found") - errInvalidGitHubWorkflow = errors.New("invalid GitHub workflow") - errInternalNoReviews = errors.New("no reviews found") - errInternalNoCommits = errors.New("no commits found") + errInternalInvalidDockerFile = errors.New("invalid Dockerfile") + errInternalInvalidYamlFile = errors.New("invalid yaml file") + errInternalFilenameMatch = errors.New("filename match error") + errInternalEmptyFile = errors.New("empty file") + errInternalCommitishNil = errors.New("commitish is nil") + errInternalBranchNotFound = errors.New("branch not found") + errInvalidGitHubWorkflow = errors.New("invalid GitHub workflow") + errInternalNoReviews = errors.New("no reviews found") + errInternalNoCommits = errors.New("no commits found") + errInternalInvalidPermissions = errors.New("invalid permissions") ) diff --git a/checks/permissions.go b/checks/permissions.go index d79af779e6e..f57a5813167 100644 --- a/checks/permissions.go +++ b/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, - dl checker.DetailLogger, pPermissions map[string]bool, +func validatePermission(permissionKey string, permissionValue *actionlint.PermissionScope, + 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] - _, ok2 := result.runLevelWritePermissions[name] - return ok1 || ok2 + return permissionIsPresentInTopLevel(result, name) || + permissionIsPresentInRunLevel(result, name) +} + +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. diff --git a/checks/permissions_test.go b/checks/permissions_test.go index 1b08c0fc189..20124328497 100644 --- a/checks/permissions_test.go +++ b/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 diff --git a/checks/testdata/github-workflow-permissions-jobs-only.yaml b/checks/testdata/github-workflow-permissions-jobs-only.yaml new file mode 100644 index 00000000000..2bf1fd06b14 --- /dev/null +++ b/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" \ No newline at end of file diff --git a/docs/checks.md b/docs/checks.md index 7efe9609c9f..f4302775790 100644 --- a/docs/checks.md +++ b/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. diff --git a/docs/checks/internal/checks.yaml b/docs/checks/internal/checks.yaml index 77df1ac95bc..0cb3585d7b2 100644 --- a/docs/checks/internal/checks.yaml +++ b/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.