Open the downloaded snapshot database read-only, keep it in a private temp directory, remove it on every path #162

New Issue

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

clawbot commented

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

Found by the security review #73. Severity: low (defence in depth on the machine holding the private key; item 2 leaves decrypted metadata on disk).

What is wrong

Restore opens the downloaded file with database.New (internal/vaultik/restore.go:647-651), the constructor for the local index: read-write, and applyMigrations runs against whatever the file carries (internal/database/database.go:158). Deep verify uses a bare sql.Open (internal/vaultik/verify.go:318). Neither opens read-only, sets query_only or trusted_schema=OFF, or checks the schema. A view named like a table, or a trigger, in a forged file changes what the restore queries return. The real schema defines no triggers, views or virtual tables.
downloadSnapshotDB removes the decrypted temp file on a write or close error but not when database.New fails (restore.go:648-651); the caller's cleanup defer is not registered yet. Reached by Ctrl-C during open, a damaged payload, or a full disk. Only the main file is ever removed, not SQLite side files (-journal, -wal, -shm), and both commands place the file directly in the shared temp directory.
Deep verify discards the error from closing the temp file before opening it (verify.go:313); restore checks the same close (restore.go:638-643).
pickNextDownload (internal/vaultik/restore_plan.go:175-198) returns the zero FileID to mean "nothing left". A file row with the nil UUID stops the loop early and silently abandons every file still pending.

Acceptable

One open function in internal/database for downloaded snapshot databases, used by restore and deep verify: read-only, never applies schema files, query_only=ON, trusted_schema=OFF, and fails if sqlite_master holds any trigger, view or virtual table or lacks an expected table.
Each command creates one private directory with os.MkdirTemp, puts the decrypted database inside it, and removes the whole directory with RemoveAll on every return path, including the open failure.
verify.go:313 checks the close error like restore does.
pickNextDownload returns (FileID, bool) like popReady, and runRestoreLoop returns an error when plan.hasPending() is still true after the loop.

Definition of done

Tests: a database containing a view named files is refused; forcing the open to fail leaves no temp directory behind; a file row with the nil UUID does not end the restore early with success.
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** (defence in depth on the machine holding the private key; item 2 leaves decrypted metadata on disk). ## What is wrong 1. Restore opens the downloaded file with `database.New` (`internal/vaultik/restore.go:647-651`), the constructor for the local index: read-write, and `applyMigrations` runs against whatever the file carries (`internal/database/database.go:158`). Deep verify uses a bare `sql.Open` (`internal/vaultik/verify.go:318`). Neither opens read-only, sets `query_only` or `trusted_schema=OFF`, or checks the schema. A view named like a table, or a trigger, in a forged file changes what the restore queries return. The real schema defines no triggers, views or virtual tables. 2. `downloadSnapshotDB` removes the decrypted temp file on a write or close error but not when `database.New` fails (`restore.go:648-651`); the caller's cleanup defer is not registered yet. Reached by Ctrl-C during open, a damaged payload, or a full disk. Only the main file is ever removed, not SQLite side files (`-journal`, `-wal`, `-shm`), and both commands place the file directly in the shared temp directory. 3. Deep verify discards the error from closing the temp file before opening it (`verify.go:313`); restore checks the same close (`restore.go:638-643`). 4. `pickNextDownload` (`internal/vaultik/restore_plan.go:175-198`) returns the zero `FileID` to mean "nothing left". A file row with the nil UUID stops the loop early and silently abandons every file still pending. ## Acceptable - One open function in `internal/database` for downloaded snapshot databases, used by restore and deep verify: read-only, never applies schema files, `query_only=ON`, `trusted_schema=OFF`, and fails if `sqlite_master` holds any trigger, view or virtual table or lacks an expected table. - Each command creates one private directory with `os.MkdirTemp`, puts the decrypted database inside it, and removes the whole directory with `RemoveAll` on every return path, including the open failure. - `verify.go:313` checks the close error like restore does. - `pickNextDownload` returns `(FileID, bool)` like `popReady`, and `runRestoreLoop` returns an error when `plan.hasPending()` is still true after the loop. ## Definition of done 1. Tests: a database containing a view named `files` is refused; forcing the open to fail leaves no temp directory behind; a file row with the nil UUID does not end the restore early with success. 2. No existing assertion weakened; `make check` green. Line numbers are as of `next` at `6fcd8e1`. model: fable-5-1

clawbot referenced this issue from a commit

2026-09-22 12:09:14 +02:00

Open the downloaded snapshot database read-only, on a private temp dir (closes #162)

clawbot referenced a pull request that will close this issue

2026-09-22 12:09:29 +02:00

Open the downloaded snapshot database read-only, on a private temp dir #186

clawbot commented

2026-09-22 12:09:56 +02:00

Implemented in #186.

Restore and deep verify now open the downloaded snapshot database through a single new read-only open function: read-only at the OS level, query_only on, trusted_schema off, no schema files applied, and it refuses any file whose schema carries a trigger, view or virtual table or lacks an expected table. Each command materializes the database in its own private 0700 temp directory and removes the whole directory on every exit path, including the open failure that previously leaked decrypted metadata. Deep verify also now checks the temp-file close error. pickNextDownload returns a found flag so a file with the nil UUID no longer ends the restore early, and the loop errors out if files remain pending.

Model: opus-4-8

Implemented in https://git.eeqj.de/sneak/vaultik/pulls/186. Restore and deep verify now open the downloaded snapshot database through a single new read-only open function: read-only at the OS level, query_only on, trusted_schema off, no schema files applied, and it refuses any file whose schema carries a trigger, view or virtual table or lacks an expected table. Each command materializes the database in its own private 0700 temp directory and removes the whole directory on every exit path, including the open failure that previously leaked decrypted metadata. Deep verify also now checks the temp-file close error. pickNextDownload returns a found flag so a file with the nil UUID no longer ends the restore early, and the loop errors out if files remain pending. Model: opus-4-8

clawbot referenced this issue from a commit

2026-09-22 12:31:42 +02:00

Open the downloaded snapshot database read-only, on a private temp dir (closes #162)

clawbot closed this issue

2026-09-22 12:45:54 +02:00

clawbot referenced this issue from a commit

2026-09-22 12:45:56 +02:00

Open the downloaded snapshot database read-only, on a private temp dir (closes #162)

clawbot referenced this issue

2026-09-22 12:48:14 +02:00

next: accumulated work for the upcoming release #114

~~clawbot referenced this issue 2026-09-22 13:09:39 +02:00~~