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

Restore must keep every write inside the target directory #154

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

Found by the security review #73. Severity: high.

What is wrong

restoreFile (internal/vaultik/restore.go:760) builds the destination as filepath.Join(opts.TargetDir, file.Path). Join cleans the result, so a stored path containing .. lands outside the target, and nothing checks the result. Symlinks are restored first with their target written verbatim (restore.go:786), and MkdirAll/Create follow them, so a symlink entry a pointing to /etc plus a file entry a/x writes /etc/x. restoreSymlink also removes the joined path first (:782) and applyFileMetadata chmods and chowns it (:823-830), so deletion and mode changes escape too. verifyRestoredFiles joins the same way (:1060).

Why it matters

age encryption does not prove who wrote a file: anyone who knows a recipient string and can write to the destination can produce a snapshot database that decrypts cleanly. A compromised backed-up host has both. Restore usually runs as root on the machine that holds the private key, so a forged snapshot becomes arbitrary file write there.

Acceptable

  • Every path from the snapshot database is rejected before any Remove, MkdirAll, Create, Symlink, Chmod or Chown unless filepath.IsLocal accepts it with the leading separator removed. verifyRestoredFiles applies the same check.
  • Before writing, each existing path component below the target is Lstated (afero LstatIfPossible) and restore refuses to pass through a symlink. The target directory itself may be a symlink.
  • Symlink targets are still written verbatim: honest backups contain links that point outside the tree.
  • Do not use os.Root: restore writes through afero.Fs and its tests run on MemMapFs.

Definition of done

  1. Tests build a snapshot database with (a) a ../ path, (b) an absolute-looking path with .. segments, (c) a symlink entry followed by a child path through it. Each makes restore fail with a clear error and nothing created, removed or chmodded outside the target.
  2. An honest snapshot containing a symlink that points outside the tree still restores.
  3. No existing assertion weakened; make check green.

Blob-hash validation is #155; do not fold it in here.

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**. ## What is wrong `restoreFile` (`internal/vaultik/restore.go:760`) builds the destination as `filepath.Join(opts.TargetDir, file.Path)`. `Join` cleans the result, so a stored path containing `..` lands outside the target, and nothing checks the result. Symlinks are restored first with their target written verbatim (`restore.go:786`), and `MkdirAll`/`Create` follow them, so a symlink entry `a` pointing to `/etc` plus a file entry `a/x` writes `/etc/x`. `restoreSymlink` also removes the joined path first (`:782`) and `applyFileMetadata` chmods and chowns it (`:823-830`), so deletion and mode changes escape too. `verifyRestoredFiles` joins the same way (`:1060`). ## Why it matters age encryption does not prove who wrote a file: anyone who knows a recipient string and can write to the destination can produce a snapshot database that decrypts cleanly. A compromised backed-up host has both. Restore usually runs as root on the machine that holds the private key, so a forged snapshot becomes arbitrary file write there. ## Acceptable - Every path from the snapshot database is rejected before any `Remove`, `MkdirAll`, `Create`, `Symlink`, `Chmod` or `Chown` unless `filepath.IsLocal` accepts it with the leading separator removed. `verifyRestoredFiles` applies the same check. - Before writing, each existing path component below the target is `Lstat`ed (afero `LstatIfPossible`) and restore refuses to pass through a symlink. The target directory itself may be a symlink. - Symlink targets are still written verbatim: honest backups contain links that point outside the tree. - Do not use `os.Root`: restore writes through `afero.Fs` and its tests run on `MemMapFs`. ## Definition of done 1. Tests build a snapshot database with (a) a `../` path, (b) an absolute-looking path with `..` segments, (c) a symlink entry followed by a child path through it. Each makes restore fail with a clear error and nothing created, removed or chmodded outside the target. 2. An honest snapshot containing a symlink that points outside the tree still restores. 3. No existing assertion weakened; `make check` green. Blob-hash validation is https://git.eeqj.de/sneak/vaultik/issues/155; do not fold it in here. Line numbers are as of `next` at `6fcd8e1`. model: fable-5-1
clawbot commented 2026-09-22 09:26:15 +02:00
Author
Collaborator
Copy Link
Copy Source

PR: #176

Restore now resolves every stored path through one containment check before any Remove, MkdirAll, Create, Symlink, Chmod or Chown, and verifyRestoredFiles uses the same check. A path is rejected unless filepath.IsLocal accepts it with the leading separator removed, and each existing ancestor below the target is Lstated to refuse descending through a symlink. The target directory itself may be a symlink, and honest symlinks pointing outside the tree are still written verbatim.

Tests cover the three required cases (a ../ path, an absolute path with .. segments, and a symlink entry followed by a child through it) plus an honest outside-pointing symlink; each traversal case was confirmed to fail without the guard. make check green.

Blob-hash validation (#155) was left out of scope.

Model: opus-4-8

PR: https://git.eeqj.de/sneak/vaultik/pulls/176 Restore now resolves every stored path through one containment check before any `Remove`, `MkdirAll`, `Create`, `Symlink`, `Chmod` or `Chown`, and `verifyRestoredFiles` uses the same check. A path is rejected unless `filepath.IsLocal` accepts it with the leading separator removed, and each existing ancestor below the target is `Lstat`ed to refuse descending through a symlink. The target directory itself may be a symlink, and honest symlinks pointing outside the tree are still written verbatim. Tests cover the three required cases (a `../` path, an absolute path with `..` segments, and a symlink entry followed by a child through it) plus an honest outside-pointing symlink; each traversal case was confirmed to fail without the guard. `make check` green. Blob-hash validation (https://git.eeqj.de/sneak/vaultik/issues/155) was left out of scope. 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#154