Limit how much fetch and check read for a manifest or a file (closes #168) #172
+5
-2
@@ -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`)
|
||||||
|
|
||||||
|
|||||||
@@ -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
@@ -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
@@ -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()
|
||||||
|
|||||||
@@ -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
|
||||||
|
|||||||
@@ -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://")
|
||||||
|
|||||||
@@ -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
@@ -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
@@ -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).
|
||||||
|
|||||||
@@ -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")
|
||||||
|
}
|
||||||
Reference in New Issue
Block a user