-
Notifications
You must be signed in to change notification settings - Fork 148
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(oracle-oval): Support multiple ELSAs per CVE #221
fix(oracle-oval): Support multiple ELSAs per CVE #221
Conversation
This is notably important for when there are additional FIPS and ksplice package patches available
Hello @bpfoster What do you think about ELSAs with different fixed versions(non
from ELSA-2021-9306:
from ELSA-2021-9362:
|
@DmitriyLewen Ah yes, that is a good point. What if we revert the change to the advisory-details key back to CVE ID, and instead change the structure of the value? For example, the two cases we're talking about would then be:
and
Collapsing CVE-2021-23133 into a single fixed version would require scanning and merging all the advisories ahead of saving them to the database, and could use While we're here, we may also want to add architecture support as the ELSAs can contain information for multiple architectures? I'm not sure if the versions ever actually differ between the archs. Pushed up another commit with changes as described above. Think this should work for these cases. |
- Simplify data structures - Reduce loops - Refactor out functions - Cleanup syntax
ksplice1 and ksplice2 are not different flavors. ksplice2 is a second revision to the userspace ksplice support code. X.ksplice2 > X.ksplice1
I'm so sorry to be late.
We have it as Trivy DB is now version 2.
As you said, we have to maintain multiple versions. We already maintain Trivy DB v1 and v2 and it is already hard. Adding v3 makes it harder. We should avoid it as much as possible. What if we use ELSA-ID as a key? Did you already discuss it? I'm sorry if I missed it.
It increases the database size since we cannot reuse the CVE-ID detail in vulnerability-detail. We may want to have CveIDs.
But it doesn't work if there are several fixed versions in the same ELSA-ID. |
I did initially take that approach. There were some complications with it and I switched back to using CVE as the key. It is most likely feasible to use ELSA ID as the key, but I think we have come to a solution with CVE ID as the key that works for all identified cases and still preserves pretty good backwards compatibility. Is there something I have missed there that ELSA ID as the key solves? I see a couple concerns with the schema you've proposed:
Keeping CVE ID as the key yields more familiar results on an older client:
All that said, if you have suggestions on how to improve this, I'm happy to make them so we can move this forward. |
@knqyf263 @DmitriyLewen I apologize if my last reply was too strong. I'd be happy to make some changes here if there's something you'd like done. |
@knqyf263 @DmitriyLewen checking in... anything needed here? Thanks! |
@knqyf263 if i remember correctly - if we use ELSAs we still need to change db structure. Because ELSA can include multiple versions (e.g. default, fips and ksplice) and we can't save them in FixedVersion field. We need to use a different field for this, ut old Trivy uses this field to get version. @bpfoster am i right? |
@DmitriyLewen I don't think there's anything technically restricting ELSAs from including multiple flavors (default, fips, and ksplice), but it looks like it's Oracle's practice to split them into separate ELSAs. All the one's I've looked at are specific to the flavor. While out of scope of this change, I would like to point out that the ELSAs support multiple architectures, and there are cases that a single ELSA has different versions for different architectures. See ELSA-2022-4803 for example, which has Trivy does not currently support architectures for Oracle Linux, but does for RedHat. If we change the key to ELSA ID and leave the database schema as-is with just a If the changes in this PR are merged in and you have interest in it, I would be happy to submit a change request to add architecture support in the future. If you'd rather fix it all at once, I'd be happy to incorporate that into this PR as well. From that standpoint, we could use ELSA ID as the key, and use the modified value schema to support this, if there is benefit in using ELSA ID as the key instead of CVE ID. It is unfortunate that we duplicate the version of the default package in the value, but I'm not sure there's an alternative that both provides the flexibility to solve all these issues and maintains backwards compatibility. I did have one concern with using ELSA ID as the key, regardless of the structure of the value. Because the existing oracle vulnsrc does not translate the DB vulnerability ID to CVE ID like the redhat one does, the report on an older client will show the ELSA ID as the vulnerability ID instead of CVE ID, and won't include the vulnerability information:
I hope that clears up my earlier comments. If I'm wrong on any analysis, please feel free to correct me, and I'm happy to make any changes requested. Looking forward to getting this resolved so I can stop getting false positives on FIPS versions 😄 . |
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.
research on the choice of structure and dimensions of the various options can be seen here - #452
versions := lo.Values(fixedVers) | ||
slices.Sort(versions) | ||
|
||
fixedVersion := fixedVers[NormalPackageFlavor] |
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.
I'm not sure about this case.
Should we use fips
or ksplice
version here?
example - https://linux.oracle.com/errata/ELSA-2016-3515.html
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.
The current DB contains fixed versions for fips
or ksplice
in this case, right? If so, we should keep it for compatibility.
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.
This is not entirely correct (which is why this PR was created), but it is only needed for backward compatibility, so let's leave the previous logic
Updated in cb14132
Ksplice1PackageFlavor PkgFlavor = "ksplice1" | ||
Ksplice2PackageFlavor PkgFlavor = "ksplice2" |
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.
We don't use ksplice1
flavor for ksplice2
(and vice versa) in Trivy:
https://github.com/aquasecurity/trivy/blob/dd0a64a1cf0cd76e6f81e3ff55fa6ccb95ce3c3d/pkg/detector/ospkg/oracle/oracle.go#L72-L77
So i separated ksplice1
and ksplice2
.
After these changes db size increased to 0.9MB
:
➜ du -sh ./assets-with-flavors
539M ./assets-with-flavors
➜ du -sh ./assets
540M ./assets
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.
It's been long enough that I've lost track of all the details on this, but I did want to pass along some information I got from @tvierling (from the Oracle Linux support/development group) when I was working though this. I won't quote his email directly without his permission, but the gist of it is that ksplice
is the flavor, and the number after ksplice
is just a revision / build number (which should be considered.. x.y.z.ksplice2
> x.y.z.ksplice1
). So I don't think you want to separate out as different flavors.
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.
Hello @bpfoster
I'm glad you're still here and helping with this task.
Sorry we're working on this after so long...
Also, I want to thank you for your work! Our work with you helped me remember the details and update this PR.
but the gist of it is that ksplice is the flavor, and the number after ksplice is just a revision / build number (which should be considered.. x.y.z.ksplice2 > x.y.z.ksplice1). So I don't think you want to separate out as different flavors.
I originally used your PackageFlavor
function.
But I was confused that we separate ksplice1 and ksplice2 in Trivy and i updated PR.
But since the person from Oracle Linux says that we shouldn't divide them, then there is a mistake in our logic Trivy.
Thanks! I will revert last commit.
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.
done - a135499
This reverts commit ed3cdbf.
@santhosh1729 Can you also please review this PR? |
@santhosh1729 If it looks good to you, please approve this PR just for the record that Aqua approves it. |
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.
Looks almost good. Left a nit comment
return nil, xerrors.Errorf("failed to unmarshal advisory JSON: %w", err) | ||
} | ||
|
||
// For backward compatibility |
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.
This is needed only when users keep using old databases, right? I don't think people use the old database for a month. We can set a deadline.
// For backward compatibility | |
// For backward compatibility (This code can be deleted after Dec 19th, 2024) |
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.
You are right.
Updated.
I'm waiting for approval from @santhosh1729. |
@knqyf263 changes look good to me, It resolves the the issue that we are experiencing |
@DmitriyLewen Did you confirm the new DB works with the current Trivy? |
…val-multi-versions-fix
Trivy v0.57.1 correctrly works with new DB: ➜ 7954 trivy -q image oraclelinux:9
oraclelinux:9 (oracle 9.4)
Total: 20 (UNKNOWN: 0, LOW: 13, MEDIUM: 7, HIGH: 0, CRITICAL: 0)
┌───────────────────────┬────────────────┬──────────┬────────┬─────────────────────────┬───────────────────────┬───────────────────────────────────────────────────────────┐
│ Library │ Vulnerability │ Severity │ Status │ Installed Version │ Fixed Version │ Title │
├───────────────────────┼────────────────┼──────────┼────────┼─────────────────────────┼───────────────────────┼───────────────────────────────────────────────────────────┤
│ expat │ CVE-2024-50602 │ MEDIUM │ fixed │ 2.5.0-2.el9_4.1 │ 2.5.0-3.el9_5.1 │ libexpat: expat: DoS via XML_ResumeParser │
│ │ │ │ │ │ │ https://avd.aquasec.com/nvd/cve-2024-50602 │
├───────────────────────┼────────────────┤ │ ├─────────────────────────┼───────────────────────┼───────────────────────────────────────────────────────────┤ |
Thanks! Merging. |
@knqyf263 i updated aquasecurity/trivy#7858 |
This is notably important for when there are additional FIPS and ksplice package patches available.
Adjust persisted advisory details object to be more like the RedHat object - each CVE key contains an array of
Entries
- each entry contains the fix version and associated ELSAs for different package flavors.We keep the
FixedVersion
field at the top of the object to preserve backwards compatibility with older clients.Since ELSA title and description can be different between the two, do not specify those in the vulnerability details, failing back to the defaults.
Inspecting the database for something like
Oracle Linux 8 / gnutls
, you can see it now changes from:to:
See that previously, CVEs CVE-2021-20231, CVE-2021-20232 and CVE-2021-3580 only had fix versions for the FIPS package. We now have fix version for both the FIPS package and the normal one.
This also now supports multiple ELSAs per CVE, rolling up to the latest fixed version. For example:
Much like FIPS, Ksplice handling is also improved and accounted for:
fixes #220