✨ Improved Security Policy Check (ossf#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 <shissam@gmail.com>

checks/evaluation/security_policy.go

@@ -15,10 +15,60 @@
 package evaluation
 
 import (
+	"fmt"
+
 	"github.com/ossf/scorecard/v4/checker"
 	sce "github.com/ossf/scorecard/v4/errors"
 )
 
+func scoreSecurityCriteria(contentLen, urls, emails, discvuls int) (int, string) {
+	score := 0
+	reason := ""
+
+	// #1: found one linked (email/http) content: score += 3
+	//     rationale: someone to collaborate with or link to
+	//     information (strong for community)
+	if urls >= 1 || emails >= 1 {
+		score += 3
+		reason += "linked content, "
+	}
+
+	// #2: more than one unique (email/http) linked content found: score += 3
+	//     rationale: if more than one link, even stronger for the community
+	if (urls + emails) > 1 {
+		score += 3
+		reason = "multiple " + reason
+	}
+
+	// #3: 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)
+	// TODO: get sum of linked contents (urls and emails here from raw/ checks)
+	//       linkContentLen would become an arg here
+	linkContentLen := 0
+	if contentLen > 1 && (contentLen > linkContentLen) {
+		score += 3
+		reason += "text, "
+	}
+
+	// #4: 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
+		reason += "vulnerability & disclosure instructions"
+	}
+
+	if reason == "" {
+		reason = "nothing"
+	}
+	reason = "security policy contains " + reason
+
+	return score, reason
+}
+
 // 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 {
@@ -32,6 +82,9 @@ func SecurityPolicy(name string, dl checker.DetailLogger, r *checker.SecurityPol
 		return checker.CreateMinScoreResult(name, "security policy file not detected")
 	}
 
+	score := 0
+	reason := ""
+	var err error
 	for _, f := range r.Files {
 		msg := checker.LogMessage{
 			Path:   f.Path,
@@ -43,7 +96,17 @@ func SecurityPolicy(name string, dl checker.DetailLogger, r *checker.SecurityPol
 		} else {
 			msg.Text = "security policy detected in current repo"
 		}
+
+		var contentLen, urls, emails, discvuls int
+		fmt.Sscanf(f.Snippet, "%d,%d,%d,%d", &contentLen, &urls, &emails, &discvuls)
+		score, reason = scoreSecurityCriteria(contentLen, urls, emails, discvuls)
+
 		dl.Info(&msg)
 	}
-	return checker.CreateMaxScoreResult(name, "security policy file detected")
+
+	if err != nil {
+		return checker.CreateRuntimeErrorResult(name, err)
+	} else {
+		return checker.CreateResultWithScore(name, reason, score)
+	}
 }

checks/raw/security_policy.go

@@ -18,6 +18,7 @@ import (
 	"errors"
 	"fmt"
 	"path"
+	"regexp"
 	"strings"
 
 	"github.com/ossf/scorecard/v4/checker"
@@ -44,7 +45,15 @@ func SecurityPolicy(c *checker.CheckRequest) (checker.SecurityPolicyData, error)
 		return checker.SecurityPolicyData{}, err
 	}
 	// If we found files in the repo, return immediately.
+	// TODO: // assert that at this point only 1 file is returned by isSecurityPolicyFile
 	if len(data.files) > 0 {
+		err := fileparser.OnMatchingFileContentDo(c.RepoClient, fileparser.PathMatcher{
+			Pattern:       data.files[0].Path,
+			CaseSensitive: false,
+		}, checkSecurityPolicyFileContent, &data.files)
+		if err != nil {
+			return checker.SecurityPolicyData{}, err
+		}
 		return checker.SecurityPolicyData{Files: data.files}, nil
 	}
 
@@ -70,6 +79,20 @@ func SecurityPolicy(c *checker.CheckRequest) (checker.SecurityPolicyData, error)
 	}
 
 	// Return raw results.
+	if len(data.files) > 0 {
+		filePattern := data.files[0].Path
+		// undo path.Join in isSecurityPolicyFile
+		if data.files[0].Type == checker.FileTypeURL {
+			filePattern = strings.Replace(data.files[0].Path, data.uri+"/", "", 1)
+		}
+		err := fileparser.OnMatchingFileContentDo(dotGitHubClient, fileparser.PathMatcher{
+			Pattern:       filePattern,
+			CaseSensitive: false,
+		}, checkSecurityPolicyFileContent, &data.files)
+		if err != nil {
+			return checker.SecurityPolicyData{}, err
+		}
+	}
 	return checker.SecurityPolicyData{Files: data.files}, nil
 }
 
@@ -85,8 +108,10 @@ var isSecurityPolicyFile fileparser.DoWhileTrueOnFilename = func(name string, ar
 	}
 	if isSecurityPolicyFilename(name) {
 		tempPath := name
+		// TODO: really should be FileTypeText (.md, .adoc, etc)
 		tempType := checker.FileTypeSource
 		if pdata.uri != "" {
+			// TODO: is joining even needed?
 			tempPath = path.Join(pdata.uri, tempPath)
 			tempType = checker.FileTypeURL
 		}
@@ -112,3 +137,85 @@ 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) != 1 {
+		return false, fmt.Errorf(
+			"checkSecurityPolicyFileContent requires exactly one argument: %w", errInvalidArgLength)
+	}
+	pfiles, ok := args[0].(*[]checker.File)
+	if !ok {
+		return false, fmt.Errorf(
+			"checkSecurityPolicyFileContent requires argument of type *[]checker.File: %w", errInvalidArgType)
+	}
+
+	if len(content) == 0 {
+		// perhaps there are more policy files somewhere else,
+		// keep looking (true)
+		return true, nil
+	}
+
+	// TODO: there is an assertion here that if content has length
+	//       then (*pfiles)[0] is not nil (can that be checked)
+	//       (so is this test necessary?)
+	if (*pfiles) != nil {
+		// preserve file type
+		tempType := (*pfiles)[0].Type
+		urls, emails, discvuls := countUniquePolicyHits(string(content))
+		contentMetrics := fmt.Sprintf("%d,%d,%d,%d", len(content), urls, emails, discvuls)
+		(*pfiles)[0] = checker.File{
+			Path:    path,
+			Type:    tempType,
+			Offset:  checker.OffsetDefault,
+			Snippet: contentMetrics,
+		}
+	}
+
+	// stop here found something, no need to look further (false)
+	return false, nil
+}
+
+func countUniquePolicyHits(policyContent string) (int, int, int) {
+	var urls, emails, discvuls int
+
+	// 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))`)
+
+	rURL := reURL.FindAllString(policyContent, -1)
+	rEML := reEML.FindAllString(policyContent, -1)
+	rDIG := reDIG.FindAllString(policyContent, -1)
+
+	urls = countUniqueInPolicy(rURL)
+
+	emails = countUniqueInPolicy(rEML)
+
+	// not really looking unique sets of numbers or words
+	// and take the raw count of hits
+	discvuls = len(rDIG)
+
+	return urls, emails, discvuls
+}
+
+//
+// Returns a count of unique items in a slice
+// src inspiration: https://codereview.stackexchange.com/questions/191238/return-unique-items-in-a-go-slice
+//
+func countUniqueInPolicy(strSlice []string) int {
+	count := 0
+	keys := make(map[string]bool)
+	for _, entry := range strSlice {
+		if _, value := keys[entry]; !value {
+			keys[entry] = true
+			count += 1
+		}
+	}
+	return count
+}

docs/checks.md

@@ -523,6 +523,9 @@ 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.
 
 **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.