fix: harden Matrix E2EE key delivery across reconnects - #93256
Conversation
Related to #61126: both refresh stale peer device lists before encrypted sends. This PR additionally verifies key-share recipients and fences lifecycle changes, so it is a broader competing approach rather than a duplicate; please consolidate the direction. |
andrexibiza
left a comment
There was a problem hiding this comment.
Reviewed exact head 73c74bec34d5f101fa88999b46327e2ce45bc125 against base 739bc555b1932e66c169b20edec3a48368e2dd3f and current main (650cf3348f8b3912e09da43b8ab96c62d88de2df at review time). This is a materially stronger treatment of the stale-device/Megolm problem than #61126, but I do not think this exact head is safe to merge yet.
Blocker 1 — the known first-refresh sentinel bug from #61126 is still present
_refresh_encrypted_room_devices() throttles with:
now - self._device_refresh_ts.get(room_id, 0.0) < self._device_refresh_intervalFor an unseen room on a machine whose monotonic uptime is below the default 300-second interval, the first readiness check returns before either query_keys() or crypto._fetch_keys() runs. A gateway started shortly after host boot therefore skips the very peer-device refresh this PR is meant to guarantee before reconnect/first send.
This is not a new theoretical concern: @teknium1 already identified the exact 0.0 sentinel defect in the review of #61126. #93256 broadens/supersedes that implementation but reintroduces the same condition. The new tests cover interval=0, retries, identity changes, etc., but do not pin time.monotonic() below the interval on a first-seen room.
Required fix: treat absence separately (last = ...get(room_id); last is not None and now-last < interval) and add a regression with e.g. monotonic=30, interval=300 proving the first readiness check executes both key-query layers.
Blocker 2 — a joined user with zero published devices is incorrectly treated as an incomplete/unsafe refresh
The refresh path hard-fails on if not devices for every requested peer, and later again on if not fetched_devices. That conflates two different states: “the homeserver failed/omitted this user” and “the user is present in the key response but currently has zero devices.”
Pinned mautrix==0.21.1 explicitly supports the latter: its normal Megolm share path logs <user> has no devices, skipping and continues sharing to other users. A room with Alice (one valid device) plus Bob (zero devices) therefore has a valid target set for Alice, but this head refuses the room entirely. On reconnect, _reconcile_encrypted_rooms() then promotes that single-room condition into a failed adapter connect.
Required fix: keep failing on a genuinely missing/incomplete user map or invalid device record, but allow a present-empty device map for an individual user. Decide readiness from the aggregate eligible device target set (the existing _current_e2ee_targets() already has the right room-level concept) and fail only when the encrypted room has no eligible targets at all. Add the two adversarial witnesses: (Alice={device}, Bob={}) succeeds and shares to Alice; (all peers={}) fails closed without sending ciphertext.
Blocker 3 — per-room E2EE uncertainty currently has adapter-wide outage authority
_reconcile_encrypted_rooms() records any room exception as success=False, and _connect_impl() immediately calls _disconnect_impl() when that aggregate is false. That means one federated homeserver key-query failure, one zero-device peer, or one otherwise-unready encrypted room takes all Matrix rooms offline, including unrelated healthy rooms. The actual send path already fails closed per room via _ensure_encrypted_room_ready(), so the global disconnect is a larger fault domain than the security invariant requires.
Please keep the safety boundary per room: an unready encrypted room must not emit ciphertext/plaintext, but it should not revoke transport availability for independent healthy rooms. At minimum add a reconnect regression with two rooms (one healthy, one key-query failure) proving the healthy room remains usable; if all-or-nothing adapter readiness is intentional, that needs an explicit contract and threat-model justification because it gives any problematic room/remote peer platform-wide denial-of-service authority.
Topology / attribution / merge order
- #61126 by @chrisplough is the direct stale-peer-device predecessor. #93256 is broader and can supersede it, but should explicitly preserve that lineage and absorb the already-reviewed first-refresh fix rather than repeat the defect.
- The authoritative room-encryption state path builds on merged #77336 by @kshitijk4poor, which itself preserved #71073 (@webtecnica) and #71543/#71547 (@ckaznocha). That is foundation work, not duplicate work.
- #89169 by @mjshorty is complementary and important for merge order: current
mainstill resolves the Matrix crypto DB at module scope, so multiplexed profiles can share one Olm store. #93256’s recipient/session guarantees should be reconciled with or land after that per-profile store-ownership fix; otherwise the new per-adapter readiness cache is sitting over a still-shared crypto authority. - #83488 by @neurosovereign is complementary own-device/server-key verification, not a duplicate of this peer-device/Megolm delivery work.
Exact-head hosted CI/Docker/Nix are currently action_required, so the PR’s focused local 247 passed, 2 skipped, 1 xfailed evidence is useful but is not yet a canonical exact-head CI receipt.
Once the first-refresh sentinel, zero-device semantics, and per-room failure scope are closed with regression witnesses, the rest of the direction here—authoritative encrypted-state inspection, lifecycle fencing, recipient accounting, session re-share, and credential-safe diagnostics—looks coherent.
|
Closing this PR. It was opened against the upstream repository instead of the requested ksamaschke fork. |
|
The head fork for this PR (
Focused suite: all five new witnesses pass; the remaining failures in the local run reproduce identically on the unmodified head (environment/fixture issues in my ad-hoc venv, not regressions). Not verified against a live homeserver — exact-head CI is the canonical receipt. Happy to open a superseding PR from |
|
@neurosovereign if you can't update in place a new PR would be helpful. Thanks! |
Summary
Verification
git diff --check: passed.Risk and rollout