Skip to content

fix(protocol,mls): report a key package refused by this device's clock - #506

Merged
bahdotsh merged 2 commits into
mainfrom
fix/key-package-clock-skew
Oct 6, 2026
Merged

bahdotsh merged 2 commits into
mainfrom
fix/key-package-clock-skew

Conversation

@kivtxs

@kivtxs kivtxs commented Oct 5, 2026

Copy link
Copy Markdown
Member

Found in the v0.28 device smoke test. Two Android phones that had never been online had clocks set to February 2024 and October 2025. They discovered each other and proved their streams, but a connection request then stayed "Pending" forever on both, and no session ever formed. Nothing reached the app. The only trace was a debug line in message_dispatch:

Auto-establish deferred (session not ready yet) ... error=MLS error: Invalid key package: The lifetime of the leaf node is not valid.

Threat-model R7b already accepts that a device with a wrong clock refuses honest peers. Its stated mitigation is that the refusal is reported under its own code, so a local fault reads as one. That is done for control frames (STALE_CONTROL_FRAME), but not for key packages. This PR reports key packages too. It does not relax the window.

The window, measured on the phones

A package is valid from 1 h before minting to 30 days after, judged by the receiver's clock. So:

Skew between the two devices What happens What this PR reports
≤ 1 h Session forms Nothing
1 h to 30 days The device that is behind refuses the peer's package. The peer (ahead) accepts this device's package and starts the session; it works. Measured with a 1-day skew: session formed, 3 of 3 messages each way. One warning on the device that is behind
> 30 days Each side refuses the other's package. No session forms. Measured with a 40-day skew. One warning on each device

Change

  • MlsError::KeyPackageOutsideValidityWindow. Mapped from OpenMLS's KeyPackageVerifyError::InvalidLifetime in one helper, validate_peer_key_package. The helper now covers all three routes that admit a peer's package: import, the cache read, and add_group_member. Every other refusal keeps its old type and text, including the width cap.
  • SecurityWarningCode::KeyPackageOutsideValidityWindow (KEY_PACKAGE_OUTSIDE_VALIDITY_WINDOW). Raised by establish_secure_session and logged at warn. Emitted at most once per peer through the control-gate throttle (bit 5): the package has not proved its sender when its window is checked, so the peer id is attacker-chosen.
  • Mirrors updated: the TS SecurityWarningCode union and its docs, the local-API spec's code list, threat-model R7b, and the changelog. An exhaustive TS switch over the union needs a new arm, which the changelog notes.

Validation

On the phones (Infinix NOTE 12 on Android 13, Seeker on Android 15, Wi-Fi Direct, built from this branch plus #505):

  • 1-day skew: the device that is behind raised the warning once; the session still formed from the other side; 3 of 3 messages each way.
  • 40-day skew: both devices raised it once; no session formed.
  • Recovery: after the clock was corrected, a connection request formed the session and 2 of 2 messages went each way. Nothing re-forms on its own within 2 minutes; the next exchange does it.

Locally:

  • cargo test --workspace --locked: all green, 2007 tests in offline-protocol.
  • New tests:
    • MLS: a window that has not started is reported as such; a window that already closed is reported as such; only this refusal changes type.
    • Protocol: a_key_package_outside_its_window_by_our_clock_is_reported_once_per_peer, with three retries giving one event.
  • cargo clippy --workspace --locked -- -D warnings and the --no-default-features variant: clean. My local clippy (1.94) also flags data_sync.rs:719, which is untouched main code that CI's stable accepts; I allowed that one lint to check the rest.
  • RUSTDOCFLAGS="-D warnings" cargo doc for both crates: clean. cargo fmt --check: clean.

Risk

  • New public API: one variant on two #[non_exhaustive] enums.
  • Behaviour: one event, once per peer; the warning is attacker-reachable but bounded like the other gate warnings.
  • No change to what is accepted.

Test fixture: MlsManager::key_package_with_window_for_testing behind the existing test-utils feature.

kivtxs and others added 2 commits October 6, 2026 10:14
A peer's key package is valid from an hour before it was minted, judged by
the receiver's clock. Between two devices whose clocks disagree by more
than that, no session forms and messages and connection requests stay
pending forever, and the only trace was a debug line. Two Android phones
that had never been online (one set to February 2024, one to October 2025)
reproduced it during the v0.28 smoke test.

- MlsError::KeyPackageOutsideValidityWindow, from OpenMLS's
  KeyPackageVerifyError::InvalidLifetime, on every route that admits a
  peer's package (import, cache read, add_group_member) through one helper.
- SecurityWarningCode::KeyPackageOutsideValidityWindow
  (KEY_PACKAGE_OUTSIDE_VALIDITY_WINDOW), raised by establish_secure_session
  once per peer through the control-gate throttle: the package has not
  proved its sender when its window is checked.
- The window itself is unchanged.
The previous commit reported a key package refused by this device's
clock, but only from the arrival-time route. The send path has its
own near-identical copy of import-then-create-session, and that copy
still did a bare `?` on the import. It is the route taken on the
first send after a restart (the warning throttle lives in memory and
the package comes back from storage) and whenever auto key exchange
is off. There the send kick swallowed the error into a log line and
queued the message "anyway". The app heard nothing.

Which is exactly the failure this whole change exists to fix, just
on the other half of the pair.

It also treated both ends of the window as a clock fault. A window
that has not started is one. A window that has closed may be one,
or may just be an old package: the receiver anchors its cached
expiry to when the frame arrived, using a remaining lifetime the
sender computed when it built the frame. So a relay that held the
package for days hands us a package whose cached expiry overshoots
its real not_after. In that gap we told the user to check a clock
that was fine, and then kept the package, so every attempt failed
hard until the cached expiry.

Let's fix both. One engine helper now admits a pending package for
both routes and does the reporting, and the throttle keeps it to one
event per peer whichever route fires first. The MLS error says which
end refused the package. A window that has not started keeps the
package, since it becomes valid once the clocks agree. A closed one
is discarded the same way an expired package already is, and the
reason names both causes.

OpenMLS exposes no lifetime on an unvalidated KeyPackageIn outside
its test-utils feature, so the direction is read back through its
serde form, and only on the failure path. That is not pretty, but
the alternative is walking the TLS encoding by hand, which is a
second key package codec. Please don't do that. If the shape ever
changes, the answer falls back to "not started", which is how the
previous commit behaved, and the tests on both ends will say so.
@bahdotsh
bahdotsh force-pushed the fix/key-package-clock-skew branch from 08805c2 to 320df7e Compare October 6, 2026 04:51
@bahdotsh
bahdotsh merged commit 7779f86 into main Oct 6, 2026
24 checks passed
@github-actions github-actions Bot locked and limited conversation to collaborators Oct 6, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants