-
Notifications
You must be signed in to change notification settings - Fork 455
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
fix: improve handling of triage data #4160
Conversation
@r-vdp It seems like the test are failing due to difference in expected and actual severity of test_exploit.py I looked into the changes not sure what exactly is causing it, also we use gitlint as one of the linter so "fix: handling of triage data" both in commit message and PR title should fix gitlint fail and black should be fixed by running black filename.py. |
Before this change, there were two issues when using an SBOM file together with a VEX file for triage: 1. new CVEs for a product for which there were existing CVEs in the triage file, were not added to the triage file 2. triage info recorded in the triage file was overwritten when cve-bin-tool was executed with both the `--triage-file` and `vex` options. This commit fixes both issues by: 1. still scanning for CVEs even if the product was already present in the triage file. Before we would not scan for CVEs as soon as we found that the product was already present, but by doing so we might miss new CVEs since the last time we did a scan. 2. merge recorded triage info into the CVEs that we found for the SBOM components. In order to properly identify products, I implemented the hash and eq methods on the `ProductInfo` type to not consider the `location` field as this field does not seem to be populated in a consistent manner.
2045d28
to
7ce4c03
Compare
I pushed a commit that should fix the tests. There are others that fail locally for me, but I think that might be an environment issue, let's see. I think we'll need to refactor the scanner a bit though because the current state is quite complex, and I feel that we don't have the best data model to track the CVE data obtained from different sources and from the triage. I think that it would be better to make this more explicit in the data model so that we can make more informed decisions on which source should get priority when they don't agree. We probably also want to add a test case for the issue that this PR resolves, so that it doesn't get broken again. I'm not sure that I'll have time to do either of these in the short term. |
@r-vdp Thanks for working on this! I think it's valuable to have this as it is even if we probably need a refactor and a test later. Did you want to file an issue with some notes about what you think we'll need in the refactor? I can file an issue about the test when this merges if you want. Did you want to tidy up the linter issues to get this into a mergeable state, or were you hoping one of us could handle that? I'm happy to do it if you're too busy, but I don't want to push to your branch if you're still working on this! |
Hi @terriko, thanks for your feedback! Yeah I do think that we can merge before the refactor indeed, but the scanning loop does feel rather brittle right now, so I wouldn't be surprised that we find other bugs still. For this PR, I have been looking at the remaining test that's failing, but I don't yet understand what's the issue. If you have some time, maybe it would be clearer for you what's going on there? Otherwise I can spend some more time on that next week. The linting issues are easy to fix, I think, we can do that once the tests are passing, I just didn't give that priority so far because I wanted to focus on the failing test, and I need to check how to run the linters locally so that I can verify that it's ok. Probably also next week. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Marking this as needed changes so I won't keep glancing at it.
Also, you should have an invite for a cve-bin-tool-read team . Accepting it will allow CI on your pull requests to run automatically so you can iterate faster without having to wait for me to approve CI on the PR every time you make a change. I realize that's not entirely obvious from the name, but that's what it's about!
Not sure what's happening with the cve scan here. Our current version of plotly.js is v2.27.1 and the latest CVE I see for that component is https://www.cvedetails.com/cve/CVE-2023-46308/ that's for versions < 2.25.2. So, the good news is it's probably not a component that actually needs an upgrade, but the bad news is that I'm not sure why this would be showing up at all. I've got to run to a meeting, but I'll try to do some investigation later today. If you just want it to work for now, feel free to throw a triage file in there to make the test pass while we figure it out. |
row_dict = dict(row) | ||
row_dict.update(triage) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@terriko the plotly.js is coming from the:
Line 76 in 2941eef
"ref": "urn:cbt:1/plotly#plotly.js-2.13.2" |
triage.json being used in test_requirement.py as triage-input since the code mentioned tries to scan for the cves even if the triage data is present, apologies for not interpreting the changes in greater depth , cve_scanner is a little complex to read but this particular line is what i suspect is causing error hope it helps!
so i mostly got it, firstly the changes are useful as it is in my opinion and can be used exactly, secondly the issue is arising due to the new found vulnerability in the plotly since this pr change s to scan both vex and the other data for cves, earlier the vex was not being scanned for cves as mentioned so the triage.json file was commited in 2022 and the cve is reported for CVE-2023-46308 was published 4 Jan 2024, also the version of plotly in triage file is quite older then the current version being used. the test passes when we also add the data for CVE-2023-46308 in the triage file with not affected i mean almost pass since it is not affected with space in between. either of two things would fix the error:
|
removed plotly data field for fixing error with #4160
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, @mastersans has removed the problematic triage entry in #4267 so i think this is safe to merge. We'll likely want to handle old triage more seamlessly (we're still debating what sort of message and what config options we need for ignoring) but that can come in a future PR.
Thanks again for working on this @r-vdp and sorry it took so long to get it merged!
Darn, won't let me merge without re-running tests, so I'll be back when they're done (or more likely, tomorrow) |
That's great! No worries at all, I also got swamped with other things, so thanks a lot to both of you for fixing this up! |
Description
Before this change, there were two issues when using an SBOM file
together with a VEX file for triage:
triage file, were not added to the triage file
cve-bin-tool was executed with both the
--triage-file
andvex
options.
This commit fixes both issues by:
the triage file. Before we would not scan for CVEs as soon as we
found that the product was already present, but by doing so we might
miss new CVEs since the last time we did a scan.
components.
In order to properly identify products, I implemented the hash and eq
methods on the
ProductInfo
type to not consider thelocation
fieldas this field does not seem to be populated in a consistent manner.
Fixes #4158
Checklist