[Security] Tombstone TTL never expires when removedAt is Infinity / garbage

[pathosDev]

Severity / Size

• Severity: LOW — bounded memory bloat over the tombstone-TTL window; no confidentiality / integrity impact. Damage is "members map grows during attack, returns to normal once attack stops + TTL reaches a sane comparison again". Realistic but small operational hazard.
• Size: S (~1d).
• Threat model: adversarial cluster peer (or MitM gossip-injector if cleartext + no auth). Attacker gossips tombstones with removedAt set to a garbage value (Infinity, NaN, a wallclock far in the future).

Affected files

• src/cluster/Cluster.ts:772-780 — tombstone-pruning condition:

  if (incoming.status === 'removed'
      && incoming.removedAt !== undefined
      && Date.now() - incoming.removedAt >= this.tombstoneTtlMs) { /* drop */ }

• src/cluster/Cluster.ts:888-905 — tombstonePruneTick() does the same comparison every 5 minutes against the local members map.

Background

After MemberRemoved (definitive removal), the member's entry stays in the local members map as a tombstone, carrying removedAt = Date.now() from #75. The tombstone is reclaimed when Date.now() - removedAt >= tombstoneTtlMs (default 24h).

The arithmetic assumes removedAt is a sane finite ms-epoch. No validation on incoming gossip. Pathological values:

• removedAt: Number.POSITIVE_INFINITY → Date.now() - Infinity = -Infinity → -Infinity >= ttl is false → tombstone never expires.
• removedAt: NaN → Date.now() - NaN = NaN → NaN >= ttl is false → tombstone never expires.
• removedAt: Number.MAX_SAFE_INTEGER → Date.now() - MAX_SAFE_INTEGER underflows to a huge negative; >= ttl false → tombstone never expires.
• removedAt: -Infinity or way-in-the-past → expires immediately (legit, no bug).

Effect: an attacker who can gossip removed members with garbage removedAt plants permanent tombstones. Each tombstone is a small entry in the members map (a Member object); a few hundred thousand of them aren't a memory crisis, but they:

• Bloat every gossip frame (members array carries them all).
• Cost CPU on every tombstonePruneTick (5min iterations through the whole map).
• Confuse operators watching cluster_member_count_total.

Exploit walkthrough

Setup: 3-node cluster, plaintext transport (or a malicious peer with TLS). Attacker has frame-write access to the cluster's gossip channel.

Step 1 — gossip injection: attacker forges a gossip frame with 1000 synthetic removed members, each with a unique address and removedAt: Infinity. Sends to all peers.

Step 2 — merge accepts: mergeMember() for each:

• incoming.status === 'removed' → check tombstone TTL.
• Date.now() - Infinity = -Infinity → -Infinity >= 24h is false → not pruned.
• Falls through to the normal merge path → stored as a new tombstone.

Step 3 — replay: attacker repeats every gossip interval. Each peer's members map accumulates: 1000 / s × 60 s = 60k tombstones / minute.

Step 4 — pruning ineffective: tombstonePruneTick runs every 5 minutes; iterates the members map; for each entry checks Date.now() - removedAt >= ttl. For the poisoned tombstones: -Infinity >= ttl is false → kept. Map size monotonically grows.

Stopping the attack: pruning still doesn't help because of the arithmetic. The only way out is to restart the cluster (which resets members maps) or manually invalidate the bad tombstones.

How the 8 already-landed security fixes inform this

• Gossip version-cap (709431b): validated incoming.version is finite + within wallclock window. Same pattern applies to removedAt: validate it's a sane finite wallclock value at the merge entry point.
• Frame-size DoS (d454079): fail fast at the boundary. Apply here too: refuse to store a tombstone with malformed removedAt.

Fix design

One-track defence, mirroring the version-cap pattern.

Track 1 — validate removedAt at merge time.

In mergeMember(), before the tombstone-TTL check:

if (incoming.status === 'removed') {
  if (incoming.removedAt !== undefined) {
    const ts = incoming.removedAt;
    // Sanity: must be a finite wallclock value within ±skew of now.
    const now = Date.now();
    if (!Number.isFinite(ts)
        || ts > now + MAX_VERSION_SKEW_MS
        || ts < now - MAX_TOMBSTONE_AGE_MS) {
      this.log.warn(`merge: rejecting tombstone for ${incoming.address} with malformed removedAt=${ts}`);
      return;
    }
  }
  // ... existing TTL check
}

MAX_TOMBSTONE_AGE_MS: a generous bound, say 30 days. Tombstones older than that should already have been pruned; an "incoming" tombstone with removedAt 30 days ago is either gossip from a node with a very skewed clock or a bug. Either way, refuse to store.

MAX_VERSION_SKEW_MS (24h, reused from the existing fix): future-side cap.

Track 2 — counter metric (operator visibility).

tombstones_rejected_total{reason: 'malformed_removedAt'} counter incremented when the validation rejects. Operators see the attack in dashboards.

Track 3 — defensive prune.

tombstonePruneTick's comparison uses the validated value, but as belt-and-suspenders also drop any tombstone whose removedAt is now somehow malformed (e.g. corrupted in-memory by a bug):

if (member.status === 'removed') {
  const ts = member.removedAt;
  if (ts === undefined || !Number.isFinite(ts) || ts < cutoff) {
    this.members.delete(addrKey);
    this.failureDetector.forget(member.address);
  }
}

So even if a poisoned tombstone slips past the merge guard, the next prune-tick reclaims it.

API surface

No public-API change. Internal MAX_TOMBSTONE_AGE_MS constant added. Optional new metric counter.

Backward compatibility

Tightens what gossip is accepted. A legitimate node sending removedAt: 0 (which is 1970-01-01, beyond MAX_TOMBSTONE_AGE_MS) would be rejected — but no real node does that. Validate via the existing multi-node tests.

Test plan

1. Exploit test: inject a gossip frame containing tombstones with removedAt: Infinity / NaN / MAX_SAFE_INTEGER → members map size unchanged; rejected-counter incremented.

2. Boundary test: tombstone with removedAt = Date.now() - MAX_TOMBSTONE_AGE_MS - 1ms → rejected; Date.now() - MAX_TOMBSTONE_AGE_MS + 1ms → accepted.

3. Defensive-prune test: bypass the merge guard by directly mutating an entry's removedAt to Infinity; tombstonePruneTick() reclaims it on next tick.

4. Regression: existing tombstone-TTL tests (cluster.test.ts, cluster-security.test.ts) still green.

Acceptance criteria

• mergeMember() validates removedAt is finite + within [now − MAX_TOMBSTONE_AGE_MS, now + MAX_VERSION_SKEW_MS].
• tombstonePruneTick() defensively reclaims tombstones with malformed removedAt.
• tombstones_rejected_total metric exposed.
• Four new tests pass; existing cluster tests still green.
• Plan-doc + README "Known security caveats" updated on land.