feat: Add Root.io vulnerability source support #546
Conversation
|
|
- Remove defaultPut function and custom put field from VulnSrc - Simplify RootIOFeed structure to store individual CVE entries - Remove PatchData and Patch types in favor of direct types.Advisory usage - Refactor save/commit flow to handle platform-based feed organization - Fix variable naming: feed -> platformFeeds for clarity
There was a problem hiding this comment.
Is this the same format as the one used in the vuln-list? https://api.root.io/external/patch_feed has a distribution key.
There was a problem hiding this comment.
Youre right. I've made some changes and didnt retest it. The correct format is what the API presents, not what the test has
pkg/vulnsrc/rootio/rootio.go
Outdated
|
|
||
| // Generate bucket name: "root.io {baseOS} {osVer}" | ||
| bucket := fmt.Sprintf(platformFormat, string(vs.baseOS), osVer) | ||
| advisories, err := vs.dbc.GetAdvisories(bucket, pkgName) |
There was a problem hiding this comment.
@chait-slim I think we also need to get advisories from the distributor's bucket and merge them with Root.io advisories as below.
https://github.com/aquasecurity/trivy/blob/1885e35fa6d9d2fce66556af258bbd6048a226d1/pkg/detector/ospkg/rootio/vulnsrc.go#L42-L73
Otherwise, Trivy detects only vulnerabilities for which Root.io distributes a patch. Am I missing something?
There was a problem hiding this comment.
If I understand you correctly the result here should be:
- Find the distros advisory <- This will always exist since we are using distros already created in the db. But it means Root needs to run last on update
- Update the advisory with the Root data
Does that make sense?
There was a problem hiding this comment.
There are following cases:
- Root.io feed includes
- Case 1: Only Root.io has a patch
- Case 2: Both Root.io and Debian have a patch
- Debian feed includes
- Case 3: Debian doesn't have a patch, but affected
- 3-1: But Root.io has a patch (same as case 1)
- 3-2 Root.io also doesn't have a patch
- Case 4: Debian has a patch
- 4-1: Root.io also has a patch (same as case 2)
- 4-2: Root.io doesn't have a patch
We think that Root.io feed will not include 3-2 and 4-2 cases.
That is why we assume:
take distros and Root.io advisories and merge them (Root.io has high priority).
in other words, for cases when Root.io doesn't have advisory for vulnID - Trivy will check this vulnID from the vendor bucket and show it (if it exists).
Do you agree with this behavior?
or do you plan to enrich the feed with advisories for which you do not have fixes yet?
There was a problem hiding this comment.
Understood the question and think it's the correct behaviour as you suggest- we don't enrich the feed with additional CVEs we don't have fixes for.
However we do at times enrich the feed in a use case not listed here explicitly: places where the distro has not have a fix and does not report the CVE at all, and root does have a fix and will report both the CVE + FIX (Alpine for example does not include it's unfixed CVE in it's feed, some other distros have a similar pattern)
There was a problem hiding this comment.
IIUC you say about case 1.
Debian doesn't have patch, Root.io has patch.
In this case after merge advisories Trivy will show vulnerability from Root.io
There was a problem hiding this comment.
@DmitriyLewen Just to validate, the comment here:
// Merge the advisories from the original distributors with Root.io's advisories
Its not really merging but more: override distro-only fix if root fix exists. Correct?
There was a problem hiding this comment.
@DmitriyLewen Looks great, thanks. I have one comment on the tests: the test json does not have any root fixes/range with root versions. My concern there is that it might not be testing the real use case. A real example of a snippet from a json:
{ "pkg": { "name": "openssl", "cves": { "CVE-2024-13176": { "vulnerable_ranges": [ "<3.0.15-1~deb12u1.root.io.1", ">3.0.15-1~deb12u1.root.io.1 <3.0.16-1~deb12u1" ], "fixed_versions": [ "3.0.15-1~deb12u1.root.io.1", "3.0.16-1~deb12u1" ] } } } },
There was a problem hiding this comment.
@DmitriyLewen Just to validate, the comment here:
// Merge the advisories from the original distributors with Root.io's advisories
Its not really merging but more: override distro-only fix if root fix exists. Correct?
We merge maps, so same keys will be overwrited.
But i added comment in f6595cc
There was a problem hiding this comment.
@DmitriyLewen Looks great, thanks. I have one comment on the tests: the test json does not have any root fixes/range with root versions. My concern there is that it might not be testing the real use case. A real example of a snippet from a json:
I added this test case - 55d3c0d
also i see that actual feed uses another format.
I said about same case here - aquasecurity/vuln-list-update#357 (comment)
Are you updated cases with ||, right?
There was a problem hiding this comment.
Thanks for catching this - The feed needs to be updated with your request to move from || to seperate lines in an array.
We will do that shortly - you are correct that it should be:
"vulnerable_ranges": [
"<2.9.14+dfsg-1.3~deb12u1.root.io.3",
">2.9.14+dfsg-1.3~deb12u1.root.io.3 <2.9.14+dfsg-1.3~deb12u1.root.io.5"
],
We will update the feed accordingly.
pkg/vulnsrc/rootio/rootio.go
Outdated
| eb := oops.In("rootio").With("root_dir", rootDir).With("base_os", vs.baseOS) | ||
|
|
||
| var feeds []RootIOFeed | ||
| err := utils.FileWalk(rootDir, func(r io.Reader, path string) error { |
There was a problem hiding this comment.
IIUC it should be single file:
https://github.com/aquasecurity/vuln-list-update/blob/d3bc87388df9cbce20081af06ba382093adf173d/rootio/rootio.go#L72-L76
Do we need run utils.FileWalk?
And testdata contains separated files
There was a problem hiding this comment.
I'm also wondering if the test data format is correct.
#546 (comment)
There was a problem hiding this comment.
@chait-slim I've updated the test data, but correct me if I'm wrong.
304e759#diff-9e8538cadd0a1396b248749743bb093061e0ab915d0270a364c37e7d2a6c9322
There was a problem hiding this comment.
You are correct @knqyf263 . Sorry for this misalignment
- Remove custom OSType enum and use standard types.SourceID instead - Rename RootIOFeed to Feed and RawRootIOFeed to RawFeed for simplicity - Update constructor and switch statements to use vulnerability constants - Update vulnsrc.go to use vulnerability constants instead of rootio.OSType - Simplify type structure by leveraging existing trivy-db types
- Update TestVulnSrc_Update to use vulnsrctest.TestUpdate with proper expected values - Add TestVulnSrc_Get using vulnsrctest.TestGet pattern from debian_test.go - Add error handling test case with broken.yaml fixture for GetAdvisories failure - Consolidate test fixtures into happy.yaml with correct bucket/pairs format - Replace old JSON test data with new format test-feed.json covering all OS types - Update fixture format from FixedVersion to PatchedVersions - Add VulnerabilityID to expected test results for proper validation - Ensure comprehensive test coverage for both success and failure scenarios
- Remove unused Option type and option handling from NewVulnSrc - Simplify constructor to directly return VulnSrc struct - Clean up unnecessary complexity as no options are currently needed
- Change URL from https://root.io/ to https://api.root.io/external/patch_feed - Update test expectations to match new URL
- take and merge baseOS advisories - add tests
| "name": "openssl", | ||
| "cves": { | ||
| "CVE-2023-0464": { | ||
| "vulnerable_ranges": [">=1.1.1, <1.1.1t"], |
There was a problem hiding this comment.
@DmitriyLewen The tests here are missing a Root fix. This is not an issue per-se, but I'm concerned they dont test a real use case. For example here, add a fixed version: 1.1.1t-1+deb11u1.root.io.1
There was a problem hiding this comment.
I think the file was just copied from vuln-list-update.
I updated test files in vuln-list-update - aquasecurity/vuln-list-update@c04ca88
and used this file in this PR - 55d3c0d
- use real versions for update test - add testcase for Get fucntion with multiple Vulnerable versions.
- for ubuntu advisory - for alpine advisory
- split VulnSrc and VulnSrcGetter - take baseOS from feed in Update function - update list all VulnSrcs - update test files
- add list of supported OSes - save only supported OSes - add testcase for unsupported OS
There was a problem hiding this comment.
Pull Request Overview
This PR adds support for Root.io as a new vulnerability source to the system. Key changes include:
- Integration of a new Root.io vulnerability source, including registration in the system and constant definitions.
- New data structures, tests, and fixtures for processing Root.io API feeds across multiple OS types.
- Updates to related test utilities and DB interface definitions to support Root.io advisories.
Reviewed Changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| pkg/vulnsrctest/vulnsrctest.go | Updated TestGet signature to use db.Getter. |
| pkg/vulnsrc/vulnsrc.go | Imported and registered the Root.io vulnerability source. |
| pkg/vulnsrc/vulnerability/const.go | Added RootIO constant to the vulnerability sources list. |
| pkg/vulnsrc/rootio/types.go | Introduced data types for handling Root.io feed formats. |
| pkg/vulnsrc/rootio/testdata/* | Added test fixtures for supported, unsupported, and broken cases. |
| pkg/vulnsrc/rootio/rootio_test.go | Added tests for update and get operations for Root.io advisories. |
| pkg/vulnsrc/rootio/rootio.go | Implemented the Root.io vulnerability source with update and get logic. |
| pkg/db/db.go | Extended the DB interface with the Getter definition. |
DmitriyLewen
changed the title
|
@knqyf263 I'm seeing some potential issues post merge that I want to better understand. Examples:
Both of those have fixed versions in our feed but they are using the "source" naming in Trivy - zlib and glibc. There are other similar examples, but in general should we be adding in our feed the names of libraries for what you capture in the "SrcName" or the more generic "Name" under "Packages" or both? We can make the change necessary in the feed without any additional changes on your end as it's just changing the names or adding new rows. cc: @chait-slim |
|
Hello @benjifin So I think it's correct to use the same logic. but perhaps @knqyf263 will correct me. |
|
Yes, most OS vendors use source package names in security advisories. So, it's fine for you to distribute source package names only in your feed. However, there are some exceptions, like Red Hat. They use binary package names, which means Trivy also needs to use binary package names. If you plan to add Red Hat support, we would need to talk about it. |
|
So good news for us in a way @knqyf263 - but "bad" news is that we don't seem to be recognizing any of our fixed versions in the images I tested - and if it's not a problem with package deceleration, then it's potentially a bit more tricky to fix. |
|
We modified the existing image for testing. If you have an official Root.io image, we can give it a try. |
|
Can you try using: |
|
Yes, it's pullable. @DmitriyLewen Can you take a look? |
|
I created fix for ➜ trivy -q image rootioinc/common-elasticsearch:9.0.1-bookworm-slim_rootio --table-mode detailed | grep zip -A 6
...
│ zip │ CVE-2018-13410 │ │ │ 3.0-13 │ │ Info-ZIP Zip 3.0, when the -T and -TT command-line options │
│ │ │ │ │ │ │ are used,... │
│ │ │ │ │ │ │ https://avd.aquasec.com/nvd/cve-2018-13410 │
├────────────────────┼─────────────────────┼──────────┼──────────────┼────────────────────────────┼───────────────┼──────────────────────────────────────────────────────────────┤
│ zlib1g │ CVE-2023-45853 │ CRITICAL │ will_not_fix │ 1:1.2.13.dfsg-1.root.io.1 │ │ zlib: integer overflow and resultant heap-based buffer │
│ │ │ │ │ │ │ overflow in zipOpenNewFileInZip4_6 │
│ │ │ │ │ │ │ https://avd.aquasec.com/nvd/cve-2023-45853 │
└────────────────────┴─────────────────────┴──────────┴──────────────┴────────────────────────────┴───────────────┴──────────────────────────────────────────────────────────────┘
after: ➜ ./trivy -q image rootioinc/common-elasticsearch:9.0.1-bookworm-slim_rootio --table-mode detailed | grep zip -A 3
│ zip │ CVE-2018-13410 │ CRITICAL │ │ 3.0-13 │ │ Info-ZIP Zip 3.0, when the -T and -TT command-line options │
│ │ │ │ │ │ │ are used,... │
│ │ │ │ │ │ │ https://avd.aquasec.com/nvd/cve-2018-13410 │
└──────────────────┴─────────────────────┴──────────┴──────────┴────────────────────────────┴───────────────┴──────────────────────────────────────────────────────────────┘
|
|
Thanks! Should we check on an Alpine and Ubuntu image as well? I can send over some coordinates. |
|
This issue should not affect Alpine images, but it is best to double-check all supported distributions.
We are already merging the fix. After merging the PR, I will write you a commit with the fix (to build the binary) and a canary image with this fix (depending on which approach is more convenient for you). |
|
Checked on Alpine and is working as expected :) |
|
You can track this milestone. |
|
Checked with canary on |
5 participants
Summary
Implementation Details
Key Features: