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

New feature: Exempt vulnerabilities for single archives (digests) #319

Merged
merged 36 commits into from
May 11, 2020

Conversation

henrikplate
Copy link
Contributor

@henrikplate henrikplate commented Jan 6, 2020

Until release 3.1.6, vulnerabilities could be exempted by combining the configuration settings vulas.report.exceptionExcludeBugs and vulas.report.exceptionExcludeBugs.<ID>, whereby the former enumerates all vulnerability identifiers that should not cause a build exception, and the latter was used to provide a corresponding reason for every identifier <ID>, e.g.,

vulas.report.exceptionExcludeBugs = ABC, 123
vulas.report.exceptionExcludeBugs.ABC = Lorem ipsum
vulas.report.exceptionExcludeBugs.123 = Foo bar

The feature implemented with this PR simplifies and extends exemptions: First, the enumeration is not required any longer. Second, the exemption can be done for single archives (by specifying their digest) or all archives (by using *)

The following example exempts

  • vulnerability ABC, no matter the affected library (indicated by the wildcard *), and
  • vulnerability XYZ for the archives with digest 123 and 456.
vulas.report.exemptBug.ABC.reason = Lorem ipsum
vulas.report.exemptBug.ABC.libraries = *

vulas.report.exemptBug.XYZ.reason = Foo bar
vulas.report.exemptBug.XYZ.libraries = 123, 456

Moreover, the configuration used to exempt entire scopes changes to vulas.report.exemptScope.

TODOs

  • Tests
  • Documentation

@henrikplate henrikplate added the wip Work in progress - Don't merge label Jan 7, 2020
@henrikplate
Copy link
Contributor Author

To be tested: (De)serialization of VulnerableDependency#exemption and information included in result reports

"modifiedBy": null,
"cvssDisplayString": "7.5 (v2.0)"
},
"constructList": [
Copy link
Contributor

Choose a reason for hiding this comment

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

I am not sure about how this test file was created but I think it comes from running the code as in the serialization of the vulnerableDependency the View to be used was missing and thus the constructList was serialized even if we never did it as it's not required by the client at this step.

}

// Deprecated format
final String[] bugs = _cfg.getStringArray(DEPRECATED_CFG_PREFIX);
Copy link
Contributor

Choose a reason for hiding this comment

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

At this step old exemptions existing in the db are not kept into account as their key does not contain the prefix vulas. and thus doesn't match DEPRECATED_CFG_PREFIX

Copy link
Contributor

@serenaponta serenaponta left a comment

Choose a reason for hiding this comment

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

There is a mismatch between old config keys where the prefix vulas. was removed and the new ones. As a result exemptions saved with older clients are not considered. This would also impact the way exemptions are read from Configuration objects vs the Map.
I also added a test to show that the digest/reasons values returned passing from the Map are different than those obtained via the Configuration objects

I can also do the changes but first I want to agree on the prefix.

@serenaponta
Copy link
Contributor

serenaponta commented Apr 24, 2020

Still something is not working as expected as the JSON from the test system contains the following (but JUnit tests are successfully checking that the content is correct)

"exemption": {
"bugId": "CVE-2020-9548",
"digest": "dig",
"reason": "3490508379D065FE3FCB80042B62F630F7588606= testing new feature"
},

This happens when separators are not escaped to be part of the key (https://commons.apache.org/proper/commons-configuration/javadocs/v1.10/apidocs/index.html?org/apache/commons/configuration/PropertiesConfiguration.html)
e.g. vulas.report.exemptBug.CVE-2020-1234.dig:ABCD= testing new feature

Also, to double check that vulas:report only exempts a certain CVE for the specified digest.


// New setting
if(_map.containsKey(CFG)) {
final String[] scopes = _map.get(DEPRECATED_CFG).split(",");
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess this should be final String[] scopes = _map.get(CFG).split(",");

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, I just pushed it.

}
else {
is_exempted = is_exempted &&
(purl.getNamespace()==null || libid.getMvnGroup().equals(purl.getNamespace())) && // No purl.namespace || purl.namespace==libid.mvnGroup
Copy link
Contributor

Choose a reason for hiding this comment

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

Why should we allow exemptions for library ids whose namespace (aka mvn group) is not specified?

Copy link
Contributor Author

@henrikplate henrikplate Apr 28, 2020

Choose a reason for hiding this comment

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

Package URLs for Python packages will not have a namespace. Alternatively, we can consider the type, but such if-else conditions would not be nice either. Moreover, I do not know whether the prg. language can be easily determined at this point in time.

if(lib.startsWith("pkg:")) {
try {
final PackageURL purl = new PackageURL(lib);
if(purl.getName()==null) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is linked to the previous comment, why not requiring that also the namespace is not null?

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
wip Work in progress - Don't merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants