✨ Improved Security Policy Check (ossf#2195)

* ✨ 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> * Repaired Security Policy to correctly use linked content length for evaluation Signed-off-by: Scott Hissam <shissam@gmail.com> * gofmt'ed changes Signed-off-by: Scott Hissam <shissam@gmail.com> * 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 <shissam@gmail.com> * added unit test cases for the new content-based Security Policy checks Signed-off-by: Scott Hissam <shissam@gmail.com> * reverted the direct (mistaken) change to checks.md and updated the checks.yaml for generate-docs Signed-off-by: Scott Hissam <shissam@gmail.com> * ✨ Improved Security Policy Check (ossf#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 <shissam@gmail.com> * 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 <shissam@gmail.com> * 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 <shissam@gmail.com> * 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 <shissam@gmail.com> * Addressed PR comments; added telemetry for policy hits in security policy file to track hits by line number Signed-off-by: Scott Hissam <shissam@gmail.com> * Resolved merge conflict with checks.yaml Signed-off-by: Scott Hissam <shissam@gmail.com> * updated raw results to emit all the raw information for the new security policy check Signed-off-by: Scott Hissam <shissam@gmail.com> * Resolved merge conflicts and lint errors with json_raw_results.go Signed-off-by: Scott Hissam <shissam@gmail.com> * Addressed review comments to reorganize security policy data struct to support the potential for multiple security policy files. Signed-off-by: Scott Hissam <shissam@gmail.com> * 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 <shissam@gmail.com> * added comments regarding the capacity to support multiple policy files and removed unneeded break statements in the code Signed-off-by: Scott Hissam <shissam@gmail.com> * 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 <shissam@gmail.com> * restored reporting full security policy path and filename for policies found in the org level repos Signed-off-by: Scott Hissam <shissam@gmail.com> * Resolved conflicts in checks.yaml for documentation Signed-off-by: Scott Hissam <shissam@gmail.com> * ✨ CLI for scorecard-attestor (ossf#2309) * Reorganize Signed-off-by: Raghav Kaul <raghavkaul@google.com> * Working commit Signed-off-by: Raghav Kaul <raghavkaul@google.com> * Compile with local scorecard; go mod tidy Signed-off-by: Raghav Kaul <raghavkaul@google.com> * Add signing code Heavily borrowed from https://github.com/grafeas/kritis/blob/master/cmd/kritis/signer/main.go Signed-off-by: Raghav Kaul <raghavkaul@google.com> * Update deps * Naming * Makefile Signed-off-by: Raghav Kaul <raghavkaul@google.com> * Edit license, add lint.yml Signed-off-by: Raghav Kaul <raghavkaul@google.com> * checks: go mod tidy, license Signed-off-by: Raghav Kaul <raghavkaul@google.com> * Address PR comments * Split into checker/signer files * Naming convention Signed-off-by: Raghav Kaul <raghavkaul@google.com> * License, remove golangci.yml Signed-off-by: Raghav Kaul <raghavkaul@google.com> * Address PR comments * Use cobra Signed-off-by: Raghav Kaul <raghavkaul@google.com> * Add tests for root command Signed-off-by: Raghav Kaul <raghavkaul@google.com> * Filter out checks that aren't needed for policy evaluation Signed-off-by: Raghav Kaul <raghavkaul@google.com> * Add `make` targets for attestor; submit coverage stats Signed-off-by: Raghav Kaul <raghavkaul@google.com> * Improvements * Use sclog instead of glog * Remove unneeded subcommands * Formatting Signed-off-by: Raghav Kaul <raghavkaul@google.com> * Flags: Make note-name constant and fix messaging Signed-off-by: Raghav Kaul <raghavkaul@google.com> * Remove SupportedRequestTypes Signed-off-by: Raghav Kaul <raghavkaul@google.com> * go mod tidy Signed-off-by: Raghav Kaul <raghavkaul@google.com> * go mod tidy, makefile Signed-off-by: Raghav Kaul <raghavkaul@google.com> * Fix GH actions run Signed-off-by: Raghav Kaul <raghavkaul@google.com> Signed-off-by: Raghav Kaul <raghavkaul@google.com> Signed-off-by: Scott Hissam <shissam@gmail.com> * removed whitespace before stanza for Run attestor e2e Signed-off-by: Scott Hissam <shissam@gmail.com> * resolved code review and doc review comments Signed-off-by: Scott Hissam <shissam@gmail.com> * repaired the link for the maintainer's guide for supporting the coordinated vulnerability disclosure guidelines Signed-off-by: Scott Hissam <shissam@gmail.com> Signed-off-by: Scott Hissam <shissam@gmail.com>
raghavkaul · Feb 9, 2023 · 59f059d · 59f059d
1 parent 9799e6c
commit 59f059d
Show file tree

Hide file tree

Showing 24 changed files with 517 additions and 64 deletions.
diff --git 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
@@ -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
@@ -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
@@ -59,32 +59,34 @@ 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,
 			},
 		},
 		{
 			name: "test_security_policy_4",
 			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,
 			},
 		},
 	}