fix(security): a relayed CMB is unverifiable, not forged

lib/frame-handler.js

@@ -348,12 +348,23 @@ class FrameHandler {
   }
 
   /**
-   * Verify the Ed25519 signature on an inbound CMB against the sending peer's
-   * announced identity key (MMP §8.3). Returns true if the CMB must be REJECTED
-   * (a present-but-invalid signature — forged/tampered; or unsigned when the
-   * node is configured to require signatures). Sets msg._cmbVerified. Unsigned
-   * CMBs, or signed CMBs from a peer whose key we haven't seen yet, are allowed
-   * through (flagged unverified) for interop unless strict mode is on.
+   * Verify the Ed25519 signature on an inbound CMB (MMP §8.3). Returns true if
+   * the CMB must be REJECTED (a present-but-invalid signature — forged/tampered;
+   * or unsigned when the node is configured to require signatures). Sets
+   * msg._cmbVerified.
+   *
+   * The key is resolved from the DELIVERING peer, but a CMB is signed by its
+   * AUTHOR (`signingPayload` binds `cmb.createdBy`). Those coincide only when
+   * the author handed the block over directly. For a RELAYED CMB the resolved
+   * key is the wrong node's, and the verdict is meaningless — so a signed CMB
+   * whose named author is not the deliverer is passed through flagged
+   * unverified rather than reported as forged. Also passed through unverified:
+   * unsigned CMBs, and signed CMBs from a peer whose key we have not seen yet.
+   * Strict mode still rejects unsigned.
+   *
+   * What this does NOT do is authenticate a relayed CMB — it cannot, because
+   * the author's key is unresolvable from a name (see the comment at the
+   * rejection branch). It stops mislabelling unverifiable as forged.
    * @private
    */
   _rejectOnBadSignature(peerId, peerName, msg) {
@@ -387,6 +398,40 @@ class FrameHandler {
 
     if (!v.valid) {
       const keyShort = String(cmb.key || '').slice(0, 16);
+
+      // The key resolved above belongs to the node that DELIVERED this CMB:
+      // `_identityKey` is keyed by nodeId and `peerId` is the transport peer.
+      // The signature is made by the AUTHOR — `signingPayload` binds
+      // `cmb.createdBy`. On a RELAYED CMB those are different nodes, so the
+      // verdict above was computed against the wrong node's public key and
+      // says nothing about authenticity: an untouched, genuinely-signed block
+      // fails it every time. Only relax the case we can PROVE is the wrong
+      // key — a named author that is not the deliverer. An absent author, or
+      // an author that IS the deliverer, still hard-rejects, so omitting
+      // `createdBy` cannot be used to dodge rejection.
+      //
+      // The author's own key is deliberately NOT resolved here: the roster is
+      // keyed by nodeId (`RosterKeyRegistry._byNode`) while `createdBy` is a
+      // NAME, so a name index would be required — and it would silently
+      // resolve to the wrong key the first time two nodes share a name.
+      // Carrying `createdByNodeId` on the CMB and binding it into
+      // `signingPayload` is the correct fix; that is an MMP schema change.
+      //
+      // Passing unverified is the posture line 381 already takes for a key we
+      // cannot resolve ("Cannot verify — do not reject"). This makes the two
+      // consistent: today an UNRESOLVABLE key passes while a resolvable-but-
+      // WRONG key is logged as forgery and dropped before SVAF ever sees it.
+      const author = cmb.createdBy || null;
+      if (author && author !== peerName) {
+        msg._cmbVerified = false;
+        this._node._log(`[sym-security] UNVERIFIABLE CMB ${keyShort} authored by ${author}, relayed via ${peerName} — this node holds no key for the author, so the signature cannot be checked; passing unverified${v.error ? ' (' + v.error + ')' : ''}`);
+        this._node.emit('metric', {
+          type: 'cmb-signature-unverifiable', from: peerName, author,
+          key: cmb.key, reason: v.error || 'author-key-unavailable',
+        });
+        return false;
+      }
+
       this._node._log(`[sym-security] BAD SIGNATURE on CMB ${keyShort} from ${peerName} — forged/tampered, rejected${v.error ? ' (' + v.error + ')' : ''}`);
       this._node.emit('metric', { type: 'cmb-signature-rejected', from: peerName, key: cmb.key, reason: 'invalid' });
       if (typeof this._node._recordDecision === 'function') {
@@ -507,11 +552,13 @@ class FrameHandler {
     }
     if (!msg.content) return;
 
-    // CMB authentication (MMP §8.3). A signed CMB MUST verify against the
-    // sending peer's Ed25519 identity key (announced in its handshake). A
-    // present-but-invalid signature means the CMB was forged or tampered in
-    // flight — reject it outright (audit-logged, never surfaced or stored).
-    // Unsigned CMBs (older peers, or peers without a known key yet) are allowed
+    // CMB authentication (MMP §8.3). A signed CMB verifies against its AUTHOR's
+    // Ed25519 key. When the author delivered it directly, the peer's announced
+    // handshake key IS that key and a present-but-invalid signature means forged
+    // or tampered in flight — reject outright (audit-logged, never surfaced or
+    // stored). When the block was RELAYED, the author's key is not resolvable
+    // here, so it is flagged unverified rather than rejected. Unsigned CMBs
+    // (older peers, or peers without a known key yet) are likewise allowed
     // through for interop but flagged unverified on msg._cmbVerified.
     if (this._rejectOnBadSignature(peerId, peerName, msg)) return;
 

tests/frame-handler-author-key.test.js

@@ -0,0 +1,152 @@
+'use strict';
+
+require('./_isolate-home'); // redirect $HOME to a temp sandbox before lib/config loads
+
+/**
+ * Regression: a RELAYED CMB must not be reported as forged.
+ *
+ * `_rejectOnBadSignature` resolves the verifying key from the DELIVERING peer
+ * (`_identityKey(peerId)`, and the roster is keyed by nodeId), while
+ * `signingPayload` binds the AUTHOR (`cmb.createdBy`). Those coincide only when
+ * the author handed the block over directly. For every relayed CMB the check
+ * ran against the wrong node's public key, so an untouched, genuinely-signed
+ * block failed 100% of the time and was dropped before SVAF with the log text
+ * "forged/tampered" — invisible in every drift distribution and admission tally
+ * because the drop happens upstream of evaluation.
+ *
+ * The relaxation is deliberately narrow: only a NAMED author that is not the
+ * deliverer is treated as unverifiable. An absent `createdBy`, or an author that
+ * IS the deliverer, still hard-rejects — otherwise omitting the field would be a
+ * way to dodge signature rejection entirely.
+ *
+ * This does NOT authenticate relayed CMBs. The author's key is unresolvable from
+ * a name (roster is nodeId-keyed), so the honest outcome is "unverified", which
+ * is the posture the no-key branch already takes. Authenticating them needs
+ * `createdByNodeId` bound into `signingPayload` — an MMP schema change.
+ */
+
+const { describe, it } = require('node:test');
+const assert = require('node:assert');
+const crypto = require('node:crypto');
+const { FrameHandler } = require('../lib/frame-handler');
+const { createCMB, signCMB } = require('@sym-bot/core');
+
+const GROUP = 'test-group';
+const RECEIVER_NODE_ID = 'receiver-node-id';
+
+/** A fresh Ed25519 identity in the raw base64url form sym stores. */
+function identity() {
+  const { publicKey, privateKey } = crypto.generateKeyPairSync('ed25519');
+  const rawPriv = privateKey.export({ type: 'pkcs8', format: 'der' }).subarray(16);
+  const rawPub = publicKey.export({ type: 'spki', format: 'der' }).subarray(12);
+  return { priv: rawPriv.toString('base64url'), pub: rawPub.toString('base64url') };
+}
+
+/** A signed CMB as it appears on the wire (emit.js: whole object, then signCMB). */
+function signedCMB({ author, signWith }) {
+  const cmb = createCMB({ fields: { focus: 'a relayed observation' }, createdBy: author });
+  cmb.group = GROUP;
+  signCMB(cmb, signWith);
+  return cmb;
+}
+
+/** Receiver node stub. `keyFor` maps the delivering peerId → the key we hold. */
+function harness(keyFor) {
+  const logs = [];
+  const metrics = [];
+  const decisions = [];
+  const node = {
+    nodeId: RECEIVER_NODE_ID,
+    _group: GROUP,
+    _requireSignedCmb: false,
+    _log: (m) => logs.push(m),
+    emit: (type, payload) => { if (type === 'metric') metrics.push(payload); },
+    _identityKey: (peerId) => keyFor[peerId],
+    _recordDecision: (d) => decisions.push(d),
+  };
+  return { fh: new FrameHandler(node, {}), logs, metrics, decisions };
+}
+
+describe('_rejectOnBadSignature — the verifying key must belong to the author', () => {
+  it('does NOT reject a CMB authored by A and relayed by B (the 100%-loss case)', () => {
+    const a = identity();
+    const b = identity();
+    const cmb = signedCMB({ author: 'node-a', signWith: a.priv });
+    // C holds B's key for the delivering peer, and has never met A.
+    const { fh, logs, metrics, decisions } = harness({ 'peer-b': b.pub });
+    const msg = { cmb, source: 'node-a' };
+
+    const rejected = fh._rejectOnBadSignature('peer-b', 'node-b', msg);
+
+    assert.strictEqual(rejected, false, 'a relayed CMB must reach SVAF, not be dropped as forged');
+    assert.strictEqual(msg._cmbVerified, false, 'and must be flagged unverified — it is NOT authenticated');
+    assert.ok(
+      !decisions.some((d) => d.decision === 'rejected-signature'),
+      `must never record rejected-signature, got: ${JSON.stringify(decisions)}`,
+    );
+    assert.ok(
+      metrics.some((m) => m.type === 'cmb-signature-unverifiable' && m.author === 'node-a'),
+      `the population must stay countable, got: ${JSON.stringify(metrics)}`,
+    );
+    assert.ok(
+      !logs.some((m) => m.includes('forged/tampered')),
+      `must not be logged as forgery, got: ${logs.join(' | ')}`,
+    );
+  });
+
+  it('still rejects a forgery from the peer that claims to have authored it', () => {
+    const a = identity();
+    const b = identity();
+    // B names itself the author but the bytes were signed by A: B's own key is
+    // the right key to check, and it fails. That is a real forgery.
+    const cmb = signedCMB({ author: 'node-b', signWith: a.priv });
+    const { fh, logs, decisions } = harness({ 'peer-b': b.pub });
+    const msg = { cmb, source: 'node-b' };
+
+    const rejected = fh._rejectOnBadSignature('peer-b', 'node-b', msg);
+
+    assert.strictEqual(rejected, true, 'author === deliverer means the verdict is meaningful');
+    assert.ok(decisions.some((d) => d.decision === 'rejected-signature'));
+    assert.ok(logs.some((m) => m.includes('forged/tampered')));
+  });
+
+  it('still rejects when createdBy is absent — omitting it cannot dodge rejection', () => {
+    const a = identity();
+    const b = identity();
+    const cmb = signedCMB({ author: 'node-a', signWith: a.priv });
+    delete cmb.createdBy;
+    const { fh, decisions } = harness({ 'peer-b': b.pub });
+    const msg = { cmb, source: 'node-a' };
+
+    const rejected = fh._rejectOnBadSignature('peer-b', 'node-b', msg);
+
+    assert.strictEqual(rejected, true, 'an unnamed author must not buy a pass');
+    assert.ok(decisions.some((d) => d.decision === 'rejected-signature'));
+  });
+
+  it('verifies normally when the author delivers its own CMB', () => {
+    const a = identity();
+    const cmb = signedCMB({ author: 'node-a', signWith: a.priv });
+    const { fh, decisions } = harness({ 'peer-a': a.pub });
+    const msg = { cmb, source: 'node-a' };
+
+    const rejected = fh._rejectOnBadSignature('peer-a', 'node-a', msg);
+
+    assert.strictEqual(rejected, false);
+    assert.strictEqual(msg._cmbVerified, true, 'the direct path must still authenticate');
+    assert.strictEqual(decisions.length, 0);
+  });
+
+  it('leaves the unresolvable-key path untouched (unverified, not rejected)', () => {
+    const a = identity();
+    const cmb = signedCMB({ author: 'node-a', signWith: a.priv });
+    const { fh, decisions } = harness({}); // no key for anyone
+    const msg = { cmb, source: 'node-a' };
+
+    const rejected = fh._rejectOnBadSignature('peer-b', 'node-b', msg);
+
+    assert.strictEqual(rejected, false);
+    assert.strictEqual(msg._cmbVerified, false);
+    assert.strictEqual(decisions.length, 0);
+  });
+});