cli/context/store: limit decompressed size of TLS files on zip import - #7105
cli/context/store: limit decompressed size of TLS files on zip import#7105UditDewan wants to merge 1 commit into
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Pull request overview
This PR hardens docker context import (zip) against decompression-based memory exhaustion by enforcing size limits on decompressed TLS material and tightening limitedReader behavior.
Changes:
- Apply
limitedReaderwhen readingtls/zip entries to cap decompressed TLS file size. - Adjust
limitedReader.Readsemantics to avoid silently truncating when reads land exactly on the limit. - Add regression tests covering oversized TLS zip entries and a boundary read case for
limitedReader.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
cli/context/store/store.go |
Caps decompressed reads for tls/ zip entries (and highlights loop-close/memory considerations). |
cli/context/store/store_test.go |
Adds a regression test for oversized decompressed TLS zip entries. |
cli/context/store/io_utils.go |
Updates limitedReader.Read to avoid truncation-at-limit behavior. |
cli/context/store/io_utils_test.go |
Extends tests to cover an exact-limit boundary case using iotest.OneByteReader. |
Comments suppressed due to low confidence (2)
cli/context/store/store.go:485
defer f.Close()inside the zip entry loop can accumulate many deferred closes when importing archives with lots of files, potentially exhausting file descriptors. Close the entry reader immediately afterio.ReadAllinstead of deferring.
data, err := io.ReadAll(&limitedReader{R: f, N: maxAllowedFileSizeToImport})
defer f.Close()
if err != nil {
return err
}
cli/context/store/store.go:487
- This change caps each TLS entry’s decompressed size, but
importZipstill accumulates all TLS file contents in memory (tlsData.Endpoints[...].Files[...] = data) with no overall decompressed-size budget. A crafted zip with many highly-compressible TLS entries could still OOM the process even though each individual entry stays under the per-file limit.
data, err := io.ReadAll(&limitedReader{R: f, N: maxAllowedFileSizeToImport})
defer f.Close()
if err != nil {
return err
}
err = importEndpointTLS(&tlsData, zf.Name, data)
if err != nil {
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| tf, err := w.Create(path.Join("tls", "docker", "ca.pem")) | ||
| assert.NilError(t, err) | ||
| _, err = tf.Write(make([]byte, 2*maxAllowedFileSizeToImport)) | ||
| assert.NilError(t, err) |
There was a problem hiding this comment.
This one compiles as-is, so I have left it unchanged.
make accepts any integer type for the size argument, not just int — the spec only requires that a constant size be representable by a value of type int. maxAllowedFileSizeToImport is a typed int64 constant, and 2 * 10 << 20 = 20 MiB is representable as int on every supported platform (including 32-bit, where int maxes out around 2.1e9).
Confirmed by go vet ./cli/context/store/ locally, and by the test / ctn job passing on this PR — it builds and runs this test file.
| if l.N < 0 { | ||
| return 0, errors.New("read exceeds the defined limit") | ||
| } | ||
| if l.N == 0 { | ||
| return 0, io.EOF | ||
| } | ||
| // have to cap N + 1 otherwise we won't hit limit err | ||
| if int64(len(p)) > l.N+1 { |
There was a problem hiding this comment.
Good catch — this was a real hole, fixed in the latest push.
limitedReader now checks the budget after decrementing and returns the limit error in preference to the underlying error:
n, err = l.R.Read(p)
l.N -= int64(n)
if l.N < 0 {
return n, errLimitExceeded
}
return n, errAdded a regression test using iotest.DataErrReader, which returns data together with io.EOF on the read that crosses the limit. Against the previous version it fails with expected an error, got nil — i.e. io.ReadAll reported success on truncated content, exactly as described.
Also hoisted the error to a package-level errLimitExceeded sentinel rather than allocating a new one on each Read.
The meta.json entry of an imported zip archive is read through limitedReader (10MB cap), but tls/ entries were read with a bare io.ReadAll. Only the compressed archive size was capped, so a small crafted zip containing a highly compressible TLS entry could decompress to gigabytes and exhaust memory. Read TLS entries through the same limitedReader already used for meta.json. This also fixes two ways in which limitedReader could silently truncate content instead of rejecting it, both of which made the cap ineffective because io.ReadAll treats io.EOF as success: - the l.N == 0 branch returned io.EOF once the budget was used up. This is reachable whenever a read lands exactly on the remaining budget, which is deterministic for zip imports since flate delivers 32KiB chunks that divide the 10MB cap evenly. - a reader is permitted to return data together with io.EOF. When such a read crossed the limit, that io.EOF was returned as-is and the limit error was never observed by the caller. The limit error now takes precedence over the underlying error as soon as the budget goes negative. True end-of-data still returns a clean io.EOF from the underlying reader. Fixes docker#6917 Signed-off-by: uditDewan <udit.dewan21@gmail.com>
1b84aa7 to
13f6437
Compare
- What I did
Fixed a memory exhaustion issue in
docker context importwith zip archives (fixes #6917).The
meta.jsonentry of an imported zip archive is read throughlimitedReader(10MB cap), buttls/entries were read with a bareio.ReadAll. Only the compressed archive size was capped, so a small crafted zip containing a highly compressible TLS entry could decompress to gigabytes and OOM the CLI.- How I did it
Read TLS entries in
importZipthrough the samelimitedReaderalready used formeta.json.That alone wasn't sufficient:
limitedReaderhad two ways of silently truncating content rather than rejecting it, both of which made the cap ineffective becauseio.ReadAlltreatsio.EOFas success.l.N == 0branch returnedio.EOFonce the budget was used up. Reachable whenever a read lands exactly on the remaining budget — deterministic for zip imports, since flate delivers 32KiB chunks that divide the 10MB cap evenly.io.EOF. When such a read crossed the limit, thatio.EOFwas returned as-is and the limit error was never observed by the caller. (Thanks to the Copilot review for catching this second one.)The limit error now takes precedence over the underlying error as soon as the budget goes negative. True end-of-data still returns a clean
io.EOFfrom the underlying reader.- How to verify it
Regression tests:
TestImportZipTLSDataTooLarge— imports a zip whose TLS entry inflates past the cap while the archive itself stays well under it, and asserts the import errors.TestLimitReaderReadAllgains two cases: aniotest.OneByteReaderthat lands a read exactly on the limit with data remaining, and aniotest.DataErrReaderthat returns data together withio.EOFon the read that crosses the limit. Both assert the limit error is surfaced; both fail against the previouslimitedReader.- Human readable description for the release notes
Left the block below empty because the changelog check requires a matching
impact/label, which I can't set. Suggested note if a maintainer wants this in the release notes:- A picture of a cute animal (not mandatory but encouraged)
🦦