diff --git a/TODO.md b/TODO.md index 06f9dba..c7ad736 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-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 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") +}