✨ [experimental] Create probes within findings (ossf#2919)

* 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]>
raghavkaul · May 3, 2023 · 4f93a00 · 4f93a00
1 parent 1cd4063
commit 4f93a00
Show file tree

Hide file tree

Showing 16 changed files with 545 additions and 132 deletions.
diff --git a/...GitHubWorkflowPermissionsStepsNoWrite.yml → ...gitHubWorkflowPermissionsStepsNoWrite.yml b/...GitHubWorkflowPermissionsStepsNoWrite.yml → ...gitHubWorkflowPermissionsStepsNoWrite.yml
@@ -12,19 +12,18 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 
+id: gitHubWorkflowPermissionsStepsNoWrite
 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
+  The probe 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:

diff --git a/...s/GitHubWorkflowPermissionsTopNoWrite.yml → ...s/gitHubWorkflowPermissionsTopNoWrite.yml b/...s/GitHubWorkflowPermissionsTopNoWrite.yml → ...s/gitHubWorkflowPermissionsTopNoWrite.yml
@@ -12,25 +12,24 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 
+id: gitHubWorkflowPermissionsTopNoWrite
 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
+    - Visit https://app.stepsecurity.io/secureworkflow/${{ metadata.repo }}/${{ metadata.workflow }}/${{ metadata.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).
+    - Visit [https://app.stepsecurity.io/secureworkflow](https://app.stepsecurity.io/secureworkflow/${{ metadata.repo }}/${{ metadata.workflow }}/${{ metadata.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."
diff --git a/checks/evaluation/permissions/permissions.go b/checks/evaluation/permissions/permissions.go
@@ -26,13 +26,18 @@ import (
 )
 
 //go:embed *.yml
-var rules embed.FS
+var probes embed.FS
 
 type permissions struct {
 	topLevelWritePermissions map[string]bool
 	jobLevelWritePermissions map[string]bool
 }
 
+var (
+	stepsNoWriteID = "gitHubWorkflowPermissionsStepsNoWrite"
+	topNoWriteID   = "gitHubWorkflowPermissionsTopNoWrite"
+)
+
 // TokenPermissions applies the score policy for the Token-Permissions check.
 func TokenPermissions(name string, c *checker.CheckRequest, r *checker.TokenPermissionsData) checker.CheckResult {
 	if r == nil {
@@ -67,24 +72,22 @@ func applyScorePolicy(results *checker.TokenPermissionsData, c *checker.CheckReq
 	dl := c.Dlogger
 	//nolint:errcheck
 	remediationMetadata, _ := remediation.New(c)
-	negativeRuleResults := map[string]bool{
-		"GitHubWorkflowPermissionsStepsNoWrite": false,
-		"GitHubWorkflowPermissionsTopNoWrite":   false,
+	negativeProbeResults := map[string]bool{
+		stepsNoWriteID: false,
+		topNoWriteID:   false,
 	}
 
 	for _, r := range results.TokenPermissions {
 		var loc *finding.Location
 		if r.File != nil {
 			loc = &finding.Location{
 				Type:      r.File.Type,
-				Value:     r.File.Path,
+				Path:      r.File.Path,
 				LineStart: &r.File.Offset,
 			}
 			if r.File.Snippet != "" {
 				loc.Snippet = &r.File.Snippet
 			}
-
-			loc.Value = r.File.Path
 		}
 
 		text, err := createText(r)
@@ -112,7 +115,7 @@ func applyScorePolicy(results *checker.TokenPermissionsData, c *checker.CheckReq
 
 			// We warn only for top-level.
 			if *r.LocationType == checker.PermissionLocationTop {
-				warnWithRemediation(dl, msg, remediationMetadata, loc, negativeRuleResults)
+				warnWithRemediation(dl, msg, remediationMetadata, loc, negativeProbeResults)
 			} else {
 				dl.Debug(msg)
 			}
@@ -123,7 +126,7 @@ func applyScorePolicy(results *checker.TokenPermissionsData, c *checker.CheckReq
 			}
 
 		case checker.PermissionLevelWrite:
-			warnWithRemediation(dl, msg, remediationMetadata, loc, negativeRuleResults)
+			warnWithRemediation(dl, msg, remediationMetadata, loc, negativeProbeResults)
 
 			// Group results by workflow name for score computation.
 			if err := updateWorkflowHashMap(hm, r); err != nil {
@@ -132,38 +135,39 @@ func applyScorePolicy(results *checker.TokenPermissionsData, c *checker.CheckReq
 		}
 	}
 
-	if err := reportDefaultFindings(results, c.Dlogger, negativeRuleResults); err != nil {
+	if err := reportDefaultFindings(results, c.Dlogger, negativeProbeResults); err != nil {
 		return checker.InconclusiveResultScore, err
 	}
 
 	return calculateScore(hm), nil
 }
 
 func reportDefaultFindings(results *checker.TokenPermissionsData,
-	dl checker.DetailLogger, negativeRuleResults map[string]bool,
+	dl checker.DetailLogger, negativeProbeResults map[string]bool,
 ) error {
+	// TODO(#2928): re-visit the need for NotApplicable outcome.
 	// No workflow files exist.
 	if len(results.TokenPermissions) == 0 {
 		text := "no workflows found in the repository"
-		if err := reportFinding("GitHubWorkflowPermissionsStepsNoWrite",
-			text, finding.OutcomeNotApplicable, dl); err != nil {
+		if err := reportFinding(stepsNoWriteID,
+			text, finding.OutcomeNotAvailable, dl); err != nil {
 			return err
 		}
-		if err := reportFinding("GitHubWorkflowPermissionsTopNoWrite",
-			text, finding.OutcomeNotApplicable, dl); err != nil {
+		if err := reportFinding(topNoWriteID,
+			text, finding.OutcomeNotAvailable, dl); err != nil {
 			return err
 		}
 		return nil
 	}
 
 	// Workflow files found, report positive findings if no
 	// negative findings were found.
-	// NOTE: we don't consider rule `GitHubWorkflowPermissionsTopNoWrite`
+	// NOTE: we don't consider probe `topNoWriteID`
 	// because positive results are already reported.
-	found := negativeRuleResults["GitHubWorkflowPermissionsStepsNoWrite"]
+	found := negativeProbeResults[stepsNoWriteID]
 	if !found {
 		text := fmt.Sprintf("no %s write permissions found", checker.PermissionLocationJob)
-		if err := reportFinding("GitHubWorkflowPermissionsStepsNoWrite",
+		if err := reportFinding(stepsNoWriteID,
 			text, finding.OutcomePositive, dl); err != nil {
 			return err
 		}
@@ -172,8 +176,12 @@ func reportDefaultFindings(results *checker.TokenPermissionsData,
 	return nil
 }
 
-func reportFinding(rule, text string, o finding.Outcome, dl checker.DetailLogger) error {
-	f, err := finding.New(rules, rule)
+func reportFinding(probe, text string, o finding.Outcome, dl checker.DetailLogger) error {
+	content, err := probes.ReadFile(probe + ".yml")
+	if err != nil {
+		return fmt.Errorf("%w", err)
+	}
+	f, err := finding.FromBytes(content, probe)
 	if err != nil {
 		return sce.WithMessage(sce.ErrScorecardInternal, err.Error())
 	}
@@ -185,11 +193,15 @@ func reportFinding(rule, text string, o finding.Outcome, dl checker.DetailLogger
 }
 
 func createLogMsg(loct *checker.PermissionLocation) (*checker.LogMessage, error) {
-	Rule := "GitHubWorkflowPermissionsStepsNoWrite"
+	probe := stepsNoWriteID
 	if loct == nil || *loct == checker.PermissionLocationTop {
-		Rule = "GitHubWorkflowPermissionsTopNoWrite"
+		probe = topNoWriteID
+	}
+	content, err := probes.ReadFile(probe + ".yml")
+	if err != nil {
+		return nil, fmt.Errorf("%w", err)
 	}
-	f, err := finding.New(rules, Rule)
+	f, err := finding.FromBytes(content, probe)
 	if err != nil {
 		return nil,
 			sce.WithMessage(sce.ErrScorecardInternal, err.Error())
@@ -201,19 +213,19 @@ func createLogMsg(loct *checker.PermissionLocation) (*checker.LogMessage, error)
 
 func warnWithRemediation(logger checker.DetailLogger, msg *checker.LogMessage,
 	rem *remediation.RemediationMetadata, loc *finding.Location,
-	negativeRuleResults map[string]bool,
+	negativeProbeResults map[string]bool,
 ) {
-	if loc != nil && loc.Value != "" {
+	if loc != nil && loc.Path != "" {
 		msg.Finding = msg.Finding.WithRemediationMetadata(map[string]string{
 			"repo":     rem.Repo,
 			"branch":   rem.Branch,
-			"workflow": strings.TrimPrefix(loc.Value, ".github/workflows/"),
+			"workflow": strings.TrimPrefix(loc.Path, ".github/workflows/"),
 		})
 	}
 	logger.Warn(msg)
 
 	// Record that we found a negative result.
-	negativeRuleResults[msg.Finding.Rule] = true
+	negativeProbeResults[msg.Finding.Probe] = true
 }
 
 func recordPermissionWrite(hm map[string]permissions, path string,

diff --git a/checks/permissions_test.go b/checks/permissions_test.go
@@ -319,8 +319,8 @@ func TestGithubTokenPermissions(t *testing.T) {
 				Error:         nil,
 				Score:         checker.MaxResultScore,
 				NumberOfWarn:  0,
-				NumberOfInfo:  2,  // This is constant.
-				NumberOfDebug: 8,  // This is 4 + (number of actions)
+				NumberOfInfo:  2, // This is constant.
+				NumberOfDebug: 8, // This is 4 + (number of actions)
 			},
 		},
 		{
@@ -488,7 +488,7 @@ func TestGithubTokenPermissionsLineNumber(t *testing.T) {
 						logMessage.Finding.Location != nil &&
 						logMessage.Finding.Location.LineStart != nil &&
 						*logMessage.Finding.Location.LineStart == expectedLog.lineNumber &&
-						logMessage.Finding.Location.Value == p &&
+						logMessage.Finding.Location.Path == p &&
 						logType == checker.DetailWarn
 				}
 				if !scut.ValidateLogMessage(isExpectedLog, &dl) {