Validate manifest entry paths on deserialize (closes #61)
check / check (push) Successful in 1m48s
check / check (push) Successful in 1m48s
Untrusted .mf files were parsed with no path validation, so an entry like ../../etc/passwd flowed into filepath.Join against the checker's base path. ValidatePath ran only on the write side (Builder). Enforce it on every entry in deserializeInner, so all consumers — Checker now, any restore/extract path later — are protected. Reject the whole manifest on the first offender rather than dropping entries, which could hide files from a check; failures wrap a new errInvalidManifestPath sentinel and name the path. A path that is not valid UTF-8 is refused at load time by the proto3 string decoder before validation runs; the test covers that plus the traversal, absolute, backslash, and empty cases, and a valid round-trip. Model: opus-4-8
This commit is contained in:
@@ -24,6 +24,8 @@ only thing left of the `chore/align-repo-policies` branch is the list below.
|
||||
|
||||
# Completed Steps
|
||||
|
||||
- 2026-09-21: validate manifest entry paths on deserialize so untrusted `.mf`
|
||||
files cannot make `Checker` stat or read outside `basePath` (#61)
|
||||
- 2026-08-09: added `.prettierrc`/`.prettierignore`, gave `script/fmt` and
|
||||
`script/fmt-check` one shared prettier file set via `script/prettier`, dropped
|
||||
the `|| true` that hid prettier failures, and added a node-based Dockerfile
|
||||
|
||||
@@ -312,6 +312,10 @@ func (c *Checker) FindExtraFiles(ctx context.Context, results chan<- Result) err
|
||||
}
|
||||
|
||||
func (c *Checker) checkFile(entry *MFFilePath, checkedBytes *FileSize) Result {
|
||||
// entry.GetPath() is safe to join here: a manifest's entry paths are
|
||||
// validated against the path invariants when it is loaded (see
|
||||
// deserializeInner) or built (see Builder.AddFile), so a traversal or
|
||||
// absolute path can never reach this point.
|
||||
absPath := filepath.Join(string(c.basePath), entry.GetPath())
|
||||
relPath := RelFilePath(entry.GetPath())
|
||||
|
||||
|
||||
@@ -25,6 +25,7 @@ var (
|
||||
errDecompressedTooLarge = errors.New("decompressed data 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")
|
||||
)
|
||||
|
||||
// validateUUID checks that the byte slice is a valid UUID (16 bytes, parseable).
|
||||
@@ -181,6 +182,19 @@ func (m *manifest) deserializeInner() error {
|
||||
return errUUIDMismatch
|
||||
}
|
||||
|
||||
// Enforce the manifest path invariants on every entry as it is loaded,
|
||||
// so that no consumer of a manifest — Checker today, any restore or
|
||||
// extract path tomorrow — acts on a traversal or absolute path from an
|
||||
// untrusted .mf. Reject loudly on the first offender rather than
|
||||
// dropping entries, which would let a hostile manifest hide files from a
|
||||
// check.
|
||||
for _, f := range m.pbInner.GetFiles() {
|
||||
err = ValidatePath(f.GetPath())
|
||||
if err != nil {
|
||||
return fmt.Errorf("%w: %w", errInvalidManifestPath, err)
|
||||
}
|
||||
}
|
||||
|
||||
log.Infof("loaded manifest with %d files", len(m.pbInner.GetFiles()))
|
||||
|
||||
return nil
|
||||
|
||||
@@ -0,0 +1,145 @@
|
||||
//nolint:testpackage // white-box tests exercise unexported internals
|
||||
package mfer
|
||||
|
||||
import (
|
||||
"bytes"
|
||||
"crypto/sha256"
|
||||
"fmt"
|
||||
"testing"
|
||||
|
||||
"github.com/google/uuid"
|
||||
"github.com/klauspost/compress/zstd"
|
||||
"github.com/stretchr/testify/assert"
|
||||
"github.com/stretchr/testify/require"
|
||||
"google.golang.org/protobuf/encoding/protowire"
|
||||
"google.golang.org/protobuf/proto"
|
||||
)
|
||||
|
||||
// craftInnerBytes builds the wire bytes of an inner MFFile holding a single
|
||||
// file entry whose path is exactly pathBytes. It writes the wire form by hand
|
||||
// so a hostile path — including one that is not valid UTF-8 — can be embedded
|
||||
// without proto.Marshal's own UTF-8 enforcement rejecting it first.
|
||||
func craftInnerBytes(id uuid.UUID, pathBytes string) []byte {
|
||||
entry := protowire.AppendTag(nil, 1, protowire.BytesType) // MFFilePath.path
|
||||
entry = protowire.AppendString(entry, pathBytes)
|
||||
|
||||
inner := protowire.AppendTag(nil, 100, protowire.VarintType) // MFFile.version
|
||||
inner = protowire.AppendVarint(inner, uint64(MFFile_VERSION_ONE))
|
||||
inner = protowire.AppendTag(inner, 101, protowire.BytesType) // MFFile.files
|
||||
inner = protowire.AppendBytes(inner, entry)
|
||||
inner = protowire.AppendTag(inner, 102, protowire.BytesType) // MFFile.uuid
|
||||
inner = protowire.AppendBytes(inner, id[:])
|
||||
|
||||
return inner
|
||||
}
|
||||
|
||||
// wrapInner wraps inner MFFile wire bytes in a complete, well-formed .mf
|
||||
// envelope (magic prefix, zstd-compressed payload, matching hash and UUID) so
|
||||
// that deserialization reaches path validation rather than failing earlier on
|
||||
// an integrity check.
|
||||
func wrapInner(t *testing.T, id uuid.UUID, innerData []byte) []byte {
|
||||
t.Helper()
|
||||
|
||||
var cbuf bytes.Buffer
|
||||
|
||||
zw, err := zstd.NewWriter(&cbuf, zstd.WithEncoderLevel(zstd.SpeedBestCompression))
|
||||
require.NoError(t, err)
|
||||
|
||||
_, err = zw.Write(innerData)
|
||||
require.NoError(t, err)
|
||||
require.NoError(t, zw.Close())
|
||||
|
||||
compressed := cbuf.Bytes()
|
||||
sum := sha256.Sum256(compressed)
|
||||
|
||||
outer := &MFFileOuter{
|
||||
InnerMessage: compressed,
|
||||
Size: int64(len(innerData)),
|
||||
Sha256: sum[:],
|
||||
Uuid: id[:],
|
||||
Version: MFFileOuter_VERSION_ONE,
|
||||
CompressionType: MFFileOuter_COMPRESSION_ZSTD,
|
||||
}
|
||||
|
||||
ob, err := proto.Marshal(outer)
|
||||
require.NoError(t, err)
|
||||
|
||||
return append([]byte(MAGIC), ob...)
|
||||
}
|
||||
|
||||
func TestDeserializeRejectsInvalidEntryPaths(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
tests := []struct {
|
||||
name string
|
||||
path string
|
||||
}{
|
||||
{"parent traversal", "../escape"},
|
||||
{"interior traversal", "a/../../escape"},
|
||||
{"absolute path", "/etc/passwd"},
|
||||
{"backslash path", `a\b`},
|
||||
{"double slash", "a//b"},
|
||||
{"empty path", ""},
|
||||
{"invalid utf-8", "abc\xff"},
|
||||
}
|
||||
|
||||
for _, tt := range tests {
|
||||
t.Run(tt.name, func(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
id := uuid.New()
|
||||
data := wrapInner(t, id, craftInnerBytes(id, tt.path))
|
||||
|
||||
_, err := NewManifestFromReader(bytes.NewReader(data))
|
||||
require.Error(t, err)
|
||||
|
||||
if tt.path == "abc\xff" {
|
||||
// A path that is not valid UTF-8 cannot survive the proto3
|
||||
// string decoder, which rejects it before path validation
|
||||
// runs; the manifest is still refused at load time.
|
||||
return
|
||||
}
|
||||
|
||||
require.ErrorIs(t, err, errInvalidManifestPath)
|
||||
|
||||
if tt.path != "" {
|
||||
// ValidatePath quotes the path with %q; assert against the
|
||||
// same rendering so escaped characters (e.g. a backslash)
|
||||
// still match.
|
||||
assert.Contains(t, err.Error(), fmt.Sprintf("%q", tt.path),
|
||||
"error must name the offending path")
|
||||
}
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
func TestDeserializeValidManifestRoundTrips(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
hash := make([]byte, 34) // multihash: 2-byte prefix + 32-byte SHA-256
|
||||
|
||||
b := NewBuilder()
|
||||
require.NoError(t, b.AddFileWithHash("dir/file.txt", 123, ModTime{}, hash))
|
||||
|
||||
var buf bytes.Buffer
|
||||
require.NoError(t, b.Build(&buf))
|
||||
|
||||
m, err := NewManifestFromReader(bytes.NewReader(buf.Bytes()))
|
||||
require.NoError(t, err)
|
||||
|
||||
files := m.Files()
|
||||
require.Len(t, files, 1)
|
||||
assert.Equal(t, "dir/file.txt", files[0].GetPath())
|
||||
assert.Equal(t, int64(123), files[0].GetSize())
|
||||
}
|
||||
|
||||
// TestValidatePathRejectsInvalidUTF8 pins the ValidatePath rule that a manifest
|
||||
// path must be valid UTF-8, independent of the proto decoder that also enforces
|
||||
// it on the wire.
|
||||
func TestValidatePathRejectsInvalidUTF8(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
err := ValidatePath("abc\xff")
|
||||
require.ErrorIs(t, err, errPathNotUTF8)
|
||||
assert.Contains(t, err.Error(), "UTF-8")
|
||||
}
|
||||
Reference in New Issue
Block a user