Audit hardening: Ed25519 key validation and input-validation fixes - #1
Open
mellowcroc wants to merge 7 commits into
Open
Audit hardening: Ed25519 key validation and input-validation fixes#1mellowcroc wants to merge 7 commits into
mellowcroc wants to merge 7 commits into
Conversation
Identity.Validate checked only that ed25519_public_key_hex decoded to 32 bytes and that public_key_fingerprint matched. It never checked that the bytes are a usable curve point, so a roster could enrol a small-order key. Ed25519 verification computes [-k]A + [S]B and compares the result to R. When A has small order that equation collapses: the signature R = identity, S = 0 verifies against every message. Anyone can then forge signatures for that identity without holding a private key, which voids every signature-based control for whichever role holds it. A coordinator who authors participants.json could plant such a key for a public witness and manufacture the receipts that exist to detect coordinator equivocation. Validate now rejects three cases: bytes that are not a curve point, non-canonical encodings, and points of small order. The canonical check matters on its own because identity uniqueness across the definition is enforced on public_key_fingerprint, a hash of these exact bytes, so two encodings of one point would otherwise register as two distinct identities. The guard lives in Identity.Validate, so it also covers the roles enrolled outside the ceremony definition: EnrollmentRecord, PublicWitnessReceipt and ImmutableMirrorReceipt each validate their embedded Identity.
Three layers disagreed on how many audits a ceremony may have. A definition may enrol two or more auditors (definition.go). SignRelease accepts two or more signed passing reports (audit.go). ProductionDecision demanded exactly two. A ceremony that enrolled three auditors, which is permitted and strictly more conservative, could therefore produce a valid signed release that could never be recorded in a valid decision. The failure surfaces at final GO signing, after the ceremony is complete and nothing can be redone. Both audit lists now require at least two rather than exactly two. Distinctness of auditor key ids and external signer fingerprints moves from a hardcoded comparison of elements 0 and 1 to a set check across every element, so the rule still holds for longer lists.
KeySource.open decodes the G1 singletons (alpha, beta, delta) and the G2 singletons (beta, delta) with NoSubgroupChecks. Skipping the subgroup check is a deliberate throughput trade on a proving key that callers are expected to digest-authenticate first, and internal/msmengine makes the same trade. The difference is that msmengine still runs IsOnCurve on every decoded point, and streampk ran no validation at all. That gap matters because OpenKeyURL reaches this code over HTTP range requests, and the URL caller in cmd/wasm-prover does not digest the proving key before opening it. A point that parses but is not on the curve therefore entered a multi-scalar multiplication unchallenged. IsOnCurve is cheap relative to the decode and is now applied to all five singletons. This does not close the missing digest verification on the URL path, which needs a separate change.
A K=21 phase close replays every accepted contribution before it writes anything, which runs for hours and produced no output. An operator could not tell a running replay from a hung one, and could not measure how long a close takes on their hardware. That measurement is not a convenience. The closure commits to a future drand round, and choosing a round far enough ahead requires knowing how long the replay will take. Misjudging it is what caused the 2026-07-24 closure-timing incident. The current code fails loudly in that case rather than publishing an invalid closure, but the operator still burns the attempt with no better information for the retry. internal/mpcceremony deliberately has no logger: it handles signing keys and secret contribution state, and having no output path is stronger than having a careful one. A callback preserves that. ReplayProgress carries a phase, a one-based index and a total, never a path or key material, and rendering is the caller's business. PhaseTranscriptPaths carries the optional callback, which reaches every replay site already threaded through that struct. The CLI writes to stderr, never stdout, which is reserved for the result contract. Single head loads pass nil because they read one record rather than replaying.
redactCLIError blanks argv-derived strings out of error messages before printing, because arguments include signing-key paths. It did so by substring replacement over the whole message, and accepted any candidate except the empty string, "-" and "--". A one or two character argument value, such as a participant index, therefore blanked unrelated substrings and left the diagnostic useless exactly when it was needed. Candidates shorter than four characters are now skipped. The values this must not print, key paths, key ids and identity names, are all comfortably longer. Redaction remains defence in depth rather than the control. The control is that internal/mpcceremony has no print path, so secret material is never in a position to be written.
Records the findings behind the preceding commits, plus the items that are not code changes, so a reviewer can see what was checked and what was left open. Each entry cites the file and line that establishes it, and separates verified findings from proposals and from items that were named but not investigated. The local runbook documents how to build the tool and stand up a ceremony on one machine. It is orientation and rehearsal only. The production procedure is docs/mpc-ceremony-runbook.md, which is absent from main and survives only in refs/pull/34/head of the upstream repository (item B1). It also records the two roots of trust, the coordinator public key and the binary, which must arrive over channels the reader already trusts. scripts/mpc-demo-init.sh runs the documented init end to end. It builds with go build rather than go run, because go run omits the VCS metadata that software.go requires, and it reads the coordinator key id back from participants.json rather than hardcoding it.
ContributionEnvironment.OS/.Architecture and audit findings used a plain `== ""` presence check, so a single space satisfied "must not be empty" and flowed into signed attestations and records. Require the trimmed, non-empty form, matching the convention already used for Identity.DisplayName and (in e9a789f) artifact names.
mellowcroc
force-pushed
the
audit/ed25519-key-validation
branch
from
August 13, 2026 09:48
4164355 to
dbbfb84
Compare
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
1 participant
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.
Security-audit hardening series for the MPC ceremony and its input-validation
surface. Commits (oldest first):
canonical, non-small-order curve points via
filippo.io/edwards25519, closinga universal-forgery vector (a small-order key verifies signatures for any
message) and a fingerprint-duplicate evasion via non-canonical encodings.
quorum to permit additional independent audits.
IsOnCurve()on the streaming proving-key decode path that opts out ofsubgroup checks.
otherwise-silent multi-hour chain replay.
redaction so short values can't leak.
ContributionEnvironment.OS/.Architectureand audit findings used a plain== ""check, so a single space satisfied "must not be empty"; require thetrimmed, non-empty form, matching
Identity.DisplayNameand artifact names.