Check blob hashes, offsets and lengths read from the destination before using them #155

New Issue

2026-09-22T00:55:13+02:00

clawbot commented

2026-09-22 00:55:13 +02:00

Found by the security review #73. Severity: high (item 1), low (items 2 and 3).

What is wrong

The restore cache stores each decrypted blob at filepath.Join(cacheDir, blobHash) opened with O_CREATE|O_TRUNC (internal/vaultik/blobcache.go:133-134, :350-351). blobHash comes straight from the downloaded snapshot database (restore.go:430); types.BlobHash is a plain string and nothing checks it is a hash. A value such as aa/../../../home/u/.profile makes restore write decrypted data outside the cache directory. The hash comparison runs only in Close, after the write (restore.go:989-991, blob_fetch.go:46-55), and the file is not removed on mismatch.
hash[:2], hash[2:4] and hash[:16] are applied to strings from the store listing (prune.go:287), the unauthenticated manifest (internal/vaultik/snapshot.go:749-750) and the database (blob_fetch.go:54, :98, verify.go:606, restore.go:929, :946). A short string panics the command, including prune on the backed-up host.
blobDiskCache.ReadAt checks only that offset+length does not exceed the entry size (blobcache.go:226); a negative length or an overflowing sum passes and reaches make([]byte, length) (:244). verify.go:445 and restore.go:1135 also allocate sizes taken from the database.

Why it matters

Item 1 is an arbitrary file write on the machine holding the private key, by anyone who knows a recipient string and can write to the destination (see #73 for why decryption does not prove authorship). Items 2 and 3 let one bad object name or row crash a command.

Acceptable

One helper says whether a string is exactly 64 lowercase hex characters. Restore checks every blob hash from the downloaded database in buildBlobIndexes and fails before any fetch. FetchBlob, verifyManifestBlobsExist and verifyBlobExistenceFromDB return an error for a bad hash; listAllRemoteBlobs skips a non-conforming name with a warning. Remote snapshot keys from the metadata/ listing are accepted only in the same form.
Do not put the check in BlobHash.Scan or on the type: the packer stores temp-placeholder-{uuid} as the hash of an unfinished blob in the local index (internal/blob/packer.go:317).
blobDiskCache.path refuses a key containing a path separator.
Short-hash prefixes in log and error text go through a helper that cannot panic.
ReadAt rejects a negative offset, a negative length, and a length greater than size-offset (subtract, do not add). The two verify sites reject negative lengths and stop allocating database-supplied sizes.

Definition of done

Tests: a snapshot database whose blob_hash contains /../ makes restore fail with nothing written outside the cache directory; a three-character object name under blobs/ does not crash prune; a manifest entry with a short hash returns an error; a blob_chunks row with a negative length returns an error.
No existing assertion weakened; make check green.

Line numbers are as of next at 6fcd8e1.

model: fable-5-1

Found by the security review https://git.eeqj.de/sneak/vaultik/issues/73. Severity: **high** (item 1), low (items 2 and 3). ## What is wrong 1. The restore cache stores each decrypted blob at `filepath.Join(cacheDir, blobHash)` opened with `O_CREATE|O_TRUNC` (`internal/vaultik/blobcache.go:133-134`, `:350-351`). `blobHash` comes straight from the downloaded snapshot database (`restore.go:430`); `types.BlobHash` is a plain string and nothing checks it is a hash. A value such as `aa/../../../home/u/.profile` makes restore write decrypted data outside the cache directory. The hash comparison runs only in `Close`, after the write (`restore.go:989-991`, `blob_fetch.go:46-55`), and the file is not removed on mismatch. 2. `hash[:2]`, `hash[2:4]` and `hash[:16]` are applied to strings from the store listing (`prune.go:287`), the unauthenticated manifest (`internal/vaultik/snapshot.go:749-750`) and the database (`blob_fetch.go:54`, `:98`, `verify.go:606`, `restore.go:929`, `:946`). A short string panics the command, including `prune` on the backed-up host. 3. `blobDiskCache.ReadAt` checks only that `offset+length` does not exceed the entry size (`blobcache.go:226`); a negative length or an overflowing sum passes and reaches `make([]byte, length)` (`:244`). `verify.go:445` and `restore.go:1135` also allocate sizes taken from the database. ## Why it matters Item 1 is an arbitrary file write on the machine holding the private key, by anyone who knows a recipient string and can write to the destination (see https://git.eeqj.de/sneak/vaultik/issues/73 for why decryption does not prove authorship). Items 2 and 3 let one bad object name or row crash a command. ## Acceptable - One helper says whether a string is exactly 64 lowercase hex characters. Restore checks every blob hash from the downloaded database in `buildBlobIndexes` and fails before any fetch. `FetchBlob`, `verifyManifestBlobsExist` and `verifyBlobExistenceFromDB` return an error for a bad hash; `listAllRemoteBlobs` skips a non-conforming name with a warning. Remote snapshot keys from the `metadata/` listing are accepted only in the same form. - Do not put the check in `BlobHash.Scan` or on the type: the packer stores `temp-placeholder-{uuid}` as the hash of an unfinished blob in the local index (`internal/blob/packer.go:317`). - `blobDiskCache.path` refuses a key containing a path separator. - Short-hash prefixes in log and error text go through a helper that cannot panic. - `ReadAt` rejects a negative offset, a negative length, and a length greater than `size-offset` (subtract, do not add). The two verify sites reject negative lengths and stop allocating database-supplied sizes. ## Definition of done 1. Tests: a snapshot database whose `blob_hash` contains `/../` makes restore fail with nothing written outside the cache directory; a three-character object name under `blobs/` does not crash `prune`; a manifest entry with a short hash returns an error; a `blob_chunks` row with a negative length returns an error. 2. No existing assertion weakened; `make check` green. Line numbers are as of `next` at `6fcd8e1`. model: fable-5-1

clawbot referenced this issue

2026-09-22 00:55:32 +02:00

Restore must keep every write inside the target directory #154

clawbot referenced this issue

2026-09-22 09:26:15 +02:00

Restore must keep every write inside the target directory #154

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: sneak/vaultik#155