diff --git a/docs/FORMAT.md b/docs/FORMAT.md index 94ec656..fa3cfcf 100644 --- a/docs/FORMAT.md +++ b/docs/FORMAT.md @@ -50,11 +50,14 @@ allows verifying data integrity before decompression. The `innerMessage` field is compressed with [Zstandard (zstd)](https://facebook.github.io/zstd/). Implementations must enforce a decompression size limit to prevent decompression bombs. The reference -implementation limits decompressed size to 256 MB. It writes zstd frames with a +implementation limits decompressed size to 256 MiB. It writes zstd frames with a window of at most 8 MiB, the largest window the zstd format recommends decoders support, and refuses frames that ask for a larger one. It also refuses an inner message whose file entries, hashes, timestamps and MIME types, counted at 176, -112, 64 and 16 bytes each, add up to more than 8 times its size. +112, 64 and 16 bytes each, add up to more than 8 times its size. It refuses a +manifest file larger than 258 MiB without reading the rest of it: zstd's worst +case grows a 256 MiB inner message by 1/256 to 257 MiB, and the last MiB is room +for the signature, the signing key and the other outer fields. ## Inner Message (`MFFile`) diff --git a/internal/cli/check.go b/internal/cli/check.go index 655b443..0913395 100644 --- a/internal/cli/check.go +++ b/internal/cli/check.go @@ -112,7 +112,10 @@ func (mfa *CLIApp) fetchManifestToTemp( } tmpPath := tmpFile.Name() - _, cpErr := io.Copy(tmpFile, rc) + + // One byte past mfer.MaxManifestSize is enough for loading the + // manifest to refuse one that is too large. + _, cpErr := io.Copy(tmpFile, io.LimitReader(rc, mfer.MaxManifestSize+1)) _ = rc.Close() _ = tmpFile.Close() diff --git a/internal/cli/fetch.go b/internal/cli/fetch.go index 3799e09..f099124 100644 --- a/internal/cli/fetch.go +++ b/internal/cli/fetch.go @@ -433,13 +433,15 @@ func fetchManifest( // Read the whole manifest before parsing it, so that a connection // lost partway through is retried rather than reported as a bad - // manifest. + // manifest. One byte past mfer.MaxManifestSize is enough for + // mfer.NewManifestFromReader to refuse a manifest that is too large. var manifestData []byte err := client.get(ctx, manifestURL, func(resp *http.Response) error { var readErr error - manifestData, readErr = io.ReadAll(resp.Body) + manifestData, readErr = io.ReadAll( + io.LimitReader(resp.Body, mfer.MaxManifestSize+1)) return readErr }) @@ -910,8 +912,10 @@ func saveResponse( progress: progress, } - // Copy content while hashing and reporting progress - written, copyErr := io.Copy(pw, resp.Body) + // Copy content while hashing and reporting progress. One byte past + // the listed size is enough for finishDownload to report a size + // mismatch. + written, copyErr := io.Copy(pw, io.LimitReader(resp.Body, expectedSize+1)) // Close file before checking errors (to flush writes) closeErr := out.Close() diff --git a/internal/cli/fetch_test.go b/internal/cli/fetch_test.go index 8fae1f8..2165135 100644 --- a/internal/cli/fetch_test.go +++ b/internal/cli/fetch_test.go @@ -441,6 +441,80 @@ func TestFetchSizeMismatch(t *testing.T) { "temp file should be cleaned up on size mismatch") } +// TestFetchStopsReadingFilePastListedSize serves 1 MiB for a file the +// manifest lists at 16 bytes. fetch must stop reading one byte past the +// listed size, report the size mismatch and remove its temp file. +// +//nolint:paralleltest // changes the process-global working directory +func TestFetchStopsReadingFilePastListedSize(t *testing.T) { + content := []byte("sixteen bytes...") + + manifest, err := mfer.NewManifestFromReader(bytes.NewReader( + manifestOf(t, map[string][]byte{testFileTxt: content}))) + require.NoError(t, err) + + server := httptest.NewServer( + http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + _, _ = w.Write(make([]byte, 1<<20)) + })) + defer server.Close() + + chdirTemp(t) + + err = downloadFile(context.Background(), testClient(), + server.URL+"/"+testFileTxt, ".", testFileTxt, manifest.Files()[0], nil) + require.ErrorIs(t, err, errSizeMismatch) + assert.EqualError(t, err, "size mismatch: expected 16 bytes, got 17") + assert.NoFileExists(t, tempPathFor(testFileTxt)) +} + +// zeros is an io.Reader of zero bytes without end. +type zeros struct{} + +func (zeros) Read(p []byte) (int, error) { + clear(p) + + return len(p), nil +} + +// TestManifestDownloadStopsPastLimit serves a manifest 32 MiB larger than +// mfer.MaxManifestSize to fetch and to check. Each must stop reading it +// once it passes the maximum, fail with an error naming the maximum and +// leave no temp file. The connection's buffers hold less than 32 MiB, so a +// client that stops reading leaves the server unable to send all of it. +// +//nolint:paralleltest // sets TMPDIR, where check writes the manifest +func TestManifestDownloadStopsPastLimit(t *testing.T) { + size := mfer.MaxManifestSize + 32<<20 + limit := strconv.FormatInt(mfer.MaxManifestSize, 10) + + tmpDir := t.TempDir() + t.Setenv("TMPDIR", tmpDir) + + for _, command := range []string{cmdFetch, cmdCheck} { + var sent atomic.Int64 + + server := httptest.NewServer( + http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + n, _ := io.CopyN(w, zeros{}, size) + sent.Store(n) + })) + + opts := testOpts([]string{testApp, command, "-q", server.URL + "/index.mf"}, + afero.NewOsFs()) + assert.Equal(t, 1, runCLI(opts), command) + assert.Contains(t, testStderr(t, opts), + "maximum allowed size of "+limit+" bytes", command) + + server.Close() + assert.Less(t, sent.Load(), size, "%s read the whole manifest", command) + } + + leftover, err := os.ReadDir(tmpDir) + require.NoError(t, err) + assert.Empty(t, leftover) +} + //nolint:paralleltest // changes the process-global working directory func TestFetchProgress(t *testing.T) { // Create source filesystem with a larger test file diff --git a/mfer/constants.go b/mfer/constants.go index 006e4b2..bad7769 100644 --- a/mfer/constants.go +++ b/mfer/constants.go @@ -12,6 +12,13 @@ const ( // memory. MaxDecompressedSize int64 = 256 * 1024 * 1024 + // MaxManifestSize is the largest manifest file mfer reads (258 MiB). + // zstd's worst case grows data it cannot compress by 1/256, so an inner + // message of MaxDecompressedSize compresses to at most 257 MiB; the + // last MiB is room for the signature, the signing key and the other + // outer fields. + MaxManifestSize = MaxDecompressedSize + MaxDecompressedSize/256 + 1<<20 + // zstdWindowSize is the zstd window zstd.SpeedBestCompression gives mfer's writer. zstdWindowSize = 8 << 20 diff --git a/mfer/deserialize.go b/mfer/deserialize.go index 7473126..293a460 100644 --- a/mfer/deserialize.go +++ b/mfer/deserialize.go @@ -23,6 +23,7 @@ var ( errCompressedHashWrong = errors.New("compressed data hash mismatch") errSignatureNoPubKey = errors.New("signature present but no public key") errDecompressedTooLarge = errors.New("decompressed data exceeds maximum allowed size") + errManifestTooLarge = errors.New("manifest exceeds maximum allowed size") errUUIDMismatch = errors.New("outer and inner UUID mismatch") errInvalidFileFormat = errors.New("invalid file format") errInvalidManifestPath = errors.New("manifest contains invalid path") @@ -308,17 +309,22 @@ func validateMagic(dat []byte) bool { return bytes.Equal(got, expected) } -// NewManifestFromReader reads a manifest from an io.Reader. +// NewManifestFromReader reads a manifest from an io.Reader. It refuses a +// manifest larger than MaxManifestSize, reading at most one byte past it. // //nolint:revive // unexported-return: exporting manifest is owner question 13 func NewManifestFromReader(input io.Reader) (*manifest, error) { m := &manifest{} - dat, err := io.ReadAll(input) + dat, err := io.ReadAll(io.LimitReader(input, MaxManifestSize+1)) if err != nil { return nil, err } + if int64(len(dat)) > MaxManifestSize { + return nil, fmt.Errorf("%w of %d bytes", errManifestTooLarge, MaxManifestSize) + } + if !validateMagic(dat) { return nil, errInvalidFileFormat } diff --git a/mfer/deserialize_test.go b/mfer/deserialize_test.go new file mode 100644 index 0000000..978cf7e --- /dev/null +++ b/mfer/deserialize_test.go @@ -0,0 +1,33 @@ +//nolint:testpackage // white-box tests exercise unexported internals +package mfer + +import ( + "io" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// zeros is an io.Reader of zero bytes without end. +type zeros struct{} + +func (zeros) Read(p []byte) (int, error) { + clear(p) + + return len(p), nil +} + +// TestNewManifestFromReaderRefusesTooLarge gives NewManifestFromReader +// 1 MiB more than MaxManifestSize. It must refuse the manifest after +// reading one byte past the maximum, and no more. +// +//nolint:paralleltest // holds 258 MiB; kept apart from other large reads +func TestNewManifestFromReaderRefusesTooLarge(t *testing.T) { + size := MaxManifestSize + 1<<20 + input := &io.LimitedReader{R: zeros{}, N: size} + + _, err := NewManifestFromReader(input) + require.ErrorIs(t, err, errManifestTooLarge) + assert.Equal(t, MaxManifestSize+1, size-input.N, "bytes read") +}