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

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: sneak/vaultik#164