ADD cve report support - #188
Conversation
build_query_command answers about a known package list, which is what lock file generation needs. Correlating installed packages with CVEs needs the opposite: the full dependency closure, since a vulnerable package is usually one nothing declared. Add build_query_all_command alongside it, reusing the same RPM_ETCCONFIGDIR/RPM_CONFIGDIR handling so the SDK sysroot — the one case with no --root — keeps working through a single code path. Output is NAME<tab>VERSION-RELEASE<tab>ARCH, matching the PKGV-PKGR a Yocto build records. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
emit_json_object is shared with the NDJSON event stream, where every object must stay on one line for consumers that parse per line. A single-shot command emitting a large document has no such constraint, and one line makes it unreviewable by hand. Add emit_json_object_pretty for that case, documented as unsafe for event streams, and leave the existing wire format untouched. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The per-machine JSON published by the build supplies package -> recipe -> CVEs. One container run dumps every sysroot's RPM database, and results are reported per scope: sdk, rootfs, initramfs, target-sysroot, runtime:<name>, ext:<runtime>/<name>. CVEs are stored once per recipe and deduplicated across scopes. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Eveything was wrote and tested with python first, then used Claude Opus 5 to rewrite to Rust and add the necessary tests to it. Tested against the feed produced by avocado-linux/meta-avocado#252 with the x86 QEMU machine. |
jetm
left a comment
There was a problem hiding this comment.
Reviewed avocado cve report cold. Twelve findings inline, most severe first. The command is well shaped and the doc comments are unusually good about naming what they defend against - which is why the top three matter: each is a path where a scan that did not happen reports as a scan that found nothing.
The first two are the same failure seen from both ends. query_root swallows rpm's exit status with || true and DISCOVER_SCRIPT sets only set -u, so a container where every rpm -qa failed still exits 0; then parse_sysroots drops the resulting empty scopes with retain, so the failure leaves no trace at all. A rootfs that could not be read comes out as [SUCCESS] 0 unpatched CVE(s) with the scope count silently one lower. Third is the same shape at the document level: an empty recipes map is accepted, correlates to zero CVEs, and is indistinguishable from a clean image - and a producer that ran without cve-check inherited emits exactly that.
Three smaller notes not worth their own thread:
SourceReport::status is #[serde(default)], so a document without it prints [SUCCESS] 12 CVE(s) across 3 scope(s). with a doubled space and no qualifier. It is also never validated, so a document generated with status = "Patched" correlates and reports identically - a user handed the wrong file sees already-fixed CVEs presented as live findings. Separately, with_composed_config (report.rs:251) has no caller in the diff and is silenced with #[allow(dead_code)] rather than omitted; main.rs:3196 builds the command without it. And the CHANGELOG entry lists six scopes but DISCOVER_SCRIPT also emits includes (line 58) and the un-prefixed ext:<name> compat form (line 71), so a consumer keying on the documented set drops those.
Some things I checked and am deliberately not filing, so they do not get re-raised: emit_json_object_pretty ignoring write errors matches the established emit_json_event convention verbatim; the kernel sysroot is correctly omitted from the discovery script, since rootfs/install.rs:114-190 shows it holds only copied Image files with no queryable rpmdb and the kernel package is scanned in the rootfs; JsonOutputGuard does correctly suppress print_*, because tui_is_active() ORs in is_json_output_active() and those helpers write to stdout; and __SDK_QUERY__ is substituted before __QUERY_FORMAT__ with no token in its expansion, so the two replace calls cannot corrupt each other.
Two things about how this was reviewed. The PR head is not fetched into any local clone - refs/pull/188/head does not resolve - so the three new files were reviewed as-diffed. For report.rs and both test files the diff is the complete file (@@ -0,0 +1,912 @@), so coverage of the new code is total; the helpers it calls I read from the base repo on disk, which is on container-dev-mode (2c8e854) rather than necessarily this PR's merge base. Nothing was built or run. Findings 1, 2, 3, and 5 are mechanically certain from the code as written; 9 and 10 are marked plausible inline with the reason.
The gap that would most change this review: I did not read the producer (bitbake avocado-cve-report, presumably in meta-avocado). Three things hinge on it - whether recipes is guaranteed to hold only CVE-bearing recipes, whether status can take values other than "Unpatched", and whether a PKGV containing a hyphen (say 1.0-rc1) can reach split_version at report.rs:195, where rsplit_once('-') would mis-split it against RPM's hyphen-free %{VERSION} and produce a spurious version_mismatch on every such package. Worth confirming that last one before merge; it is cheap to check and would be noisy in the field.
| }); | ||
| } | ||
|
|
||
| sysroots.retain(|s| !s.packages.is_empty()); |
There was a problem hiding this comment.
Empty scopes are dropped, so a failed rpm query vanishes
parse_sysroots ends with sysroots.retain(|s| !s.packages.is_empty()), which erases any sysroot whose rpm -qa produced no lines. A scope that failed to be read becomes indistinguishable from one that was never installed, and invisible in both the human table and the JSON scopes map.
A project has rootfs, runtime:dev, and the SDK installed. The rootfs RPM database is in a format the SDK's rpm cannot open (bdb-vs-sqlite backend mismatch), so rpm -qa --root=$AVOCADO_PREFIX/rootfs writes an error to stderr and nothing to stdout. The ##SCOPE\trootfs\t... marker was already printed, so a Sysroot with zero packages is built and then silently discarded here. sysroots is still non-empty, so the bail! at line 279 does not fire. Output: [SUCCESS] 0 unpatched CVE(s) across 2 scope(s)., exit 0, and JSON counts.scopes says 2.
The entire shipped root filesystem was never scanned and nothing in the output says so. This is the precise false-clean the module's own doc comments claim to defend against. Keeping the empty scope and reporting it as unscanned costs one field.
| esac | ||
| SEEN="$SEEN $real" | ||
| printf '##SCOPE\t%s\t%s\n' "$scope" "$real" | ||
| (unset RPM_ETCCONFIGDIR RPM_CONFIGDIR; rpm -qa --root="$root" __QUERY_FORMAT__) || true |
There was a problem hiding this comment.
|| true hides every rpm failure from the caller
query_root swallows rpm's exit status, and DISCOVER_SCRIPT sets only set -u (not set -e), so the script exits 0 even when every rpm invocation failed. run_in_container_with_output (src/utils/container.rs:1446-1467) returns Ok(None) and prints a failure notice only when out.success is false, so it sees a successful run and reports nothing.
The container image's rpm is missing, or the state volume is mounted read-only. Every rpm -qa exits non-zero, || true masks each one, the final for loop exits 0, the container exits 0. out.success is true, so the helper returns Ok(Some("")) with no diagnostic. The command then hits the bail! at line 279 and tells the user "Run avocado install first" - a confidently wrong diagnosis for a broken container. In the partial case above it does not bail at all.
Worth contrasting with the sibling build_query_command at src/utils/lockfile.rs:190, where || true is justified by a documented, narrow reason: rpm -q returns non-zero when any named package is absent. rpm -qa has no such excuse - its non-zero exit is always a real error.
| let source: SourceReport = serde_json::from_str(&raw) | ||
| .with_context(|| format!("Failed to parse CVE report '{}'", self.file))?; | ||
|
|
||
| if source.packages.is_empty() { |
There was a problem hiding this comment.
Empty recipes map accepted as a clean scan
load_source rejects an empty packages map but applies no equivalent check to recipes, and the unit test at line 896 explicitly blesses {"recipes": {}, "packages": {...}} as legitimate.
The Yocto side runs bitbake avocado-cve-report on a build where cve-check was not inherited, or where the NVD database fetch timed out. The producer still enumerates all 1,400 packages into packages but writes "recipes": {}. load_source passes because packages is non-empty. correlate looks up every package's recipe, misses every time at line 677, and returns an empty cves map. Output: [SUCCESS] 0 unpatched CVE(s) across 5 scope(s)., exit 0, counts.cves: 0, counts.packages_unknown: 0 - indistinguishable from a genuinely clean image.
The module rejects a missing recipes key for exactly this reason (line 92 comment). An empty one is the far likelier producer failure mode and gets no guard.
| // Matched by name. A different upstream version means the report may | ||
| // not describe this package at all; a different revision means the | ||
| // same sources were repackaged, which can still change patches. | ||
| if !source_package.version.is_empty() && !installed.version.is_empty() { |
There was a problem hiding this comment.
Missing version fields silence the mismatch check
The version-drift comparison is gated on !source_package.version.is_empty() && !installed.version.is_empty(), and SourcePackage::version carries #[serde(default)]. A report whose package entries omit version correlates purely on package name, with no version_mismatch entry, no warning, and no JSON signal.
A producer or a schema revision emits "packages": {"libssl3": {"recipe": "openssl"}} with no version. Every installed package matches by name, version_mismatch and revision_mismatch stay empty, and the human output prints neither warning. The report then attributes openssl's CVE list to the installed libssl3 regardless of whether the image ships 3.5.7 or 3.0.2, and the user has no indication the version cross-check was skipped rather than passed.
The parallel guard for the whole document - recipes/packages deliberately lacking #[serde(default)] - shows the author already knows silent defaults are dangerous here. This field got one anyway.
| let Some(machine) = source.machine.as_deref() else { | ||
| return; | ||
| }; | ||
| if machine.contains(target) { |
There was a problem hiding this comment.
MACHINE check uses substring, not equality
warn_on_machine_mismatch returns early when machine.contains(target), so a target name that is a prefix or substring of an unrelated MACHINE suppresses the warning that exists precisely to stop a cross-machine correlation.
The report was generated for MACHINE = "avocado-qemuarm64". The user runs avocado cve report -f report.json --target qemuarm against a 32-bit ARM project. "avocado-qemuarm64".contains("qemuarm") is true, so the function returns here and prints nothing. The 32-bit and 64-bit images share most package names but not their versions or their patch sets, so the command produces a full, confident, wrong CVE list with no caveat.
The same trap fires for imx8mp vs imx8mp-lpddr4, raspberrypi4 vs raspberrypi4-64, and any foo/foo64 pair - which covers a large share of the machines this project actually builds.
| ( | ||
| id.clone(), | ||
| serde_json::json!({ | ||
| "scorev2": hit.cve.scorev2, |
There was a problem hiding this comment.
JSON omits the resolved score used for ranking
build_json's CVE index emits the raw scorev2 and scorev3 strings but never the value SourceCve::score() computes from them, so every machine consumer has to independently reimplement the prefer-v3 rule and the non-obvious "0.0 means unscored, not harmless" convention documented at line 118.
A CI gate does jq '[.cves[] | select((.scorev3 // "0") | tonumber >= 7.0)] | length'. For a CVE whose entry is {"scorev3": "0.0", "scorev2": "9.0"} - cve-check's shape for a CVE with only a v2 score - the filter reads 0.0 >= 7.0, excludes it, and the gate passes on a critical this tool's own human output would have ranked first.
The consumer's numbers and the CLI's numbers disagree and nothing in the JSON reveals which is authoritative. Emitting "score": 9.0 alongside (and ideally "score_source": "v2") makes the document self-describing.
| result.scanned += sysroot.packages.len(); | ||
|
|
||
| for installed in &sysroot.packages { | ||
| if is_extension && baseline.get(installed.name.as_str()) == Some(&&*installed.version) { |
There was a problem hiding this comment.
Inherited-package check ignores arch (PLAUSIBLE - mechanism traced, precondition unconfirmed)
The baseline map built at line 636 is keyed on package name only, mapping to a single version, so baseline.get(name) == Some(&version) can match an extension's genuinely-own package against an unrelated same-name rootfs entry and mark it inherited. InstalledPackage::arch is parsed at line 616 and echoed into JSON but never consulted during matching.
The rootfs contains two entries named libfoo at 1.0-r0, one noarch and one core2_64 (multilib, or a mixed-arch feed); the HashMap retains whichever was read last. An extension ships its own libfoo 1.0-r0 for the other arch. Here the name and version both match, so the extension's package is counted as inherited and continued past - its recipe's CVEs are never attributed to that extension scope, and scopes["ext:dev/foo"].affected omits it.
Ranked here rather than higher because the CVE still surfaces under rootfs, so this loses scope attribution rather than the finding itself. Marked plausible because I could not confirm from the diff whether same-name-different-arch entries actually occur in an Avocado rootfs.
| } | ||
| } | ||
|
|
||
| let Some(recipe) = source.recipes.get(&source_package.recipe) else { |
There was a problem hiding this comment.
Missing recipe entry is counted as clean, and uncounted (PLAUSIBLE - design intent confirmed, producer contract not)
When a package's recipe string has no corresponding key in source.recipes, the loop continues with no counter and no list. The package lands in none of affected, unknown, version_mismatch, or revision_mismatch, so the report offers no way to tell "this recipe genuinely has no CVEs" from "this recipe's entry is missing".
The producer truncates its output - disk full, or a partial write during bitbake shutdown - emitting a complete packages map but only the first 200 of 900 recipes entries. Every package whose recipe fell off the end takes this continue. The unknown counter, explicitly designed to surface "unchecked, not known to be clean" (line 561), stays at zero, because those packages were found in packages. The user sees a warning-free report.
The design intent here is legitimate and confirmed by the fixture at line 744 (bash appears in packages with no recipes entry, and the test at 793 asserts it is neither affected nor unknown), so this is an observability gap rather than an outright bug. A recipes_missing counter alongside unknown closes it. Marked plausible because I could not read the producer to confirm whether recipes is documented as CVE-bearing-only.
| #[test] | ||
| fn test_missing_report_file_fails() { | ||
| let result = common::run_cli_in_temp_with_config(&["cve", "report", "-f", "no-such.json"]); | ||
| assert_ne!(result.exit_code, 0, "a missing report must not succeed"); |
There was a problem hiding this comment.
Tests assert only a non-zero exit code
All four negative-path tests (lines 36, 49, 65, 78) assert nothing but assert_ne!(result.exit_code, 0), so each passes for any failure reason and cannot detect whether the validation it names actually ran.
Delete load_source's packages.is_empty() guard entirely, or reorder execute so Config::load_composed runs first, and test_report_without_packages_map_fails still passes - the command now fails later, at config resolution, at resolve_target_required, or at the container step (no Docker in CI), and still exits non-zero.
Workspace CLAUDE.md: "Every test must fail when the implementation is wrong (flip the logic and check)" and "Assert exact expected state, not just non-nil / no-error / truthy." The fix is available and has in-repo precedent: TestResult exposes stderr (tests/common/mod.rs:11) and tests/commands/avocado/fetch/mod.rs:36 already asserts on format!("{}{}", result.stdout, result.stderr). Each test should assert its own error text.
| if let Some(ref configdir) = self.rpm_configdir { | ||
| env_setup.push_str(&format!("export RPM_CONFIGDIR=\"{configdir}\"; ")); | ||
| } | ||
| format!("({env_setup}rpm -qa --root=\"{root_path}\" {qf}) || true") |
There was a problem hiding this comment.
Shell values interpolated unescaped, and no unit test pins the format
build_query_all_command splices root_path, rpm_etcconfigdir, and rpm_configdir into a shell string inside plain double quotes with no escaping - and unlike its sibling build_query_command, which has three unit tests at lines 1759, 1772, and 2124, this one ships with none.
Injection path: root_path for the SDK case derives from SysrootType::Sdk(target.to_string()) where target comes from --target or avocado.yaml. A target value containing $(...) or a " reaches the generated command and executes inside the SDK container. Severity is limited since the user already controls what runs in their own container, but --target is exactly the sort of field that gets populated from a CI variable.
The untested-contract half matters more. RPM_QUERY_ALL_FORMAT at line 194 is deliberately pub so this builder and report.rs's parser share one definition, yet nothing asserts the two agree. Add a field to the --qf, or swap a tab for a space, and parse_sysroots' split('\t') at report.rs:604 breaks silently: every line fails the two-field check, every scope comes back empty, retain drops them all, and the command bails with the misleading "run avocado install" message.
Add the new command to generate the cve report. This PR depends on avocado-linux/meta-avocado#252.