✨ Structured results for permissions (#2584)

* update Signed-off-by: laurentsimon <[email protected]> * update Signed-off-by: laurentsimon <[email protected]> * update Signed-off-by: laurentsimon <[email protected]> * update Signed-off-by: laurentsimon <[email protected]> * update Signed-off-by: laurentsimon <[email protected]> * update Signed-off-by: laurentsimon <[email protected]> * update Signed-off-by: laurentsimon <[email protected]> * update Signed-off-by: laurentsimon <[email protected]> * update Signed-off-by: laurentsimon <[email protected]> * update Signed-off-by: laurentsimon <[email protected]> * update Signed-off-by: laurentsimon <[email protected]> * update Signed-off-by: laurentsimon <[email protected]> * update Signed-off-by: laurentsimon <[email protected]> * update Signed-off-by: laurentsimon <[email protected]> * Update checks/evaluation/permissions/GitHubWorkflowPermissionsTopNoWrite.yml Co-authored-by: Joyce <[email protected]> Signed-off-by: laurentsimon <[email protected]> * update Signed-off-by: laurentsimon <[email protected]> * Update checks/evaluation/permissions/GitHubWorkflowPermissionsStepsNoWrite.yml Co-authored-by: Joyce <[email protected]> Signed-off-by: laurentsimon <[email protected]> * Update checks/evaluation/permissions/GitHubWorkflowPermissionsStepsNoWrite.yml Co-authored-by: Joyce <[email protected]> Signed-off-by: laurentsimon <[email protected]> * Update checks/evaluation/permissions/GitHubWorkflowPermissionsStepsNoWrite.yml Co-authored-by: Joyce <[email protected]> Signed-off-by: laurentsimon <[email protected]> * update Signed-off-by: laurentsimon <[email protected]> * update Signed-off-by: laurentsimon <[email protected]> * update Signed-off-by: laurentsimon <[email protected]> * update Signed-off-by: laurentsimon <[email protected]> * update Signed-off-by: laurentsimon <[email protected]> * update Signed-off-by: laurentsimon <[email protected]> * update Signed-off-by: laurentsimon <[email protected]> * update Signed-off-by: laurentsimon <[email protected]> * update Signed-off-by: laurentsimon <[email protected]> * update Signed-off-by: laurentsimon <[email protected]> * update Signed-off-by: laurentsimon <[email protected]> * update Signed-off-by: laurentsimon <[email protected]> * update Signed-off-by: laurentsimon <[email protected]> * update Signed-off-by: laurentsimon <[email protected]> * update Signed-off-by: laurentsimon <[email protected]> * update Signed-off-by: laurentsimon <[email protected]> --------- Signed-off-by: laurentsimon <[email protected]> Signed-off-by: laurentsimon <[email protected]> Co-authored-by: Joyce <[email protected]>
ossf · Jan 31, 2023 · 2ea140a · 2ea140a
1 parent 4ebe521
commit 2ea140a
Show file tree

Hide file tree

Showing 55 changed files with 1,563 additions and 330 deletions.
diff --git a/attestor/policy/attestation_policy.go b/attestor/policy/attestation_policy.go
@@ -25,6 +25,7 @@ import (
 	"github.com/ossf/scorecard/v4/checker"
 	"github.com/ossf/scorecard/v4/checks"
 	sce "github.com/ossf/scorecard/v4/errors"
+	"github.com/ossf/scorecard/v4/finding"
 	sclog "github.com/ossf/scorecard/v4/log"
 )
 
@@ -170,7 +171,7 @@ func CheckPreventBinaryArtifacts(
 			logger.Info(
 				fmt.Sprintf(
 					"binary detected path:%s type: %v offset:%v",
-					artifactFile.Path, checker.FileTypeBinary, artifactFile.Offset,
+					artifactFile.Path, finding.FileTypeBinary, artifactFile.Offset,
 				),
 			)
 			return Fail, nil

diff --git a/checker/check_result.go b/checker/check_result.go
@@ -18,13 +18,14 @@ package checker
 import (
 	"fmt"
 	"math"
+
+	"github.com/ossf/scorecard/v4/finding"
+	"github.com/ossf/scorecard/v4/rule"
 )
 
 type (
 	// DetailType is the type of details.
 	DetailType int
-	// FileType is the type of a file.
-	FileType int
 )
 
 const (
@@ -49,42 +50,18 @@ const (
 	DetailDebug
 )
 
-const (
-	// FileTypeNone is a default, not defined.
-	// FileTypeNone must be `0`.
-	FileTypeNone FileType = iota
-	// FileTypeSource is for source code files.
-	FileTypeSource
-	// FileTypeBinary is for binary files.
-	FileTypeBinary
-	// FileTypeText is for text files.
-	FileTypeText
-	// FileTypeURL for URLs.
-	FileTypeURL
-)
-
 // CheckResult captures result from a check run.
 //
 //nolint:govet
 type CheckResult struct {
 	Name    string
 	Version int
 	Error   error
-	Details []CheckDetail
 	Score   int
 	Reason  string
-}
-
-// Remediation represents a remediation.
-type Remediation struct {
-	// Code snippet for humans.
-	Snippet string
-	// Diff for machines.
-	Diff string
-	// Help text for humans.
-	HelpText string
-	// Help text in markdown format for humans.
-	HelpMarkdown string
+	Details []CheckDetail
+	// Structured results.
+	Rules []string // TODO(X): add support.
 }
 
 // CheckDetail contains information for each detail.
@@ -98,13 +75,17 @@ type CheckDetail struct {
 //
 //nolint:govet
 type LogMessage struct {
-	Text        string       // A short string explaining why the detail was recorded/logged.
-	Path        string       // Fullpath to the file.
-	Type        FileType     // Type of file.
-	Offset      uint         // Offset in the file of Path (line for source/text files).
-	EndOffset   uint         // End of offset in the file, e.g. if the command spans multiple lines.
-	Snippet     string       // Snippet of code
-	Remediation *Remediation // Remediation information, if any.
+	// Structured resuts.
+	Finding *finding.Finding
+
+	// Non-structured results.
+	Text        string            // A short string explaining why the detail was recorded/logged.
+	Path        string            // Fullpath to the file.
+	Type        finding.FileType  // Type of file.
+	Offset      uint              // Offset in the file of Path (line for source/text files).
+	EndOffset   uint              // End of offset in the file, e.g. if the command spans multiple lines.
+	Snippet     string            // Snippet of code
+	Remediation *rule.Remediation // Remediation information, if any.
 }
 
 // CreateProportionalScore creates a proportional score.

diff --git a/checker/raw_result.go b/checker/raw_result.go
@@ -18,6 +18,7 @@ import (
 	"time"
 
 	"github.com/ossf/scorecard/v4/clients"
+	"github.com/ossf/scorecard/v4/finding"
 )
 
 // RawResults contains results before a policy
@@ -286,11 +287,11 @@ type ArchivedStatus struct {
 // File represents a file.
 type File struct {
 	Path      string
-	Snippet   string   // Snippet of code
-	Offset    uint     // Offset in the file of Path (line for source/text files).
-	EndOffset uint     // End of offset in the file, e.g. if the command spans multiple lines.
-	FileSize  uint     // Total size of file.
-	Type      FileType // Type of file.
+	Snippet   string           // Snippet of code
+	Offset    uint             // Offset in the file of Path (line for source/text files).
+	EndOffset uint             // End of offset in the file, e.g. if the command spans multiple lines.
+	FileSize  uint             // Total size of file.
+	Type      finding.FileType // Type of file.
 	// TODO: add hash.
 }
 

diff --git a/checks/evaluation/binary_artifacts.go b/checks/evaluation/binary_artifacts.go
@@ -17,6 +17,7 @@ package evaluation
 import (
 	"github.com/ossf/scorecard/v4/checker"
 	sce "github.com/ossf/scorecard/v4/errors"
+	"github.com/ossf/scorecard/v4/finding"
 )
 
 // BinaryArtifacts applies the score policy for the Binary-Artifacts check.
@@ -36,7 +37,7 @@ func BinaryArtifacts(name string, dl checker.DetailLogger,
 	score := checker.MaxResultScore
 	for _, f := range r.Files {
 		dl.Warn(&checker.LogMessage{
-			Path: f.Path, Type: checker.FileTypeBinary,
+			Path: f.Path, Type: finding.FileTypeBinary,
 			Offset: f.Offset,
 			Text:   "binary detected",
 		})

diff --git a/checks/evaluation/ci_tests.go b/checks/evaluation/ci_tests.go
@@ -19,6 +19,7 @@ import (
 	"strings"
 
 	"github.com/ossf/scorecard/v4/checker"
+	"github.com/ossf/scorecard/v4/finding"
 )
 
 const (
@@ -85,7 +86,7 @@ func prHasSuccessStatus(r checker.RevisionCIInfo, dl checker.DetailLogger) (bool
 		if isTest(status.Context) || isTest(status.TargetURL) {
 			dl.Debug(&checker.LogMessage{
 				Path: status.URL,
-				Type: checker.FileTypeURL,
+				Type: finding.FileTypeURL,
 				Text: fmt.Sprintf("CI test found: pr: %s, context: %s", r.HeadSHA,
 					status.Context),
 			})
@@ -109,7 +110,7 @@ func prHasSuccessfulCheck(r checker.RevisionCIInfo, dl checker.DetailLogger) (bo
 		if isTest(cr.App.Slug) {
 			dl.Debug(&checker.LogMessage{
 				Path: cr.URL,
-				Type: checker.FileTypeURL,
+				Type: finding.FileTypeURL,
 				Text: fmt.Sprintf("CI test found: pr: %d, context: %s", r.PullRequestNumber,
 					cr.App.Slug),
 			})

diff --git a/checks/evaluation/dependency_update_tool_test.go b/checks/evaluation/dependency_update_tool_test.go
@@ -19,6 +19,7 @@ import (
 
 	"github.com/ossf/scorecard/v4/checker"
 	sce "github.com/ossf/scorecard/v4/errors"
+	"github.com/ossf/scorecard/v4/finding"
 	scut "github.com/ossf/scorecard/v4/utests"
 )
 
@@ -95,7 +96,7 @@ func TestDependencyUpdateTool(t *testing.T) {
 										[dependency-update-tool]
 										enabled = true
 										`,
-									Type: checker.FileTypeSource,
+									Type: finding.FileTypeSource,
 								},
 							},
 						},

diff --git a/checks/evaluation/license.go b/checks/evaluation/license.go
@@ -17,6 +17,7 @@ package evaluation
 import (
 	"github.com/ossf/scorecard/v4/checker"
 	sce "github.com/ossf/scorecard/v4/errors"
+	"github.com/ossf/scorecard/v4/finding"
 )
 
 func scoreLicenseCriteria(f *checker.LicenseFile,
@@ -25,12 +26,12 @@ func scoreLicenseCriteria(f *checker.LicenseFile,
 	var score int
 	msg := checker.LogMessage{
 		Path:   "",
-		Type:   checker.FileTypeNone,
+		Type:   finding.FileTypeNone,
 		Text:   "",
 		Offset: 1,
 	}
 	msg.Path = f.File.Path
-	msg.Type = checker.FileTypeSource
+	msg.Type = finding.FileTypeSource
 	// #1 a license file was found.
 	score += 6
 

diff --git a/checks/evaluation/permissions/GitHubWorkflowPermissionsStepsNoWrite.yml b/checks/evaluation/permissions/GitHubWorkflowPermissionsStepsNoWrite.yml
@@ -0,0 +1,33 @@
+# Copyright 2023 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.
+
+short: Checks that GitHub workflows do not have steps with dangerous write permissions
+desc: This rule checks that GitHub workflows do not have steps with dangerous write permissions
+motivation: >
+  Even with permissions default set to read, some scopes having write permissions in their steps brings incurs a risk to the project.
+  By giving write permission to the Actions you call in jobs, an external Action you call could abuse them. Depending on the permissions,
+  this could let the external Action commit unreviewed code, remove pre-submit checks to introduce a bug.
+  For more information about the scopes and the vulnerabilities involved, see https://github.com/ossf/scorecard/blob/main/docs/checks.md#token-permissions.
+
+implementation: >
+  The rule is implemented by checking whether the `permissions` keyword is given non-write permissions for the following
+  scopes: `statuses`, `checks`, `security-events`, `deployments`, `contents`, `packages`, `actions`.
+  Write permissions given to recognized packaging actions or commands are allowed and are considered an acceptable risk.
+risk: Medium
+remediation:
+  effort: High
+  text:
+    - Verify which permissions are needed and consider whether you can reduce them.
+  markdown:
+    - Verify which permissions are needed and consider whether you can reduce them.
diff --git a/checks/evaluation/permissions/GitHubWorkflowPermissionsTopNoWrite.yml b/checks/evaluation/permissions/GitHubWorkflowPermissionsTopNoWrite.yml
@@ -0,0 +1,36 @@
+# Copyright 2023 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.
+
+short: Checks that GitHub workflows do not have default write permissions
+desc: This rule checks that GitHub workflows do not have default write permissions
+motivation: >
+  If no permissions are declared, a workflow's GitHub token's permissions default to write for all scopes.
+  This include write permissions to push to the repository, to read encrypted secrets, etc.
+  For more information, see https://docs.github.com/en/actions/security-guides/automatic-token-authentication#permissions-for-the-github_token. 
+implementation: >
+  The rule is implemented by checking whether the `permissions` keyword is defined at the top of the workflow,
+  and that no write permissions are given.
+risk: High
+remediation:
+  effort: Low
+  text:
+    - Visit https://app.stepsecurity.io/secureworkflow/${{ repo }}/${{ workflow }}/${{ branch }}?enable=permissions
+    - Tick the 'Restrict permissions for GITHUB_TOKEN'
+    - Untick other options
+    - "NOTE: If you want to resolve multiple issues at once, you can visit https://app.stepsecurity.io/securerepo instead."
+  markdown:
+    - Visit [https://app.stepsecurity.io/secureworkflow](https://app.stepsecurity.io/secureworkflow/${{ repo }}/${{ workflow }}/${{ branch }}?enable=permissions).
+    - Tick the 'Restrict permissions for GITHUB_TOKEN'
+    - Untick other options
+    - "NOTE: If you want to resolve multiple issues at once, you can visit [https://app.stepsecurity.io/securerepo](https://app.stepsecurity.io/securerepo) instead."