Skip to content

[Security] Decoded wire frames are cast to WireMessage with no shape validation, so a null frame or a null NodeAddressData field null-derefs in the unguarded dispatch loop #705

Description

@pathosDev

Component: src/cluster/Transport.ts
Severity (assessment): MEDIUM
CWE: CWE-20 (Improper Input Validation) / CWE-248 (Uncaught Exception)
Related: #563, #587

FrameDecoder.push does JSON.parse(json) as WireMessage — a compile-time cast, not a runtime check — and TcpTransport.onData wraps only the decode step in try, not the dispatch loop. Several handlers then dereference payload fields unguarded, and emitStatusTransition feeds a wire-supplied status string into match(...).exhaustive(), which throws by design on an unmatched value.

Exploit walkthrough

Preconditions: a completed hello handshake (unauthenticated in the default config; any valid member under mTLS).

Variant A — remote process kill, 8 bytes: after hello, send the frame 00 00 00 04 + null. FrameDecoder yields null; onMessage reaches this.handler(connection.peer, null); handleWire matches nothing and calls onUnhandledWire(null, from), which reads message.tTypeError. Nothing between there and Node's raw.on('data', ...) catches it → uncaughtException → process exits 1 (the framework installs no process.on('uncaughtException') anywhere in src/). Equivalent one-frame kills: {"t":"gossip","from":null,"members":[]}, {"t":"gossip"} (then for (const data of undefined)), {"t":"leave","node":null}, {"t":"heartbeat","from":null,"seq":1,"ts":<now>} (the plausibility guard checks seq/ts but never from).

Variant B — cluster-wide poison pill: send {"t":"gossip","from":{...attacker...},"members":[{"address":{"systemName":"app","host":"h","port":1},"status":"pwned","version":<Date.now()>}]}. mergeMember's version and removedAt guards both pass, the member is written into this.members, and then emitStatusTransition throws on .exhaustive(). The member is already stored, and gossipTick serialises the whole map unfiltered — so on any runtime where the throw is not fatal the poisoned entry is pushed to a real peer, which stores it and throws in turn. On Deno the throw is caught by the read loop (DenoTcpBackend.attach, src/runtime/tcp/DenoTcpBackend.ts:120-128) and converted into onError + connection close, so the observable effect is: every node acquires an entry that tears down whichever cluster connection carries it, on every gossip round, indefinitely — membership stops converging. On Node the first node to receive it dies.

Honest scope note: I proved fatality on Node (probe above) and connection-teardown on Deno by reading DenoTcpBackend. I did not measure Bun's behaviour for a throw inside a Bun.listen data callback; the poisoned-entry half of variant B holds on every runtime regardless.

Evidence — src/cluster/Transport.ts

src/cluster/Protocol.ts:208-212 — the blind cast:

      try {
        out.push(JSON.parse(json) as WireMessage);
      } catch (e) {
        throw new Error(`Invalid wire frame JSON: ${(e as Error).message}`);
      }

src/cluster/Transport.ts:206-219 — the try covers decoder.push only; the dispatch loop is bare:

    let frames: WireMessage[];
    try {
      frames = connection.decoder.push(chunk);
    } catch (err) {
      ...
      return;
    }
    for (const message of frames) this.onMessage(connection, message);   // <-- no try

src/cluster/Cluster.ts:511-516 — message.t is read off an unvalidated value in the otherwise arm:

  private onUnhandledWire(message: WireMessage, from: NodeAddress): void {
    const custom = this.wireHandlers.get(message.t);
    if (custom) custom(message, from);
  }

src/cluster/Cluster.ts:951-967 — wire-supplied status reaches .exhaustive():

  private emitStatusTransition(prev: Member, next: Member): void {
    if (prev.status === next.status) return;
    match(next.status)
      .with('up', () => { ... })
      ...
      .with('joining', () => { /* transient; no event */ })
      .exhaustive();

reached from src/cluster/Cluster.ts:885-895 and :931-932, both of which store the member before throwing:

    this.members.set(incoming.address.toString(), incoming);
    this.emitStatusTransition(existing, incoming);

Same class, unguarded dereferences of payload objects: Cluster.ts:562 const sender = NodeAddress.fromJSON(message.from);, Cluster.ts:566 for (const data of message.members), Cluster.ts:664 NodeAddress.fromJSON(message.node), Cluster.ts:547 NodeAddress.fromJSON(message.from), and NodeAddress.fromJSON itself (src/cluster/NodeAddress.ts:29-31) which dereferences data.systemName with no null check.

Probes I ran (outside the repo) confirming each link:

  • ts-pattern .exhaustive() on 'bogus-status' against the seven MemberStatus arms → THREW: Pattern matching error: no pattern matches value "bogus-status".
  • The same match(...) chain as handleWire evaluated against nullTHREW: TypeError Cannot read properties of null (reading 't') (123, "str", [1,2], {} all fall through harmlessly — null is the one that reaches the deref).
  • A throw inside a node:net socket 'data' listener under Node v26 → process died with EXIT=1; the 1-second "STILL ALIVE" timer never fired.

Why the existing guard does not cover it

TcpTransport.onData has exactly the right shape one layer up — it catches decoder errors and closes the connection with a comment about not "letting the error propagate up the runtime's socket-data callback" (src/cluster/Transport.ts:209-217) — but the guard stops at the decode call and the dispatch loop on the very next line is unprotected. FrameDecoder validates the length prefix thoroughly (DEFAULT_MAX_FRAME_BYTES, rejection before buffering) and nothing about the contents. isPlausibleHeartbeat (Cluster.ts:534-543) validates seq and ts but not from — and tests/multi-node/cluster-security.test.ts:479 only ever injects from: peer.toJSON(), so the missing check is invisible to the suite. Cluster.emit wraps listener calls in try/catch and evaluateDowning wraps downing.decide, showing the codebase does defend other callbacks — the wire dispatch path is the gap.

Suggested fix

Two independent fixes, both needed. (1) Wrap the dispatch loop: for (const message of frames) { try { this.onMessage(connection, message); } catch (err) { log.warn(...); sock.end(); return; } } so no wire-driven throw can ever reach the runtime's socket callback. (2) Validate frames at the decode boundary before they are typed as WireMessage: reject a non-object payload, require typeof t === 'string', and per-kind require a well-formed NodeAddressData (systemName/host strings, port a finite integer) plus status ∈ the seven MemberStatus literals and version a finite number. Reject the frame rather than normalising it, matching how mergeMember already treats an implausible version.

Relationship to existing issues

Adjacent to #563, #587, but a distinct mechanism. Two halves, and they split. The status:'pwned' half IS #563 verbatim — 'Unvalidated MemberStatus from gossip reaches match(...).exhaustive() — one frame throws out of the socket callback and the poisoned member is stored and re-gossiped' — including the store-before-throw at Cluster.ts:886/931, the re-gossip via gossipTick, the Node-vs-Deno runtime split, and the fix 'wrap the for (const message of frames) this.onMessage(...) loop in Transport.ts:219'. That half must not be re-filed. What #563 does NOT cover is the frame-shape half: JSON.parse(json) as WireMessage (Protocol.ts:209) is a cast, and I confirmed NodeAddress.fromJSON (NodeAddress.ts:29-31) dereferences data.systemName with no null check, while onUnhandledWire (Cluster.ts:511-515) reads message.t off whatever fell through the match. A bare null frame, or {"t":"gossip","from":null} / {"t":"gossip"} / {"t":"leave","node":null} / {"t":"heartbeat","from":null,...} (isPlausibleHeartbeat checks only seq/ts, confirmed at :534-543) each produces a TypeError on a path a MemberStatus allow-list would never touch. So the surviving contribution is: no runtime validation of frame shape at the decode boundary, giving several one-frame null-deref crashes independent of the status bug. Cross-reference #563 (same escape path, same dispatch-loop fix) and #587 (the mirror-image missing decode guard in ClusterClient).

Verification status

Found in the second, independent whole-framework security re-audit of 2026-08-02 (v0.12.0) — a fresh pass run without reference to the first wave's findings, then triaged against the existing tracker and adjudicated by verifiers instructed to refute it.

Verifier note

Checked every citation against the source.

Citations that hold. FrameDecoder.push does out.push(JSON.parse(json) as WireMessage) at Protocol.ts:209 — a compile-time cast with no runtime check; the surrounding try only re-labels a JSON syntax error. TcpTransport.onData (Transport.ts:206-219) wraps connection.decoder.push(chunk) in try/catch and then runs for (const message of frames) this.onMessage(connection, message); at line 219, outside it — exactly as claimed, and the catch's own comment at 210-212 ("rather than letting the error propagate up the runtime's socket-data callback") shows the intent stops one line short. NodeAddress.fromJSON (NodeAddress.ts:29-31) is new NodeAddress(data.systemName, data.host, data.port) with no null check. The unguarded call sites are all where the finder says: Cluster.ts:547 (onHeartbeat), :562 and :566 (onGossip), :664 (onLeave). isPlausibleHeartbeat (Cluster.ts:534-543) validates seq and ts and never touches from, so {t:'heartbeat',from:null,seq:1,ts:Date.now()} passes the guard and then throws at :547.

Searched for the guard the finder says is absent. No wire-frame shape validation exists: grep over src/cluster/ and src/runtime/tcp/ for isWireMessage / validateFrame / typeof message returns hits only in the sharding sub-protocol (ShardCoordinator.ts:47, ShardingProtocol.ts:288, ShardRegion.ts:79) — nothing on the cluster wire path. Cluster._start binds the handler raw at Cluster.ts:405 (setHandler((from, message) => this.handleWire(from, message))) with no try. No process.on('uncaughtException') or unhandledRejection anywhere in src/. Runtime escape confirmed: NodeTcpBackend.ts:26 and :70 register raw.on('data', chunk => options.handlers.onData(...)) bare, and BunTcpBackend.ts:29/:60 pass onData straight through as the data: callback; only DenoTcpBackend.ts:114-127 wraps its read loop, converting the throw to onError + close, so the finder's Node-fatal / Deno-teardown split is right and the Bun caveat is honestly flagged.

Exploit chain re-traced, and it is shorter than the finder thought. JSON.parse("null") yields null, pushed as a WireMessage; onData hands it to onMessage, which reads message.t at Transport.ts:223 and throws TypeError: Cannot read properties of null (reading 't') — before the if (!connection.peer) handshake gate at Transport.ts:273. So the 8-byte frame 00 00 00 04 + null kills a Node/Bun node with no handshake, not "after a completed hello" as the finding states. The handshake-gated variants ({t:'gossip',from:null}, {t:'gossip'}, {t:'leave',node:null}, {t:'heartbeat',from:null,...}) all check out at the lines above.

Test coverage claim verified. tests/multi-node/cluster-security.test.ts injects from: peer.toJSON() in every case (e.g. :467, :495) and pins NaN/Infinity/negative seq and bad removedAt — never a null or absent address object, so the gap is genuinely invisible to the suite.

Dedup. gh issue view 563 confirms the adjudicator: #563 is the MemberStatus half and already lists the Transport.ts:219 loop wrap as its third fix. #587 is the ClusterClient mirror, a different file. Tracker searches for the null-deref / frame-shape angle return nothing, so the surviving contribution — no runtime shape check at the decode boundary, yielding several one-frame TypeErrors on paths a status allow-list would never touch — is novel.

Severity. MEDIUM is right and I am not moving it. It is an unauthenticated one-frame remote crash, which the calibration treats as legitimate (this is not a "the wire is unauthenticated" finding — it breaks a fully mTLS'd cluster too, where any member can kill any other node). But the escape mechanism and half the fix are already carried by #563 at critical, and with that loop wrapped the residual defect is a peer sending garbage getting disconnected. File it as the input-validation complement to #563, not as an independent process-kill.

Correction applied: Three corrections. (1) The null-frame crash site is mis-cited. A bare null frame never reaches Cluster.onUnhandledWireTcpTransport.onMessage dereferences it first at Transport.ts:223 (if (message.t === 'hello')). Cluster.ts:511-516 is in fact NOT a live crash site: any value that survives the two .t reads in onMessage (223, 252) is non-null, so this.wireHandlers.get(message.t) there cannot throw ((123).t / ('str').t are undefined, not a TypeError). That evidence bullet should be dropped from the filing. (2) Because the deref is at Transport.ts:223, which sits ABOVE the handshake gate at Transport.ts:273 (if (!connection.peer) { warn; return; }), the bare-null variant needs no completed hello at all — the finding's stated precondition understates its own reachability. On the default plain-TCP listener this is a zero-precondition 8-byte remote process kill on Node. (3) Variant B (status:'pwned'.exhaustive() → store-before-throw → re-gossip) is issue #563 verbatim, including #563's own suggested fix 'wrap the for (const message of frames) this.onMessage(...) loop in Transport.ts:219'. It must not be re-filed here, and suggested fix (1) of this finding is likewise already #563's. The novel ask is fix (2) alone: runtime shape validation at the decode boundary. Note the consequence bound this implies — once #563's loop wrap lands, the remaining shape defect degrades from process death to connection teardown, which is why this stays MEDIUM and not higher.

Metadata

Metadata

Assignees

No one assigned

    Labels

    bugSomething isn't workingpriority: mediumUseful, not urgentsecuritySecurity-relevant — see severity label for impact tierseverity: mediumModerate impact or requires specific conditions

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions