fix(images): fail closed on DiffID/layer count mismatch#772
Conversation
verify_diff_ids() silently skipped verification (warn + Ok) when the config's rootfs.diff_ids count did not match the layer count. OCI requires exactly one diff_id per layer, so a mismatch is a malformed or tampered manifest — and the silent skip let an attacker disable DiffID verification simply by supplying a short/long diff_ids list. Return an error instead. Scope: this is the unambiguously-safe part of audit finding boxlite-ai#19. Empty diff_ids remain a skip (local bundles legitimately carry none, and the bundle blob-path traversal vector is already closed by validate_digest), and per-layer verify errors stay lenient (intentional, for layer compression formats the verifier can't decode) — changing those risks breaking legitimate images and is left as a documented follow-up. Two-sided test: verify_diff_ids_rejects_count_mismatch fails (returns Ok) with the change reverted and passes with it applied; verify_diff_ids_allows_empty guards that empty diff_ids still skip on both sides. Audit finding boxlite-ai#19 (low, partial). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthrough
ChangesDiffID and Tarball Verification Hardening
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
boxlite security fixes seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
|
boxlite security fixes seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/boxlite/src/images/object.rs (1)
157-168:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFail the DiffID count check before extracting layers.
The new hard error is correct, but
layer_extracted()still callsextract_layers()beforeverify_diff_ids(), so a malformed/tampered manifest can trigger extraction/cache writes before being rejected. Move the cheap count validation ahead of extraction and keep the same check insideverify_diff_ids()for direct callers/tests.Suggested refactor
pub async fn layer_extracted(&self) -> BoxliteResult<Vec<PathBuf>> { + self.validate_diff_id_count()?; + let digests: Vec<String> = self .manifest .layers .iter() .map(|l| l.digest.clone()) @@ fn verify_diff_ids(&self) -> BoxliteResult<()> { use crate::images::archive::LayerVerifier; + self.validate_diff_id_count()?; + let diff_ids = &self.manifest.diff_ids; if diff_ids.is_empty() { return Ok(()); } let layers = &self.manifest.layers; - if diff_ids.len() != layers.len() { - // The config declares rootfs.diff_ids; OCI requires exactly one per - // layer. A non-matching count is a malformed or tampered manifest, so - // fail closed instead of silently skipping verification (which would - // let an attacker disable DiffID checks by supplying a short list). - return Err(BoxliteError::Image(format!( - "DiffID count ({}) does not match layer count ({}); refusing to use image with inconsistent rootfs.diff_ids", - diff_ids.len(), - layers.len() - ))); - } for (i, (layer, diff_id)) in layers.iter().zip(diff_ids.iter()).enumerate() { let tarball_path = self.blob_source.layer_tarball_path(&layer.digest);fn validate_diff_id_count(&self) -> BoxliteResult<()> { let diff_ids = &self.manifest.diff_ids; let layer_count = self.manifest.layers.len(); if diff_ids.is_empty() || diff_ids.len() == layer_count { return Ok(()); } Err(BoxliteError::Image(format!( "DiffID count ({}) does not match layer count ({}) for image {} (manifest {}); refusing to use image with inconsistent rootfs.diff_ids", diff_ids.len(), layer_count, self.reference, self.manifest.manifest_digest, ))) }As per coding guidelines, “Prepare before execute — setup before irreversible operations in Rust code”, “Perform validation at boundaries in Rust code”, and “Fail fast ... include operation, resource id ... and input shape in error messages”.
Also applies to: 188-196
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/boxlite/src/images/object.rs` around lines 157 - 168, The issue is that in layer_extracted(), the extract_layers() call happens before verify_diff_ids() is called, allowing a malformed manifest to trigger extraction and cache writes before validation rejects it. Create a new method validate_diff_id_count() that performs the cheap DiffID count check (comparing the lengths of diff_ids and layers), then call this validation at the start of layer_extracted() before extract_layers() is invoked. Keep the same validation logic inside verify_diff_ids() to maintain backward compatibility for direct callers and tests. This ensures validation happens before any irreversible extraction operations occur.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/boxlite/src/images/object.rs`:
- Around line 325-340: The test verify_diff_ids_rejects_count_mismatch currently
only covers the case where diff_ids length exceeds layers length (3 diff_ids vs
1 layer). Add an additional test case or assertion that covers the inverse
scenario where the diff_ids list is shorter than the layers list, ensuring that
the verify_diff_ids method correctly rejects mismatches in both directions. This
will prevent regressions that might allow a short rootfs.diff_ids list to pass
validation.
---
Outside diff comments:
In `@src/boxlite/src/images/object.rs`:
- Around line 157-168: The issue is that in layer_extracted(), the
extract_layers() call happens before verify_diff_ids() is called, allowing a
malformed manifest to trigger extraction and cache writes before validation
rejects it. Create a new method validate_diff_id_count() that performs the cheap
DiffID count check (comparing the lengths of diff_ids and layers), then call
this validation at the start of layer_extracted() before extract_layers() is
invoked. Keep the same validation logic inside verify_diff_ids() to maintain
backward compatibility for direct callers and tests. This ensures validation
happens before any irreversible extraction operations occur.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: b3ffbafd-0064-461d-80cd-aceb3200493c
📒 Files selected for processing (1)
src/boxlite/src/images/object.rs
| fn verify_diff_ids_rejects_count_mismatch() { | ||
| let obj = object_with( | ||
| vec![layer( | ||
| "sha256:aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa", | ||
| )], | ||
| vec![ | ||
| "sha256:bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb" | ||
| .to_string(), | ||
| "sha256:cccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccc" | ||
| .to_string(), | ||
| ], | ||
| ); | ||
| assert!( | ||
| obj.verify_diff_ids().is_err(), | ||
| "expected count-mismatch diff_ids to be rejected" | ||
| ); |
There was a problem hiding this comment.
Add the short-list mismatch case to this regression test.
This only covers diff_ids.len() > layers.len(). Add the inverse case too, so a regression that still permits a short rootfs.diff_ids list cannot pass.
Suggested test expansion
#[test]
fn verify_diff_ids_rejects_count_mismatch() {
- let obj = object_with(
+ let obj = object_with(
vec![layer(
"sha256:aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa",
)],
vec![
@@
assert!(
obj.verify_diff_ids().is_err(),
"expected count-mismatch diff_ids to be rejected"
);
+
+ let obj = object_with(
+ vec![
+ layer("sha256:aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"),
+ layer("sha256:dddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddd"),
+ ],
+ vec!["sha256:bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb"
+ .to_string()],
+ );
+ assert!(
+ obj.verify_diff_ids().is_err(),
+ "expected short diff_ids list to be rejected"
+ );
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| fn verify_diff_ids_rejects_count_mismatch() { | |
| let obj = object_with( | |
| vec![layer( | |
| "sha256:aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa", | |
| )], | |
| vec![ | |
| "sha256:bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb" | |
| .to_string(), | |
| "sha256:cccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccc" | |
| .to_string(), | |
| ], | |
| ); | |
| assert!( | |
| obj.verify_diff_ids().is_err(), | |
| "expected count-mismatch diff_ids to be rejected" | |
| ); | |
| fn verify_diff_ids_rejects_count_mismatch() { | |
| let obj = object_with( | |
| vec![layer( | |
| "sha256:aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa", | |
| )], | |
| vec![ | |
| "sha256:bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb" | |
| .to_string(), | |
| "sha256:cccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccc" | |
| .to_string(), | |
| ], | |
| ); | |
| assert!( | |
| obj.verify_diff_ids().is_err(), | |
| "expected count-mismatch diff_ids to be rejected" | |
| ); | |
| let obj = object_with( | |
| vec![ | |
| layer("sha256:aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"), | |
| layer("sha256:dddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddd"), | |
| ], | |
| vec!["sha256:bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb" | |
| .to_string()], | |
| ); | |
| assert!( | |
| obj.verify_diff_ids().is_err(), | |
| "expected short diff_ids list to be rejected" | |
| ); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/boxlite/src/images/object.rs` around lines 325 - 340, The test
verify_diff_ids_rejects_count_mismatch currently only covers the case where
diff_ids length exceeds layers length (3 diff_ids vs 1 layer). Add an additional
test case or assertion that covers the inverse scenario where the diff_ids list
is shorter than the layers list, ensuring that the verify_diff_ids method
correctly rejects mismatches in both directions. This will prevent regressions
that might allow a short rootfs.diff_ids list to pass validation.
…err, and IO-err in DiffID verify Extends boxlite-ai#772 to close the remaining three fail-open paths in verify_diff_ids() that the original commit deliberately left out: 1. Empty rootfs.diff_ids on a remote-pulled (BlobSource::Store) image. The previous logic returned Ok(()) on empty regardless of source — convenient for local bundles (which legitimately load without a config.json), but on a remote pull the config-download step ALWAYS populates rootfs.diff_ids. An empty list there is config tampering / a silent parse failure, and the unconditional skip turned the whole chain of trust into a no-op. Now matches on the BlobSource discriminant: LocalBundle keeps the skip (legit local- load ergonomics), Store fails closed. 2. Malformed diff_id strings inside the list (wrong algorithm prefix, empty hash, etc.) — LayerVerifier::new returns Err. The previous tracing::warn + continue skipped that one layer's verification. That's a targetable per-layer bypass: an attacker who controls the manifest can pick the layer they want to tamper and ship a bad diff_id alongside it to disable verification for that layer only. Now fail closed with the parse error surfaced in the message. 3. verify_tarball() returning Err — historical comment claimed it was for 'unsupported format', but archive::compression::TarballReader only knows gzip vs raw and treats anything else as raw tar, so an unsupported compression format surfaces as Ok(false) hash mismatch (already fails closed). The Err variants are real IO failures (file missing, mid-stream read error) — that's a verification gap, not an unverifiable layer. Now fail closed with the IO cause. Two-side verified: applying the file revert one branch at a time each makes exactly the corresponding new test fail with the expected 'must fail closed' assertion, while count_mismatch and the LocalBundle empty case stay green; restoring the fix makes all five pass. Audit finding boxlite-ai#19, completes the partial fix from the previous commit.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
src/boxlite/src/images/object.rs (1)
218-225: ⚡ Quick winRemove the duplicated DiffID parse rationale.
Lines 218-225 repeat the same four-line comment twice; keep one copy so the fail-closed rationale stays readable.
As per coding guidelines, “Comments in Rust code should explain why, not what” and “Delete comments that restate code or preserve dead decisions.”
Proposed cleanup
- // A malformed diff_id in the list (wrong algorithm prefix, - // empty hash, etc.) means the config is tampered or - // malformed. Skipping that one layer's verification turned - // into a targetable per-layer bypass — fail closed. // A malformed diff_id in the list (wrong algorithm prefix, // empty hash, etc.) means the config is tampered or // malformed. Skipping that one layer's verification turned🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/boxlite/src/images/object.rs` around lines 218 - 225, The comment block explaining the fail-closed rationale for malformed diff_id validation is duplicated, appearing twice in succession with the exact same four-line text about wrong algorithm prefix and empty hash. Remove one copy of this duplicate comment, keeping only a single instance of the fail-closed rationale to maintain code readability while avoiding redundancy.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/boxlite/src/images/object.rs`:
- Around line 424-430: The test hunk in the object_with function call around
line 428 does not conform to Rust formatting standards. Run cargo fmt to
automatically reformat the code according to Rust conventions. This will adjust
the formatting of the object_with call with its vec parameters and
store_blob_source argument to match the project's style guidelines.
- Around line 195-199: The Store diff_ids loader is converting config read/parse
failures into empty Vec results, which loses error context before verification.
Modify the diff_ids loader function in store.rs (around lines 508-537) to return
Result<Vec<String>> instead of converting errors to Vec::new(), preserving the
original config load/parse errors. Then update the BlobSource::Store branch in
object.rs (around verify_diff_ids call) to handle the Result, propagating
config-related errors with their original context while reserving the "diff_ids
is empty" error message for the case where config loads successfully but
rootfs.diff_ids is genuinely empty.
- Around line 193-200: The verify_diff_ids method has a shared reference to self
(&self) but attempts to match on self.blob_source directly, which tries to move
the non-Copy BlobSource enum out of the shared reference. Change the match
statement from match self.blob_source to match &self.blob_source to borrow the
field instead of moving it, allowing the match to work with a shared reference.
---
Nitpick comments:
In `@src/boxlite/src/images/object.rs`:
- Around line 218-225: The comment block explaining the fail-closed rationale
for malformed diff_id validation is duplicated, appearing twice in succession
with the exact same four-line text about wrong algorithm prefix and empty hash.
Remove one copy of this duplicate comment, keeping only a single instance of the
fail-closed rationale to maintain code readability while avoiding redundancy.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: fdf84b9f-486d-42e9-8d72-230c8be9acda
📒 Files selected for processing (1)
src/boxlite/src/images/object.rs
| return match self.blob_source { | ||
| BlobSource::LocalBundle(_) => Ok(()), | ||
| BlobSource::Store(_) => Err(BoxliteError::Image( | ||
| "rootfs.diff_ids is empty on a remote-pulled image; refusing to use image \ | ||
| without DiffID verification (config tampering / parse failure)" | ||
| .to_string(), | ||
| )), | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Confirm ImageObject::verify_diff_ids matches a borrowed field by value
# and inspect BlobSource's definition/derives.
rg -n -C3 'match\s+self\.blob_source|enum\s+BlobSource|derive\([^)]*Copy|impl\s+Copy\s+for\s+BlobSource' --glob '*.rs'Repository: boxlite-ai/boxlite
Length of output: 14510
Match blob_source by reference here.
verify_diff_ids() only has &self; match self.blob_source tries to move the non-Copy BlobSource out of a shared reference. Match on &self.blob_source instead.
Proposed fix
- return match self.blob_source {
+ return match &self.blob_source {
BlobSource::LocalBundle(_) => Ok(()),
BlobSource::Store(_) => Err(BoxliteError::Image(📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| return match self.blob_source { | |
| BlobSource::LocalBundle(_) => Ok(()), | |
| BlobSource::Store(_) => Err(BoxliteError::Image( | |
| "rootfs.diff_ids is empty on a remote-pulled image; refusing to use image \ | |
| without DiffID verification (config tampering / parse failure)" | |
| .to_string(), | |
| )), | |
| }; | |
| return match &self.blob_source { | |
| BlobSource::LocalBundle(_) => Ok(()), | |
| BlobSource::Store(_) => Err(BoxliteError::Image( | |
| "rootfs.diff_ids is empty on a remote-pulled image; refusing to use image \ | |
| without DiffID verification (config tampering / parse failure)" | |
| .to_string(), | |
| )), | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/boxlite/src/images/object.rs` around lines 193 - 200, The verify_diff_ids
method has a shared reference to self (&self) but attempts to match on
self.blob_source directly, which tries to move the non-Copy BlobSource enum out
of the shared reference. Change the match statement from match self.blob_source
to match &self.blob_source to borrow the field instead of moving it, allowing
the match to work with a shared reference.
| BlobSource::Store(_) => Err(BoxliteError::Image( | ||
| "rootfs.diff_ids is empty on a remote-pulled image; refusing to use image \ | ||
| without DiffID verification (config tampering / parse failure)" | ||
| .to_string(), | ||
| )), |
There was a problem hiding this comment.
Preserve Store config-load failures instead of collapsing them into diff_ids.is_empty().
This branch now fails closed, but the provided src/boxlite/src/images/store.rs:508-537 context shows config read/parse failures already get converted to Vec::new(). That loses the config path/digest and original error before verify_diff_ids() can report it; make the Store diff_ids loader return a Result<Vec<String>> and reserve this branch for a config that truly contains an empty rootfs.diff_ids.
As per coding guidelines, “Fail fast on missing config or invalid inputs in Rust code; include operation, resource id, endpoint/status, and input shape in error messages. Preserve the original cause when wrapping errors. Never swallow errors silently.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/boxlite/src/images/object.rs` around lines 195 - 199, The Store diff_ids
loader is converting config read/parse failures into empty Vec results, which
loses error context before verification. Modify the diff_ids loader function in
store.rs (around lines 508-537) to return Result<Vec<String>> instead of
converting errors to Vec::new(), preserving the original config load/parse
errors. Then update the BlobSource::Store branch in object.rs (around
verify_diff_ids call) to handle the Result, propagating config-related errors
with their original context while reserving the "diff_ids is empty" error
message for the case where config loads successfully but rootfs.diff_ids is
genuinely empty.
Source: Coding guidelines
| let obj = object_with( | ||
| vec![layer( | ||
| "sha256:aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa", | ||
| )], | ||
| vec![], | ||
| store_blob_source(&tmp), | ||
| ); |
There was a problem hiding this comment.
Run rustfmt for this test hunk.
The Rust formatting check reports a diff around Line 428 in src/boxlite/src/images/object.rs; apply cargo fmt before merging.
🧰 Tools
🪛 GitHub Check: Check Rust formatting
[warning] 428-428:
Diff in /home/runner/work/boxlite/boxlite/src/boxlite/src/images/object.rs
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/boxlite/src/images/object.rs` around lines 424 - 430, The test hunk in
the object_with function call around line 428 does not conform to Rust
formatting standards. Run cargo fmt to automatically reformat the code according
to Rust conventions. This will adjust the formatting of the object_with call
with its vec parameters and store_blob_source argument to match the project's
style guidelines.
Source: Linters/SAST tools
Summary
Source-level security audit fix.
verify_diff_ids()silently skippedverification (warn + Ok) when the config's
rootfs.diff_idscount did not matchthe layer count. OCI requires exactly one diff_id per layer, so a mismatch is a
malformed/tampered manifest — and the silent skip let an attacker disable DiffID
verification by supplying a short/long list.
Fix
Return an error on count mismatch. Scope: this is the unambiguously-safe part of
finding #19. Empty
diff_idsremain a skip (local bundles legitimately carrynone; the blob-path traversal vector is closed separately by
validate_digest),and per-layer verify errors stay lenient (intentional, for layer compression
formats the verifier can't decode).
Test (two-sided)
verify_diff_ids_rejects_count_mismatchfails (returns Ok) with the changereverted and passes with it applied;
verify_diff_ids_allows_emptyguards thatempty diff_ids still skip on both sides.
Audit finding #19 (low, partial).
🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
LocalBundlesources and rejected forStoresources.Tests