Skip to content

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

Merged
jeremylong merged 2 commits intojeremylong:mainfrom
chadlwilson:lenient-parsing
Jun 29, 2024
Merged

fix: workaround lack of CVSSv4 support with consistently lenient JSON parsing#165
jeremylong merged 2 commits intojeremylong:mainfrom
chadlwilson:lenient-parsing

Conversation

@chadlwilson
Copy link
Copy Markdown
Contributor

@chadlwilson chadlwilson commented Jun 29, 2024

As noted in jeremylong/DependencyCheck#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 jeremylong/DependencyCheck#6747 and jeremylong/DependencyCheck#6746 in the short term, before #163 is implemented.

@chadlwilson chadlwilson changed the title Enable lenient JSON parsing everywhere Fix lack of CVSSv4 support with consistent lenient JSON parsing Jun 29, 2024
@chadlwilson chadlwilson changed the title Fix lack of CVSSv4 support with consistent lenient JSON parsing fix: Workaround lack of CVSSv4 support with consistently lenient JSON parsing Jun 29, 2024
@chadlwilson chadlwilson changed the title fix: Workaround lack of CVSSv4 support with consistently lenient JSON parsing fix: workaround lack of CVSSv4 support with consistently lenient JSON parsing Jun 29, 2024
Copy link
Copy Markdown
Owner

@jeremylong jeremylong left a comment

Choose a reason for hiding this comment

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

LGTM

@chadlwilson

This comment was marked as resolved.

@jeremylong
Copy link
Copy Markdown
Owner

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

@jeremylong
Copy link
Copy Markdown
Owner

I'll update the objects to support 4 shortly.

Copy link
Copy Markdown
Owner

@jeremylong jeremylong left a comment

Choose a reason for hiding this comment

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

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

Comment thread vulnz/src/main/java/io/github/jeremylong/vulnz/cli/model/BasicOutput.java Outdated
Comment thread vulnz/src/main/java/io/github/jeremylong/vulnz/cli/model/BasicOutput.java Outdated
@aikebah
Copy link
Copy Markdown
Contributor

aikebah commented Jun 29, 2024

@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

This comment was marked as resolved.

@chadlwilson chadlwilson force-pushed the lenient-parsing branch 3 times, most recently from 330e38d to e63fafe Compare June 29, 2024 11:41
@chadlwilson
Copy link
Copy Markdown
Contributor Author

chadlwilson commented Jun 29, 2024

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

  • CveItem had already been changed earlier, despite NVD not changing its schema
  • References changed
  • Metrics changed
  • Inlined comment responses for Config, CpeMatch, Node, CvssV2Data, CvssV3Data` which had values earlier which did not match their schema entries.

Let me know :-)

@chadlwilson chadlwilson requested a review from jeremylong June 29, 2024 12:05
@chadlwilson chadlwilson marked this pull request as ready for review June 29, 2024 12:23
@chadlwilson
Copy link
Copy Markdown
Contributor Author

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
Copy link
Copy Markdown
Owner

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

Ignore additional properties for all models where they do NOT have
a JSON schema that declares "additionalProperties: false" for the
element/node.
Copy link
Copy Markdown
Owner

@jeremylong jeremylong left a comment

Choose a reason for hiding this comment

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

LGTM

@chadlwilson
Copy link
Copy Markdown
Contributor Author

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. :-(

@jeremylong jeremylong merged commit e5f28dc into jeremylong:main Jun 29, 2024
@chadlwilson chadlwilson deleted the lenient-parsing branch June 29, 2024 15:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

The lastest dependecy check release version is taking more than 40 min utes to build ot check for nvd updates any reason? is something wrong?

3 participants