sneak/vaultik
1
0
Fork 0
Code Issues 25 Pull Requests 2 Actions Packages Projects Releases Wiki Activity

Restore: create files 0600, remove partial files, and make the blob hash check impossible to skip #163

New Issue
Closed
opened 2026-09-22 00:55:14 +02:00 by clawbot · 1 comment
clawbot commented 2026-09-22 00:55:14 +02:00
Collaborator
Copy Link
Copy Source

Found by the security review #73. Severity: low (hardening; item 1 matters on a multi-user restore machine).

What is wrong

  1. restoreRegularFile creates each file with the default mode (internal/vaultik/restore.go:871), writes all content, closes, and only then applies the stored mode (:902). Until then another local user can read a file whose stored mode is restrictive but whose directory is not; in a path-filtered restore that is every file, because ancestor directories outside the restored set are created 0755 (:764). When writeFileChunks fails, the partial file stays in place with the default mode (:880-883). A chmod failure is only a debug line (:823-826).
  2. hashVerifyReader.Close compares the blob hash only if Read reached EOF (internal/vaultik/blob_fetch.go:42-63); a caller that closes early gets nil. The single production caller reads to EOF and checks Close, so restore is correct today, but the one function guarding against a swapped blob passes silently when skipped. downloadBlobToCache also leaves the cache file of a blob that failed its hash until the deferred cache close (restore.go:989-999).
  3. verifyFile (restore.go:1126-1158), used by --verify, accepts trailing bytes after the last chunk.

Acceptable

  • Remove any existing entry at the target, then create the file 0600 with OpenFile and O_CREATE|O_EXCL (the mode argument is ignored for an existing file, and the README documents re-running over partial output). Apply the stored mode after close. A chmod failure on a regular file is a user-visible warning. A file whose restore failed is removed.
  • Close returns an error when called before EOF. downloadBlobToCache deletes the cache entry when Close fails.
  • verifyFile confirms the file is at EOF after the last chunk.
  • Do not compare bytes written with files.size: that is the stat taken during the scan, while the chunker reads to EOF later, so a file that changed during backup would fail to restore. If a whole-file length check is wanted, compare against the sum of chunks.size.

Whether the default restore path should also hash every chunk before writing it is an owner question on #73; not part of this issue.

Definition of done

  1. Tests under umask 022: a restored 0600 file is never observable with a wider mode; a failed file is removed; reading part of a blob then Close returns an error; a restored file with trailing bytes fails --verify.
  2. 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: **low** (hardening; item 1 matters on a multi-user restore machine). ## What is wrong 1. `restoreRegularFile` creates each file with the default mode (`internal/vaultik/restore.go:871`), writes all content, closes, and only then applies the stored mode (`:902`). Until then another local user can read a file whose stored mode is restrictive but whose directory is not; in a path-filtered restore that is every file, because ancestor directories outside the restored set are created 0755 (`:764`). When `writeFileChunks` fails, the partial file stays in place with the default mode (`:880-883`). A chmod failure is only a debug line (`:823-826`). 2. `hashVerifyReader.Close` compares the blob hash only if `Read` reached EOF (`internal/vaultik/blob_fetch.go:42-63`); a caller that closes early gets nil. The single production caller reads to EOF and checks `Close`, so restore is correct today, but the one function guarding against a swapped blob passes silently when skipped. `downloadBlobToCache` also leaves the cache file of a blob that failed its hash until the deferred cache close (`restore.go:989-999`). 3. `verifyFile` (`restore.go:1126-1158`), used by `--verify`, accepts trailing bytes after the last chunk. ## Acceptable - Remove any existing entry at the target, then create the file 0600 with `OpenFile` and `O_CREATE|O_EXCL` (the mode argument is ignored for an existing file, and the README documents re-running over partial output). Apply the stored mode after close. A chmod failure on a regular file is a user-visible warning. A file whose restore failed is removed. - `Close` returns an error when called before EOF. `downloadBlobToCache` deletes the cache entry when `Close` fails. - `verifyFile` confirms the file is at EOF after the last chunk. - Do not compare bytes written with `files.size`: that is the stat taken during the scan, while the chunker reads to EOF later, so a file that changed during backup would fail to restore. If a whole-file length check is wanted, compare against the sum of `chunks.size`. Whether the default restore path should also hash every chunk before writing it is an owner question on https://git.eeqj.de/sneak/vaultik/issues/73; not part of this issue. ## Definition of done 1. Tests under umask 022: a restored 0600 file is never observable with a wider mode; a failed file is removed; reading part of a blob then `Close` returns an error; a restored file with trailing bytes fails `--verify`. 2. No existing assertion weakened; `make check` green. Line numbers are as of `next` at `6fcd8e1`. model: fable-5-1
clawbot commented 2026-09-22 11:22:05 +02:00
Author
Collaborator
Copy Link
Copy Source

Done in #182 (base next).

  • Regular files are created with O_EXCL at 0600, then given their stored mode only after the content is written and closed; a failed write or close removes the file; a chmod failure is a user-visible warning.
  • The blob verifying reader now errors if closed before EOF, and the download drops the cache entry on any copy/close failure, so an unverified blob can never be read back as valid.
  • --verify now rejects a restored file with bytes past its last chunk (no files.size comparison added).

Tests under umask 022 cover all four definition-of-done cases. make check is green.

Model: opus-4-8

Done in https://git.eeqj.de/sneak/vaultik/pulls/182 (base `next`). - Regular files are created with `O_EXCL` at 0600, then given their stored mode only after the content is written and closed; a failed write or close removes the file; a chmod failure is a user-visible warning. - The blob verifying reader now errors if closed before EOF, and the download drops the cache entry on any copy/close failure, so an unverified blob can never be read back as valid. - `--verify` now rejects a restored file with bytes past its last chunk (no `files.size` comparison added). Tests under umask 022 cover all four definition-of-done cases. `make check` is green. Model: opus-4-8
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
merge-ready
All checks pass, reviewed, rebased, ready for merge
 needs-checks
make check does not pass cleanly
 needs-rebase
PR has merge conflicts or is behind main
 needs-review
Code review not yet done
 needs-rework
Code review found issues to fix
No labels
Milestone
No items
No Milestone
Projects
Clear projects
No projects
Assignees
Clear assignees
clawbot sneak (Jeffrey Paul)
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: sneak/vaultik#163