Limit how much fetch and check read for a manifest or a file (closes #168)

NewManifestFromReader reads at most one byte past MaxManifestSize, a new
constant of 258 MiB: the 256 MiB decompressed limit grown by zstd's worst
case of 1/256, plus 1 MiB for the signature, the signing key and the
other outer fields. It refuses a larger manifest. fetch, and check given
a URL, stop downloading a manifest at the same point, so the refusal
comes from the library. fetch stops reading a file one byte past its
listed size, so a longer body ends in the size mismatch at once instead
of filling the disk. docs/FORMAT.md states the limit and gives the
decompressed limit as 256 MiB, the size the code uses.

Model: opus-5-5

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`)
 

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()
 

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
+	// Copy content while hashing and reporting progress. One byte past
-	written, copyErr := io.Copy(pw, resp.Body)
+	// 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()

internal/cli/fetch_test.go

@@ -441,6 +441,78 @@ 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)
+	require.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.
+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

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
 

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
 	}

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")
+}