destroyAll() zeroes session keys that hooks.server.ts hands out by reference #141

New issue

Open

opened 2026-09-11 13:04:47 +02:00 by lz · 0 comments

lz commented

2026-09-11 13:04:47 +02:00

Owner

SessionStore.destroyAll() zeroes every stored session key in place, but those
same buffers are published to live request scopes by reference. A request already in
flight when a passphrase rotation lands has its masterKey zeroed underneath it, and
will seal its next secret under 32 zero bytes — writing a row that nothing can ever
open again.

Silent, and it corrupts data rather than failing.

The two halves

nexus/src/lib/server/auth/session.ts:92 — destroyAll() does s.key.fill(0) on
every stored session key.

nexus/src/hooks.server.ts:75 — event.locals.masterKey = session.key, no copy.
Every route handler that seals a value (connections, repo_env_vars) holds that
exact buffer for the life of the request.

nexus/src/routes/api/auth/rotate-passphrase/+server.ts:43 is the only caller, so the
window is one rotation, but any concurrent request sealing a secret inside it writes an
unopenable row.

Not the same as the `result.newKey.fill(0)` next to it

Worth stating, because the two look identical and only one is a bug. Rotation's own
fill(0) (rotate-passphrase/+server.ts:45) is safe: unlockMasterPassphrase
always allocates fresh (open() returns a Buffer.concat, newDek() returns
randomBytes), and the route copies with Buffer.from before storing, so it can only
reach memory nothing else holds. Verified by probe during the review of b6daea5.

destroyAll() is the inverse: the store zeroes memory it has already published. Same
mechanism, opposite ownership.

Envelope encryption makes the fix nearly free

Before envelope encryption, zeroing other sessions' keys had a point — rotation changed
the key the data was sealed under, so those copies were both stale and dangerous.

After it (PR for the vault envelope work), the DEK does not change across a rotation.
Zeroing the other sessions' copies buys nothing: the identical bytes stay resident in the
caller's newly created session either way. destroyAll() can simply drop the map entries
and let GC take the buffers.

If the in-place wipe is kept for hygiene, then hooks.server.ts:75 must hand out a copy
instead — but that allocates on every authenticated request to defend against a single
rare operator action, which is the worse trade.

Reproduction sketch

Unlock, so a session exists and a request is holding locals.masterKey.
Begin a request that seals a value but has not yet written it.
Rotate the passphrase from another tab.
Let the first request complete — the row it writes is sealed under 32 zero bytes.

Provenance

Found during the code-quality review of the vault envelope-encryption work
(b6daea5). Both halves pre-date that commit and nothing in it introduces or
widens the problem — the diff does not touch either file. Filed separately rather than
fixed there to keep that change to its reviewed scope.

`SessionStore.destroyAll()` zeroes every stored session key **in place**, but those same buffers are published to live request scopes by reference. A request already in flight when a passphrase rotation lands has its `masterKey` zeroed underneath it, and will seal its next secret under 32 zero bytes — writing a row that nothing can ever open again. Silent, and it corrupts data rather than failing. ## The two halves `nexus/src/lib/server/auth/session.ts:92` — `destroyAll()` does `s.key.fill(0)` on every stored session key. `nexus/src/hooks.server.ts:75` — `event.locals.masterKey = session.key`, **no copy**. Every route handler that seals a value (`connections`, `repo_env_vars`) holds that exact buffer for the life of the request. `nexus/src/routes/api/auth/rotate-passphrase/+server.ts:43` is the only caller, so the window is one rotation, but any concurrent request sealing a secret inside it writes an unopenable row. ## Not the same as the `result.newKey.fill(0)` next to it Worth stating, because the two look identical and only one is a bug. Rotation's own `fill(0)` (`rotate-passphrase/+server.ts:45`) is **safe**: `unlockMasterPassphrase` always allocates fresh (`open()` returns a `Buffer.concat`, `newDek()` returns `randomBytes`), and the route copies with `Buffer.from` before storing, so it can only reach memory nothing else holds. Verified by probe during the review of `b6daea5`. `destroyAll()` is the inverse: the store zeroes memory it has already published. Same mechanism, opposite ownership. ## Envelope encryption makes the fix nearly free Before envelope encryption, zeroing other sessions' keys had a point — rotation changed the key the data was sealed under, so those copies were both stale and dangerous. After it (PR for the vault envelope work), the DEK does **not** change across a rotation. Zeroing the other sessions' copies buys nothing: the identical bytes stay resident in the caller's newly created session either way. `destroyAll()` can simply drop the map entries and let GC take the buffers. If the in-place wipe is kept for hygiene, then `hooks.server.ts:75` must hand out a copy instead — but that allocates on every authenticated request to defend against a single rare operator action, which is the worse trade. ## Reproduction sketch 1. Unlock, so a session exists and a request is holding `locals.masterKey`. 2. Begin a request that seals a value but has not yet written it. 3. Rotate the passphrase from another tab. 4. Let the first request complete — the row it writes is sealed under 32 zero bytes. ## Provenance Found during the code-quality review of the vault envelope-encryption work (`b6daea5`). **Both halves pre-date that commit** and nothing in it introduces or widens the problem — the diff does not touch either file. Filed separately rather than fixed there to keep that change to its reviewed scope.

lz added the

bug

label

2026-09-11 13:04:47 +02:00

lz referenced this issue

2026-09-15 18:25:48 +02:00

Slice 2: machine principal, create_workspace and start_workspace #159