Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add CVE Information to Release Notes #1441

Merged
merged 4 commits into from
Mar 1, 2021
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion pkg/notes/document/document.go
Original file line number Diff line number Diff line change
Expand Up @@ -253,7 +253,10 @@ func New(
cve.LinkedPRs = append(cve.LinkedPRs, prid.(int))
}
}
// TODO: Add a validation function to check the struct
// Verify that CVE data has the minimum fields defined
if err := cve.Validate(); err != nil {
return nil, errors.Wrapf(err, "checking CVE map file for PR #%d", pr)
}
doc.CVEList = append(doc.CVEList, cve)
}

Expand Down
6 changes: 3 additions & 3 deletions pkg/notes/document/template.go
Original file line number Diff line number Diff line change
Expand Up @@ -68,12 +68,12 @@ filename | sha512 hash

This release contains changes that address the following vulnerabilities:
{{range .}}
### {{.ID}} {{.Title}}
### {{.ID}}: {{.Title}}

{{.Description}}

CVSS Rating: {{.CVSSRating}} ({{.CVSSScore}}) {{.CVSSVector}}
Tracking Issue: {{.TrackingIssue}}
**CVSS Rating:** {{.CVSSRating}} ({{.CVSSScore}}) {{.CVSSVector}}<br>
**Tracking Issue:** {{.TrackingIssue}}

{{ end }}
{{- end -}}
Expand Down
38 changes: 38 additions & 0 deletions pkg/notes/notes.go
Original file line number Diff line number Diff line change
Expand Up @@ -1086,3 +1086,41 @@ func (rn *ReleaseNote) ContentHash() (string, error) {
}
return fmt.Sprintf("%x", h.Sum(nil)), nil
}

// Validate checks the data defined in a CVE map is complete and valid
func (cve *CVEData) Validate() error {
if cve.CVSSRating == "" {
return errors.New("CVSS rating missing from CVE data")
}

// Check rating is a valid string
if cve.CVSSRating != "None" &&
cve.CVSSRating != "Low" &&
cve.CVSSRating != "Medium" &&
cve.CVSSRating != "High" &&
cve.CVSSRating != "Critical" {
return errors.New("Invalida CVSS rating")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo -> Invalid

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✔️ Thanks!

}

if cve.CVSSVector == "" {
return errors.New("CVSS vector string missing from CVE data")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a library that can validate the form is the expected CVSS:3.1/AV:N/AC:H/PR:H/UI:R/S:U/C:H/I:H/A:H sort of form?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm trying this one and seems to do the job just fine: https://github.com/spiegel-im-spiegel/go-cvss

}

if cve.CVSSScore == 0 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should validate versus the known range of 0..10.

return errors.New("CVSS score missing from CVE data")
}

if cve.ID == "" {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this validate more that the id is of the expected form for a CVE?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✔️ Added a basic regexp validation

return errors.New("ID missing from CVE data")
}

if cve.Title == "" {
return errors.New("Title missing from CVE data")
}

if cve.Description == "" {
return errors.New("CVE description missing from CVE data")
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can see the new TrackingIssue perhaps being empty, but can't tell if that's intended. A couple changes to consider depending on if it is required or not

  • in notes.go CVEData comment the TrackingIssue as "Optional link to the vulnerability tracking issues"
  • add a validator here to require it to be non-empty (or set it to "n/a"?)
  • conditionally print the "Tracking Issue: {{.TrackingIssue}}" portion of the template only if non-empty

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've modified it to make the field optional. Once it is in use we can decide if we enforce it.

return nil
}