Skip to content

Commit 77f30c7

Browse files
committed
docs: fix what a review of the second correction pass found
Third and final correction round. Two reviewers went over 4a9fe0c; one approved the PR outright, the other found that the rewritten acknowledgement rule had picked up a new wrong qualifier. The acknowledgement rule, once more. Calling the identity bindings "post-decrypt" is wrong for the session slot binding, which runs before any AEAD precisely because every failure the MLS library raises below it happens before it authenticates anything. R2 of the same document already said so. The bullet also named three identity bindings where the code and delivery-and-acks.md name four. Both fixed, and the policy row now says its acknowledgement is an arrival-path rule: a frame that reaches the group buffer and later resolves as a policy refusal is never acknowledged, which is what group-message-lifecycle.md says one file over. R1 understated a forged membership answer. The members cache it corrupts also feeds the sealed rich payload gate, which requires every non-self member to be known rich-capable. An attacker-chosen identifier is not, so the gate closes and reply context and forward attribution move from inside the MLS AEAD to hop-visible cleartext while media secrets are dropped. That defeats a control this document lists by name, reached by the adversary that control names. The spec claimed a working relay-side remove path. The check is right and required, but a genuinely relay-originated remove is injected unattributed, so its sender can never be an administrator and the frame is always dropped: relay-native remove reconciliation is inert, and the live path is the removing administrator's own signed notification. Stated as the consequence of the rule that it is. Recovered the one thing the restructure actually lost: the leaf identity binding's mutation-check inventory, ten named sabotages plus the adversarial fixture behind the mls crate's test-utils feature, without which the use-time seam cannot be tested at all. ADR 0010 also gains the adopt-path ordering rule and the "what would undo this" section every other ADR carries. Also: the BLE fallback caveat swept into its two restatements, R9 no longer claims a design nobody can point at, the outbox repark edge qualified to plain DMs, and C5 notes that C6's retry fallbacks are a fifth set of the same shape.
1 parent 4a9fe0c commit 77f30c7

6 files changed

Lines changed: 67 additions & 23 deletions

File tree

CLAUDE.md

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -132,7 +132,9 @@ These fail silently if broken. Each is documented in full where it is linked.
132132
- **Adding a control-message prefix** means adding it to the registry that
133133
drives injection prevention
134134
([spec](docs/spec/control-messages.md#reserved-prefix-registry)).
135-
- **Never add a catch-all arm to a telemetry reason classifier**
135+
- **Never add a catch-all arm to a telemetry reason classifier that matches on
136+
an enum** (a classifier over an open wire string may, if the fallback returns
137+
a fixed token and never the input)
136138
([ADR 0013](docs/adr/0013-exhaustive-privacy-classifier.md)).
137139

138140
### Safety rules

docs/adr/0010-unconditional-leaf-identity-binding.md

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -108,3 +108,22 @@ delivered it, so it names **this device** as the subject, and the remedy it
108108
implies is to abandon the group rather than to evict a member. The leaf cannot
109109
speak, but it holds live group secrets and reads everything, which no later
110110
refusal undoes.
111+
112+
The adopt path's ordering carries the same weight as the checks themselves: the
113+
forged-tree test MUST run **before** the duplicate-session check. The adopt is
114+
stage-then-replace and refuses non-destructively (`join_group_replacing`), so an
115+
existing session survives the refusal; in the other order the forgery is
116+
swallowed as a harmless retransmit. It is the opposite of harmless, and reaching
117+
it needs only that a session with the peer already exists, which is either half
118+
of a both-create race or any re-invite.
119+
120+
## What would undo this
121+
122+
Deleting an arm that looks redundant. Each of the four commit-walk sources and
123+
each seam was individually proven load-bearing by mutation testing: the binding
124+
is checked against ten named sabotages, each removing one arm and expecting the
125+
suite to fail. The adversarial fixture that seats a forged leaf directly into a
126+
key store, bypassing both entry gates, is
127+
`MlsManager::seat_forged_leaf_for_testing`, behind the mls crate's `test-utils`
128+
feature. Without it the use-time seam cannot be tested at all, because no honest
129+
code path produces the state it defends against.

docs/security/threat-model.md

Lines changed: 20 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -152,15 +152,21 @@ The rule the protocol settled on:
152152

153153
- A frame refused on **security** grounds gets **no acknowledgement**, and its
154154
identifier is unmarked. That covers the signature gate, inbound plaintext
155-
refused by encryption policy, and the post-decrypt identity bindings (sender,
156-
session and leaf address mismatch), which are intercepted *before*
157-
classification precisely so they cannot inherit the policy disposition below.
155+
refused by encryption policy, and all four identity bindings (sender identity,
156+
session slot, leaf address, unsupported sender), which are intercepted
157+
*before* classification precisely so they cannot inherit the policy
158+
disposition below. The session slot binding runs before any AEAD, since every
159+
failure the MLS library raises below it happens before it authenticates
160+
anything; the credential comparisons run once decryption has succeeded.
158161
Acknowledging would confirm to an attacker that this device is online and
159162
processing, and unmarking is what stops an exact replay from reaching the
160163
duplicate re-acknowledgement path and leaking the same fact anyway.
161-
- A frame refused on **policy** grounds keeps its acknowledgement. A membership
162-
commit refused by opt-in enforcement is the case in point: the refusal is
163-
permanent, so a resend could only waste work.
164+
- A frame refused on **policy** grounds keeps its acknowledgement **when the
165+
refusal happens on the arrival path**. A membership commit refused by opt-in
166+
enforcement is the case in point: the refusal is permanent, so a resend could
167+
only waste work. A frame already deferred into the group buffer that later
168+
resolves as a policy refusal is never acknowledged, because the arrival path
169+
withheld the acknowledgement already.
164170
- A frame that failed for a **recoverable** reason gets no acknowledgement, so
165171
the sender's resend is the recovery path.
166172

@@ -194,8 +200,13 @@ answers corrupt the members cache, which is **not** the MLS roster and is never
194200
read by roster-derived logic, but is read verbatim as the group fan-out send
195201
cache: an accepted forgery therefore makes this device address every subsequent
196202
group ciphertext to an attacker-chosen identifier, or stop addressing a real
197-
member. Adds are gated on internet arrival and removes on an administrator
198-
check, which is what bounds this.
203+
member. The same list feeds the sealed rich payload gate, which requires every
204+
non-self member to be known rich-capable, so an unknown spliced identifier
205+
closes it: reply context and forward attribution move from inside the MLS AEAD
206+
to hop-visible cleartext and media secrets are dropped, until the next commit
207+
refreshes the cache. That defeats a control this document lists by name, reached
208+
by the adversary that control names. Adds are gated on internet arrival and
209+
removes on an administrator check, which is what bounds all of this.
199210

200211
**Why it stands:** these frames have no signer. Closing it means moving relay
201212
answers onto dedicated entry points.
@@ -315,7 +326,7 @@ is the one application-supplied payload that boundary 5 does **not** cover, so
315326

316327
**Why it stands:** discovery is a broadcast to peers with whom no session
317328
necessarily exists, so there is no established group to encrypt to; encrypting
318-
request and response bodies is designed but not implemented.
329+
request and response bodies is not implemented.
319330

320331
**What application teams must do today:** treat a service body as public, and
321332
encrypt anything sensitive above the SDK before handing it over.

docs/spec/control-messages.md

Lines changed: 7 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -114,11 +114,13 @@ two mandatory properties, and both are load-bearing:
114114
2. They MUST be pinned to the internet transport rather than routed by the
115115
transport selector. The selector demotes the internet transport by design,
116116
and some mesh transports swallow a self-addressed frame: Wi-Fi Direct and
117-
Reticulum enqueue it unconditionally and report success (Bluetooth LE fails
118-
closed instead, because self is never a connected peer). The caller then
119-
believes the broadcast succeeded and skips its per-member fallback,
120-
delivering to nobody. One such transport is enough to lose the frame, which
121-
is why the rule is a pin rather than a preference.
117+
Reticulum enqueue it unconditionally and report success. Bluetooth LE fails
118+
closed, because self is never a connected peer, but that only helps on a
119+
BLE-only device: a synchronous refusal is a fallback trigger, so the frame
120+
reaches one of the others anyway. The caller then believes the broadcast
121+
succeeded and skips its per-member fallback, delivering to nobody. One such
122+
transport is enough to lose the frame, which is why the rule is a pin rather
123+
than a preference.
122124

123125
Retry policy for these frames lives at the application layer instead, with
124126
explicit trackers and bounded attempts.

docs/spec/group-protocol.md

Lines changed: 15 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -202,13 +202,21 @@ Roster change events carry a three-valued authorization field:
202202
An implementation MUST NOT emit "authorized" from a path that ran no check. The
203203
third state exists precisely so that path has something honest to say.
204204

205-
Relay-reconciled **removes** are the asymmetry in that last row, and they are
206-
not "not evaluated": the remove path MUST check the authenticated wire sender
207-
against the administrative set and drop the frame when the sender is not an
208-
administrator, then report a real verdict. Adds cannot do this, because the
209-
frame authenticates the *path* rather than the actor, so `added_by` may be the
210-
relay itself. Omitting the remove-side check lets any peer evict members from
211-
the fan-out send cache and silently deny them group traffic.
205+
**Removes** are the asymmetry in that last row. The remove handler MUST check
206+
the authenticated wire sender against the administrative set, drop the frame
207+
when that sender is not an administrator, and only then report a real verdict.
208+
Adds cannot do this, because the frame authenticates the *path* rather than the
209+
actor, so `added_by` may be the relay itself. Omitting the remove-side check
210+
lets any peer evict members from the fan-out send cache and silently deny them
211+
group traffic.
212+
213+
Note what that check implies for genuinely relay-originated removes: the relay's
214+
answer is injected unattributed, so its sender is a placeholder that can never
215+
be an administrator and the frame is always dropped. **Relay-native remove
216+
reconciliation is therefore inert**, and the path that actually removes a member
217+
is the removing administrator's own signed notification. This is a consequence
218+
of the rule, not a gap in it: restoring relay-native reconciliation means moving
219+
relay answers onto a dedicated entry point rather than weakening the check.
212220

213221
### The delta is derived only when both roster reads succeed
214222

docs/state-machines/transport-lifecycle.md

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -178,4 +178,6 @@ This is a recurring source of confusion for application teams:
178178
Only the last one is delivery. Unconditional-enqueue transports are the reason
179179
relay hint frames must be pinned rather than routed: such a transport swallows a
180180
self-addressed frame and reports success. Wi-Fi Direct and Reticulum behave this
181-
way; Bluetooth LE fails closed, because self is never a connected peer.
181+
way. Bluetooth LE fails closed, because self is never a connected peer, but that
182+
only helps on a BLE-only device: the transport manager treats a synchronous
183+
refusal as a fallback trigger and hands the frame to one of the others anyway.

0 commit comments

Comments
 (0)