From dd4aa21d74e7488213eda4951da7543596c489e3 Mon Sep 17 00:00:00 2001 From: "SYM.BOT" Date: Thu, 30 Jul 2026 05:58:22 +0100 Subject: [PATCH] A relayed CMB is unverifiable, not forged MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit _rejectOnBadSignature resolved 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. Every relayed CMB was therefore checked against the wrong node's public key, failed, and was dropped before SVAF with the log text "forged/tampered". The drop happens upstream of evaluation, so the population appears in no drift distribution and no admission tally: every complementarity figure published so far is conditioned on the survivors, and the exclusion is not random. WHY THIS IS A CONSISTENCY FIX, NOT A WEAKENING. Thirty lines above the reject branch, a key we cannot resolve at all is deliberately passed through: "Cannot verify — do not reject; treat unverified". So an UNRESOLVABLE key passed while a resolvable-but-WRONG key was reported as forgery. This makes the two agree. The relaxation is narrow on purpose: only a NAMED author that is not the deliverer becomes unverifiable. An absent createdBy, or an author that IS the deliverer, still hard-rejects — otherwise dropping the field would be a way to dodge signature rejection. Pinned by test. WHAT THIS DOES NOT DO is authenticate relayed CMBs. It cannot: the roster is nodeId-keyed and createdBy is a NAME, so resolving the author's key needs a name index, which would silently pick the wrong key the first time two nodes share a name. Carrying createdByNodeId and binding it into signingPayload is the correct fix and is an MMP schema change. Until then "unverified" is the honest verdict, and it is the one the no-key branch already returns. The new metric cmb-signature-unverifiable keeps the population countable rather than moving it from a wrong bucket into no bucket. Verification: 297 tests / 297 pass. Baseline on the parent commit is 292/292, so the five new tests are additive with no regressions. Built in a worktree outside the repo; no live tree touched, nothing pushed. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01SpDHL4kTRWvkuw63KADT6R --- lib/frame-handler.js | 69 +++++++++-- tests/frame-handler-author-key.test.js | 152 +++++++++++++++++++++++++ 2 files changed, 210 insertions(+), 11 deletions(-) create mode 100644 tests/frame-handler-author-key.test.js diff --git a/lib/frame-handler.js b/lib/frame-handler.js index 82822a8..1b56f9f 100644 --- a/lib/frame-handler.js +++ b/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; diff --git a/tests/frame-handler-author-key.test.js b/tests/frame-handler-author-key.test.js new file mode 100644 index 0000000..88a0a4e --- /dev/null +++ b/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); + }); +});