Bound what a downloaded object may expand to, and escape control characters from the destination before printing #164

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 (denial of service and terminal spoofing by a party who can write to the destination; item 1 reaches the backed-up host, which has no private key).

What is wrong

All four zstd decoders are created with no options (internal/snapshot/manifest.go:34, internal/blobgen/reader.go:32, internal/vaultik/verify.go:288, :354). Total output is unbounded. The manifest is decoded with json.NewDecoder(...).Decode, which buffers the whole value, so a manifest of tens of kilobytes can expand to gigabytes of memory. This needs no key and hits prune, snapshot list, remote info and shallow verify.
Restore reads the encrypted database fully into memory and then the decompressed database fully into memory (internal/vaultik/restore.go:597, :614). Deep verify already streams it to a file (verify.go:305).
Blobs stream into the restore cache with no limit (internal/vaultik/blobcache.go:139) although the snapshot database records uncompressed_size.
FetchBlob takes expectedSize and makes an extra Stat request per blob, but only writes both numbers to the debug log (internal/vaultik/blob_fetch.go:95-127); both callers discard the returned size.
On a terminal, TTYHandler prints messages and attribute values with plain %s (internal/log/tty_handler.go:120-123, :263-265), as does ui.Writer (internal/ui/ui.go:315-321). Values that come unchecked from the destination reach it: the manifest timestamp, storage error text, and paths and symlink targets from the snapshot database. The JSON handler escapes; the terminal path does not.

Acceptable

DecodeManifest reads through a byte limit on both compressed and decompressed input, set far above any manifest the writer can emit. (Prune must already abort on an unreadable manifest for this to be safe: do #157 first.)
downloadSnapshotDB streams to its temp file with io.Copy in place of the two ReadAll calls.
Blob decompression stops with an error once output exceeds the uncompressed_size recorded in the snapshot database. Do not use the restoring host's blob_size_limit; that config can differ from the backup host's.
Either FetchBlob returns an error when the Stat size differs from expectedSize, or the Stat, the parameter and the returned size are removed.
TTYHandler escapes control characters in the message, attribute keys and values before adding its own colour codes (for example strconv.Quote when a value contains a non-printable rune). ui.Writer escapes untrusted text before colour is applied, not on the finished line, because its value formatters return already coloured strings.

Definition of done

Tests: a small, highly compressible object fed to each reader fails within the bound; a value containing ESC and a newline logged through TTYHandler and ui.Writer shows neither byte in the output.
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** (denial of service and terminal spoofing by a party who can write to the destination; item 1 reaches the backed-up host, which has no private key). ## What is wrong 1. All four zstd decoders are created with no options (`internal/snapshot/manifest.go:34`, `internal/blobgen/reader.go:32`, `internal/vaultik/verify.go:288`, `:354`). Total output is unbounded. The manifest is decoded with `json.NewDecoder(...).Decode`, which buffers the whole value, so a manifest of tens of kilobytes can expand to gigabytes of memory. This needs no key and hits `prune`, `snapshot list`, `remote info` and shallow verify. 2. Restore reads the encrypted database fully into memory and then the decompressed database fully into memory (`internal/vaultik/restore.go:597`, `:614`). Deep verify already streams it to a file (`verify.go:305`). 3. Blobs stream into the restore cache with no limit (`internal/vaultik/blobcache.go:139`) although the snapshot database records `uncompressed_size`. 4. `FetchBlob` takes `expectedSize` and makes an extra `Stat` request per blob, but only writes both numbers to the debug log (`internal/vaultik/blob_fetch.go:95-127`); both callers discard the returned size. 5. On a terminal, `TTYHandler` prints messages and attribute values with plain `%s` (`internal/log/tty_handler.go:120-123`, `:263-265`), as does `ui.Writer` (`internal/ui/ui.go:315-321`). Values that come unchecked from the destination reach it: the manifest `timestamp`, storage error text, and paths and symlink targets from the snapshot database. The JSON handler escapes; the terminal path does not. ## Acceptable - `DecodeManifest` reads through a byte limit on both compressed and decompressed input, set far above any manifest the writer can emit. (Prune must already abort on an unreadable manifest for this to be safe: do https://git.eeqj.de/sneak/vaultik/issues/157 first.) - `downloadSnapshotDB` streams to its temp file with `io.Copy` in place of the two `ReadAll` calls. - Blob decompression stops with an error once output exceeds the `uncompressed_size` recorded in the snapshot database. Do not use the restoring host's `blob_size_limit`; that config can differ from the backup host's. - Either `FetchBlob` returns an error when the `Stat` size differs from `expectedSize`, or the `Stat`, the parameter and the returned size are removed. - `TTYHandler` escapes control characters in the message, attribute keys and values before adding its own colour codes (for example `strconv.Quote` when a value contains a non-printable rune). `ui.Writer` escapes untrusted text before colour is applied, not on the finished line, because its value formatters return already coloured strings. ## Definition of done 1. Tests: a small, highly compressible object fed to each reader fails within the bound; a value containing ESC and a newline logged through `TTYHandler` and `ui.Writer` shows neither byte in the output. 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 14:31:11 +02:00

next: integrate accumulated work into main #114

clawbot referenced this issue from a commit

2026-09-22 16:35:03 +02:00

Bound download expansion and escape control chars on the terminal (closes #164)

clawbot referenced a pull request that will close this issue

2026-09-22 16:35:24 +02:00

Bound download expansion and escape control chars on the terminal (closes #164) #197

clawbot commented

2026-09-22 16:35:35 +02:00

Implemented in #197 (targets next).

All Acceptable items: a byte-cap reader (fails past the limit rather than a silent EOF) bounds DecodeManifest on both compressed and decompressed size; blob decompression stops once output exceeds the snapshot database's uncompressed_size (not the restoring host's blob_size_limit); downloadSnapshotDB streams to its temp file with io.Copy; FetchBlob drops the Stat, its size parameter and returned size; TTYHandler and ui.Writer escape control characters before colour is applied. Tests cover a compressible object failing within each bound and an ESC-and-newline value showing neither byte raw.

Model: opus-4-8

Implemented in https://git.eeqj.de/sneak/vaultik/pulls/197 (targets `next`). All Acceptable items: a byte-cap reader (fails past the limit rather than a silent EOF) bounds `DecodeManifest` on both compressed and decompressed size; blob decompression stops once output exceeds the snapshot database's `uncompressed_size` (not the restoring host's `blob_size_limit`); `downloadSnapshotDB` streams to its temp file with `io.Copy`; `FetchBlob` drops the `Stat`, its size parameter and returned size; `TTYHandler` and `ui.Writer` escape control characters before colour is applied. Tests cover a compressible object failing within each bound and an ESC-and-newline value showing neither byte raw. Model: opus-4-8

clawbot closed this issue

2026-09-22 17:00:36 +02:00

clawbot referenced this issue from a commit

2026-09-22 17:00:37 +02:00

Bound download expansion and escape control chars on the terminal (closes #164)

clawbot referenced this issue

2026-09-23 01:49:08 +02:00

Security review of the crypto and blob-generation seams #73

clawbot referenced this issue

2026-10-06 01:49:46 +02:00

`remote info` counts a live snapshot's blobs as orphaned when a manifest cannot be read, and prints store names unescaped #228

Sign in to join this conversation.