Cap the bundle layer read in ociremote.Bundle - #5052
Open
arpitjain099 wants to merge 1 commit into
Open
Conversation
Bundle() decompressed the new-bundle-format attestation layer and read all of it into memory with no size check, while every other layer read in pkg/oci calls payloadsize.CheckSize first. A registry the user does not control could make cosign verify-attestation --new-bundle-format allocate without bound from a small compressed layer. The size a registry declares is the compressed size, and this is the only one of these reads that goes through Uncompressed(), so checking that size is necessary but not sufficient. This does both: the declared size is checked first so an oversized layer is rejected before anything is read, and the decompressed stream is then bounded by the same limit. GetBundles calls Bundle() for every manifest in a referrers index and swallows the error with continue, since non-Sigstore referrers are expected there, so without the second check each oversized layer in an index was read in full before its error was reached. MaxSize is exported from the payload size package because a caller reading a decompressed stream needs the limit itself rather than a yes or no answer about a size it already knows. Signed-off-by: Arpit Jain <arpitjain099@gmail.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5052 +/- ##
==========================================
- Coverage 40.10% 39.49% -0.61%
==========================================
Files 155 207 +52
Lines 10044 13047 +3003
==========================================
+ Hits 4028 5153 +1125
- Misses 5530 7162 +1632
- Partials 486 732 +246 ☔ View full report in Codecov by Harness. 🚀 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.
Summary
Adds a size cap to the bundle layer read in
ociremote.Bundle(). Follow-up to the private report onGHSA-cq2j-v5qh-cjqh, which @Hayden-IO closed with the note that DoS vectors do not need an advisory and that a patch adding a sensible limit would be welcome. This is that patch.Bundle()decompressed the new-bundle-format attestation layer and read all of it into memory with no check, while the three sibling read sinks inpkg/ociall callpayloadsize.CheckSizefirst:pkg/oci/internal/signature/layer.go,pkg/oci/static/file.goandpkg/oci/remote/remote.go.Why the declared size alone is not enough
The three siblings read through
Compressed(), so the size the registry declares genuinely bounds what they read.Bundle()is the one that goes throughUncompressed(), so the declared size does not bound the result and a small compressed blob can inflate past any limit.So this does both:
This matters more than a single read suggests.
GetBundlesinpkg/cosign/verify.gocallsBundle()for every manifest in a referrers index and swallows the error withcontinue, because non-Sigstore referrers are expected there. Without the second check, each oversized layer in an index was read in full before its error was reached.Changes
internal/pkg/cosign/payload/size: exportsMaxSize(), andCheckSizenow calls it. A caller reading a decompressed stream needs the limit itself rather than a yes or no answer about a size it already knows. Behaviour is unchanged, including the fallback to the default whenCOSIGN_MAX_ATTACHMENT_SIZEfails to parse.pkg/oci/remote/signatures.go: the declared-size check, plus areadCappedhelper that reads one byte past the maximum so that hitting the limit is distinguishable from a payload that is exactly the maximum size.Tests
pkg/oci/remote/signatures_capped_test.go, three cases: a payload at exactly the limit is accepted, one byte over is rejected, and a gzip stream inflating from roughly 64KiB to 64MiB is rejected against a 1MiB cap. That third one is the case the declared-size check cannot catch, and it asserts the compressed size is under the cap so the test cannot pass for the wrong reason.With the cap removed, the two rejection tests fail:
With it in place,
./pkg/oci/...and./internal/pkg/cosign/payload/size/...are green, andgo build ./...andgo vetare clean.A note on the original report
I verified the code paths by reading them at
v3.1.3and I said so in the report: I did not stand up a crafted registry, so I had no measured RSS figure. The decompression behaviour is now covered by the test above rather than by reasoning.