Skip to content

Room contract: MembersV1::apply_delta does not bound delta.added, so a member can force unbounded signature verification #551

Description

@sanity

Problem

MembersV1::apply_delta (common/src/room_state/member.rs) puts no bound on delta.added.len() before verifying it. A current member can generate K keypairs offline and sign K AuthorizedMembers with their own key, each invited_by themselves. Every one of those signatures is genuine, so nothing rejects them and the ? never short-circuits — the contract verifies all K invite signatures before remove_excess_members enforces max_members, which only runs afterwards.

max_members is therefore not a bound on the work; it is a bound on what survives the work.

Why this is not bounded elsewhere

MAX_STATE_SIZE in freenet-core is 50 MiB (crates/core/src/wasm_runtime/state_store.rs:40). An AuthorizedMember is roughly 140 bytes on the wire, so K ≈ 19,000 is about 2.7 MB — comfortably under the wire cap, and the attack scales well past freenet-core's 5s WASM execution budget from there. There is no wire-level cap that makes this self-limiting.

The asymmetry

The ban path already has exactly this guard. BansV1::apply_delta rejects a delta carrying more than max_user_bans new bans:

if delta.len() > max_bans {
    return Err(format!(
        "Ban delta of {} exceeds max_user_bans ({}); refusing to process a flood",
        delta.len(), max_bans
    ));
}

added by #411 round 3 item C for precisely this reason ("a larger delta can only be a forged flood; rejecting it bounds the O(N) signature-verification work below"). The member path never got the equivalent, and it is the path with the larger per-entry cost.

Relationship to #422 / PR #548

Found during the security review of #548. #548 makes this path cheaper — invite-chain validation goes from O(M x D) to O(M) Ed25519 verifications — but it does not close it: the work is still linear in an unbounded attacker-chosen K.

Deliberately NOT fixed in #548. That PR's whole safety argument is that it is output-identical: it changes no accept/reject decision for any input. A length bound rejects deltas that previously succeeded, which is a genuine merge-semantics change. Bundling it would destroy the one property that makes #548 straightforward to reason about, and would hide a semantics change inside a performance PR.

What a fix needs

  • A bound on delta.added.len(), mirroring BansV1::apply_delta's. The natural analogue is max_members, but note the asymmetry: a legitimate members delta CAN exceed the current member count during a cold full-state merge, in a way a legitimate ban delta cannot, so the bound needs thought rather than a copy-paste.
  • A rollout story. Old peers will keep emitting deltas that new peers now reject. The same transient-skew reasoning that BansV1::apply_delta's comment records for a max_user_bans reduction applies here, and needs to be worked through for the member case before it ships.
  • A regression test that the flood is refused, and one that a legitimate large first-sync delta is not.

Needs @sanity's call on the bound and the rollout, since it changes what the contract accepts.

Related: #422, #548, #411 (round 3 item C).

[AI-assisted - Claude]

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions