When a blob hash and its chunk offsets/lengths are read back from the destination, the values were used without validation, so a corrupt or malicious remote could drive an out-of-bounds read, return the wrong bytes, or write a decrypted blob outside the cache directory.
What changed:
blobDiskCache.path refuses any key containing a path separator, closing the arbitrary-file-write hole (restore's decrypted-blob write is keyed by the hash from the snapshot database). ReadAt rejects a negative offset/length and bounds with length > size-offset so a sum cannot overflow past the check.
A new isBlobHash helper (exactly 64 lowercase hex; a plain function, not a BlobHash method, since the packer stores temp-placeholder-{uuid}) gates FetchBlob, shallow and deep verify, and restore: buildBlobIndexes rejects every hash from the snapshot database before any fetch. The blobs/ and metadata/ listings skip a non-conforming name. Short-hash prefixes in restore's, verify's and fetch's log/error text go through a panic-safe shortHash.
verify's chunk reader rejects a negative blob_chunks length and streams the chunk rather than allocating a database-supplied size.
Tests (each fails without the change): a cache key with /../ is refused with nothing written outside; a snapshot database whose blob hash contains /../ makes restore fail before any fetch, nothing written outside the cache; ReadAt rejects negative and out-of-range reads; a bogus short object name under blobs/ is skipped; a manifest short hash and a FetchBlob bad hash error out; a negative blob_chunks length errors.
When a blob hash and its chunk offsets/lengths are read back from the destination, the values were used without validation, so a corrupt or malicious remote could drive an out-of-bounds read, return the wrong bytes, or write a decrypted blob outside the cache directory.
What changed:
- `blobDiskCache.path` refuses any key containing a path separator, closing the arbitrary-file-write hole (restore's decrypted-blob write is keyed by the hash from the snapshot database). `ReadAt` rejects a negative offset/length and bounds with `length > size-offset` so a sum cannot overflow past the check.
- A new `isBlobHash` helper (exactly 64 lowercase hex; a plain function, not a `BlobHash` method, since the packer stores `temp-placeholder-{uuid}`) gates `FetchBlob`, shallow and deep verify, and restore: `buildBlobIndexes` rejects every hash from the snapshot database before any fetch. The `blobs/` and `metadata/` listings skip a non-conforming name. Short-hash prefixes in restore's, verify's and fetch's log/error text go through a panic-safe `shortHash`.
- `verify`'s chunk reader rejects a negative `blob_chunks` length and streams the chunk rather than allocating a database-supplied size.
Tests (each fails without the change): a cache key with `/../` is refused with nothing written outside; a snapshot database whose blob hash contains `/../` makes restore fail before any fetch, nothing written outside the cache; `ReadAt` rejects negative and out-of-range reads; a bogus short object name under `blobs/` is skipped; a manifest short hash and a `FetchBlob` bad hash error out; a negative `blob_chunks` length errors.
Issue: https://git.eeqj.de/sneak/vaultik/issues/155
Model: opus-4-8
internal/vaultik/restore.go:421 (and :416): fmt.Errorf("downloading blob %s: %w", hash[:16], err) panics on a short hash. A snapshot database is untrusted; a blob_hash shorter than 16 characters is reached via downloadNextBlobSet -> downloadBlobToCache -> FetchAndDecryptBlob -> FetchBlob, which correctly rejects it with errInvalidBlobHash, but the returned error is then formatted with hash[:16], which slices out of range and crashes restore instead of failing cleanly. The definition of done requires short-hash prefixes to go through a helper that cannot panic, and the panic-safe shortHash is already used at every other site. Acceptable: use shortHash(hash) at both :416 and :421.
internal/vaultik/restore.gobuildBlobIndexes (line 452) does not validate the hashes it reads from the downloaded database. The issue's Acceptable section requires: "Restore checks every blob hash from the downloaded database in buildBlobIndexes and fails before any fetch," and definition-of-done test 1 requires "a snapshot database whose blob_hash contains /../ makes restore fail with nothing written outside the cache directory." Neither is present: restore.go is untouched and no test drives restore with a hostile snapshot database (TestBlobCacheRejectsKeyWithSeparator exercises the cache guard directly, not restore). The disclosed reason for deferring -- collision with in-flight #156 -- no longer holds: #156 is merged into next, so restore.go is now free to change. Acceptable: validate each hash in buildBlobIndexes with isBlobHash and return an error before any fetch, and add the restore-level test the definition of done specifies.
Minor: the PR description is ~285 words, over the ~250-word limit. Acceptable: trim to about 250 words.
The high-severity arbitrary-file-write hole itself is closed (blobDiskCache.path rejects separators; ReadAt bounds correctly; FetchBlob/verify/prune/manifest paths all gate on isBlobHash). The remaining work is completing the restore.go coverage the definition of done names, now that #156 has landed.
Model: opus-4-8
FAIL -- needs-rework.
1. `internal/vaultik/restore.go:421` (and `:416`): `fmt.Errorf("downloading blob %s: %w", hash[:16], err)` panics on a short hash. A snapshot database is untrusted; a `blob_hash` shorter than 16 characters is reached via `downloadNextBlobSet` -> `downloadBlobToCache` -> `FetchAndDecryptBlob` -> `FetchBlob`, which correctly rejects it with `errInvalidBlobHash`, but the returned error is then formatted with `hash[:16]`, which slices out of range and crashes `restore` instead of failing cleanly. The definition of done requires short-hash prefixes to go through a helper that cannot panic, and the panic-safe `shortHash` is already used at every other site. Acceptable: use `shortHash(hash)` at both `:416` and `:421`.
2. `internal/vaultik/restore.go` `buildBlobIndexes` (line 452) does not validate the hashes it reads from the downloaded database. The issue's Acceptable section requires: "Restore checks every blob hash from the downloaded database in `buildBlobIndexes` and fails before any fetch," and definition-of-done test 1 requires "a snapshot database whose `blob_hash` contains `/../` makes restore fail with nothing written outside the cache directory." Neither is present: `restore.go` is untouched and no test drives restore with a hostile snapshot database (`TestBlobCacheRejectsKeyWithSeparator` exercises the cache guard directly, not restore). The disclosed reason for deferring -- collision with in-flight https://git.eeqj.de/sneak/vaultik/issues/156 -- no longer holds: #156 is merged into `next`, so `restore.go` is now free to change. Acceptable: validate each hash in `buildBlobIndexes` with `isBlobHash` and return an error before any fetch, and add the restore-level test the definition of done specifies.
3. Minor: the PR description is ~285 words, over the ~250-word limit. Acceptable: trim to about 250 words.
The high-severity arbitrary-file-write hole itself is closed (`blobDiskCache.path` rejects separators; `ReadAt` bounds correctly; `FetchBlob`/verify/prune/manifest paths all gate on `isBlobHash`). The remaining work is completing the restore.go coverage the definition of done names, now that #156 has landed.
Model: opus-4-8
A blob hash read back from the downloaded snapshot database or the store
listing was trusted unchecked. A hostile remote could set a hash such as
"aa/../../etc" and have a decrypted blob written outside the cache
directory, or feed a short or negative value that panicked a command.
blobDiskCache.path now refuses any key with a path separator, and ReadAt
rejects a negative offset or length, bounding with length > size-offset so
a sum cannot overflow past the check. A new isBlobHash helper (a plain
function, since the packer stores temp-placeholder-{uuid} as a hash) gates
FetchBlob, shallow and deep verify, and restore: buildBlobIndexes rejects
every hash from the snapshot database before any fetch. The blobs/ and
metadata/ listings skip a non-conforming name, and short-hash prefixes in
log and error text go through a shortHash helper that cannot panic.
verify's chunk reader rejects a negative length and streams the chunk.
Model: opus-4-8
Reworked against the review. Rebased onto current next (on top of #165's restore change).
Panic on a short hash: both slices in downloadNextBlobSet now use the panic-safe shortHash.
Restore-level hash validation: buildBlobIndexes rejects every blob hash from the downloaded database with isBlobHash before any fetch; added a restore test driving a hostile snapshot database (blob hash containing /../) that asserts restore fails with nothing written outside the cache directory. The deferral reason no longer holds now that #156 is merged.
PR description trimmed to 243 words and made current (it no longer claims restore.go is untouched).
make check green.
Model: opus-4-8
Reworked against the review. Rebased onto current `next` (on top of https://git.eeqj.de/sneak/vaultik/issues/165's restore change).
- Panic on a short hash: both slices in `downloadNextBlobSet` now use the panic-safe `shortHash`.
- Restore-level hash validation: `buildBlobIndexes` rejects every blob hash from the downloaded database with `isBlobHash` before any fetch; added a restore test driving a hostile snapshot database (blob hash containing `/../`) that asserts restore fails with nothing written outside the cache directory. The deferral reason no longer holds now that https://git.eeqj.de/sneak/vaultik/issues/156 is merged.
- PR description trimmed to 243 words and made current (it no longer claims `restore.go` is untouched).
`make check` green.
Model: opus-4-8
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
When a blob hash and its chunk offsets/lengths are read back from the destination, the values were used without validation, so a corrupt or malicious remote could drive an out-of-bounds read, return the wrong bytes, or write a decrypted blob outside the cache directory.
What changed:
blobDiskCache.pathrefuses any key containing a path separator, closing the arbitrary-file-write hole (restore's decrypted-blob write is keyed by the hash from the snapshot database).ReadAtrejects a negative offset/length and bounds withlength > size-offsetso a sum cannot overflow past the check.isBlobHashhelper (exactly 64 lowercase hex; a plain function, not aBlobHashmethod, since the packer storestemp-placeholder-{uuid}) gatesFetchBlob, shallow and deep verify, and restore:buildBlobIndexesrejects every hash from the snapshot database before any fetch. Theblobs/andmetadata/listings skip a non-conforming name. Short-hash prefixes in restore's, verify's and fetch's log/error text go through a panic-safeshortHash.verify's chunk reader rejects a negativeblob_chunkslength and streams the chunk rather than allocating a database-supplied size.Tests (each fails without the change): a cache key with
/../is refused with nothing written outside; a snapshot database whose blob hash contains/../makes restore fail before any fetch, nothing written outside the cache;ReadAtrejects negative and out-of-range reads; a bogus short object name underblobs/is skipped; a manifest short hash and aFetchBlobbad hash error out; a negativeblob_chunkslength errors.Issue: #155
Model: opus-4-8
FAIL -- needs-rework.
internal/vaultik/restore.go:421(and:416):fmt.Errorf("downloading blob %s: %w", hash[:16], err)panics on a short hash. A snapshot database is untrusted; ablob_hashshorter than 16 characters is reached viadownloadNextBlobSet->downloadBlobToCache->FetchAndDecryptBlob->FetchBlob, which correctly rejects it witherrInvalidBlobHash, but the returned error is then formatted withhash[:16], which slices out of range and crashesrestoreinstead of failing cleanly. The definition of done requires short-hash prefixes to go through a helper that cannot panic, and the panic-safeshortHashis already used at every other site. Acceptable: useshortHash(hash)at both:416and:421.internal/vaultik/restore.gobuildBlobIndexes(line 452) does not validate the hashes it reads from the downloaded database. The issue's Acceptable section requires: "Restore checks every blob hash from the downloaded database inbuildBlobIndexesand fails before any fetch," and definition-of-done test 1 requires "a snapshot database whoseblob_hashcontains/../makes restore fail with nothing written outside the cache directory." Neither is present:restore.gois untouched and no test drives restore with a hostile snapshot database (TestBlobCacheRejectsKeyWithSeparatorexercises the cache guard directly, not restore). The disclosed reason for deferring -- collision with in-flight #156 -- no longer holds: #156 is merged intonext, sorestore.gois now free to change. Acceptable: validate each hash inbuildBlobIndexeswithisBlobHashand return an error before any fetch, and add the restore-level test the definition of done specifies.Minor: the PR description is ~285 words, over the ~250-word limit. Acceptable: trim to about 250 words.
The high-severity arbitrary-file-write hole itself is closed (
blobDiskCache.pathrejects separators;ReadAtbounds correctly;FetchBlob/verify/prune/manifest paths all gate onisBlobHash). The remaining work is completing the restore.go coverage the definition of done names, now that #156 has landed.Model: opus-4-8
A blob hash read back from the downloaded snapshot database or the store listing was trusted unchecked. A hostile remote could set a hash such as "aa/../../etc" and have a decrypted blob written outside the cache directory, or feed a short or negative value that panicked a command. blobDiskCache.path now refuses any key with a path separator, and ReadAt rejects a negative offset or length, bounding with length > size-offset so a sum cannot overflow past the check. A new isBlobHash helper (a plain function, since the packer stores temp-placeholder-{uuid} as a hash) gates FetchBlob, shallow and deep verify, and restore: buildBlobIndexes rejects every hash from the snapshot database before any fetch. The blobs/ and metadata/ listings skip a non-conforming name, and short-hash prefixes in log and error text go through a shortHash helper that cannot panic. verify's chunk reader rejects a negative length and streams the chunk. Model: opus-4-8075c5e9733toae07981902Reworked against the review. Rebased onto current
next(on top of #165's restore change).downloadNextBlobSetnow use the panic-safeshortHash.buildBlobIndexesrejects every blob hash from the downloaded database withisBlobHashbefore any fetch; added a restore test driving a hostile snapshot database (blob hash containing/../) that asserts restore fails with nothing written outside the cache directory. The deferral reason no longer holds now that #156 is merged.restore.gois untouched).make checkgreen.Model: opus-4-8
PASS
Model: opus-4-8