-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
vulnsrc_rhel: one vulnerability by CVE #499
Conversation
ext/vulnsrc/rhel/rhel.go
Outdated
vulnerabilities = append(vulnerabilities, vulnerability) | ||
|
||
// One vulnerability by CVE | ||
for _, reference := range definition.References { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's put this logic in the name function.
Also, let's try to do this and if there isn't a Source reference, we can failover to the RHSA value.
The same logic can be applied to links in the link function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that the name function was convenient when there was only one vulnerability to deal with.
This for loop creates one vulnerability for each CVE referenced by the RHSA (ie: (len(references) - 1) vulnerabilities).
Maybe I could iterate on definition.References on line 188 and call name and link with the reference instead of the definition?
Something like:
for _, definition := range ov.Definitions {
pkgs := toFeatures(definition.Criteria)
if len(pkgs) > 0 {
var affected []database.AffectedFeature
for _, p := range pkgs {
affected = append(affected, p)
}
for _, reference := range definition.References {
if reference.Source == "CVE" {
vulnerability := database.VulnerabilityWithAffected{
Vulnerability: database.Vulnerability{
Severity: severity(definition),
Description: description(definition),
Name: name(reference),
Link: link(reference),
},
Affected: affected,
}
vulnerabilities = append(vulnerabilities, vulnerability)
}
}
}
}
And then failover to a unique vulnerability with RHSA ID if there are no CVE at all.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That sounds reasonable.
vulnerability := database.VulnerabilityWithAffected{ | ||
Vulnerability: database.Vulnerability{ | ||
Name: name(definition), | ||
Link: link(definition), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we could leave this code the same, but rename the functions rhsaName(def)
and rhsaLink(def)
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree on that, we can default to RHSA name and link.
Severity: severity(definition), | ||
Description: description(definition), | ||
}, | ||
} | ||
for _, p := range pkgs { | ||
vulnerability.Affected = append(vulnerability.Affected, p) | ||
} | ||
vulnerabilities = append(vulnerabilities, vulnerability) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see the reasoning behind removing this line and moving it anywhere else.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have to move this line because of the new behavior.
I'll try to explain with an example.
These are the references for an RHSA:
> <reference source="RHSA" ref_id="RHSA-2015:1207-00" ref_url="https://rhn.redhat.com/errata/RHSA-2015-1207.html"/>
> <reference source="CVE" ref_id="CVE-2015-2722" ref_url="https://access.redhat.com/security/cve/CVE-2015-2722"/>
> <reference source="CVE" ref_id="CVE-2015-2724" ref_url="https://access.redhat.com/security/cve/CVE-2015-2724"/>
> <reference source="CVE" ref_id="CVE-2015-2725" ref_url="https://access.redhat.com/security/cve/CVE-2015-2725"/>
The actual behavior is to create only one vulnerability and name it "RHSA-2015:1207" which references these 3 CVE.
If we want to name the vulnerabilities with the CVE ID, we now have to create the 3 following vulnerabilities:
That's why I iterate over definition.References and append a new vulnerability for each loop.
vulnerabilities = append(vulnerabilities, vulnerability) | ||
|
||
// Only RHSA is present | ||
if len(definition.References) == 1 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we default to rhsaName and rhsaLink, we could make this code a little more simple by making it along the lines of
// Prefer the CVE name and link if available.
if len(definition.References) > 1 {
for _, ref := range definition.References {
if ref.Source == "CVE" {
vulnerability.Name = ref.ID
vulnerability.Link = ref.URI
}
}
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's take the same example as above:
> <reference source="RHSA" ref_id="RHSA-2015:1207-00" ref_url="https://rhn.redhat.com/errata/RHSA-2015-1207.html"/>
> <reference source="CVE" ref_id="CVE-2015-2722" ref_url="https://access.redhat.com/security/cve/CVE-2015-2722"/>
> <reference source="CVE" ref_id="CVE-2015-2724" ref_url="https://access.redhat.com/security/cve/CVE-2015-2724"/>
> <reference source="CVE" ref_id="CVE-2015-2725" ref_url="https://access.redhat.com/security/cve/CVE-2015-2725"/>
If we do that we will only create one vulnerability (CVE-2015-2725) and lose the data for the 2 others CVE.
ext/vulnsrc/rhel/rhel.go
Outdated
vulnerability.Link = definition.References[0].URI | ||
vulnerabilities = append(vulnerabilities, vulnerability) | ||
} else { | ||
for _, reference := range definition.References[1:] { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't have to test if (ref.Source == "CVE") since all the references starting from the index 1 are CVE.
We create one vulnerability for each CVE.
As append copies the vulnerability, we can reuse the same object for each loop and only change the name and the link.
|
||
func rhsaLink(def definition) (link string) { | ||
if len(def.References) > 0 { | ||
link = def.References[0].URI |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The RHSA is always the first reference.
|
||
// Only RHSA is present | ||
if len(definition.References) == 1 { | ||
vulnerabilities = append(vulnerabilities, vulnerability) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only one reference, it's the RHSA. We append the default vulnerability.
Let's take 3 examples to cover all cases and explain the behavior Example1:
We will only create one vulnerability:
Example2:
We'll create one vulnerability: Example3:
We'll create 3 vulnerabilities: |
This change looks good. Can you add a test for the Example 3 case? Thanks a bunch. I'm sorry for the delay and any confusion. |
Sure, I'll add a new test to cover all the cases. |
I've slightly changed the behavior. |
Sorry about the delay. If you can rebase this change, I will merge it immediately. |
Get one vulnerability by CVE_ID for RHEL instead of one by RHSA_ID so we can have NVD metadata added to the vulnerabilities. Fixes quay#495
If no CVE is present, create a vulnerability with rhsa ID
Code reorganisation
delete a useless line
Add test for multiple CVE
use the specific CVE's impact field instead of the RHSA's one
d169adf
to
c4ffa0c
Compare
Done :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thank you for your patience. I'm really glad to get this merged.
Get one vulnerability by CVE_ID for RHEL instead of one by RHSA_ID so we can have NVD metadata added to the vulnerabilities.
Fixes #495