Repository navigation
fix: skip NVD entries whose target_sw names another language runtime - #8834
Akhil-1527 wants to merge 1 commit into
Conversation
|
|
||
| @Test | ||
| void testTargetSoftwareForTheSameRuntimeMatches() { | ||
| assertTrue(ecosystemMatchesTargetSoftware(Ecosystem.JAVA, "java")); |
There was a problem hiding this comment.
This is a question to maintainers and others. Why does it have to match a lower case string and is there a guarantee that the string passed to this function is always lowercase ?
i.e, if i pass
ecosystemMatchesTargetSoftware(Ecosystem.JAVA, "Java") This still asserts true because there is no runtime match to "Java" right ?
There was a problem hiding this comment.
Keeping in mind I had no idea we even had any of this logic here prior to this PR (otherwise I'd have submitted a PR myself ages ago), my understanding is
- technically speaking,for
target_swwithin the CPE naming spec, the values could be of any capitalization but the comparison internally between different CPEs MUST be case sensitive, i.eJava==javawhen comparing. - since the comparison is defined as case insensitive, generally most things are thus lower case in the official CPE directory and affected versions
- Ecosystem is ODC's own calculated/derived concept based on the CPE fields, but worth noting that it predates the enumeration of ecosystems from PURL so is different to PURL's concept.
- So when matching/converting to our own ecosystem it is our choice how to do that matching, but doing it case insensitively is both sensible and consistent with CPE<>CPE literal string comparisons. So we can normalise to whatever we like, and a case insensitive match like is currently done is fine, since we define that all ecosystems are lowercase, and we decide what they mean.
There was a problem hiding this comment.
Would it be better to convert these magic strings into an ENUM ? @Akhil-1527 @chadlwilson
There was a problem hiding this comment.
Yeah, the whole thing is a bit of a mess IMHO, however to some extent classes like Ecosystem form part of a public API of ODC which may be used by others within code, so my understanding is that we generally couldn't just convert the type to an Enum or value type, unless we are pretty confident it's unlikely to be used directly by outside code/plugins/integrations.
Regardless of this, per my other comments, some of the code here is in the wrong place that will make it just as difficult to maintain correctly as the current situation.
In some sense, the reason this has become broken seems to be that the filtering hasn't been properly maintained over time, probably because no-one realised a bunch of filtering hacks were buried inside NvdCveAnalyzer and CveItemOperator, rather than closer to the CPE, the VulnerableSoftware, the CpeAnalyzer, the Ecosystem .... or other relevant domain matching logic.
There was a problem hiding this comment.
I think making any kind of "proper" fix where you consolidate logic from NvdCveAnalyzer and CveItemOperator would only result in breaking changes to these external plugins relying on them (which I think exist primarily to override these custom hacks).
Injecting an orchestrator like class to handle these cases could be one possible way but I am unsure if that will create more problems than it resolves.
I don't have sufficient historical context or domain expertise to propose another viable solution.
There was a problem hiding this comment.
There's a difference to changing behaviour to the interface. And the analyser logic is clearly internal code, despite its lax API (due to the constraints of the java package system for defining API intention).
Its generally only risky for domain types where people might have written custom analyzers that work with them, and where it is used to bind to things like hints. Much of the current logic is in private methods so we can do what we want.
Generally speaking the behaviour of stuff other than the Engine is not a contract, to my knowledge.
There was a problem hiding this comment.
I am not yet an expert on this logic, however I am less confident that the overall approach here is correct, because there is a lot of logic around that I was not aware of, such as cpeEcosystemCache in the database etc, and I have no idea how the existing logic is/was intended to work and its limits.
At the moment from my perspective this PR has the normal "LLM PR" problems if not clearly articulating which problems it fixes vs. does not fix, and the design reasoning for where the changes are made and how it related to other logic. As usual, too much focus on the "what" small picture stuff, not enough on the "why" big picture design. Y'know, the bit LLMs make a mess of if not directed properly.
We should at least be able to answer questions like
- what happens if the CPE value is
-orNA? (and what should happen) - what happens if the CPE value is
*orANY? (and what should happen) - what is happening for a suite of real vulnerabilities? with real upstream analyzers?
In my opinion, we probably shouldnt really have hacks for webjars, because that just opens the door to needing hacks for everything (rubygems distributed via maven central, java extensions inside a ruby gem, all sorts of things). The logic here is in the "wrong" place; only analyzers should really know much about the nature of artifacts they discover are; not the
Javascript and webjars aren't that special; and in most cases when embedded in other artifacts they should be detected independently of the wrapping jar, as shaded - so appear as an embedded npm: or javascript: dependency independently of the wrapping jar/gem/whatever in most cases.
Perhaps it needs to be looked at separately if it's a "real" problem that these thing are false negatives without the hacks here, however to me this is essentially a modelling problem that should be solved earlier in the process, where analyzers populate EvidenceType.ECOSYSTEM or something like that and the whole matching is done earlier in the CpeAnalyzer.
I'd also like to see some analysis/count of the values of target_sw from affected software expressions across a real dump of vulnerabilities so we have some notion of what coverage we java, and can map things appropriately. Right now this mainly assumes that the values are the same as our existing ecosystems, which I am not sure is necessarily true.
For example, most golang vulns for such mult-lang libraries I have anecdotally seen as cpe:2.3:a:grpc:grpc:*:*:*:*:*:go:*:* i.e target_sw = go like https://nvd.nist.gov/vuln/detail/cve-2026-33186. This change will not fix that it seems, because it expects golang, and go is not a known value when normalizing eocsystems, making it incomplete to resolve #7139 because it fails to actually check real data to determine that modelling and mapping of these values is incomplete.
| /** | ||
| * Ecosystems of language runtimes. Target software that names one of these | ||
| * does not apply to a dependency from another one. JavaScript is left out | ||
| * because JavaScript libraries are often bundled in other packages, such as | ||
| * webjars, NuGet packages and gems. | ||
| */ | ||
| private static final Set<String> RUNTIME_ECOSYSTEMS = Set.of(Ecosystem.DOTNET, Ecosystem.GOLANG, | ||
| Ecosystem.JAVA, Ecosystem.PERL, Ecosystem.PHP, Ecosystem.PYTHON, Ecosystem.RUBY, Ecosystem.RUST); | ||
|
|
There was a problem hiding this comment.
I do not like the naming, nor maintaining this enumeration here. In my opinion it should be maintained in the Ecosystem class or CveItemOperator where the rest of the logic is.
With current setup it's not clear
- what have language runtimes got to do with it?
- when it is appropriate to add a new value to this set?
- how/where to handle differences in the real world about how NVD names target_sw vs our Ecosystem concept? do I have to add more hacks like for nodejs?
| * the target software from the NVD. A vulnerability is kept if any of its | ||
| * vulnerable software matches. CveDB caches the list per CPE and shares it, | ||
| * and its vulnerabilities, with other dependencies, so neither is changed. |
There was a problem hiding this comment.
| * the target software from the NVD. A vulnerability is kept if any of its | |
| * vulnerable software matches. CveDB caches the list per CPE and shares it, | |
| * and its vulnerabilities, with other dependencies, so neither is changed. | |
| * the target software from the NVD. |
Public javadoc comments shouldn't contain unnecessary implementation details. Normal LLM slop, unfortunately, models trained mostly on ancient coding practices.
|
Ill open a discussion so that we can discuss how to implement this change correctly. Currently, there are no test cases for these "hacks" also which makes impact/blast radius of any change hard to understand. |
|
@neilscallywag feel free to do so, or we can discuss here or on the linked issue. There are basically no regular contributors outside myself (and I have looked at limited areas) right now so PRs are sometimes a reasonable place to discuss since Jeremy checks them periodically - probably mostly ignores issues etc (since there is so much noise relative to the # of actual code contributors). This weakness is by far the biggest source of ongoing false positives for NVD CVEs so I'm motivated to get it sorted, since I do basically all of the issue & FP triage & suppressions at the moment. Getting this right would clean up a lot. If we're confident this change doesn't make things worse I am happy to continue, but I'd be more confident if the requester engaged. At the moment this looks like the typical drive-by AI PR where the raiser has never used the software in question, which is discouraging - but let's see. If no engagement we can always pick to a new branch and open an alternative PR we can collaborate on. |
|
Ill just type here. I supposed the first step would be to have some kind of regression test cases to capture existing behavior first for these hacks so that any change we make does not break anything or introduce something new. These are things listed in the issue #7139 We can also categorise these "hacks" as ingestion time or scan time. Since they roughly happen before creating the db and then during scan. For ingestion time: For scan time:
I'm not what the best way would be to collapse these into one module as they pertain different stages of the model, maybe it would make more sense to first inventory the actual target_sw values in a recent NVD dataset and classify them? I suppose this PR can be closed for now or should be changed to just add test cases for behavioural coverage instead of code changes. Subsequent smaller PRs can do the clean up once we have decided on a high level design. These are just my 2 cents, I am not an expert in this domain. Edit: I noticed there is a False Positive analyser . But no changes have been made to it for 4 years. |
|
Relevant to @neilscallywag's inventory suggestion, I'm sharing results from a previously completed, bounded, AI-assisted contributor-side inventory of the official NVD JSON 2.0 annual 2026 feed. This covers the raw inventory and a narrow literal comparison, not ecosystem classification or a review verdict on @Akhil-1527's PR. The feed's embedded timestamp was The inventory found:
A second traversal/tokenizer, implemented separately on the contributor side, reproduced those headline counts. Both checks are ours, so this is a self-consistency check, not independent third-party validation. Occurrences can repeat within a CVE, and the 2026 annual feed is neither all NVD history nor a count of scanner false positives, installations, or affected users. For the Go case @chadlwilson raised in review, raw As one data example, the record @chadlwilson cited—CVE-2026-33186—appears in the pinned feed with No uppercase letters appeared in the 85 raw values in this snapshot. The list below is raw inventory, not effective scanner classification: parsing/unescaping, intentional wildcard/native handling, and later analyzer logic can change the result. For example, the feed contains escaped raw values such as All 85 raw values (occurrences / distinct CVEs, sorted by occurrence count)This gives the design discussion a concrete Go mapping/test case. It does not show that any Dependency-Check scan produced a false positive for these CVEs, establish that adding one literal mapping is sufficient, or settle whether handling belongs at ingestion or scan time. I can share the rerunnable extraction method and per-value vulnerable/application breakdowns if useful. |
|
Thanks for the detailed review, and sorry for the slow reply. You're right about On On where it lives: I take the point on webjars. If embedded JS is supposed to be found as its own dependency, the exception just papers over that, so I'll drop it. What I'd like to do next, in this order:
I'll also take the javadoc suggestion. Happy to hold the code changes until the data is posted if that's easier to review. |
Adds tests for what the code does today, before changing it for dependency-check#7139. CveItemOperatorTest covers the ecosystem that extractEcosystem stores for each target_sw value, the vendor and product checks that run first, and the fallback to the base ecosystem. NvdCveAnalyzerTest covers which CVEs and vulnerable software entries a dependency keeps for a matched CPE. Node.js dependencies only keep Node.js, * and - entries, and every other dependency keeps everything, including the CVEs from dependency-check#3876 and dependency-check#6851 that only target Python or Rust.
94442bc to
f5e570b
Compare
|
Step 1 is pushed, so the PR is tests only for now (f5e570b). The Here's step 2, target_sw across every NVD year. I used all 25 annual JSON 2.0 feeds (2002 to 2026), downloaded 2026-10-14 and checked against the sha256 in each
This is what I'd add to the switch (CVE counts across all years):
Language names, package registries and frameworks always map. Host apps follow the PHP entries the switch already has (
Most of what's left says where the software runs rather than what it's written in: The biggest unmapped value that does imply a language is Two things in the data change how I'd build this:
So for the code I'd put the new values in the switch and have the That changes two things for Node.js dependencies. They'd keep entries with unmapped targets like I'll start on the scans next. All 162 target_sw values with at least 5 CVEs (598 in total)
|
|
Here's step 3, the scans. Short version: everything that drops is a CVE for another language, the branch picks up 10 real findings that main misses, and it loses 3 real ones in two ways I go into below. I built the CLI from main (b801bff) and from this branch with the reworked change from my last comment (not pushed yet). Each build loaded the same NVD 2.0 feeds into its own database, then both scanned the same files offline with the same options: experimental analyzers on, and off for OSS Index, Central, the npm, yarn and pnpm audits, KEV, RetireJS, bundle audit and mix audit, plus the .NET assembly and Go module analyzers since there's no dotnet or go on this machine. What I scanned:
I picked most of the manifest packages because their products have CVEs for more than one language, so this hits the affected products much more often than a normal project would. NVD findings across all samples:
For the second row I swapped both packaged suppression files for a single rule that never matches. Dropped. I checked each one against its NVD entries, and each matched only through an entry for another language.
The APM one only shows with suppressions removed. By default the base suppression added for #4168 hides it on both builds. Its note says it's there because Added, all real:
Lost, both real:
Of the 512 products that get a label for the first time, 23 have CVEs with a I looked at counting Everything else:
NVD findings by sample
I can share the manifests and the diff script if anyone wants to rerun this. If the above looks fine, I'll push the code next and rewrite the PR description. |
|
Thanks for the detailed scans, @Akhil-1527. Tagging @chadlwilson and A feed-level check against a saved NVD annual-feed set found vulnerable The two mechanisms seem to need different treatment:
A narrower middle path for this PR could be to leave the persisted For the narrow path, the highest-value regression questions seem to be whether Because you note that the scanned implementation is not pushed yet, could you So I would not leave the reported losses as accepted behavior, but I also would |
|
I would prefer to talk to humans. The agents you two are using seem to be missing the point |
Welcome to my world. 🤦♂️ |
|
Not that the above isn't helpful (and an impressive amount of token commitment!) but it is overwhelming text vomit, and is indeed missing some of the wider points summarised at an appropriate level of abstraction before drilling down into immense detail. |
|
@chadlwilson it might be worth adding some form of agents.md or equivalent |
I'm not a maintainer (just a contributor & triager) so I don't make these calls. Otherwise I'd have a very different code of conduct, personally 😁 |
|
Anyways, I wanted to check if this is an intentional behaviour that I noticed during some testing Let's say some java dependency has a cpe X We call, the first time vulnerabilities are looked up for this CPE in the DB, we get something like CveDB then stores this result in its in memory cache So when |
|
Im pretty sure that mutation is unintentional as its logically inxorrect. I read that above in the initial PR when reviewing. There are many such problems in ODC with intentionally mutable and unintentionally mutable collections, alas. I'll probably cherry pick, amend and submit a bugfix PR independent of this, as theres no need to conflate these. |
|
This is a mess. It was force-pushed over the top of the earlier changes and lost all the earlier reviewed actual production code, and thus all basis for earlier conversation and work put in by real humans. I'm closing this, as this PR is being used as some step 1 / step 2 / step 3 scratchpad by a slop-bot that is now completely out of sync with its title and PR description. That's completely opaque, and an absolutely ridiculous way to try and work with OSS. Don't force push over a PR and turn it into something completely different. If you want to create tests alone, open a new PR that does this. Then stack another (draft) PR on top of it if you're trying to make things independently reviewable and note that it's to be reviewed only after the first one. |
4 participants
Description of Change
NvdCveAnalyzer.ecosystemMatchesTargetSoftwareonly looked attarget_swfor Node.js dependencies and let everything else match. So a CPE match on a Java dependency kept CVEs whose only entries target another runtime, like CVE-2019-7617 (cpe:2.3:a:elastic:apm_agent:*:*:*:*:*:python:*:*) on the Java APM agent (#3876) and CVE-2020-36448 (cpe:2.3:a:cache_project:cache:*:*:*:*:*:rust:*:*) on cache4k-jvm (#6851).Changes:
target_swswitch inCveItemOperator.extractEcosystemis nowCveItemOperator.ecosystemOfTargetSw, used by both the database build and the analyzer.rustwas missing and is added.target_swnames a language runtime (.NET, Go, Java, Perl, PHP, Python, Ruby, Rust) and the dependency is from a different one. Other targets keep matching: wildcards, native ones likelinuxorc/c++, iOS, ColdFusion, unmapped values, and JavaScript and Node.js, since JavaScript libraries ship inside webjars, NuGet packages and gems. Dependencies outside those runtimes (no ecosystem, native, iOS, JavaScript) are not filtered, and Node.js dependencies keep their current rules.filterEcosystemno longer edits its input.CveDB.getVulnerabilitiescaches the list per CPE and hands the same list andVulnerabilityobjects to every dependency with that CPE. With more ecosystems filtering, a Python dependency could remove a CVE that a Java dependency with the same CPE still needs. It now returns a new list. Since aVulnerabilityhas no copy, it also stops removing the non-matching software entries from a kept vulnerability. That pruning only happened for Node.js dependencies, so their reports may now list a CVE's other entries next to the matched one. Happy to keep the pruning with a copy instead if you prefer.Removing the bundled suppressions this makes redundant can be a follow-up PR.
Related issues
cache4k-jvm, even with latest (10.0.3) Dependency Check version #6851Have test cases been added to cover the new functionality?
Yes. A new
NvdCveAnalyzerTestcovers the runtime matching, the Node.js rules and that the cached list is left alone, andCveItemOperatorTestcovers the mapping.mvn -pl core -am testpasses locally (3492 tests in core).