From 0deacfc7edd98fe5e2ba4833ab5f31388ffb40d3 Mon Sep 17 00:00:00 2001 From: clawbot <35+clawbot@noreply.example.org> Date: Tue, 22 Sep 2026 00:47:26 +0200 Subject: [PATCH] Validate manifest entry paths on deserialize (closes #61) Untrusted .mf files were parsed with no path validation, so an entry like ../../etc/passwd reached filepath.Join against the checker's base path and mfer check could stat and read outside it. ValidatePath ran only when building a manifest. It now runs on every entry as the manifest loads, so every consumer is covered. The whole manifest is rejected on the first bad entry instead of dropping it, which could hide files from a check; the error wraps errInvalidManifestPath and names the path. Disclosure: a path that is not valid UTF-8 is refused earlier, by the protobuf string decoder, so that error does not name the path. Model: opus-4-8 (implementation); fable-5-1 (summary) --- TODO.md | 2 + mfer/checker.go | 4 + mfer/deserialize.go | 14 ++++ mfer/deserialize_path_test.go | 145 ++++++++++++++++++++++++++++++++++ 4 files changed, 165 insertions(+) create mode 100644 mfer/deserialize_path_test.go diff --git a/TODO.md b/TODO.md index 0340c54..35bbf76 100644 --- a/TODO.md +++ b/TODO.md @@ -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-09-21: rewrote `script/test` to the canonical pattern (30s timeout, `-race -cover`, quiet-first with verbose-on-failure rerun) and fixed the process-global logger data race it surfaced (#67) diff --git a/mfer/checker.go b/mfer/checker.go index fe5969e..a47d4d1 100644 --- a/mfer/checker.go +++ b/mfer/checker.go @@ -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()) diff --git a/mfer/deserialize.go b/mfer/deserialize.go index a099d4a..b4c2b94 100644 --- a/mfer/deserialize.go +++ b/mfer/deserialize.go @@ -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 diff --git a/mfer/deserialize_path_test.go b/mfer/deserialize_path_test.go new file mode 100644 index 0000000..b83f9fb --- /dev/null +++ b/mfer/deserialize_path_test.go @@ -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") +}