Validate manifest paths on read, not only on write (path traversal in Checker) #61

New Issue

clawbot · 2026-08-09T03:38:08+02:00

clawbot commented

2026-08-09 03:38:08 +02:00

Context

ValidatePath() (mfer/builder.go:35-66) enforces the path invariants —
valid UTF-8, no backslashes, no leading /, no .. or empty segments — and
is called from Builder.AddFile (mfer/builder.go:142) and
Builder.AddFileWithHash (mfer/builder.go:232).

That is write-side only. Paths arriving from an untrusted .mf file are
never validated:

NewManifestFromReader / deserializeInner() in mfer/deserialize.go do
not validate any entry path.
mfer/checker.go:315 does
absPath := filepath.Join(string(c.basePath), entry.GetPath()) on the raw
manifest path with no validation and no sanitization at all.

So a hostile index.mf containing ../../etc/passwd causes mfer check to
stat and read outside basePath. internal/cli/fetch.go has its own
separate sanitizePath() and is not affected; the library Checker is.

Severity is limited for check today because the operation is read-only and
reports a mismatch rather than writing anything — but this is a library
consumed by other programs, the invariants are already specified, and the
enforcement gap is exactly the kind of thing that becomes a write primitive
the first time someone adds a restore or extract path.

Definition of done

Entry paths are validated on deserialization, so that every consumer of a
manifest benefits rather than each call site re-implementing the check.
A manifest containing an entry that fails ValidatePath is rejected with
a wrapped sentinel error naming the offending path.
mfer/checker.go never joins an unvalidated manifest path onto
c.basePath.
Tests: a table-driven test that constructs manifests containing
../escape, a/../../escape, an absolute path, a backslash path, an
empty path, and invalid UTF-8, and asserts each is rejected at load time
with an error identifying the path. Plus a test asserting a valid manifest
still round-trips unchanged.
make check passes. TODO.md updated in the same commit.

Implementation requirements

Reject at deserialize time. Do not fix this only inside Checker — the
gap is that untrusted input enters the library unvalidated, and every
future consumer would have to remember the check.
Reuse the existing ValidatePath rather than writing a second, subtly
different validator. If ValidatePath is missing a rule needed here
(e.g. rejecting absolute Windows-style paths), extend ValidatePath and
cover the new rule with a test.
Use a package-level sentinel error wrapped with %w, consistent with the
existing error style in the package.
Do not silently skip or drop invalid entries — a manifest that violates
the format is invalid and must fail loudly. Silently ignoring bad entries
would let an attacker hide files from a check.
Do not change the wire format or the proto in this issue.
Commit title must end with (closes #61).

## Context `ValidatePath()` (`mfer/builder.go:35-66`) enforces the path invariants — valid UTF-8, no backslashes, no leading `/`, no `..` or empty segments — and is called from `Builder.AddFile` (`mfer/builder.go:142`) and `Builder.AddFileWithHash` (`mfer/builder.go:232`). That is **write-side only**. Paths arriving from an untrusted `.mf` file are never validated: - `NewManifestFromReader` / `deserializeInner()` in `mfer/deserialize.go` do not validate any entry path. - `mfer/checker.go:315` does `absPath := filepath.Join(string(c.basePath), entry.GetPath())` on the raw manifest path with no validation and no sanitization at all. So a hostile `index.mf` containing `../../etc/passwd` causes `mfer check` to stat and read outside `basePath`. `internal/cli/fetch.go` has its own separate `sanitizePath()` and is not affected; the library `Checker` is. Severity is limited for `check` today because the operation is read-only and reports a mismatch rather than writing anything — but this is a library consumed by other programs, the invariants are already specified, and the enforcement gap is exactly the kind of thing that becomes a write primitive the first time someone adds a restore or extract path. ## Definition of done - Entry paths are validated on deserialization, so that every consumer of a manifest benefits rather than each call site re-implementing the check. A manifest containing an entry that fails `ValidatePath` is rejected with a wrapped sentinel error naming the offending path. - `mfer/checker.go` never joins an unvalidated manifest path onto `c.basePath`. - Tests: a table-driven test that constructs manifests containing `../escape`, `a/../../escape`, an absolute path, a backslash path, an empty path, and invalid UTF-8, and asserts each is rejected at load time with an error identifying the path. Plus a test asserting a valid manifest still round-trips unchanged. - `make check` passes. `TODO.md` updated in the same commit. ## Implementation requirements - Reject at deserialize time. Do not fix this only inside `Checker` — the gap is that untrusted input enters the library unvalidated, and every future consumer would have to remember the check. - Reuse the existing `ValidatePath` rather than writing a second, subtly different validator. If `ValidatePath` is missing a rule needed here (e.g. rejecting absolute Windows-style paths), extend `ValidatePath` and cover the new rule with a test. - Use a package-level sentinel error wrapped with `%w`, consistent with the existing error style in the package. - Do not silently skip or drop invalid entries — a manifest that violates the format is invalid and must fail loudly. Silently ignoring bad entries would let an attacker hide files from a `check`. - Do not change the wire format or the proto in this issue. - Commit title must end with ` (closes #61)`.

clawbot added this to the 1.0.0 milestone 2026-08-09 03:38:08 +02:00

clawbot referenced this issue

2026-08-09 03:44:03 +02:00

DECISION: wire format questions blocking the 1.0 format freeze #81

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: sneak/mfer#61