fix(security): a relayed CMB is unverifiable, not forged - #39
Open
sym-bot wants to merge 1 commit into
Open
Conversation
_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) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SpDHL4kTRWvkuw63KADT6R
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stacked on
feat/cmb-only-pin-core(PR 2/4 of the cmb-only cutover), because the granularv.errorthis reasons about needs core 0.4.0. Base it elsewhere and the diff will look larger than it is.The defect
_rejectOnBadSignatureresolves the verifying key from the delivering peer —_identityKey(peerId), andRosterKeyRegistryis keyed by nodeId — whilesigningPayloadbinds 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 with the log text
forged/tampered.The drop happens upstream of SVAF, so the population appears in no drift distribution and no admission tally. Any complementarity figure 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:
So an unresolvable key passed while a resolvable-but-wrong key was reported as forgery. This makes the two agree.
createdByabsentThe relaxation is narrow on purpose: only a named author that is not the deliverer becomes unverifiable. An absent
createdBystill hard-rejects, so dropping the field cannot be used to dodge rejection. Pinned by test.What this does NOT do
It does not authenticate relayed CMBs. It cannot: the roster is nodeId-keyed and
createdByis 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 (and sym auto-suffixes collisions, so suffixed names do occur in practice). CarryingcreatedByNodeIdand binding it intosigningPayloadis the correct fix and is a protocol schema change.Until then
unverifiedis the honest verdict, and it is the one the no-key branch already returns.It also does not cover the signed-legacy population.
verifyCMBreturnslegacy-key-rejectedbefore any crypto runs, so signed-legacy lands in the same branch and still hard-rejects while unsigned-legacy passes — the same inconsistency in shape. Butcmb-signing.jsand the §19.2 note make that membrane deliberate, with upgrade as the remedy, so relaxing it is a posture decision rather than a bug fix and is deliberately out of scope here.A note on the author field, for whoever reviews
createdByis stripped from CMBs on the way into the store — it is absent from stored blocks across every node store sampled. This fix works only becausecreateCMBsets it and the whole object goes on the wire; persistence drops it afterwards.That matters for anyone tempted to mirror
signingPayload'screatedBy || sourcehere: on a receiver,sourceis the composed remix string ("<receiver>+<author>"), never a bare peer name — so the fallback would classify every CMB as relayed and silently disable signature rejection entirely. This usescreatedByalone.The same dropped field is why stored remixes fail content-address recompute:
emit.jsbinds the author's name into the remix key viamintRemixKey(fields, parents, this.name), persistence dropscreatedBy, andrecomputeKeyfalls back tosource. Both defects are one fault — authorship is bound into the crypto and then dropped by persistence. Not fixed here.Verification
297 tests / 297 pass locally on Node 22. Baseline on the parent commit is 292 / 292, so the five new tests are additive with no regressions.
The new suite pins: a relayed CMB is not rejected and is flagged unverified; a forgery from the peer claiming authorship still rejects; an absent
createdBystill rejects; the direct path still authenticates; and the unresolvable-key path is untouched.pull_requesttomain, and this targetsfeat/cmb-only-pin-core— sogh pr checksreports none while the merge state still readsCLEAN. The verification above is a local Node 22 run and nothing else. Separately, the Node 18 matrix job is currently hanging onmain-targeted PRs.New metric
cmb-signature-unverifiablekeeps the population countable rather than moving it from a wrong bucket into no bucket.Built in a worktree outside the repo; no live tree touched.
🤖 Generated with Claude Code