From d1e8603062add99342c307c9a46550d0ecfccc03 Mon Sep 17 00:00:00 2001 From: scott hissam Date: Fri, 4 Nov 2022 16:35:44 -0500 Subject: [PATCH] :sparkles: Improved Security Policy Check (#2195) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * :sparkles: Improved Security Policy Check (#2137) * Examines and awards points for linked content (URLs / Emails) * Examines and awards points for hints of disclosure and vulnerability practices * Examines and awards points for hints of elaboration of timelines Signed-off-by: Scott Hissam * Repaired Security Policy to correctly use linked content length for evaluation Signed-off-by: Scott Hissam * gofmt'ed changes Signed-off-by: Scott Hissam * Repaired the case in the evaluation which was too sensitive to content length over the length of the linked content for urls and emails Signed-off-by: Scott Hissam * added unit test cases for the new content-based Security Policy checks Signed-off-by: Scott Hissam * reverted the direct (mistaken) change to checks.md and updated the checks.yaml for generate-docs Signed-off-by: Scott Hissam * :sparkles: Improved Security Policy Check (#2137) (revisted based on comments) * replaced reason strings with log.Info & log.Warn (as seen in --show-details) * internal assertion check for nil (*pinfo) and empty pfile * internal switched to FileTypeText over FileTypeSource * internal implement type SecurityPolicyInformationType/SecurityPolicyInformation revised SecurityPolicyData to support only one file * revised expected unit-test results and revised unit-test to reflect the new SecurityPolicyData type Signed-off-by: Scott Hissam * revised the score value based on observation of one *or more* url(s) or one email(s) found; unit tests update accordingly Signed-off-by: Scott Hissam * revised the score value based on observation of one *or more* url(s) or one email(s) found; unit tests update accordingly Signed-off-by: Scott Hissam * revised the score value based on observation of one *or more* url(s) or one email(s) found; e2e tests update accordingly Signed-off-by: Scott Hissam * Addressed PR comments; added telemetry for policy hits in security policy file to track hits by line number Signed-off-by: Scott Hissam * Resolved merge conflict with checks.yaml Signed-off-by: Scott Hissam * updated raw results to emit all the raw information for the new security policy check Signed-off-by: Scott Hissam * Resolved merge conflicts and lint errors with json_raw_results.go Signed-off-by: Scott Hissam * Addressed review comments to reorganize security policy data struct to support the potential for multiple security policy files. Signed-off-by: Scott Hissam * Added logic to the security policy to process multiple security policy files only after future improvements to aggregating scoring across such files are designed. For now the security policy behaves as originally designed to stop once one of the expected policy files are found in the repo Signed-off-by: Scott Hissam * added comments regarding the capacity to support multiple policy files and removed unneeded break statements in the code Signed-off-by: Scott Hissam * Addressed review comments to remove the dependency on the path in the filename from the code and introduced FileSize to checker.File type and removed the SecurityContentLength which was used to hold that information for the new security policy assessment Signed-off-by: Scott Hissam * restored reporting full security policy path and filename for policies found in the org level repos Signed-off-by: Scott Hissam * Resolved conflicts in checks.yaml for documentation Signed-off-by: Scott Hissam * ✨ CLI for scorecard-attestor (#2309) * Reorganize Signed-off-by: Raghav Kaul * Working commit Signed-off-by: Raghav Kaul * Compile with local scorecard; go mod tidy Signed-off-by: Raghav Kaul * Add signing code Heavily borrowed from https://github.com/grafeas/kritis/blob/master/cmd/kritis/signer/main.go Signed-off-by: Raghav Kaul * Update deps * Naming * Makefile Signed-off-by: Raghav Kaul * Edit license, add lint.yml Signed-off-by: Raghav Kaul * checks: go mod tidy, license Signed-off-by: Raghav Kaul * Address PR comments * Split into checker/signer files * Naming convention Signed-off-by: Raghav Kaul * License, remove golangci.yml Signed-off-by: Raghav Kaul * Address PR comments * Use cobra Signed-off-by: Raghav Kaul * Add tests for root command Signed-off-by: Raghav Kaul * Filter out checks that aren't needed for policy evaluation Signed-off-by: Raghav Kaul * Add `make` targets for attestor; submit coverage stats Signed-off-by: Raghav Kaul * Improvements * Use sclog instead of glog * Remove unneeded subcommands * Formatting Signed-off-by: Raghav Kaul * Flags: Make note-name constant and fix messaging Signed-off-by: Raghav Kaul * Remove SupportedRequestTypes Signed-off-by: Raghav Kaul * go mod tidy Signed-off-by: Raghav Kaul * go mod tidy, makefile Signed-off-by: Raghav Kaul * Fix GH actions run Signed-off-by: Raghav Kaul Signed-off-by: Raghav Kaul Signed-off-by: Scott Hissam * removed whitespace before stanza for Run attestor e2e Signed-off-by: Scott Hissam * resolved code review and doc review comments Signed-off-by: Scott Hissam * repaired the link for the maintainer's guide for supporting the coordinated vulnerability disclosure guidelines Signed-off-by: Scott Hissam Signed-off-by: Scott Hissam Signed-off-by: nathaniel.wert --- .github/workflows/integration.yml | 2 +- checker/raw_result.go | 31 +++- checks/evaluation/security_policy.go | 120 ++++++++++++- checks/evaluation/security_policy_test.go | 18 +- checks/raw/security_policy.go | 162 ++++++++++++++++-- checks/raw/security_policy_test.go | 31 +++- checks/security_policy_test.go | 73 ++++++-- checks/testdata/securitypolicy/00_1byte | 1 + checks/testdata/securitypolicy/00_empty | 0 .../testdata/securitypolicy/03_securitypolicy | 1 + checks/testdata/securitypolicy/03_textOnly | 1 + .../securitypolicy/04_textAndDisclosureVuls | 1 + checks/testdata/securitypolicy/06_emailOnly | 1 + .../securitypolicy/06_urlAndEmailOnly | 1 + checks/testdata/securitypolicy/06_urlOnly | 1 + .../securitypolicy/09_linkedContentAndText | 2 + .../10_linkedContentAndTextAndDisclosureVuls | 2 + checks/testdata/securitypolicy/10_realworld | 22 +++ .../testdata/securitypolicy/10_realworldtwo | 11 ++ cron/internal/format/json_raw_results.go | 4 +- docs/checks.md | 24 ++- docs/checks/internal/checks.yaml | 25 ++- e2e/security_policy_test.go | 10 +- pkg/json_raw_results.go | 37 +++- 24 files changed, 517 insertions(+), 64 deletions(-) create mode 100644 checks/testdata/securitypolicy/00_1byte create mode 100644 checks/testdata/securitypolicy/00_empty create mode 100644 checks/testdata/securitypolicy/03_securitypolicy create mode 100644 checks/testdata/securitypolicy/03_textOnly create mode 100644 checks/testdata/securitypolicy/04_textAndDisclosureVuls create mode 100644 checks/testdata/securitypolicy/06_emailOnly create mode 100644 checks/testdata/securitypolicy/06_urlAndEmailOnly create mode 100644 checks/testdata/securitypolicy/06_urlOnly create mode 100644 checks/testdata/securitypolicy/09_linkedContentAndText create mode 100644 checks/testdata/securitypolicy/10_linkedContentAndTextAndDisclosureVuls create mode 100644 checks/testdata/securitypolicy/10_realworld create mode 100644 checks/testdata/securitypolicy/10_realworldtwo diff --git a/.github/workflows/integration.yml b/.github/workflows/integration.yml index fbd2de59fa3c..4c525c0d94a7 100644 --- a/.github/workflows/integration.yml +++ b/.github/workflows/integration.yml @@ -76,7 +76,7 @@ jobs: retry_on: error timeout_minutes: 30 command: make e2e-pat - + - name: Run attestor e2e #using retry because the GitHub token is being throttled. uses: nick-invision/retry@3e91a01664abd3c5cd539100d10d33b9c5b68482 env: diff --git a/checker/raw_result.go b/checker/raw_result.go index a0af75763d7a..e4dc93dc3f3c 100644 --- a/checker/raw_result.go +++ b/checker/raw_result.go @@ -163,11 +163,37 @@ type VulnerabilitiesData struct { Vulnerabilities []clients.Vulnerability } +type SecurityPolicyInformationType string + +const ( + // forms of security policy hints being evaluated. + SecurityPolicyInformationTypeEmail SecurityPolicyInformationType = "emailAddress" + SecurityPolicyInformationTypeLink SecurityPolicyInformationType = "httpLink" + SecurityPolicyInformationTypeText SecurityPolicyInformationType = "vulnDisclosureText" +) + +type SecurityPolicyValueType struct { + Match string // Snippet of match + LineNumber uint // Line number in policy file of match + Offset uint // Offset in the line of the match +} + +type SecurityPolicyInformation struct { + InformationType SecurityPolicyInformationType + InformationValue SecurityPolicyValueType +} + +type SecurityPolicyFile struct { + // security policy information found in repo or org + Information []SecurityPolicyInformation + // file that contains the security policy information + File File +} + // SecurityPolicyData contains the raw results // for the Security-Policy check. type SecurityPolicyData struct { - // Files contains a list of files. - Files []File + PolicyFiles []SecurityPolicyFile } // BinaryArtifactData contains the raw results @@ -236,6 +262,7 @@ type File struct { 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. // TODO: add hash. } diff --git a/checks/evaluation/security_policy.go b/checks/evaluation/security_policy.go index aa80adedb1ee..82bdb5df0112 100644 --- a/checks/evaluation/security_policy.go +++ b/checks/evaluation/security_policy.go @@ -19,6 +19,104 @@ import ( sce "github.com/ossf/scorecard/v4/errors" ) +func scoreSecurityCriteria(f checker.File, + info []checker.SecurityPolicyInformation, + dl checker.DetailLogger, +) int { + var urls, emails, discvuls, linkedContentLen, score int + + emails = countSecInfo(info, checker.SecurityPolicyInformationTypeEmail, true) + urls = countSecInfo(info, checker.SecurityPolicyInformationTypeLink, true) + discvuls = countSecInfo(info, checker.SecurityPolicyInformationTypeText, false) + + for _, i := range findSecInfo(info, checker.SecurityPolicyInformationTypeEmail, true) { + linkedContentLen += len(i.InformationValue.Match) + } + for _, i := range findSecInfo(info, checker.SecurityPolicyInformationTypeLink, true) { + linkedContentLen += len(i.InformationValue.Match) + } + + msg := checker.LogMessage{ + Path: f.Path, + Type: f.Type, + Text: "", + } + + // #1: linked content found (email/http): score += 6 + if (urls + emails) > 0 { + score += 6 + msg.Text = "Found linked content in security policy" + dl.Info(&msg) + } else { + msg.Text = "no email or URL found in security policy" + dl.Warn(&msg) + } + + // #2: more bytes than the sum of the length of all the linked content found: score += 3 + // rationale: there appears to be information and context around those links + // no credit if there is just a link to a site or an email address (those given above) + // the test here is that each piece of linked content will likely contain a space + // before and after the content (hence the two multiplier) + if f.FileSize > 1 && (f.FileSize > uint(linkedContentLen+((urls+emails)*2))) { + score += 3 + msg.Text = "Found text in security policy" + dl.Info(&msg) + } else { + msg.Text = "No text (beyond any linked content) found in security policy" + dl.Warn(&msg) + } + + // #3: found whole number(s) and or match(es) to "Disclos" and or "Vuln": score += 1 + // rationale: works towards the intent of the security policy file + // regarding whom to contact about vuls and disclosures and timing + // e.g., we'll disclose, report a vulnerabily, 30 days, etc. + // looking for at least 2 hits + if discvuls > 1 { + score += 1 + msg.Text = "Found disclosure, vulnerability, and/or timelines in security policy" + dl.Info(&msg) + } else { + msg.Text = "One or no descriptive hints of disclosure, vulnerability, and/or timelines in security policy" + dl.Warn(&msg) + } + + return score +} + +func countSecInfo(secInfo []checker.SecurityPolicyInformation, + infoType checker.SecurityPolicyInformationType, + unique bool, +) int { + keys := make(map[string]bool) + count := 0 + for _, entry := range secInfo { + if _, present := keys[entry.InformationValue.Match]; !present && entry.InformationType == infoType { + keys[entry.InformationValue.Match] = true + count += 1 + } else if !unique && entry.InformationType == infoType { + count += 1 + } + } + return count +} + +func findSecInfo(secInfo []checker.SecurityPolicyInformation, + infoType checker.SecurityPolicyInformationType, + unique bool, +) []checker.SecurityPolicyInformation { + keys := make(map[string]bool) + var secList []checker.SecurityPolicyInformation + for _, entry := range secInfo { + if _, present := keys[entry.InformationValue.Match]; !present && entry.InformationType == infoType { + keys[entry.InformationValue.Match] = true + secList = append(secList, entry) + } else if !unique && entry.InformationType == infoType { + secList = append(secList, entry) + } + } + return secList +} + // SecurityPolicy applies the score policy for the Security-Policy check. func SecurityPolicy(name string, dl checker.DetailLogger, r *checker.SecurityPolicyData) checker.CheckResult { if r == nil { @@ -27,23 +125,31 @@ func SecurityPolicy(name string, dl checker.DetailLogger, r *checker.SecurityPol } // Apply the policy evaluation. - if r.Files == nil || len(r.Files) == 0 { - // If the file is null or has zero lengths, directly return as not detected. + if len(r.PolicyFiles) == 0 { + // If the file is unset, directly return as not detected. return checker.CreateMinScoreResult(name, "security policy file not detected") } - for _, f := range r.Files { + // TODO: although this a loop, the raw checks will only return one security policy + // when more than one security policy file can be aggregated into a composite + // score, that logic can be comprehended here. + score := 0 + for _, spd := range r.PolicyFiles { + score = scoreSecurityCriteria(spd.File, + spd.Information, dl) + msg := checker.LogMessage{ - Path: f.Path, - Type: f.Type, - Offset: f.Offset, + Path: spd.File.Path, + Type: spd.File.Type, } if msg.Type == checker.FileTypeURL { msg.Text = "security policy detected in org repo" } else { msg.Text = "security policy detected in current repo" } + dl.Info(&msg) } - return checker.CreateMaxScoreResult(name, "security policy file detected") + + return checker.CreateResultWithScore(name, "security policy file detected", score) } diff --git a/checks/evaluation/security_policy_test.go b/checks/evaluation/security_policy_test.go index 2c6c2df19577..b772e3def106 100644 --- a/checks/evaluation/security_policy_test.go +++ b/checks/evaluation/security_policy_test.go @@ -59,16 +59,17 @@ func TestSecurityPolicy(t *testing.T) { args: args{ name: "test_security_policy_3", r: &checker.SecurityPolicyData{ - Files: []checker.File{ - { + PolicyFiles: []checker.SecurityPolicyFile{{ + File: checker.File{ Path: "/etc/security/pam_env.conf", Type: checker.FileTypeURL, }, + Information: make([]checker.SecurityPolicyInformation, 0), }, - }, + }}, }, want: checker.CheckResult{ - Score: 10, + Score: 0, }, }, { @@ -76,15 +77,16 @@ func TestSecurityPolicy(t *testing.T) { args: args{ name: "test_security_policy_4", r: &checker.SecurityPolicyData{ - Files: []checker.File{ - { + PolicyFiles: []checker.SecurityPolicyFile{{ + File: checker.File{ Path: "/etc/security/pam_env.conf", }, + Information: make([]checker.SecurityPolicyInformation, 0), }, - }, + }}, }, want: checker.CheckResult{ - Score: 10, + Score: 0, }, }, } diff --git a/checks/raw/security_policy.go b/checks/raw/security_policy.go index 991d56aaad59..de55b3b5594c 100644 --- a/checks/raw/security_policy.go +++ b/checks/raw/security_policy.go @@ -15,10 +15,12 @@ package raw import ( + "bufio" "errors" "fmt" "os" "path" + "regexp" "strings" "github.com/ossf/scorecard/v4/checker" @@ -32,14 +34,14 @@ import ( type securityPolicyFilesWithURI struct { uri string - files []checker.File + files []checker.SecurityPolicyFile } -// SecurityPolicy checks for presence of security policy. +// SecurityPolicy checks for presence of security policy +// and applicable content discovered by checkSecurityPolicyFileContent(). func SecurityPolicy(c *checker.CheckRequest) (checker.SecurityPolicyData, error) { data := securityPolicyFilesWithURI{ - uri: "", - files: make([]checker.File, 0), + uri: "", files: make([]checker.SecurityPolicyFile, 0), } err := fileparser.OnAllFilesDo(c.RepoClient, isSecurityPolicyFile, &data) if err != nil { @@ -47,7 +49,16 @@ func SecurityPolicy(c *checker.CheckRequest) (checker.SecurityPolicyData, error) } // If we found files in the repo, return immediately. if len(data.files) > 0 { - return checker.SecurityPolicyData{Files: data.files}, nil + for idx := range data.files { + err := fileparser.OnMatchingFileContentDo(c.RepoClient, fileparser.PathMatcher{ + Pattern: data.files[idx].File.Path, + CaseSensitive: false, + }, checkSecurityPolicyFileContent, &data.files[idx].File, &data.files[idx].Information) + if err != nil { + return checker.SecurityPolicyData{}, err + } + } + return checker.SecurityPolicyData{PolicyFiles: data.files}, nil } // Check if present in parent org. @@ -82,7 +93,24 @@ func SecurityPolicy(c *checker.CheckRequest) (checker.SecurityPolicyData, error) } // Return raw results. - return checker.SecurityPolicyData{Files: data.files}, nil + if len(data.files) > 0 { + for idx := range data.files { + filePattern := data.files[idx].File.Path + // undo path.Join in isSecurityPolicyFile just + // for this call to OnMatchingFileContentsDo + if data.files[idx].File.Type == checker.FileTypeURL { + filePattern = strings.Replace(filePattern, data.uri+"/", "", 1) + } + err := fileparser.OnMatchingFileContentDo(dotGitHubClient, fileparser.PathMatcher{ + Pattern: filePattern, + CaseSensitive: false, + }, checkSecurityPolicyFileContent, &data.files[idx].File, &data.files[idx].Information) + if err != nil { + return checker.SecurityPolicyData{}, err + } + } + } + return checker.SecurityPolicyData{PolicyFiles: data.files}, nil } // Check repository for repository-specific policy. @@ -97,16 +125,27 @@ var isSecurityPolicyFile fileparser.DoWhileTrueOnFilename = func(name string, ar } if isSecurityPolicyFilename(name) { tempPath := name - tempType := checker.FileTypeSource + tempType := checker.FileTypeText if pdata.uri != "" { + // report complete path for org-based policy files tempPath = path.Join(pdata.uri, tempPath) + // FileTypeURL is used in Security-Policy to + // only denote for the details report that the + // policy was found at the org level rather + // than the repo level tempType = checker.FileTypeURL } - pdata.files = append(pdata.files, checker.File{ - Path: tempPath, - Type: tempType, - Offset: checker.OffsetDefault, + pdata.files = append(pdata.files, checker.SecurityPolicyFile{ + File: checker.File{ + Path: tempPath, + Type: tempType, + Offset: checker.OffsetDefault, + FileSize: checker.OffsetDefault, + }, + Information: make([]checker.SecurityPolicyInformation, 0), }) + // TODO: change 'false' to 'true' when multiple security policy files are supported + // otherwise this check stops at the first security policy found return false, nil } return true, nil @@ -124,3 +163,104 @@ func isSecurityPolicyFilename(name string) bool { strings.EqualFold(name, "doc/security.rst") || strings.EqualFold(name, "docs/security.rst") } + +var checkSecurityPolicyFileContent fileparser.DoWhileTrueOnFileContent = func(path string, content []byte, + args ...interface{}, +) (bool, error) { + if len(args) != 2 { + return false, fmt.Errorf( + "checkSecurityPolicyFileContent requires exactly two arguments: %w", errInvalidArgLength) + } + pfiles, ok := args[0].(*checker.File) + if !ok { + return false, fmt.Errorf( + "checkSecurityPolicyFileContent requires argument of type *checker.File: %w", errInvalidArgType) + } + pinfo, ok := args[1].(*[]checker.SecurityPolicyInformation) + if !ok { + return false, fmt.Errorf( + "%s requires argument of type *[]checker.SecurityPolicyInformation: %w", + "checkSecurityPolicyFileContent", errInvalidArgType) + } + + if len(content) == 0 { + // perhaps there are more policy files somewhere else, + // keep looking (true) + return true, nil + } + + if pfiles != nil && (*pinfo) != nil { + pfiles.Offset = checker.OffsetDefault + pfiles.FileSize = uint(len(content)) + policyHits := collectPolicyHits(content) + if len(policyHits) > 0 { + (*pinfo) = append((*pinfo), policyHits...) + } + } else { + e := sce.WithMessage(sce.ErrScorecardInternal, "bad file or information reference") + return false, e + } + + // stop here found something, no need to look further (false) + return false, nil +} + +func collectPolicyHits(policyContent []byte) []checker.SecurityPolicyInformation { + var hits []checker.SecurityPolicyInformation + + // pattern for URLs + reURL := regexp.MustCompile(`(http|https)://[a-zA-Z0-9./?=_%:-]*`) + // pattern for emails + reEML := regexp.MustCompile(`\b[A-Za-z0-9._%+-]+@[A-Za-z0-9.-]+\.[A-Za-z]{2,6}\b`) + // pattern for 1 to 4 digit numbers + // or + // strings 'disclos' as in "disclosure" or 'vuln' as in "vulnerability" + reDIG := regexp.MustCompile(`(?i)(\b*[0-9]{1,4}\b|(Disclos|Vuln))`) + + lineNum := 0 + for { + advance, token, err := bufio.ScanLines(policyContent, true) + if advance == 0 || err != nil { + break + } + + lineNum += 1 + if len(token) != 0 { + for _, indexes := range reURL.FindAllIndex(token, -1) { + hits = append(hits, checker.SecurityPolicyInformation{ + InformationType: checker.SecurityPolicyInformationTypeLink, + InformationValue: checker.SecurityPolicyValueType{ + Match: string(token[indexes[0]:indexes[1]]), // Snippet of match + LineNumber: uint(lineNum), // line number in file + Offset: uint(indexes[0]), // Offset in the line + }, + }) + } + for _, indexes := range reEML.FindAllIndex(token, -1) { + hits = append(hits, checker.SecurityPolicyInformation{ + InformationType: checker.SecurityPolicyInformationTypeEmail, + InformationValue: checker.SecurityPolicyValueType{ + Match: string(token[indexes[0]:indexes[1]]), // Snippet of match + LineNumber: uint(lineNum), // line number in file + Offset: uint(indexes[0]), // Offset in the line + }, + }) + } + for _, indexes := range reDIG.FindAllIndex(token, -1) { + hits = append(hits, checker.SecurityPolicyInformation{ + InformationType: checker.SecurityPolicyInformationTypeText, + InformationValue: checker.SecurityPolicyValueType{ + Match: string(token[indexes[0]:indexes[1]]), // Snippet of match + LineNumber: uint(lineNum), // line number in file + Offset: uint(indexes[0]), // Offset in the line + }, + }) + } + } + if advance <= len(policyContent) { + policyContent = policyContent[advance:] + } + } + + return hits +} diff --git a/checks/raw/security_policy_test.go b/checks/raw/security_policy_test.go index ae5d30896012..f9c98caa9849 100644 --- a/checks/raw/security_policy_test.go +++ b/checks/raw/security_policy_test.go @@ -15,6 +15,8 @@ package raw import ( + "fmt" + "os" "testing" "github.com/golang/mock/gomock" @@ -65,6 +67,7 @@ func TestSecurityPolicy(t *testing.T) { tests := []struct { name string files []string + path string result checker.SecurityPolicyData wantErr bool want scut.TestReturn @@ -74,30 +77,35 @@ func TestSecurityPolicy(t *testing.T) { files: []string{ "security.md", }, + path: "", }, { name: ".github/security.md", files: []string{ ".github/security.md", }, + path: "", }, { name: "docs/security.md", files: []string{ "docs/security.md", }, + path: "", }, { name: "docs/security.rst", files: []string{ "docs/security.rst", }, + path: "", }, { name: "doc/security.rst", files: []string{ "doc/security.rst", }, + path: "", }, } for _, tt := range tests { @@ -110,6 +118,25 @@ func TestSecurityPolicy(t *testing.T) { mockRepoClient.EXPECT().ListFiles(gomock.Any()).Return(tt.files, nil).AnyTimes() mockRepo.EXPECT().Org().Return(nil).AnyTimes() + // + // the revised Security Policy will immediate go for the + // file contents once found. This test will return that + // mock file, but this specific unit test is not testing + // for content. As such, this test will crash without + // a mock GetFileContent, so this will return no content + // for the existing file. content test are in overall check + // + mockRepoClient.EXPECT().GetFileContent(gomock.Any()).DoAndReturn(func(fn string) ([]byte, error) { + if tt.path == "" { + return nil, nil + } + content, err := os.ReadFile(tt.path) + if err != nil { + return content, fmt.Errorf("%w", err) + } + return content, nil + }).AnyTimes() + dl := scut.TestDetailLogger{} c := checker.CheckRequest{ RepoClient: mockRepoClient, @@ -128,8 +155,8 @@ func TestSecurityPolicy(t *testing.T) { return } - if len(res.Files) != len(tt.files) { - t.Errorf("test failed: number of files returned is not correct: %+v", res) + if (res.PolicyFiles[0].File.Path) != (tt.files[0]) { + t.Errorf("test failed: the file returned is not correct: %+v", res) } }) } diff --git a/checks/security_policy_test.go b/checks/security_policy_test.go index e689b9378361..be541d529e91 100644 --- a/checks/security_policy_test.go +++ b/checks/security_policy_test.go @@ -15,6 +15,8 @@ package checks import ( + "fmt" + "os" "testing" "github.com/golang/mock/gomock" @@ -29,118 +31,141 @@ func TestSecurityPolicy(t *testing.T) { //nolint tests := []struct { name string + path string files []string wantErr bool want scut.TestReturn }{ { name: "security.md", + path: "./testdata/securitypolicy/10_realworld", files: []string{ "security.md", }, want: scut.TestReturn{ Score: 10, - NumberOfInfo: 1, + NumberOfInfo: 4, + NumberOfWarn: 0, }, }, { name: ".github/security.md", + path: "./testdata/securitypolicy/10_realworldtwo", files: []string{ ".github/security.md", }, want: scut.TestReturn{ Score: 10, - NumberOfInfo: 1, + NumberOfInfo: 4, + NumberOfWarn: 0, }, }, { name: "docs/security.md", + path: "./testdata/securitypolicy/04_textAndDisclosureVuls", files: []string{ "docs/security.md", }, want: scut.TestReturn{ - Score: 10, - NumberOfInfo: 1, + Score: 4, + NumberOfInfo: 3, + NumberOfWarn: 1, }, }, { name: "security.rst", + path: "./testdata/securitypolicy/03_textOnly", files: []string{ "security.rst", }, want: scut.TestReturn{ - Score: 10, - NumberOfInfo: 1, + Score: 3, + NumberOfInfo: 2, + NumberOfWarn: 2, }, }, { name: ".github/security.rst", + path: "./testdata/securitypolicy/06_urlOnly", files: []string{ ".github/security.rst", }, want: scut.TestReturn{ - Score: 10, - NumberOfInfo: 1, + Score: 6, + NumberOfInfo: 2, + NumberOfWarn: 2, }, }, { name: "docs/security.rst", + path: "./testdata/securitypolicy/06_emailOnly", files: []string{ "docs/security.rst", }, want: scut.TestReturn{ - Score: 10, - NumberOfInfo: 1, + Score: 6, + NumberOfInfo: 2, + NumberOfWarn: 2, }, }, { name: "doc/security.rst", + path: "./testdata/securitypolicy/06_urlAndEmailOnly", files: []string{ "doc/security.rst", }, want: scut.TestReturn{ - Score: 10, - NumberOfInfo: 1, + Score: 6, + NumberOfInfo: 2, + NumberOfWarn: 2, }, }, { name: "security.adoc", + path: "./testdata/securitypolicy/09_linkedContentAndText", files: []string{ "security.adoc", }, want: scut.TestReturn{ - Score: 10, - NumberOfInfo: 1, + Score: 9, + NumberOfInfo: 3, + NumberOfWarn: 1, }, }, { name: ".github/security.adoc", + path: "./testdata/securitypolicy/10_linkedContentAndTextAndDisclosureVuls", files: []string{ ".github/security.adoc", }, want: scut.TestReturn{ Score: 10, - NumberOfInfo: 1, + NumberOfInfo: 4, + NumberOfWarn: 0, }, }, { name: "docs/security.adoc", + path: "./testdata/securitypolicy/00_empty", files: []string{ "docs/security.adoc", }, want: scut.TestReturn{ - Score: 10, + Score: 0, NumberOfInfo: 1, + NumberOfWarn: 3, }, }, { name: "Pass Case: Case-insensitive testing", + path: "./testdata/securitypolicy/00_1byte", files: []string{ "dOCs/SeCuRIty.rsT", }, want: scut.TestReturn{ - Score: 10, + Score: 0, NumberOfInfo: 1, + NumberOfWarn: 3, }, }, } @@ -153,6 +178,18 @@ func TestSecurityPolicy(t *testing.T) { mockRepo := mockrepo.NewMockRepoClient(ctrl) mockRepo.EXPECT().ListFiles(gomock.Any()).Return(tt.files, nil).AnyTimes() + + mockRepo.EXPECT().GetFileContent(gomock.Any()).DoAndReturn(func(fn string) ([]byte, error) { + if tt.path == "" { + return nil, nil + } + content, err := os.ReadFile(tt.path) + if err != nil { + return content, fmt.Errorf("%w", err) + } + return content, nil + }).AnyTimes() + dl := scut.TestDetailLogger{} c := checker.CheckRequest{ RepoClient: mockRepo, @@ -162,7 +199,7 @@ func TestSecurityPolicy(t *testing.T) { res := SecurityPolicy(&c) if !scut.ValidateTestReturn(t, tt.name, &tt.want, &res, &dl) { - t.Errorf("test failed: log message not present: %+v", tt.want) + t.Errorf("test failed: log message not present: %+v on %+v", tt.want, res) } }) } diff --git a/checks/testdata/securitypolicy/00_1byte b/checks/testdata/securitypolicy/00_1byte new file mode 100644 index 000000000000..8b137891791f --- /dev/null +++ b/checks/testdata/securitypolicy/00_1byte @@ -0,0 +1 @@ + diff --git a/checks/testdata/securitypolicy/00_empty b/checks/testdata/securitypolicy/00_empty new file mode 100644 index 000000000000..e69de29bb2d1 diff --git a/checks/testdata/securitypolicy/03_securitypolicy b/checks/testdata/securitypolicy/03_securitypolicy new file mode 100644 index 000000000000..450c3c918987 --- /dev/null +++ b/checks/testdata/securitypolicy/03_securitypolicy @@ -0,0 +1 @@ +now is the time for all goodness for vulnerabilities and disclosures diff --git a/checks/testdata/securitypolicy/03_textOnly b/checks/testdata/securitypolicy/03_textOnly new file mode 100644 index 000000000000..a75492cecf66 --- /dev/null +++ b/checks/testdata/securitypolicy/03_textOnly @@ -0,0 +1 @@ +now is the time for all goodness diff --git a/checks/testdata/securitypolicy/04_textAndDisclosureVuls b/checks/testdata/securitypolicy/04_textAndDisclosureVuls new file mode 100644 index 000000000000..450c3c918987 --- /dev/null +++ b/checks/testdata/securitypolicy/04_textAndDisclosureVuls @@ -0,0 +1 @@ +now is the time for all goodness for vulnerabilities and disclosures diff --git a/checks/testdata/securitypolicy/06_emailOnly b/checks/testdata/securitypolicy/06_emailOnly new file mode 100644 index 000000000000..7ead344a58cf --- /dev/null +++ b/checks/testdata/securitypolicy/06_emailOnly @@ -0,0 +1 @@ +security@example.com diff --git a/checks/testdata/securitypolicy/06_urlAndEmailOnly b/checks/testdata/securitypolicy/06_urlAndEmailOnly new file mode 100644 index 000000000000..bff8aa0585e0 --- /dev/null +++ b/checks/testdata/securitypolicy/06_urlAndEmailOnly @@ -0,0 +1 @@ +https://security.example.com security@example.com diff --git a/checks/testdata/securitypolicy/06_urlOnly b/checks/testdata/securitypolicy/06_urlOnly new file mode 100644 index 000000000000..2786204fedc0 --- /dev/null +++ b/checks/testdata/securitypolicy/06_urlOnly @@ -0,0 +1 @@ +https://security.example.com diff --git a/checks/testdata/securitypolicy/09_linkedContentAndText b/checks/testdata/securitypolicy/09_linkedContentAndText new file mode 100644 index 000000000000..b41b6042f089 --- /dev/null +++ b/checks/testdata/securitypolicy/09_linkedContentAndText @@ -0,0 +1,2 @@ +https://security.example.com security@example.com +now is the time for all goodness diff --git a/checks/testdata/securitypolicy/10_linkedContentAndTextAndDisclosureVuls b/checks/testdata/securitypolicy/10_linkedContentAndTextAndDisclosureVuls new file mode 100644 index 000000000000..6a887ae1d0ec --- /dev/null +++ b/checks/testdata/securitypolicy/10_linkedContentAndTextAndDisclosureVuls @@ -0,0 +1,2 @@ +https://security.example.com security@example.com +now is the time for all goodness for vulnerabilities and disclosures diff --git a/checks/testdata/securitypolicy/10_realworld b/checks/testdata/securitypolicy/10_realworld new file mode 100644 index 000000000000..2083d44cdf90 --- /dev/null +++ b/checks/testdata/securitypolicy/10_realworld @@ -0,0 +1,22 @@ +# Security Policy + +## Security Announcements + +Join the [kubernetes-security-announce] group for security and vulnerability announcements. + +You can also subscribe to an RSS feed of the above using [this link][kubernetes-security-announce-rss]. + +## Reporting a Vulnerability + +Instructions for reporting a vulnerability can be found on the +[Kubernetes Security and Disclosure Information] page. + +## Supported Versions + +Information about supported Kubernetes versions can be found on the +[Kubernetes version and version skew support policy] page on the Kubernetes website. + +[kubernetes-security-announce]: https://groups.google.com/forum/#!forum/kubernetes-security-announce +[kubernetes-security-announce-rss]: https://groups.google.com/forum/feed/kubernetes-security-announce/msgs/rss_v2_0.xml?num=50 +[Kubernetes version and version skew support policy]: https://kubernetes.io/docs/setup/release/version-skew-policy/#supported-versions +[Kubernetes Security and Disclosure Information]: https://kubernetes.io/docs/reference/issues-security/security/#report-a-vulnerability diff --git a/checks/testdata/securitypolicy/10_realworldtwo b/checks/testdata/securitypolicy/10_realworldtwo new file mode 100644 index 000000000000..65b40899abbb --- /dev/null +++ b/checks/testdata/securitypolicy/10_realworldtwo @@ -0,0 +1,11 @@ +# Reporting Security Issues + +To report a security issue, please email +[oss-security@googlegroups.com](mailto:oss-security@googlegroups.com) +with a description of the issue, the steps you took to create the issue, +affected versions, and, if known, mitigations for the issue. + +Our vulnerability management team will respond within 3 working days of your +email. If the issue is confirmed as a vulnerability, we will open a +Security Advisory and acknowledge your contributions as part of it. This project +follows a 90 day disclosure timeline. diff --git a/cron/internal/format/json_raw_results.go b/cron/internal/format/json_raw_results.go index 41f696599170..f5682c4309c3 100644 --- a/cron/internal/format/json_raw_results.go +++ b/cron/internal/format/json_raw_results.go @@ -169,9 +169,9 @@ func addBinaryArtifactRawResults(r *jsonScorecardRawResult, ba *checker.BinaryAr //nolint:unparam func addSecurityPolicyRawResults(r *jsonScorecardRawResult, sp *checker.SecurityPolicyData) error { r.Results.SecurityPolicies = []jsonFile{} - for _, v := range sp.Files { + for idx := range sp.PolicyFiles { r.Results.SecurityPolicies = append(r.Results.SecurityPolicies, jsonFile{ - Path: v.Path, + Path: sp.PolicyFiles[idx].File.Path, }) } return nil diff --git a/docs/checks.md b/docs/checks.md index 1920cc6c5c57..d7aa26503ac3 100644 --- a/docs/checks.md +++ b/docs/checks.md @@ -527,11 +527,33 @@ well-known directories. A security policy (typically a `SECURITY.md` file) can give users information about what constitutes a vulnerability and how to report one securely so that information about a bug is not publicly visible. + +This check examines the contents of the security policy file awarding points +for those policies that express vulnerability process(es), disclosure timelines, +and have links (e.g., URL(s) and email(s)) to support the users. + +Linking Requirements (one or more) (6/10 points): + - A valid form of an email address to contact for vulnerabilities + - A valid form of a http/https address to support vulnerability reporting + +Free Form Text (3/10 points): + - Free form text is present in the security policy file which is beyond + simply having a http/https address and/or email in the file + - The string length of any such links in the policy file do not count + towards detecting free form text + +Security Policy Specific Text (1/10 points): + - Specific text providing basic or general information about vulnerability + and disclosure practices, expectations, and/or timelines + - Text should include a total of 2 or more hits which match (case insensitive) + `vuln` and as in "Vulnerability" or "vulnerabilities"; + `disclos` as "Disclosure" or "disclose"; + and numbers which convey expectations of times, e.g., 30 days or 90 days **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). Follow the [coordinated vulnerability disclosure guidelines](https://github.com/ossf/oss-vulnerability-guide/blob/main/guide.md) to respond to vulnerability disclosures. +- 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). Follow the [coordinated vulnerability disclosure guidelines](https://github.com/ossf/oss-vulnerability-guide/blob/main/maintainer-guide.md) to respond to vulnerability disclosures. - For GitHub, see more information [here](https://docs.github.com/en/code-security/getting-started/adding-a-security-policy-to-your-repository). ## Signed-Releases diff --git a/docs/checks/internal/checks.yaml b/docs/checks/internal/checks.yaml index a421ae558fd6..574cd40fd606 100644 --- a/docs/checks/internal/checks.yaml +++ b/docs/checks/internal/checks.yaml @@ -559,6 +559,29 @@ checks: A security policy (typically a `SECURITY.md` file) can give users information about what constitutes a vulnerability and how to report one securely so that information about a bug is not publicly visible. + + This check examines the contents of the security policy file awarding points + for those policies that express vulnerability process(es), disclosure timelines, + and have links (e.g., URL(s) and email(s)) to support the users. + + Linking Requirements (one or more) (6/10 points): + - A valid form of an email address to contact for vulnerabilities + - A valid form of a http/https address to support vulnerability reporting + + Free Form Text (3/10 points): + - Free form text is present in the security policy file which is beyond + simply having a http/https address and/or email in the file + - The string length of any such links in the policy file do not count + towards detecting free form text + + Security Policy Specific Text (1/10 points): + - Specific text providing basic or general information about vulnerability + and disclosure practices, expectations, and/or timelines + - Text should include a total of 2 or more hits which match (case insensitive) + `vuln` and as in "Vulnerability" or "vulnerabilities"; + `disclos` as "Disclosure" or "disclose"; + and numbers which convey expectations of times, e.g., 30 days or 90 days + remediation: - >- Place a security policy file `SECURITY.md` in the root directory of your @@ -567,7 +590,7 @@ checks: 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). Follow the - [coordinated vulnerability disclosure guidelines](https://github.com/ossf/oss-vulnerability-guide/blob/main/guide.md) + [coordinated vulnerability disclosure guidelines](https://github.com/ossf/oss-vulnerability-guide/blob/main/maintainer-guide.md) to respond to vulnerability disclosures. - >- For GitHub, see more information diff --git a/e2e/security_policy_test.go b/e2e/security_policy_test.go index 8c3bcee86144..856ad474a7a7 100644 --- a/e2e/security_policy_test.go +++ b/e2e/security_policy_test.go @@ -50,7 +50,7 @@ var _ = Describe("E2E TEST:"+checks.CheckSecurityPolicy, func() { Error: nil, Score: checker.MaxResultScore, NumberOfWarn: 0, - NumberOfInfo: 1, + NumberOfInfo: 4, NumberOfDebug: 0, } result := checks.SecurityPolicy(&req) @@ -76,7 +76,7 @@ var _ = Describe("E2E TEST:"+checks.CheckSecurityPolicy, func() { Error: nil, Score: checker.MaxResultScore, NumberOfWarn: 0, - NumberOfInfo: 1, + NumberOfInfo: 4, NumberOfDebug: 0, } result := checks.SecurityPolicy(&req) @@ -102,7 +102,7 @@ var _ = Describe("E2E TEST:"+checks.CheckSecurityPolicy, func() { Error: nil, Score: checker.MaxResultScore, NumberOfWarn: 0, - NumberOfInfo: 1, + NumberOfInfo: 4, NumberOfDebug: 0, } result := checks.SecurityPolicy(&req) @@ -128,7 +128,7 @@ var _ = Describe("E2E TEST:"+checks.CheckSecurityPolicy, func() { Error: nil, Score: checker.MaxResultScore, NumberOfWarn: 0, - NumberOfInfo: 1, + NumberOfInfo: 4, NumberOfDebug: 0, } result := checks.SecurityPolicy(&req) @@ -165,7 +165,7 @@ var _ = Describe("E2E TEST:"+checks.CheckSecurityPolicy, func() { Error: nil, Score: checker.MaxResultScore, NumberOfWarn: 0, - NumberOfInfo: 1, + NumberOfInfo: 4, NumberOfDebug: 0, } result := checks.SecurityPolicy(&req) diff --git a/pkg/json_raw_results.go b/pkg/json_raw_results.go index 637f18ab0643..42d071aa4abb 100644 --- a/pkg/json_raw_results.go +++ b/pkg/json_raw_results.go @@ -221,6 +221,19 @@ type jsonTokenPermission struct { Type string `json:"type"` } +type jsonSecurityFile struct { + Path string `json:"path"` + Hits []jsonSecurityPolicyHits `json:"matches,omitempty"` + ContentLength uint `json:"contentLength,omitempty"` +} + +type jsonSecurityPolicyHits struct { + Type string `json:"type"` + Match string `json:"match,omitempty"` + LineNumber uint `json:"lineNumber,omitempty"` + Offset uint `json:"offset,omitempty"` +} + //nolint:govet type jsonRawResults struct { // Workflow results. @@ -239,7 +252,7 @@ type jsonRawResults struct { Binaries []jsonFile `json:"binaries"` // List of security policy files found in the repo. // Note: we return one at most. - SecurityPolicies []jsonFile `json:"securityPolicies"` + SecurityPolicies []jsonSecurityFile `json:"securityPolicies"` // List of update tools. // Note: we return one at most. DependencyUpdateTools []jsonTool `json:"dependencyUpdateTools"` @@ -587,11 +600,23 @@ func (r *jsonScorecardRawResult) addBinaryArtifactRawResults(ba *checker.BinaryA //nolint:unparam func (r *jsonScorecardRawResult) addSecurityPolicyRawResults(sp *checker.SecurityPolicyData) error { - r.Results.SecurityPolicies = []jsonFile{} - for _, v := range sp.Files { - r.Results.SecurityPolicies = append(r.Results.SecurityPolicies, jsonFile{ - Path: v.Path, - }) + r.Results.SecurityPolicies = []jsonSecurityFile{} + if len(sp.PolicyFiles) > 0 { + for idx := range sp.PolicyFiles { + r.Results.SecurityPolicies = append(r.Results.SecurityPolicies, jsonSecurityFile{ + Path: sp.PolicyFiles[idx].File.Path, + ContentLength: sp.PolicyFiles[idx].File.FileSize, + Hits: []jsonSecurityPolicyHits{}, + }) + for _, entry := range sp.PolicyFiles[idx].Information { + r.Results.SecurityPolicies[idx].Hits = append(r.Results.SecurityPolicies[idx].Hits, jsonSecurityPolicyHits{ + Type: string(entry.InformationType), + Match: entry.InformationValue.Match, + LineNumber: entry.InformationValue.LineNumber, + Offset: entry.InformationValue.Offset, + }) + } + } } return nil }