From 9347492f7398d520aec5044ddd54777b182b228a Mon Sep 17 00:00:00 2001 From: clawbot <35+clawbot@noreply.example.org> Date: Wed, 7 Oct 2026 09:49:54 +0000 Subject: [PATCH] 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 one byte past the same size and report it as too large; tests lower that size to keep their memory small. 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 | 7 +++- internal/cli/check.go | 9 ++++- internal/cli/entry.go | 18 +++++---- internal/cli/fetch.go | 21 +++++++--- internal/cli/fetch_test.go | 70 +++++++++++++++++++++++++++++++++ internal/cli/manifest_loader.go | 4 ++ internal/cli/mfer.go | 4 ++ mfer/constants.go | 9 ++++- mfer/deserialize.go | 21 +++++++++- mfer/deserialize_test.go | 33 ++++++++++++++++ 10 files changed, 176 insertions(+), 20 deletions(-) create mode 100644 mfer/deserialize_test.go diff --git a/docs/FORMAT.md b/docs/FORMAT.md index 3d7e3d5..d078ab4 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 0d43bb7..b629a93 100644 --- a/internal/cli/check.go +++ b/internal/cli/check.go @@ -112,10 +112,17 @@ func (mfa *CLIApp) fetchManifestToTemp( } tmpPath := tmpFile.Name() - _, cpErr := io.Copy(tmpFile, rc) + + // Copying stops one byte past mfa.maxManifestSize, which is enough to + // tell that the manifest is too large. + written, cpErr := io.Copy(tmpFile, io.LimitReader(rc, mfa.maxManifestSize+1)) _ = rc.Close() _ = tmpFile.Close() + if cpErr == nil && written > mfa.maxManifestSize { + cpErr = fmt.Errorf("%w of %d bytes", errManifestTooLarge, mfa.maxManifestSize) + } + if cpErr != nil { _ = mfa.Fs.Remove(tmpPath) diff --git a/internal/cli/entry.go b/internal/cli/entry.go index 3bdd6eb..19d2681 100644 --- a/internal/cli/entry.go +++ b/internal/cli/entry.go @@ -5,6 +5,7 @@ import ( "os" "github.com/spf13/afero" + "sneak.berlin/go/mfer/mfer" ) // NoColor disables colored output when set. Automatically true if the @@ -56,14 +57,15 @@ func Run(appname, version, gitrev string) int { // RunWithOptions creates and runs the CLI application with the given options. func RunWithOptions(opts *RunOptions) int { m := &CLIApp{ - appname: opts.Appname, - version: opts.Version, - gitrev: opts.Gitrev, - exitCode: 0, - Stdin: opts.Stdin, - Stdout: opts.Stdout, - Stderr: opts.Stderr, - Fs: opts.Fs, + appname: opts.Appname, + version: opts.Version, + gitrev: opts.Gitrev, + exitCode: 0, + maxManifestSize: mfer.MaxManifestSize, + Stdin: opts.Stdin, + Stdout: opts.Stdout, + Stderr: opts.Stderr, + Fs: opts.Fs, } m.run(opts.Args) diff --git a/internal/cli/fetch.go b/internal/cli/fetch.go index e30a4a0..0244d67 100644 --- a/internal/cli/fetch.go +++ b/internal/cli/fetch.go @@ -361,7 +361,7 @@ func (mfa *CLIApp) fetchManifestOperation( firstDelay: firstRetryDelay, } - manifestData, files, err := fetchManifest(ctx, cmd, client, manifestURL) + manifestData, files, err := mfa.fetchManifest(ctx, cmd, client, manifestURL) if err != nil { return err } @@ -426,20 +426,22 @@ func (mfa *CLIApp) fetchManifestOperation( // that lists a file where fetch writes another or a mode outside 0777. It // returns the manifest as downloaded, to be saved once the files are in // place, and the files it lists. -func fetchManifest( +func (mfa *CLIApp) fetchManifest( ctx context.Context, cmd *cli.Command, client retryingClient, manifestURL string, ) ([]byte, []*mfer.MFFilePath, error) { log.Infof("fetching manifest from %s", manifestURL) // Read the whole manifest before parsing it, so that a connection // lost partway through is retried rather than reported as a bad - // manifest. + // manifest. Reading stops one byte past mfa.maxManifestSize, which is + // enough to tell that the manifest 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, mfa.maxManifestSize+1)) return readErr }) @@ -447,6 +449,11 @@ func fetchManifest( return nil, nil, fmt.Errorf("failed to fetch manifest: %w", err) } + if int64(len(manifestData)) > mfa.maxManifestSize { + return nil, nil, fmt.Errorf("failed to fetch manifest: %w of %d bytes", + errManifestTooLarge, mfa.maxManifestSize) + } + // Parse manifest //nolint:contextcheck // mfer loads a manifest without a context manifest, err := mfer.NewManifestFromReader(bytes.NewReader(manifestData)) @@ -908,8 +915,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 6f0038e..bb79298 100644 --- a/internal/cli/fetch_test.go +++ b/internal/cli/fetch_test.go @@ -26,6 +26,7 @@ import ( "github.com/spf13/afero" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + urfcli "github.com/urfave/cli/v3" "google.golang.org/protobuf/proto" "sneak.berlin/go/mfer/mfer" ) @@ -441,6 +442,75 @@ func TestFetchSizeMismatch(t *testing.T) { "temp file should be cleaned up on size mismatch") } +// 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 +} + +// TestFetchStopsReadingFilePastListedSize serves a body that never ends +// for a file listed 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) { + server := httptest.NewServer( + http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + _, _ = io.Copy(w, zeros{}) + })) + defer server.Close() + + chdirTemp(t) + + err := downloadFile(context.Background(), testClient(), + server.URL+"/"+testFileTxt, ".", testFileTxt, + &mfer.MFFilePath{Path: testFileTxt, Size: 16}, nil) + require.ErrorIs(t, err, errSizeMismatch) + require.EqualError(t, err, "size mismatch: expected 16 bytes, got 17") + assert.NoFileExists(t, tempPathFor(testFileTxt)) +} + +// TestManifestDownloadStopsAtLimit serves a manifest that never ends to +// fetch and to check, with the most they download of a manifest lowered +// to 64 KiB. Each must stop reading at that limit, fail with an error +// naming it and leave no temp file. +func TestManifestDownloadStopsAtLimit(t *testing.T) { + server := httptest.NewServer( + http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + _, _ = io.Copy(w, zeros{}) + })) + defer server.Close() + + tmpDir := t.TempDir() + t.Setenv("TMPDIR", tmpDir) + + mfa := &CLIApp{Fs: afero.NewOsFs(), maxManifestSize: 64 << 10} + + fetch := mfa.fetchCommand() + fetch.Action = mfa.fetchManifestOperation + + check := mfa.checkCommand() + check.Action = mfa.checkManifestOperation + + for _, cmd := range []*urfcli.Command{fetch, check} { + // Both operations log to the process-global logger. + err := runLocked(func() error { + return cmd.Run(context.Background(), + []string{cmd.Name, server.URL + "/index.mf"}) + }) + require.ErrorIs(t, err, errManifestTooLarge, cmd.Name) + require.ErrorContains(t, err, + "maximum allowed size of 65536 bytes", cmd.Name) + } + + 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/internal/cli/manifest_loader.go b/internal/cli/manifest_loader.go index 583fba7..ca3e64b 100644 --- a/internal/cli/manifest_loader.go +++ b/internal/cli/manifest_loader.go @@ -23,6 +23,10 @@ const manifestFetchTimeout = 30 * time.Second // its message. var errHTTPStatus = errors.New("HTTP") +// errManifestTooLarge indicates a manifest download that passed +// CLIApp.maxManifestSize. +var errManifestTooLarge = errors.New("manifest exceeds maximum allowed size") + // isHTTPURL returns true if the string starts with http:// or https://. func isHTTPURL(s string) bool { return strings.HasPrefix(s, "http://") || strings.HasPrefix(s, "https://") diff --git a/internal/cli/mfer.go b/internal/cli/mfer.go index 6fed3ff..c2a0366 100644 --- a/internal/cli/mfer.go +++ b/internal/cli/mfer.go @@ -59,6 +59,10 @@ type CLIApp struct { exitCode int app *cli.Command + // maxManifestSize is the most of a manifest that fetch, and check + // given a URL, download: mfer.MaxManifestSize, which tests lower. + maxManifestSize int64 + Stdin io.Reader // Standard input stream Stdout io.Writer // Standard output stream for normal output Stderr io.Writer // Standard error stream for diagnostics diff --git a/mfer/constants.go b/mfer/constants.go index 006e4b2..f5005ef 100644 --- a/mfer/constants.go +++ b/mfer/constants.go @@ -8,10 +8,17 @@ const ( ReleaseDate = "2025-12-17" // MaxDecompressedSize is the maximum allowed size of decompressed manifest - // data (256 MB). This prevents decompression bombs from consuming excessive + // data (256 MiB). This prevents decompression bombs from consuming excessive // 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 85ec2b6..6da64c6 100644 --- a/mfer/deserialize.go +++ b/mfer/deserialize.go @@ -24,6 +24,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") @@ -318,13 +319,14 @@ 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 := readAtMost(input, MaxManifestSize) if err != nil { return nil, err } @@ -356,6 +358,21 @@ func NewManifestFromReader(input io.Reader) (*manifest, error) { return m, nil } +// readAtMost reads all of input, or refuses it with errManifestTooLarge +// once it passes maxSize bytes, after reading one byte past maxSize. +func readAtMost(input io.Reader, maxSize int64) ([]byte, error) { + dat, err := io.ReadAll(io.LimitReader(input, maxSize+1)) + if err != nil { + return nil, err + } + + if int64(len(dat)) > maxSize { + return nil, fmt.Errorf("%w of %d bytes", errManifestTooLarge, maxSize) + } + + return dat, nil +} + // ManifestFromFileOptions configures NewManifestFromFile. type ManifestFromFileOptions struct { // Path is the manifest file to read (required). diff --git a/mfer/deserialize_test.go b/mfer/deserialize_test.go new file mode 100644 index 0000000..0a8f272 --- /dev/null +++ b/mfer/deserialize_test.go @@ -0,0 +1,33 @@ +//nolint:testpackage // white-box tests exercise unexported internals +package mfer + +import ( + "bytes" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// TestReadAtMost gives readAtMost exactly its maximum, which it must +// return whole, and twice its maximum, which it must refuse after reading +// one byte past the maximum, and no more. NewManifestFromReader reads +// through it with MaxManifestSize; the test uses 64 KiB, since reading +// MaxManifestSize under the race detector takes gigabytes of memory. +func TestReadAtMost(t *testing.T) { + t.Parallel() + + const maxSize = 64 << 10 + + dat, err := readAtMost(bytes.NewReader(make([]byte, maxSize)), maxSize) + require.NoError(t, err) + assert.Len(t, dat, maxSize) + + input := bytes.NewReader(make([]byte, 2*maxSize)) + + _, err = readAtMost(input, maxSize) + require.ErrorIs(t, err, errManifestTooLarge) + require.EqualError(t, err, + "manifest exceeds maximum allowed size of 65536 bytes") + assert.Equal(t, maxSize-1, input.Len(), "bytes left unread") +} -- 2.54.0