oci/layer: error instead of panic when diff_ids and layer counts differ - #676
Open
arpitjain099 wants to merge 1 commit into
Open
oci/layer: error instead of panic when diff_ids and layer counts differ#676arpitjain099 wants to merge 1 commit into
arpitjain099 wants to merge 1 commit into
Conversation
…smatch A manifest's layer list and the config's rootfs.diff_ids both come from the untrusted image, but nothing checked that they were the same length. When a crafted image lists more layers than there are diff_ids, the unpack loop in UnpackRootfs walks off the end of config.RootFS.DiffIDs and panics with an index-out-of-range. The Stat path in utils.go has the same problem: it indexes both slices by a counter that increments per non-empty history entry without any bounds check. Guard both paths so umoci returns a clear error naming the mismatched counts rather than panicking. Add a regression test that builds an image whose config drops one diff_id and asserts UnpackManifest fails cleanly. Signed-off-by: Arpit Jain <arpitjain099@gmail.com>
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #676 +/- ##
==========================================
+ Coverage 72.89% 74.11% +1.21%
==========================================
Files 71 71
Lines 6164 4639 -1525
==========================================
- Hits 4493 3438 -1055
+ Misses 1213 749 -464
+ Partials 458 452 -6
🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
I've been looking at umoci's unpack path from a supply-chain angle, specifically what happens when the image metadata is hostile rather than well-formed.
A manifest's
layerslist and the config'srootfs.diff_idsare both taken straight from the (untrusted) image, but nothing checks that the two line up. If a crafted image lists more layers than there are diff_ids,UnpackRootfspanics:oci/layer/unpack.go: the extraction loop doeslayerDiffID := config.RootFS.DiffIDs[idx]whereidxranges overmanifest.Layers, so a shorterDiffIDsruns off the end with an index-out-of-range.utils.go(Stat): the history walk indexesconfig.RootFS.DiffIDs[layerIdx]andmanifest.Layers[layerIdx]by a counter that increments per non-empty history entry, again with no bounds check.This turns a malformed image into a panic instead of a clean error, which isn't great for anything that unpacks or inspects untrusted images.
The change adds a length guard before the unpack loop and a bounds check before the stat indexing, so umoci now returns an error that names the mismatched counts. I added a regression test that builds an image whose config drops one diff_id and asserts
UnpackManifestreturns an error rather than panicking.Worth noting the existing fuzzer (
layer_fuzzer.go) constructs the config's diff_ids to be exactly as long as the layer list, so it never exercises this mismatch, which is probably why it went unnoticed.go test ./oci/layer/...for the affected tests passes, andgofmt/go vetare clean. (TestGeneratefails on my macOS box independently of this change, since it exercises Linux-only lstat behaviour.)