Rework VulnerabilityResolutionReason for CRA / DORA requirements
#10886
Conversation
There are cases where a vulnerability is returned for a given package coordinate, but that package is actually modified compared to the upstream version and contains a fix. This can now be resolved explicitly by using `FIXED_VULNERABILITY`. The requirment for this reason stems from CRA / DORA implications. Signed-off-by: Sebastian Schuberth <[email protected]>
This is a more explicit alternative to `WILL_NOT_FIX_VULNERABILITY` (or also `CANT_FIX_VULNERABILITY`) to document that the reason why a vulnerability was not fixed is that the business risk was accepted. The requirment for this reason stems from CRA / DORA implications. Signed-off-by: Sebastian Schuberth <[email protected]>
Suggest `NOT_EXPLOITABLE_VULNERABILITY` instead as the term "exploitable" is more common in this context than "ineffective". Signed-off-by: Sebastian Schuberth <[email protected]>
Suggest `FALSE_POSITIVE_VULNERABILITY` instead which is a bit more common and general as it does not imply that the false positive was due to an "invalid match". Signed-off-by: Sebastian Schuberth <[email protected]>
|
@willebra, please have a look at this draft. Does this match your specification? |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
model/src/main/kotlin/config/VulnerabilityResolutionReason.kt
Dismissed
Show dismissed
Hide dismissed
model/src/main/kotlin/config/VulnerabilityResolutionReason.kt
Dismissed
Show dismissed
Hide dismissed
model/src/main/kotlin/config/VulnerabilityResolutionReason.kt
Dismissed
Show dismissed
Hide dismissed
model/src/main/kotlin/config/VulnerabilityResolutionReason.kt
Dismissed
Show dismissed
Hide dismissed
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #10886 +/- ##
============================================
+ Coverage 57.53% 57.55% +0.01%
Complexity 1700 1700
============================================
Files 346 346
Lines 12823 12829 +6
Branches 1212 1212
============================================
+ Hits 7378 7384 +6
Misses 4978 4978
Partials 467 467
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Why do we add the |
I agree, but I was just aligning to existing code. |
| * The code in which the vulnerability was found is neither invoked in the project's code nor indirectly | ||
| * via another open source component. | ||
| */ | ||
| @Deprecated("Use NOT_EXPLOITABLE_VULNERABILITY instead", replaceWith = ReplaceWith("NOT_EXPLOITABLE_VULNERABILITY")) |
There was a problem hiding this comment.
Looking at 1, I propose to name it NOT_AFFECTED*.
There was a problem hiding this comment.
@willebra please comment.
There was a problem hiding this comment.
I believe the VEX status document describes primarily vulnerability handling statuses (AFFECTED, UNDER INVESTIGATION), but then has a bit of reasons for why to resolve these (FIXED, NOT AFFECTED). I believe the rationale in VulnerabilityResolutionReason in ORT is about providing a justification for why a vulnerability was resolved. It should not be mixed with handling status. As it provides justifications, and those are valuable parts in proving proper handling of vulnerabilities, this should be more detailed than just on the level of "Fixed" and "Not affected".
As to the NOT_EXPLOITABLE_VULNERABILITY is directly following from EU regulations, where the obligation to address vulnerabilities is tied to the vulnerability being exploitable. Therefore a resolution reason "not_exploitable" clearly ties the decision to these requirements, making it easy for the users to understand what it means, and align in their internal policies with that - since the same docs need to be also aligned with regulations. The EU regulations have not invented this concept, so it is also being used in U.S. CISA's Known Exploited Vulnerabilities and in the Exploit Prediction Scoring System (EPSS). The VEX status document clearly also revolves around the concept of exploitability, so this term is not foreign there either. Possibly NOT_EXPLOITABLE is equal with NOT AFFECTED, but I haven't investigated the VEX status document in detail.
There was a problem hiding this comment.
@fviernau @willebra My first thoughts we similar to Frank to align VulnerabilityResolutionReason closes to VEX enums but as you correctly pointed out VEX is status and our resolution is about the why a vulnerability finding was resolved. This use case is what is vulnerability-analysis's justification in CycloneDX.
Question is do we align with what CycloneDX or CSAF have or do we go our own way? My thinking was that vulnerability resolutions need to be translatable to SBOM so one can in a machine readable standard inform customers why a vulnerability finding / CVE is not applicable. Would like to see a mapping from ORT to CycloneDX/CSAF vulnerability impact justifications.
There was a problem hiding this comment.
This use case is what is vulnerability-analysis's justification in CycloneDX.
These justifications seem to be for triage-results? I.e. not the final result how the issue was fixed, but the potential result of a triage? All of these seem detailed justifications that fall under NOT_EXPLOITABLE_VULNERABILITY. Logically, these should be exit points from the vulnerability management process, and if none can apply, then actual fixing/remediating should occur? What is missing here are the different flavous of "this should be fixed" or "this was fixed", but that is understandable, it these are just exit-points.
This is why we would need end results when fixing/remediating was necessary: FIXED_VULNERABILITY WORKAROUND_FOR_VULNERABILITY, MITIGATED_VULNERABILITY. (In VulnerabilityResolutionReason we don't follow the statuses, just the end-results, so therefore the "this should be fixed" does not belong here.)
(By the way, the more CRA/ISO27001/WG9 way to say fixed, would be REMEDIATED_VULNERABILITY instead of FIXED_VULNERABILITY.)
I'm following/participating in the work of CEN/CENELEC in JT13, WG9, which is creating the standard for vulnerability handling on the assignment of the EU Comission. That will be the foundational standard for the EU CRA vulnerability handling. The justifications proposed in this PR derive from the CRA, but the standards work is still going forward. It seems to remain at the same high level as the current ORT enums and the proposed ORT enums, so the current PR is okay from this perspective.
The standard text has six steps:
- preparation (e.g. SECURITY.MD is in place etc)
- Receipt of vuln information, such as monitoring of vulns databases against SBOMs
- Verification (assessment of notifications, triage) <-- here are not exploitable outcomes but also such as what type of remediation is required, what is the remediation plan, so these requirements do high-light the nature of the CycloneDX-list that is specific to the triage point.
- Remediation (fixing)
- Release
- Post-release
Anyhow, it would be good to satisfy as many schemes for vulnerability handling as possible, i.e. being CycloneDX "compatible" and CRA "compatible". That can be done in code or in process. We need the highest level at least in code. And then when going to detailed reasons, they can be either code - if reasonable - or comments/process. E.g. the current PR's list of enums could be nicely used with the list from CycloneDX, by applying NOT_EXPLOITABLE_VULNERABILITY, and then writing the more specific CycloneDX-justification there as a comment, possibly using the MITIGATED_VULNERABILITY. The high level in this PR seems therefore already aligned with CycloneDX, and then also the other high-level enums are covered, which the highlighed part of the CycloneDX-doc does not cover (they are likely elsewhere there?).
| * The vulnerability exists in a component, but is not exploitable due to the product's specific architecture or | ||
| * configuration. | ||
| */ | ||
| NOT_EXPLOITABLE_VULNERABILITY, |
There was a problem hiding this comment.
Regarding the entires PR, have you considered aligning with statuses defined in 1.
E.g. why should we deviated from that?
There was a problem hiding this comment.
I'm following @willebra's advice / specification here.
There was a problem hiding this comment.
A foundational requirement in the EU CRA is that products must be delivered "without any known exploitable vulnerabilities". So this choice of term makes ORT CRA-ready.
| /** | ||
| * The required fix (e.g. patch, update) has been successfully applied and verified, eliminating the vulnerability. | ||
| */ | ||
| FIXED_VULNERABILITY, |
There was a problem hiding this comment.
The semantics described in the commit message IIUC differs from the one in the KDoc.
I believe KDOC should be aligned.
There was a problem hiding this comment.
Please explain where you feel is a mismatch, because I don't see any after re-reading.
For using this enum value the question boils down to "How can a vulnerability actually be fixed if it is reported by an advisor for that package?". And one of the answers is the example mentioned in the commit message: As vulnerabilities are looked up by package coordinates / purls, not by package content, the advisor cannot know anything about patches being applied while keeping coordinates the same.
There was a problem hiding this comment.
Basically, the advisor reported a false positive, because the identifier of the package is ambiguous. So, could we instead use the one currently named "false positive" ? Also, invalid match seems to fit.
Please explain where you feel is a mismatch
I found it deviates, because the KDoc does not speak about any package context.
For the use case where one has a third party depenceny with a vulnerability included in a product, I guess there is also the possibility that the "FIX" goes into product code, e.g. ensuring that the vulnerable code path is not used.
Looking at all values existing in this enum right now, it seems there is more overlap inbetween several values. I wonder if the enum elements could be cleaned up a bit, such as merging WORKAROUND_FOR_VULNERABILITY, MITIGATED_VULNERABILIY, INEFFECTIVE_VULNERABILITY, NOT_EXPLOITABLE_VULNERABILITY into just NOT_EXPLOITABLE_VULNERABILITY.
What do you think?
|
To add some general context to all of these changes.
|
|
I'm sharing here the research work/definition document that provides a high-level view of the European regulations and then proposes these justifications. I did it internally for Double Open, but I feel sharing it is fair in this context. I created this with the help of Google Gemini. However, I had to do quite a bit of work on top of that, so I'm not assigning all the blame to GG. https://docs.google.com/document/d/1JCTmBwL835tAnuAWzx9jw8QYYPeBPufwp9NnAaQbrwk/edit?tab=t.0 |
I would also prefer to get rid of the |
Maybe we could add the new reasons without the suffix already and then separately decide how the other reasons could be migrated in a way that it is not an immediate breaking change. |
Please have a look at the individual commit messages for the details.