diff --git a/checker/check_result.go b/checker/check_result.go index 30e9fb13185..e73a9aea0cd 100644 --- a/checker/check_result.go +++ b/checker/check_result.go @@ -93,6 +93,18 @@ type CheckResult struct { Reason string `json:"-"` // A sentence describing the check result (score, etc) } +// 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 +} + // CheckDetail contains information for each detail. type CheckDetail struct { Msg LogMessage @@ -103,12 +115,13 @@ type CheckDetail struct { // This allows updating the definition easily. // 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 + 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. } // CreateProportionalScore creates a proportional score. diff --git a/checks/permissions.go b/checks/permissions.go index ba114c2f565..2e999219fa1 100644 --- a/checks/permissions.go +++ b/checks/permissions.go @@ -82,6 +82,11 @@ func TokenPermissions(c *checker.CheckRequest) checker.CheckResult { data := permissionCbData{ workflows: make(map[string]permissions), } + + if err := remdiationSetup(c); err != nil { + createResultForLeastPrivilegeTokens(data, err) + } + err := fileparser.OnMatchingFileContentDo(c.RepoClient, fileparser.PathMatcher{ Pattern: ".github/workflows/*", CaseSensitive: false, @@ -157,22 +162,24 @@ func validatePermission(permissionKey permission, permissionValue *actionlint.Pe if strings.EqualFold(val, "write") { if isPermissionOfInterest(permissionKey, ignoredPermissions) { dl.Warn(&checker.LogMessage{ - Path: path, - Type: checker.FileTypeSource, - Offset: lineNumber, - Text: fmt.Sprintf("%s '%v' permission set to '%v'", permLevel, permissionKey, val), - // TODO: set Snippet. + Path: path, + Type: checker.FileTypeSource, + Offset: lineNumber, + Text: fmt.Sprintf("%s '%v' permission set to '%v'", permLevel, permissionKey, val), + Snippet: val, + Remediation: createWorkflowPermissionRemediation(path), }) recordPermissionWrite(pPermissions, permissionKey) } else { // Only log for debugging, otherwise // it may confuse users. dl.Debug(&checker.LogMessage{ - Path: path, - Type: checker.FileTypeSource, - Offset: lineNumber, - Text: fmt.Sprintf("%s '%v' permission set to '%v'", permLevel, permissionKey, val), - // TODO: set Snippet. + Path: path, + Type: checker.FileTypeSource, + Offset: lineNumber, + Text: fmt.Sprintf("%s '%v' permission set to '%v'", permLevel, permissionKey, val), + Snippet: val, + Remediation: createWorkflowPermissionRemediation(path), }) } return nil @@ -243,22 +250,24 @@ func validatePermissions(permissions *actionlint.Permissions, permLevel, path st lineNumber := fileparser.GetLineNumber(permissions.All.Pos) if !strings.EqualFold(val, "read-all") && val != "" { dl.Warn(&checker.LogMessage{ - Path: path, - Type: checker.FileTypeSource, - Offset: lineNumber, - Text: fmt.Sprintf("%s permissions set to '%v'", permLevel, val), - // TODO: set Snippet. + Path: path, + Type: checker.FileTypeSource, + Offset: lineNumber, + Text: fmt.Sprintf("%s permissions set to '%v'", permLevel, val), + Snippet: val, + Remediation: createWorkflowPermissionRemediation(path), }) recordAllPermissionsWrite(pdata, permLevel, path) return nil } dl.Info(&checker.LogMessage{ - Path: path, - Type: checker.FileTypeSource, - Offset: lineNumber, - Text: fmt.Sprintf("%s permissions set to '%v'", permLevel, val), - // TODO: set Snippet. + Path: path, + Type: checker.FileTypeSource, + Offset: lineNumber, + Text: fmt.Sprintf("%s permissions set to '%v'", permLevel, val), + Snippet: val, + Remediation: createWorkflowPermissionRemediation(path), }) } else /* scopeIsSet == true */ if err := validateMapPermissions(permissions.Scopes, permLevel, path, dl, getWritePermissionsMap(pdata, path, permLevel), ignoredPermissions); err != nil { @@ -273,10 +282,11 @@ func validateTopLevelPermissions(workflow *actionlint.Workflow, path string, // Check if permissions are set explicitly. if workflow.Permissions == nil { dl.Warn(&checker.LogMessage{ - Path: path, - Type: checker.FileTypeSource, - Offset: checker.OffsetDefault, - Text: fmt.Sprintf("no %s permission defined", topLevelPermission), + Path: path, + Type: checker.FileTypeSource, + Offset: checker.OffsetDefault, + Text: fmt.Sprintf("no %s permission defined", topLevelPermission), + Remediation: createWorkflowPermissionRemediation(path), }) recordAllPermissionsWrite(pdata, topLevelPermission, path) return nil @@ -296,10 +306,11 @@ func validatejobLevelPermissions(workflow *actionlint.Workflow, path string, // so only top-level read-only permissions need to be declared. if job.Permissions == nil { dl.Debug(&checker.LogMessage{ - Path: path, - Type: checker.FileTypeSource, - Offset: fileparser.GetLineNumber(job.Pos), - Text: fmt.Sprintf("no %s permission defined", jobLevelPermission), + Path: path, + Type: checker.FileTypeSource, + Offset: fileparser.GetLineNumber(job.Pos), + Text: fmt.Sprintf("no %s permission defined", jobLevelPermission), + Remediation: createWorkflowPermissionRemediation(path), }) recordAllPermissionsWrite(pdata, jobLevelPermission, path) continue diff --git a/checks/pinned_dependencies.go b/checks/pinned_dependencies.go index 7c4a66a4503..8d9e65be42c 100644 --- a/checks/pinned_dependencies.go +++ b/checks/pinned_dependencies.go @@ -63,6 +63,10 @@ func PinnedDependencies(c *checker.CheckRequest) checker.CheckResult { } */ + if remErr := remdiationSetup(c); remErr != nil { + return checker.CreateRuntimeErrorResult(CheckPinnedDependencies, remErr) + } + // GitHub actions. actionScore, actionErr := isGitHubActionsWorkflowPinned(c) if actionErr != nil { @@ -702,10 +706,11 @@ var validateGitHubActionWorkflow fileparser.DoWhileTrueOnFileContent = func( if !match { dl.Warn(&checker.LogMessage{ Path: pathfn, Type: checker.FileTypeSource, - Offset: uint(execAction.Uses.Pos.Line), - EndOffset: uint(execAction.Uses.Pos.Line), // `Uses` always span a single line. - Snippet: execAction.Uses.Value, - Text: fmt.Sprintf("%s action not pinned by hash", owner), + Offset: uint(execAction.Uses.Pos.Line), + EndOffset: uint(execAction.Uses.Pos.Line), // `Uses` always span a single line. + Snippet: execAction.Uses.Value, + Text: fmt.Sprintf("%s action not pinned by hash", owner), + Remediation: createWorkflowPinningRemediation(pathfn), }) } diff --git a/checks/raw/binary_artifact.go b/checks/raw/binary_artifact.go index 5b3338e38f1..3f7da96eabc 100644 --- a/checks/raw/binary_artifact.go +++ b/checks/raw/binary_artifact.go @@ -95,10 +95,10 @@ var checkBinaryFileContent fileparser.DoWhileTrueOnFileContent = func(path strin exists1 := binaryFileTypes[t.Extension] if exists1 { *pfiles = append(*pfiles, checker.File{ - Path: path, - Type: checker.FileTypeBinary, - Offset: checker.OffsetDefault, - }) + Path: path, + Type: checker.FileTypeBinary, + Offset: checker.OffsetDefault, + }) return true, nil } diff --git a/checks/remediations.go b/checks/remediations.go new file mode 100644 index 00000000000..a523fcd39a3 --- /dev/null +++ b/checks/remediations.go @@ -0,0 +1,89 @@ +// Copyright 2022 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. + +package checks + +import ( + "errors" + "fmt" + "strings" + "sync" + + "github.com/ossf/scorecard/v4/checker" + "github.com/ossf/scorecard/v4/clients" +) + +var ( + remediationBranch string + remediationRepo string + remediationOnce *sync.Once + remediationSetupErr error +) + +var ( + workflowText = "update your workflow using https://app.stepsecurity.io/secureworkflow/%s/%s/%s?enable=%s" + //nolint + workflowMarkdown = "update your workflow using [https://app.stepsecurity.io](https://app.stepsecurity.io/secureworkflow/%s/%s/%s?enable=%s)" +) + +//nolint:gochecknoinits +func init() { + remediationOnce = new(sync.Once) +} + +func remdiationSetup(c *checker.CheckRequest) error { + remediationOnce.Do(func() { + // Get the branch for remediation. + b, err := c.RepoClient.GetDefaultBranch() + if err != nil && !errors.Is(err, clients.ErrUnsupportedFeature) { + remediationSetupErr = err + return + } + if b.Name != nil { + remediationBranch = *b.Name + uri := c.Repo.URI() + parts := strings.Split(uri, "/") + if len(parts) != 3 { + remediationSetupErr = fmt.Errorf("%w: %s", errInvalidArgLength, uri) + return + } + remediationRepo = fmt.Sprintf("%s/%s", parts[1], parts[2]) + } + }) + + return remediationSetupErr +} + +func createWorkflowPermissionRemediation(filepath string) *checker.Remediation { + return createWorkflowRemediation(filepath, "permissions") +} + +func createWorkflowPinningRemediation(filepath string) *checker.Remediation { + return createWorkflowRemediation(filepath, "pin") +} + +func createWorkflowRemediation(path, t string) *checker.Remediation { + p := strings.TrimPrefix(path, ".github/workflows/") + if remediationBranch == "" || remediationRepo == "" { + return nil + } + + text := fmt.Sprintf(workflowText, remediationRepo, p, remediationBranch, t) + markdown := fmt.Sprintf(workflowMarkdown, remediationRepo, p, remediationBranch, t) + + return &checker.Remediation{ + HelpText: text, + HelpMarkdown: markdown, + } +} diff --git a/pkg/sarif.go b/pkg/sarif.go index 32fc7d29a21..dcbb63e4cdf 100644 --- a/pkg/sarif.go +++ b/pkg/sarif.go @@ -64,7 +64,8 @@ type location struct { PhysicalLocation physicalLocation `json:"physicalLocation"` //nolint // This is optional https://docs.github.com/en/code-security/code-scanning/integrating-with-code-scanning/sarif-support-for-code-scanning#location-object. - Message *text `json:"message,omitempty"` + Message *text `json:"message,omitempty"` + HasRemediation bool `json:"-"` } //nolint @@ -300,6 +301,12 @@ func detailsToLocations(details []checker.CheckDetail, Message: &text{Text: d.Msg.Text}, } + // Add remediaiton information + if d.Msg.Remediation != nil { + loc.Message.Text = fmt.Sprintf("%s\nRemediation tip: %s", loc.Message.Text, d.Msg.Remediation.HelpMarkdown) + loc.HasRemediation = true + } + // Set the region depending on the file type. loc.PhysicalLocation.Region = detailToRegion(&d) locs = append(locs, loc) @@ -428,12 +435,17 @@ func createSARIFRule(checkName, checkID, descURL, longDesc, shortDesc, risk stri } func createSARIFCheckResult(pos int, checkID, message string, loc *location) result { + t := fmt.Sprintf("%s\nClick Remediation section below to solve this issue", message) + if loc.HasRemediation { + t = fmt.Sprintf("%s\nClick Remediation section below for further remediation help", message) + } + return result{ RuleID: checkID, // https://github.com/microsoft/sarif-tutorials/blob/main/docs/2-Basics.md#level // Level: scoreToLevel(minScore, score), RuleIndex: pos, - Message: text{Text: fmt.Sprintf("%s\nClick Remediation section below to solve this issue", message)}, + Message: text{Text: t}, Locations: []location{*loc}, } } diff --git a/pkg/sarif_test.go b/pkg/sarif_test.go index 9bbeb97b209..db859d3905b 100644 --- a/pkg/sarif_test.go +++ b/pkg/sarif_test.go @@ -127,6 +127,60 @@ func TestSARIFOutput(t *testing.T) { result ScorecardResult policy spol.ScorecardPolicy }{ + { + name: "check with detail remediation", + showDetails: true, + expected: "./testdata/check-remediation.sarif", + logLevel: log.DebugLevel, + policy: spol.ScorecardPolicy{ + Version: 1, + Policies: map[string]*spol.CheckPolicy{ + "Check-Name": { + Score: checker.MaxResultScore, + Mode: spol.CheckPolicy_ENFORCED, + }, + "Check-Name2": { + Score: checker.MaxResultScore, + Mode: spol.CheckPolicy_DISABLED, + }, + }, + }, + result: ScorecardResult{ + Repo: RepoInfo{ + Name: repoName, + CommitSHA: repoCommit, + }, + Scorecard: ScorecardInfo{ + Version: scorecardVersion, + CommitSHA: scorecardCommit, + }, + Date: date, + Checks: []checker.CheckResult{ + { + Details2: []checker.CheckDetail{ + { + Type: checker.DetailWarn, + Msg: checker.LogMessage{ + Text: "warn message", + Path: "src/file1.cpp", + Type: checker.FileTypeSource, + Offset: 5, + Snippet: "if (bad) {BUG();}", + Remediation: &checker.Remediation{ + HelpMarkdown: "this is the custom markdown help", + HelpText: "this is the custom text help", + }, + }, + }, + }, + Score: 5, + Reason: "half score reason", + Name: "Check-Name", + }, + }, + Metadata: []string{}, + }, + }, { name: "check-1", showDetails: true, @@ -135,11 +189,11 @@ func TestSARIFOutput(t *testing.T) { policy: spol.ScorecardPolicy{ Version: 1, Policies: map[string]*spol.CheckPolicy{ - "Check-Name": &spol.CheckPolicy{ + "Check-Name": { Score: checker.MaxResultScore, Mode: spol.CheckPolicy_ENFORCED, }, - "Check-Name2": &spol.CheckPolicy{ + "Check-Name2": { Score: checker.MaxResultScore, Mode: spol.CheckPolicy_DISABLED, }, @@ -185,11 +239,11 @@ func TestSARIFOutput(t *testing.T) { policy: spol.ScorecardPolicy{ Version: 1, Policies: map[string]*spol.CheckPolicy{ - "Check-Name": &spol.CheckPolicy{ + "Check-Name": { Score: checker.MaxResultScore, Mode: spol.CheckPolicy_ENFORCED, }, - "Check-Name2": &spol.CheckPolicy{ + "Check-Name2": { Score: checker.MaxResultScore, Mode: spol.CheckPolicy_DISABLED, }, @@ -234,15 +288,15 @@ func TestSARIFOutput(t *testing.T) { policy: spol.ScorecardPolicy{ Version: 1, Policies: map[string]*spol.CheckPolicy{ - "Check-Name": &spol.CheckPolicy{ + "Check-Name": { Score: checker.MaxResultScore, Mode: spol.CheckPolicy_ENFORCED, }, - "Check-Name2": &spol.CheckPolicy{ + "Check-Name2": { Score: checker.MaxResultScore, Mode: spol.CheckPolicy_ENFORCED, }, - "Check-Name3": &spol.CheckPolicy{ + "Check-Name3": { Score: checker.MaxResultScore, Mode: spol.CheckPolicy_ENFORCED, }, @@ -341,15 +395,15 @@ func TestSARIFOutput(t *testing.T) { policy: spol.ScorecardPolicy{ Version: 1, Policies: map[string]*spol.CheckPolicy{ - "Check-Name": &spol.CheckPolicy{ + "Check-Name": { Score: checker.MaxResultScore, Mode: spol.CheckPolicy_ENFORCED, }, - "Check-Name2": &spol.CheckPolicy{ + "Check-Name2": { Score: checker.MaxResultScore, Mode: spol.CheckPolicy_ENFORCED, }, - "Check-Name3": &spol.CheckPolicy{ + "Check-Name3": { Score: checker.MaxResultScore, Mode: spol.CheckPolicy_ENFORCED, }, @@ -448,7 +502,7 @@ func TestSARIFOutput(t *testing.T) { policy: spol.ScorecardPolicy{ Version: 1, Policies: map[string]*spol.CheckPolicy{ - "Check-Name": &spol.CheckPolicy{ + "Check-Name": { Score: 5, Mode: spol.CheckPolicy_ENFORCED, }, @@ -496,7 +550,7 @@ func TestSARIFOutput(t *testing.T) { policy: spol.ScorecardPolicy{ Version: 1, Policies: map[string]*spol.CheckPolicy{ - "Check-Name": &spol.CheckPolicy{ + "Check-Name": { Score: checker.MaxResultScore, Mode: spol.CheckPolicy_ENFORCED, }, @@ -540,15 +594,15 @@ func TestSARIFOutput(t *testing.T) { policy: spol.ScorecardPolicy{ Version: 1, Policies: map[string]*spol.CheckPolicy{ - "Check-Name": &spol.CheckPolicy{ + "Check-Name": { Score: checker.MaxResultScore, Mode: spol.CheckPolicy_ENFORCED, }, - "Check-Name2": &spol.CheckPolicy{ + "Check-Name2": { Score: checker.MaxResultScore, Mode: spol.CheckPolicy_DISABLED, }, - "Check-Name3": &spol.CheckPolicy{ + "Check-Name3": { Score: checker.MaxResultScore, Mode: spol.CheckPolicy_DISABLED, }, @@ -647,19 +701,19 @@ func TestSARIFOutput(t *testing.T) { policy: spol.ScorecardPolicy{ Version: 1, Policies: map[string]*spol.CheckPolicy{ - "Check-Name4": &spol.CheckPolicy{ + "Check-Name4": { Score: checker.MaxResultScore, Mode: spol.CheckPolicy_ENFORCED, }, - "Check-Name": &spol.CheckPolicy{ + "Check-Name": { Score: checker.MaxResultScore, Mode: spol.CheckPolicy_ENFORCED, }, - "Check-Name5": &spol.CheckPolicy{ + "Check-Name5": { Score: checker.MaxResultScore, Mode: spol.CheckPolicy_ENFORCED, }, - "Check-Name6": &spol.CheckPolicy{ + "Check-Name6": { Score: checker.MaxResultScore, Mode: spol.CheckPolicy_ENFORCED, }, diff --git a/pkg/testdata/check-remediation.sarif b/pkg/testdata/check-remediation.sarif new file mode 100644 index 00000000000..540ca0fca7d --- /dev/null +++ b/pkg/testdata/check-remediation.sarif @@ -0,0 +1,76 @@ +{ + "$schema": "https://raw.githubusercontent.com/oasis-tcs/sarif-spec/master/Schemata/sarif-schema-2.1.0.json", + "version": "2.1.0", + "runs": [ + { + "automationDetails": { + "id": "supply-chain/local/ccbc59901773ab4c051dfcea0cc4201a1567abdd-17 Aug 21 18:57 +0000" + }, + "tool": { + "driver": { + "name": "Scorecard", + "informationUri": "https://github.com/ossf/scorecard", + "semanticVersion": "1.2.3", + "rules": [ + { + "id": "CheckNameID", + "name": "Check-Name", + "helpUri": "https://github.com/ossf/scorecard/blob/main/docs/checks.md#check-name", + "shortDescription": { + "text": "Check-Name" + }, + "fullDescription": { + "text": "short description" + }, + "help": { + "text": "short description", + "markdown": "**Remediation (click \"Show more\" below)**:\n\n- not-used1\n\n- not-used2\n\n\n\n**Severity**: High\n\n\n\n**Details**:\n\nlong description\n\n other line" + }, + "defaultConfiguration": { + "level": "error" + }, + "properties": { + "precision": "high", + "problem.severity": "error", + "security-severity": "7.0", + "tags": [ + "tag1", + "tag2" + ] + } + } + ] + } + }, + "results": [ + { + "ruleId": "CheckNameID", + "ruleIndex": 0, + "message": { + "text": "score is 5: warn message\nRemediation tip: this is the custom markdown help\nClick Remediation section below for further remediation help" + }, + "locations": [ + { + "physicalLocation": { + "region": { + "startLine": 5, + "endLine": 5, + "snippet": { + "text": "if (bad) {BUG();}" + } + }, + "artifactLocation": { + "uri": "src/file1.cpp", + "uriBaseId": "%SRCROOT%" + } + }, + "message": { + "text": "warn message\nRemediation tip: this is the custom markdown help" + } + } + ] + } + ] + } + ] +}