jeremylong / Open-Vulnerability-Project

Java libraries for working with available vulnerability data sources (GitHub Security Advisories, NVD, EPSS, CISA Known Exploited Vulnerabilities, etc.)
Apache License 2.0
107 stars 30 forks source link

fix: workaround lack of CVSSv4 support with consistently lenient JSON parsing #165

Closed chadlwilson closed 2 weeks ago

chadlwilson commented 3 weeks ago

As noted in https://github.com/jeremylong/DependencyCheck/issues/6747 NVD have added cvssMetricV40 to their API which breaks this library as it is using strict deserialisation in many places it shouldnt.

This change helps workaround/fix https://github.com/jeremylong/DependencyCheck/issues/6747 and https://github.com/jeremylong/DependencyCheck/issues/6746 in the short term, before #163 is implemented.

chadlwilson commented 3 weeks ago

@jeremylong Before merging this, could you please take a look at the comment at https://github.com/jeremylong/Open-Vulnerability-Project/issues/163#issuecomment-2198098898 ?

It's possible that we should only mark Metrics, Reference and CveItem as ignoring properties if we instead want to match their schema directly rather than making an arbitrary decision to be lenient everywhere. I hadn't looked at the specific changes made for https://nvd.nist.gov/general/news/api-cves-schema-restriction-update and in #158 when making this change originally.

Your call.

jeremylong commented 3 weeks ago

I think we should only ignore properties on Metrics, Reference and CveItem.

jeremylong commented 3 weeks ago

I'll update the objects to support 4 shortly.

aikebah commented 3 weeks ago

@jeremylong as indicated in the thread on #163 any schema-type that does not include "additionalProperties: false" is by definition communicating that additional properties may be added without violating the JSON scheme (JSON is by-design lenient, contrary to XML XSDs, where this lenient behaviour needs to be explicitly opted in to by adding an <xs:any/> to any type-definition you want to be able to silently expand with new elements)

So for any type that represents a JSON structure for which the schema does not explicitly forbid extension (additionalProperties: false) the mapping should be coded to ignore unknown properties

chadlwilson commented 3 weeks ago

@jeremylong as indicated in the thread on #163 any schema-type that does not include "additionalProperties: false" is by definition communicating that additional properties may be added without violating the JSON scheme (JSON is by-design lenient, contrary to XML XSDs, where this lenient behaviour needs to be explicitly opted in to by adding an <xs:any/> to any type-definition you want to be able to silently expand with new elements)

So for any type that represents a JSON structure for which the schema does not explicitly forbid extension (additionalProperties: false) the mapping should be coded to ignore unknown properties

I'll try and go through and review this, at least for the NVD stuff.

I don't know if there are actually schemas for KEV and EVSS but IMHO things should default to leniency, and that's the way most APIs on the internet are expected to be consumed (which is why the default in JSON Schema is to allow it).

chadlwilson commented 3 weeks ago

only ignore additional properties on the references, cve_item, and metrics.

As @aikebah enumerates, there are more than these three to fix as some others were strict when they should not be, allowing for possible future bugs. However, I have updated and rebased to remove @JsonIgnoreProperties(ignoreUnknown = true) ONLY where an explicit schema exists that says to disallow additional properties for that element.

Summary for non NVD:

Summary for NVD

Let me know :-)

chadlwilson commented 2 weeks ago

Hiya @jeremylong - would it be possible to focus on trying to get a fix out for ODC independently of/prior to supporting CVSS v4? Right now more people are probably blocked by having ODC fall over rather than worrying about CVSS v4, and it might be wise to decouple the two.

jeremylong commented 2 weeks ago

I wanted to add this one last - after I added support for CVSS v4.

chadlwilson commented 2 weeks ago

OK, rebased.

I was afraid that the CVSS v4 support might cause other unexpected problems (esp if they change their API again) and we are in a position we we have no working version of dependency check for longer. :-(