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

Merged
clawbot merged 1 commits from issue-168-limit-downloads into next 2026-10-07 14:25:46 +02:00
10 changed files with 176 additions and 20 deletions
+5 -2
View File
@@ -50,11 +50,14 @@ allows verifying data integrity before decompression.
The `innerMessage` field is compressed with The `innerMessage` field is compressed with
[Zstandard (zstd)](https://facebook.github.io/zstd/). Implementations must [Zstandard (zstd)](https://facebook.github.io/zstd/). Implementations must
enforce a decompression size limit to prevent decompression bombs. The reference 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 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 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, 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`) ## Inner Message (`MFFile`)
+8 -1
View File
@@ -112,10 +112,17 @@ func (mfa *CLIApp) fetchManifestToTemp(
} }
tmpPath := tmpFile.Name() 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() _ = rc.Close()
_ = tmpFile.Close() _ = tmpFile.Close()
if cpErr == nil && written > mfa.maxManifestSize {
cpErr = fmt.Errorf("%w of %d bytes", errManifestTooLarge, mfa.maxManifestSize)
}
if cpErr != nil { if cpErr != nil {
_ = mfa.Fs.Remove(tmpPath) _ = mfa.Fs.Remove(tmpPath)
+10 -8
View File
@@ -5,6 +5,7 @@ import (
"os" "os"
"github.com/spf13/afero" "github.com/spf13/afero"
"sneak.berlin/go/mfer/mfer"
) )
// NoColor disables colored output when set. Automatically true if the // 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. // RunWithOptions creates and runs the CLI application with the given options.
func RunWithOptions(opts *RunOptions) int { func RunWithOptions(opts *RunOptions) int {
m := &CLIApp{ m := &CLIApp{
appname: opts.Appname, appname: opts.Appname,
version: opts.Version, version: opts.Version,
gitrev: opts.Gitrev, gitrev: opts.Gitrev,
exitCode: 0, exitCode: 0,
Stdin: opts.Stdin, maxManifestSize: mfer.MaxManifestSize,
Stdout: opts.Stdout, Stdin: opts.Stdin,
Stderr: opts.Stderr, Stdout: opts.Stdout,
Fs: opts.Fs, Stderr: opts.Stderr,
Fs: opts.Fs,
} }
m.run(opts.Args) m.run(opts.Args)
+15 -6
View File
@@ -361,7 +361,7 @@ func (mfa *CLIApp) fetchManifestOperation(
firstDelay: firstRetryDelay, firstDelay: firstRetryDelay,
} }
manifestData, files, err := fetchManifest(ctx, cmd, client, manifestURL) manifestData, files, err := mfa.fetchManifest(ctx, cmd, client, manifestURL)
if err != nil { if err != nil {
return err return err
} }
@@ -426,20 +426,22 @@ func (mfa *CLIApp) fetchManifestOperation(
// that lists a file where fetch writes another or a mode outside 0777. It // 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 // returns the manifest as downloaded, to be saved once the files are in
// place, and the files it lists. // place, and the files it lists.
func fetchManifest( func (mfa *CLIApp) fetchManifest(
ctx context.Context, cmd *cli.Command, client retryingClient, manifestURL string, ctx context.Context, cmd *cli.Command, client retryingClient, manifestURL string,
) ([]byte, []*mfer.MFFilePath, error) { ) ([]byte, []*mfer.MFFilePath, error) {
log.Infof("fetching manifest from %s", manifestURL) log.Infof("fetching manifest from %s", manifestURL)
// Read the whole manifest before parsing it, so that a connection // Read the whole manifest before parsing it, so that a connection
// lost partway through is retried rather than reported as a bad // 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 var manifestData []byte
err := client.get(ctx, manifestURL, func(resp *http.Response) error { err := client.get(ctx, manifestURL, func(resp *http.Response) error {
var readErr error var readErr error
manifestData, readErr = io.ReadAll(resp.Body) manifestData, readErr = io.ReadAll(
io.LimitReader(resp.Body, mfa.maxManifestSize+1))
return readErr return readErr
}) })
@@ -447,6 +449,11 @@ func fetchManifest(
return nil, nil, fmt.Errorf("failed to fetch manifest: %w", err) 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 // Parse manifest
//nolint:contextcheck // mfer loads a manifest without a context //nolint:contextcheck // mfer loads a manifest without a context
manifest, err := mfer.NewManifestFromReader(bytes.NewReader(manifestData)) manifest, err := mfer.NewManifestFromReader(bytes.NewReader(manifestData))
@@ -908,8 +915,10 @@ func saveResponse(
progress: progress, 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) // Close file before checking errors (to flush writes)
closeErr := out.Close() closeErr := out.Close()
+70
View File
@@ -26,6 +26,7 @@ import (
"github.com/spf13/afero" "github.com/spf13/afero"
"github.com/stretchr/testify/assert" "github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require" "github.com/stretchr/testify/require"
urfcli "github.com/urfave/cli/v3"
"google.golang.org/protobuf/proto" "google.golang.org/protobuf/proto"
"sneak.berlin/go/mfer/mfer" "sneak.berlin/go/mfer/mfer"
) )
@@ -441,6 +442,75 @@ func TestFetchSizeMismatch(t *testing.T) {
"temp file should be cleaned up on size mismatch") "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 //nolint:paralleltest // changes the process-global working directory
func TestFetchProgress(t *testing.T) { func TestFetchProgress(t *testing.T) {
// Create source filesystem with a larger test file // Create source filesystem with a larger test file
+4
View File
@@ -23,6 +23,10 @@ const manifestFetchTimeout = 30 * time.Second
// its message. // its message.
var errHTTPStatus = errors.New("HTTP") 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://. // isHTTPURL returns true if the string starts with http:// or https://.
func isHTTPURL(s string) bool { func isHTTPURL(s string) bool {
return strings.HasPrefix(s, "http://") || strings.HasPrefix(s, "https://") return strings.HasPrefix(s, "http://") || strings.HasPrefix(s, "https://")
+4
View File
@@ -59,6 +59,10 @@ type CLIApp struct {
exitCode int exitCode int
app *cli.Command 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 Stdin io.Reader // Standard input stream
Stdout io.Writer // Standard output stream for normal output Stdout io.Writer // Standard output stream for normal output
Stderr io.Writer // Standard error stream for diagnostics Stderr io.Writer // Standard error stream for diagnostics
+8 -1
View File
@@ -8,10 +8,17 @@ const (
ReleaseDate = "2025-12-17" ReleaseDate = "2025-12-17"
// MaxDecompressedSize is the maximum allowed size of decompressed manifest // 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. // memory.
MaxDecompressedSize int64 = 256 * 1024 * 1024 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 is the zstd window zstd.SpeedBestCompression gives mfer's writer.
zstdWindowSize = 8 << 20 zstdWindowSize = 8 << 20
+19 -2
View File
@@ -24,6 +24,7 @@ var (
errCompressedHashWrong = errors.New("compressed data hash mismatch") errCompressedHashWrong = errors.New("compressed data hash mismatch")
errSignatureNoPubKey = errors.New("signature present but no public key") errSignatureNoPubKey = errors.New("signature present but no public key")
errDecompressedTooLarge = errors.New("decompressed data exceeds maximum allowed size") 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") errUUIDMismatch = errors.New("outer and inner UUID mismatch")
errInvalidFileFormat = errors.New("invalid file format") errInvalidFileFormat = errors.New("invalid file format")
errInvalidManifestPath = errors.New("manifest contains invalid path") errInvalidManifestPath = errors.New("manifest contains invalid path")
@@ -318,13 +319,14 @@ func validateMagic(dat []byte) bool {
return bytes.Equal(got, expected) 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 //nolint:revive // unexported-return: exporting manifest is owner question 13
func NewManifestFromReader(input io.Reader) (*manifest, error) { func NewManifestFromReader(input io.Reader) (*manifest, error) {
m := &manifest{} m := &manifest{}
dat, err := io.ReadAll(input) dat, err := readAtMost(input, MaxManifestSize)
if err != nil { if err != nil {
return nil, err return nil, err
} }
@@ -356,6 +358,21 @@ func NewManifestFromReader(input io.Reader) (*manifest, error) {
return m, nil 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. // ManifestFromFileOptions configures NewManifestFromFile.
type ManifestFromFileOptions struct { type ManifestFromFileOptions struct {
// Path is the manifest file to read (required). // Path is the manifest file to read (required).
+33
View File
@@ -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")
}