[review] matrix-rtc E2EE: media-key delivery correctness gaps (needs live E2EE verification) #69

New issue

Open

opened 2026-07-07 13:52:34 +00:00 by robocub · 0 comments

robocub commented

2026-07-07 13:52:34 +00:00

Collaborator

F3-1 — P2 / LIVE / CONFIRMED

Location: crates/matrix-rtc/src/rtc/key_transport.rs:201 @ 968da57

Defect: PlaintextTransport to-device txn ids ('{call_device_id}-key-{seq}') restart from 0 on every transport instance while the device id and as_token are stable, so a ghost respawn or a quick process restart reuses txn ids from the prior life and a spec-compliant homeserver's PUT /sendToDevice idempotency cache silently drops the re-sends (200 replay, no delivery) — the comment on seq claims exactly this cannot happen.

Failure scenario: Bridge configured with media_e2ee on + key_transport=plaintext; Discord user 42's ghost lives 10 min (sends txns D42-s-key-0..13), the user leaves and rejoins (or the bridge restarts, which also mints a NEW media key), a Matrix listener joins during the gap; the respawned transport re-sends starting at txn D42-s-key-0, the server dedupes every send inside its txn-retention window, the listener never receives the key, and the ghost is silent/static for minutes while logs show successful 'ghost media key sent'.

Verifier: Every step of the scenario holds in the actual code. (1) PlaintextTransport is built fresh per distribute_key_task spawn (ghost.rs:1254) with seq starting at 0 (key_transport.rs:160); unlike the Olm transport (GhostCryptoCache) and the media key (GhostKeyCache), it has no process-lifetime cache, so every ghost respawn restarts seq at 0. (2) The txn id '{device_id}-key-{seq}' (key_transport.rs:201) uses the stable call device id D{key}-{salt} (ghost.rs:694,736) and the stable as_token bearer, so a respawn's txn ids and idempotency scope are byte-identical to the prior life's. (3) A dedup replay is a 200, which the code maps to delivered=recipients.len() with no failures (key_transport.rs:215-224), so distribute_key_loop logs 'ghost media key sent', marks the listener keyed, drains the burst, and drops to the 60s heartbeat — a new listener joining after a respawn stays keyless for roughly (prior-life-seq-count minus 6) minutes (6-burst at 3s then 1 seq/min) while logs show success. The process-restart variant is worse: GhostKeyCache is process-lifetime so a restart mints a NEW key under recycled txn ids, giving existing listeners static within the server's txn-retention window. (4) The config is reachable and test-pinned: config tests assert explicit key_transport=plaintext survives in an encrypted room and media_e2ee=Some(true) on an unencrypted room auto-selects plaintext; the distribute task only runs with media_e2ee on. (5) The seq comment (key_transport.rs:139-141) indeed claims non-collision but only delivers it within one instance; no test or doc blesses cross-life txn reuse — the only PlaintextTransport test is an empty-recipients no-op. The sole live-dependent residual is whether Tuwunel's dedup cache/retention actually fires, but the claim conditions on a spec-compliant homeserver (the spec documents PUT /sendToDevice txnId as ensuring idempotency, and the Conduit lineage persists txn ids), so the code defect stands as claimed at P2 [LIVE].

Regression test sketch: Not required (P2, live-dependent delivery effect). If desired: in key_transport.rs tests, construct two PlaintextTransport instances with the same device_id (simulating respawn) against a mock server that records PUT paths, drive one send on each, and assert the second instance's txn path differs from the first's — pinning that a fresh instance must not reuse prior txn ids (fix: seed seq from a timestamp or add a per-instance random component).

F3-2 — P2 / LOCAL / CONFIRMED

Location: crates/matrix-rtc/src/rtc/ghost.rs:1431 @ 968da57

Defect: The #52 session-rotation clock (last_rotation = Instant::now()) is local to each distribute_key_loop invocation, but the OlmMachine it ages is the process-lifetime GhostCryptoCache entry reused across respawns — every ghost respawn resets the max-age clock to zero without rotating, so SESSION_ROTATE_MAX_AGE (15 min) is not actually a bound on session age.

Failure scenario: Encrypted room; a listener's inbound Olm session for ghost X is dead (the Ruffle-class wedge #52 exists to heal); the Discord speaker behind X cycles voice (leave/rejoin, AFK) every ~10-14 min; each respawn reuses the cached wedged machine via bootstrap_olm_transport and restarts the loop with last_rotation=now, so the 'max-age' rotation never fires and (absent Matrix membership bumps) the listener stays unable to hear X indefinitely — the exact 'until the bridge restarts' outcome #52 was shipped to eliminate. Pinnable with the existing MockTransport: run the loop 14 min, cancel, rerun with the same transport 14 more min → zero rotations despite 28 min of machine age.

Verifier: Every step of the failure scenario re-derives from the code in /root/nvb-refactor. (1) The rotation clock is loop-local: let mut last_rotation = tokio::time::Instant::now() at crates/matrix-rtc/src/rtc/ghost.rs:1431, initialized fresh on every distribute_key_loop invocation; no rotation timestamp is stored in GhostCrypto, OlmTransport, or the cache (verified key_transport.rs — rotate_sessions:476 rebuilds the machine but records no age). (2) The OlmMachine with its established outbound sessions IS process-lifetime: bootstrap_olm_transport (ghost.rs:1321-1333) returns the cached GhostCrypto on respawn ("reused cached OlmMachine across rejoin"), and normal sends reuse existing sessions — ensure_tracked_and_sessions (key_transport.rs:643-647) only claims OTKs via get_missing_sessions, so a listener-side dead session is never replaced except by rotate_sessions. (3) The respawn path is reachable: Discord voice leave → capture_router.release (discord/src/puppet.rs:2683 "Discord user left — ghost released") → GhostCommand::Release cancels the ghost token (ghost.rs:567-577), which is the same token the distribute_key_task was spawned with (ghost.rs:819), killing the loop; rejoin → Assign → new run_ghost → new loop with last_rotation=now and rotate_pending=false, over the same aged machine and the same cached media key (GhostKeyCache, ghost.rs:746-760, so the listener can't obtain the key by any other path). (4) The membership-rotation escape hatch requires a members_gen bump, which fires ONLY when a human Matrix member newly appears (matrix.rs:196; ghosts excluded at matrix.rs:150) — a static listener never bumps it. So a speaker cycling voice at <15 min intervals resets the max-age clock forever, and the documented invariant at ghost.rs:1220-1224 ("worst-case inaudibility ≈ this + one heartbeat, instead of 'until the bridge restarts'") is violated. Not deliberate: the comment at ghost.rs:1428-1429 justifies the fresh clock with "just (re)built or reused a machine", which is only valid for the fresh-bootstrap branch (genuinely sessionless machine) — the reuse branch carries arbitrarily old sessions. No test pins cross-invocation behavior; all rotation tests (key_loop_rotates_at_max_age_in_static_call etc., ghost.rs:2400-2520) exercise a single loop invocation. Severity P2 is fair: any human Matrix join/leave during the window rotates via the membership path after 90s, so it needs a static Matrix side plus a cycling Discord speaker — plausible (AFK-timeout moves) but narrow.

Regression test sketch: Belongs in the existing distribute_key_loop mock section of the #[cfg(test)] module in crates/matrix-rtc/src/rtc/ghost.rs (next to key_loop_rotates_at_max_age_in_static_call). #[tokio::test(start_paused = true)]: build one MockTransport (stands in for the cached machine — its rotations vec is the machine-lifetime record) and one MockSource with a static recipient; spawn distribute_key_loop(rotate=true), advance SESSION_ROTATE_MAX_AGE - 60s, cancel, await; spawn a SECOND distribute_key_loop with the SAME transport (simulating the ghost respawn reusing the cache), advance SESSION_ROTATE_MAX_AGE - 60s again, cancel. Assert transport.rotations is non-empty (machine age ~28 min > 15 min max-age). Today it asserts-fails with zero rotations, pinning the bug; the fix (moving last_rotation into the transport/GhostCrypto, or seeding the loop with the machine's real last-rotation instant) makes it pass.

F3-3 — P2 / LOCAL / CONFIRMED

Location: crates/matrix-rtc/src/rtc/key_transport.rs:538 @ 968da57

Defect: encrypt_and_send_key bases its outcome on attempted = devices.len() AFTER resolve_devices silently dropped recipients whose devices aren't in the machine's store (debug-level log only), so an unresolved in-call recipient appears in neither delivered nor failed — the send reports full success, distribute_key_loop's documented 'unreachable devices' INFO/'reached NO devices' WARN distinction never fires for them, and they are marked keyed (ghost.rs:1537) as if delivered.

Failure scenario: Encrypted room, ghost live with human A already keyed; human B joins but B's device keys stay unfetchable to the ghost's /keys/query past the 6x3s burst (federation/HS lag, or a device the server never returns): every send resolves only A, returns KeySendOutcome{delivered:1, failed:[]}, the loop logs the full-success 'ghost media key sent' arm listing B among recipients and marks B keyed — B silently never gets the key (re-tried only at the 60s heartbeat, with no WARN ever), so B hears static from that ghost while logs claim complete delivery, defeating the per-device-unreachable diagnostics contract.

Verifier: Re-derivation from actual code holds at every step. In encrypt_and_send_key (key_transport.rs:521-585), resolve_devices (673-693) drops a recipient whose device get_device returns Ok(None) at DEBUG only; attempted = devices.len() (538) counts only resolved devices, and failed is built solely from withheld devices and failed PUT chunks — both post-resolution. So with A resolved and B unresolved, the outcome is {delivered:1, failed:[]}, which distribute_key_loop's full-success arm (ghost.rs:1478-1487) logs as "ghost media key sent" with recipients listing B, and B enters keyed (1537), so the burst is never re-armed for B and only the 60s heartbeat retries — with neither the partial-delivery "unreachable devices" INFO (1495) nor the "reached NO recipient devices" WARN (1512) ever firing for B. Crucially this is NOT documented-deliberate: it contradicts the module's own contract — FailedRecipient's doc (key_transport.rs:54-56) explicitly names "a recipient device not yet tracked" as an example of what failed carries, and the KeyTransport trait doc (102-104) says the outcome reports "which didn't" get the key. No #[cfg(test)] test pins the omission (key_transport tests cover identity/upload/parsing; the ghost.rs ColdThenWarmTransport mock actually models the opposite convention, putting unreached devices in failed). The state is reachable: recipients come from per-device m.call.member state, and the Ok(None)/"will retry" arm exists precisely because /keys/query can lag or never return a device (the guard at 533 only errors when NO device resolves). The only live-dependent aspect (how long B stays unresolvable) affects severity, not the misreporting itself, which fires on every such send. P2 diagnostics/contract defect as claimed.

Regression test sketch: Not required (P2), but if wanted: in ghost.rs's existing distribute_key_loop test module, the fix's natural pin is a unit test on the outcome contract — factor the recipients→failed accounting so it's testable, or add a MockTransport variant returning {delivered:1, failed:[]} for a 2-recipient send and assert the loop treats a send whose delivered+failed < recipients.len() as partial (unreachable arm / burst kept), not full success.

Filed from an automated multi-agent code review of master @ 968da57 (8 scoped finders + adversarial verification). Every finding below was re-derived from the code by an independent verifier. Fixes for the confirmed local-verifiable defects (allocator anchor panic F1-1/F1-2/F1-3/F2-1/F2-2, capture-sink panic F2-7, config env-fallback F5-4, opt-out RMW race F5-2) already landed on branch refactor/review-and-split.

Three key-transport issues, all on the do-not-disturb E2EE path — verify against a live encrypted bridge before changing: plaintext to-device txn-ids restart from 0 on respawn while device-id/as_token are stable (dedup/replace hazard); the #52 session-rotation max-age clock is per-invocation but the OlmMachine is process-lifetime (max-age rotation never fires across respawns); and `encrypt_and_send_key` reports success even when `resolve_devices` silently dropped an in-call recipient (no `failed`/`delivered` accounting → silent no-key). ### F3-1 — P2 / LIVE / CONFIRMED **Location:** `crates/matrix-rtc/src/rtc/key_transport.rs:201` @ `968da57` **Defect:** PlaintextTransport to-device txn ids ('{call_device_id}-key-{seq}') restart from 0 on every transport instance while the device id and as_token are stable, so a ghost respawn or a quick process restart reuses txn ids from the prior life and a spec-compliant homeserver's PUT /sendToDevice idempotency cache silently drops the re-sends (200 replay, no delivery) — the comment on `seq` claims exactly this cannot happen. **Failure scenario:** Bridge configured with media_e2ee on + key_transport=plaintext; Discord user 42's ghost lives 10 min (sends txns D42-s-key-0..13), the user leaves and rejoins (or the bridge restarts, which also mints a NEW media key), a Matrix listener joins during the gap; the respawned transport re-sends starting at txn D42-s-key-0, the server dedupes every send inside its txn-retention window, the listener never receives the key, and the ghost is silent/static for minutes while logs show successful 'ghost media key sent'. **Verifier:** Every step of the scenario holds in the actual code. (1) PlaintextTransport is built fresh per distribute_key_task spawn (ghost.rs:1254) with seq starting at 0 (key_transport.rs:160); unlike the Olm transport (GhostCryptoCache) and the media key (GhostKeyCache), it has no process-lifetime cache, so every ghost respawn restarts seq at 0. (2) The txn id '{device_id}-key-{seq}' (key_transport.rs:201) uses the stable call device id D{key}-{salt} (ghost.rs:694,736) and the stable as_token bearer, so a respawn's txn ids and idempotency scope are byte-identical to the prior life's. (3) A dedup replay is a 200, which the code maps to delivered=recipients.len() with no failures (key_transport.rs:215-224), so distribute_key_loop logs 'ghost media key sent', marks the listener keyed, drains the burst, and drops to the 60s heartbeat — a new listener joining after a respawn stays keyless for roughly (prior-life-seq-count minus 6) minutes (6-burst at 3s then 1 seq/min) while logs show success. The process-restart variant is worse: GhostKeyCache is process-lifetime so a restart mints a NEW key under recycled txn ids, giving existing listeners static within the server's txn-retention window. (4) The config is reachable and test-pinned: config tests assert explicit key_transport=plaintext survives in an encrypted room and media_e2ee=Some(true) on an unencrypted room auto-selects plaintext; the distribute task only runs with media_e2ee on. (5) The seq comment (key_transport.rs:139-141) indeed claims non-collision but only delivers it within one instance; no test or doc blesses cross-life txn reuse — the only PlaintextTransport test is an empty-recipients no-op. The sole live-dependent residual is whether Tuwunel's dedup cache/retention actually fires, but the claim conditions on a spec-compliant homeserver (the spec documents PUT /sendToDevice txnId as ensuring idempotency, and the Conduit lineage persists txn ids), so the code defect stands as claimed at P2 [LIVE]. **Regression test sketch:** Not required (P2, live-dependent delivery effect). If desired: in key_transport.rs tests, construct two PlaintextTransport instances with the same device_id (simulating respawn) against a mock server that records PUT paths, drive one send on each, and assert the second instance's txn path differs from the first's — pinning that a fresh instance must not reuse prior txn ids (fix: seed seq from a timestamp or add a per-instance random component). ### F3-2 — P2 / LOCAL / CONFIRMED **Location:** `crates/matrix-rtc/src/rtc/ghost.rs:1431` @ `968da57` **Defect:** The #52 session-rotation clock (`last_rotation = Instant::now()`) is local to each distribute_key_loop invocation, but the OlmMachine it ages is the process-lifetime GhostCryptoCache entry reused across respawns — every ghost respawn resets the max-age clock to zero without rotating, so SESSION_ROTATE_MAX_AGE (15 min) is not actually a bound on session age. **Failure scenario:** Encrypted room; a listener's inbound Olm session for ghost X is dead (the Ruffle-class wedge #52 exists to heal); the Discord speaker behind X cycles voice (leave/rejoin, AFK) every ~10-14 min; each respawn reuses the cached wedged machine via bootstrap_olm_transport and restarts the loop with last_rotation=now, so the 'max-age' rotation never fires and (absent Matrix membership bumps) the listener stays unable to hear X indefinitely — the exact 'until the bridge restarts' outcome #52 was shipped to eliminate. Pinnable with the existing MockTransport: run the loop 14 min, cancel, rerun with the same transport 14 more min → zero rotations despite 28 min of machine age. **Verifier:** Every step of the failure scenario re-derives from the code in /root/nvb-refactor. (1) The rotation clock is loop-local: `let mut last_rotation = tokio::time::Instant::now()` at crates/matrix-rtc/src/rtc/ghost.rs:1431, initialized fresh on every distribute_key_loop invocation; no rotation timestamp is stored in GhostCrypto, OlmTransport, or the cache (verified key_transport.rs — rotate_sessions:476 rebuilds the machine but records no age). (2) The OlmMachine with its established outbound sessions IS process-lifetime: bootstrap_olm_transport (ghost.rs:1321-1333) returns the cached GhostCrypto on respawn ("reused cached OlmMachine across rejoin"), and normal sends reuse existing sessions — ensure_tracked_and_sessions (key_transport.rs:643-647) only claims OTKs via get_missing_sessions, so a listener-side dead session is never replaced except by rotate_sessions. (3) The respawn path is reachable: Discord voice leave → capture_router.release (discord/src/puppet.rs:2683 "Discord user left — ghost released") → GhostCommand::Release cancels the ghost token (ghost.rs:567-577), which is the same token the distribute_key_task was spawned with (ghost.rs:819), killing the loop; rejoin → Assign → new run_ghost → new loop with last_rotation=now and rotate_pending=false, over the same aged machine and the same cached media key (GhostKeyCache, ghost.rs:746-760, so the listener can't obtain the key by any other path). (4) The membership-rotation escape hatch requires a members_gen bump, which fires ONLY when a human Matrix member newly appears (matrix.rs:196; ghosts excluded at matrix.rs:150) — a static listener never bumps it. So a speaker cycling voice at <15 min intervals resets the max-age clock forever, and the documented invariant at ghost.rs:1220-1224 ("worst-case inaudibility ≈ this + one heartbeat, instead of 'until the bridge restarts'") is violated. Not deliberate: the comment at ghost.rs:1428-1429 justifies the fresh clock with "just (re)built or reused a machine", which is only valid for the fresh-bootstrap branch (genuinely sessionless machine) — the reuse branch carries arbitrarily old sessions. No test pins cross-invocation behavior; all rotation tests (key_loop_rotates_at_max_age_in_static_call etc., ghost.rs:2400-2520) exercise a single loop invocation. Severity P2 is fair: any human Matrix join/leave during the window rotates via the membership path after 90s, so it needs a static Matrix side plus a cycling Discord speaker — plausible (AFK-timeout moves) but narrow. **Regression test sketch:** Belongs in the existing distribute_key_loop mock section of the #[cfg(test)] module in crates/matrix-rtc/src/rtc/ghost.rs (next to key_loop_rotates_at_max_age_in_static_call). #[tokio::test(start_paused = true)]: build one MockTransport (stands in for the cached machine — its `rotations` vec is the machine-lifetime record) and one MockSource with a static recipient; spawn distribute_key_loop(rotate=true), advance SESSION_ROTATE_MAX_AGE - 60s, cancel, await; spawn a SECOND distribute_key_loop with the SAME transport (simulating the ghost respawn reusing the cache), advance SESSION_ROTATE_MAX_AGE - 60s again, cancel. Assert transport.rotations is non-empty (machine age ~28 min > 15 min max-age). Today it asserts-fails with zero rotations, pinning the bug; the fix (moving last_rotation into the transport/GhostCrypto, or seeding the loop with the machine's real last-rotation instant) makes it pass. ### F3-3 — P2 / LOCAL / CONFIRMED **Location:** `crates/matrix-rtc/src/rtc/key_transport.rs:538` @ `968da57` **Defect:** encrypt_and_send_key bases its outcome on `attempted = devices.len()` AFTER resolve_devices silently dropped recipients whose devices aren't in the machine's store (debug-level log only), so an unresolved in-call recipient appears in neither `delivered` nor `failed` — the send reports full success, distribute_key_loop's documented 'unreachable devices' INFO/'reached NO devices' WARN distinction never fires for them, and they are marked `keyed` (ghost.rs:1537) as if delivered. **Failure scenario:** Encrypted room, ghost live with human A already keyed; human B joins but B's device keys stay unfetchable to the ghost's /keys/query past the 6x3s burst (federation/HS lag, or a device the server never returns): every send resolves only A, returns KeySendOutcome{delivered:1, failed:[]}, the loop logs the full-success 'ghost media key sent' arm listing B among recipients and marks B keyed — B silently never gets the key (re-tried only at the 60s heartbeat, with no WARN ever), so B hears static from that ghost while logs claim complete delivery, defeating the per-device-unreachable diagnostics contract. **Verifier:** Re-derivation from actual code holds at every step. In encrypt_and_send_key (key_transport.rs:521-585), resolve_devices (673-693) drops a recipient whose device get_device returns Ok(None) at DEBUG only; `attempted = devices.len()` (538) counts only resolved devices, and `failed` is built solely from withheld devices and failed PUT chunks — both post-resolution. So with A resolved and B unresolved, the outcome is {delivered:1, failed:[]}, which distribute_key_loop's full-success arm (ghost.rs:1478-1487) logs as "ghost media key sent" with `recipients` listing B, and B enters `keyed` (1537), so the burst is never re-armed for B and only the 60s heartbeat retries — with neither the partial-delivery "unreachable devices" INFO (1495) nor the "reached NO recipient devices" WARN (1512) ever firing for B. Crucially this is NOT documented-deliberate: it contradicts the module's own contract — FailedRecipient's doc (key_transport.rs:54-56) explicitly names "a recipient device not yet tracked" as an example of what `failed` carries, and the KeyTransport trait doc (102-104) says the outcome reports "which didn't" get the key. No #[cfg(test)] test pins the omission (key_transport tests cover identity/upload/parsing; the ghost.rs ColdThenWarmTransport mock actually models the opposite convention, putting unreached devices in `failed`). The state is reachable: recipients come from per-device m.call.member state, and the Ok(None)/"will retry" arm exists precisely because /keys/query can lag or never return a device (the guard at 533 only errors when NO device resolves). The only live-dependent aspect (how long B stays unresolvable) affects severity, not the misreporting itself, which fires on every such send. P2 diagnostics/contract defect as claimed. **Regression test sketch:** Not required (P2), but if wanted: in ghost.rs's existing distribute_key_loop test module, the fix's natural pin is a unit test on the outcome contract — factor the recipients→failed accounting so it's testable, or add a MockTransport variant returning {delivered:1, failed:[]} for a 2-recipient send and assert the loop treats a send whose delivered+failed < recipients.len() as partial (unreachable arm / burst kept), not full success. --- *Filed from an automated multi-agent code review of `master` @ `968da57` (8 scoped finders + adversarial verification). Every finding below was re-derived from the code by an independent verifier. Fixes for the confirmed local-verifiable defects (allocator anchor panic F1-1/F1-2/F1-3/F2-1/F2-2, capture-sink panic F2-7, config env-fallback F5-4, opt-out RMW race F5-2) already landed on branch `refactor/review-and-split`.*

robocub referenced this issue

2026-07-19 22:12:58 +00:00

e2e nightly RED 8 days — real multi-channel/encrypted/recovery audio failure cluster (NOT contention); env-vs-code TBD #124