Skip to content

[Security] onGossip trusts the payload's from for liveness and inserts an unknown sender without maySpeakFor, so any peer can hold a crashed node's failure detector open and one frame naming a phantom low address makes isLeader() false #939

Description

@pathosDev

Component: src/cluster/Cluster.ts
Severity (assessment): HIGH
CWE: CWE-345 (insufficient verification of data authenticity)

onGossip reads the payload's from twice, and both reads are authority decisions. It bumps the failure detector for whatever address the payload names, and it inserts that address into the member map without going through maySpeakFor. Both are the defect #572 removed from the heartbeat path — the liveness signal is "traffic arrived on this connection", so it is the connection's peer that is demonstrably alive, not whoever a payload field names. One consequence is that any peer can keep a crashed node's detector fresh indefinitely, which suspends singleton and shard failover. The other is that one frame naming a low-sorting address that does not exist makes isLeader() false on the node that receives it, and — because the leader then promotes the phantom to up and gossips that — on every node it reaches.

Exploit walkthrough

Attacker position: any peer that can complete the hello handshake, which today carries no credential (#912).

A. Suspend failover for a node that is already dead. Send one gossip frame per failure-detector interval whose payload reads from: <the dead node's address>. handleWire has already bumped the detector for the real sender; line 608 bumps it a second time for the named address. failureDetectionTick therefore never sees the dead node time out: it is never marked unreachable, never downed, and its shards and singleton are never relocated.

B. Freeze the coordinator and misplace the singleton. Send one gossip frame whose payload reads from: { systemName: 's', host: '0.0.0.0', port: 1 } — an address that sorts below every real one. Lines 621-625 insert it as a joining member with no authority check at all. Lines 628-635 then have the leader promote every joining member to up, so the phantom becomes an up-member on the spot; leader() is the lowest-addressed up-member, so isLeader() flips to false. ShardCoordinator.onReceive drops every shard message while !isLeader() (src/cluster/sharding/ShardCoordinator.ts:254) and singletonHost returns cluster.leader() (src/cluster/singleton/ClusterSingletonManager.ts:43-46), so the singleton is placed on a node that will never run it. The promoted record gossips onward as an ordinary up member from a sender with standing, so peers merge it through maySpeakFor legitimately.

Evidence — src/cluster/Cluster.ts:606-625

src/cluster/Cluster.ts:606-625
  private onGossip(from: NodeAddress, message: GossipMessage): void {
    const sender = NodeAddress.fromJSON(message.from);
    this.failureDetector.heartbeat(sender);
    this.log.debug(`gossip from ${sender}: ${message.members.length} member(s)`);

    // Snapshot the sender's standing *before* merging: this frame may be the
    // one that introduces the sender, and a claim must not be authorised by a
    // membership the same frame just created.
    const senderStatus = this.members.get(from.toString())?.status;

    for (const data of message.members) {
      this.mergeMember(from, senderStatus, data);
    }

    // Ensure we know about the sender itself.
    if (!this.members.has(sender.toString())) {
      const member = new Member(sender, 'joining', 1);
      this.members.set(sender.toString(), member);
      this.emit(new MemberJoined(member));
    }

The mergeMember loop was fixed to key on from; the two sender reads on either side of it were not. The heartbeat path shows what the corrected shape looks like:

src/cluster/Cluster.ts:584-587
  private onHeartbeat(from: NodeAddress, message: HeartbeatMessage): void {
    if (!this.isPlausibleHeartbeat(from, message)) return;
    const peer = from;
    this.failureDetector.heartbeat(peer);

Why the existing guard does not cover it

maySpeakFor is the guard, and neither read consults it. The insert at :621-625 writes to this.members directly, so the rule "third-party claims need a sender with standing" is not applied to the one claim that creates a member out of nothing. handleWire's own this.failureDetector.heartbeat(from) at :526 is correct and is not the problem — :608 adds a second, payload-controlled bump on top of it.

The sibling fixes do not reach here: #572 bound the heartbeat's from to the connection, #574 and #582 did the same for other frames. This is the same class at two more call sites in the frame those fixes were written next to.

#138 covers the cardinality of what :621-625 can add (a peer spamming synthetic addresses fills the map). This issue is about the authority: the entry is created without a rule, and the addresses it creates change leadership and defeat the failure detector. Fixing #138 with a cap would not close either.

Suggested fix

  • :608 — bump the detector for from, not for sender, or drop the line entirely since handleWire:526 has already done it.
  • :621-625 — route the insert through maySpeakFor(from, senderStatus, sender, 'joining'), or simply delete it: a node that gossips its own record already arrives through mergeMember's subject.equals(from) branch, which is the announcement path this block duplicates without the check.
  • Consider refusing a gossip frame whose payload from disagrees with the connection peer at all, the way onLeave refuses a leave for someone else (:732-739). A well-behaved node never sends one.

Acceptance criteria

  • A gossip frame whose payload from names an address other than the connection peer does not create a failure-detector sample for that address.
  • A gossip frame cannot introduce a member the sender has no standing to speak for.
  • A crashed member is downed on schedule even while a third party keeps naming it in gossip payloads.
  • A test asserts isLeader() is unchanged after a frame naming a low-sorting phantom address.

Verification status

Found in the ten-lens production-readiness review of 2026-08-05 (v0.13.0) and re-verified before filing: reproduced by execution. A single-node Cluster was given a stub Transport so frames could be injected with an explicit connection peer, with unreachableAfterMs: 400 / downAfterMs: 900:

t0 (self up, no seeds): leader=sys@10.0.0.1:2552 isLeader=true members=[sys@10.0.0.1:2552:up]
after 1 gossip frame naming a phantom: leader=sys@0.0.0.0:1 isLeader=false members=[sys@10.0.0.1:2552:up, sys@0.0.0.0:1:up]
   failureDetector has a sample for the phantom: true
after 3s of DEAD being kept alive by PEER (downAfterMs = 900): ... DEAD status: up
after 3s of the phantom being named once per FD interval: leader=sys@0.0.0.0:1 isLeader=false
   isLeader() over 3s: false (20 samples)

One frame flipped isLeader(); naming the phantom once per detector interval held it false for every one of 20 samples over 3 s. A separate phase kept a member that had stopped sending anything at up for 3 s — 3.3× downAfterMs — purely by naming it in another peer's gossip payload. The onward propagation to peers (the promoted phantom gossiping as an ordinary up member) is read from maySpeakFor's rule 2 rather than reproduced; it needs a live multi-node cluster.

Part of the production-readiness review batch — tracked in #913.

Metadata

Metadata

Assignees

No one assigned

    Labels

    bugSomething isn't workingpriority: highTop priority — high impact, plan nextproduction-goalBlocks or defines the path to production readinesssecuritySecurity-relevant — see severity label for impact tierseverity: highSignificant impact, exploitable in standard threat model

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions