diff --git a/CHANGELOG.md b/CHANGELOG.md index 4917fb0..143113f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,9 @@ # Changelog +## Unreleased + +- Reject otherwise valid equal local/remote confirmation tags in both `Finish` methods with `ErrConfirmationFailed` (#308). With forced equal shares and equal associated data, the old responder accepted its own B tag reflected as C before initiator confirmation. No feasible network attack causing this condition with fresh production randomness was established. The correction preserves wire and MAC derivation, releases no C or Session on rejection, and consumes state; it also rejects the negligible honest equal-message collision case. See [the analysis](docs/key-confirmation-reflection.md) for regression coverage and required release-evidence refresh. + ## v0.1.3 - 2026-08-01 - Pre-v1 caller-input API change (breaking relative to `v0.1.2`): replace public `Config` with role-local `Input`. Removed fields are `InitiatorID`, `ResponderID`, and `AssociatedData`; callers now use `SelfID`, `PeerID`, and `LocalAssociatedData`. Migration rule: the initiator calls `Start` with `SelfID=initiator, PeerID=responder`, and the responder calls `Respond` with `SelfID=responder, PeerID=initiator`. `Password`, `Context`, `SessionID`, and `AllowEmptySessionID` keep the same semantics, and wire format is unchanged. diff --git a/api.go b/api.go index 81126d7..bd50dd4 100644 --- a/api.go +++ b/api.go @@ -88,6 +88,7 @@ func respondWithRandom(input Input, messageA []byte, random io.Reader) (*Respond // Finish consumes message B, verifies the responder confirmation tag, and // returns message C plus an authenticated session. The initiator state is // consumed even when message parsing or confirmation fails. +// A valid peer tag equal to the local tag is rejected with ErrConfirmationFailed. func (i *Initiator) Finish(messageB []byte) ([]byte, *Session, error) { core, err := i.finishCore() if err != nil { @@ -122,6 +123,7 @@ func (i *Initiator) Close() error { // Finish consumes message C, verifies the initiator confirmation tag, and // returns an authenticated session. The responder state is consumed even when // message parsing or confirmation fails. +// A valid peer tag equal to the local tag is rejected with ErrConfirmationFailed. func (r *Responder) Finish(messageC []byte) (*Session, error) { core, err := r.finishCore() if err != nil { diff --git a/core.go b/core.go index b6e3f3b..d268858 100644 --- a/core.go +++ b/core.go @@ -67,6 +67,11 @@ func (c *initiatorCore) finish(peerYb, peerAdb, peerTag []byte) ([]byte, *Sessio return nil, nil, ErrConfirmationFailed } tagA := tr.initiatorConfirmationTag(isk, c.sid) + // Reject equal confirmation tags before releasing our tag or a Session. + // CI role binding does not distinguish identical MAC inputs (#308). + if hmac.Equal(tagA, peerTag) { + return nil, nil, ErrConfirmationFailed + } return tagA, newSession(isk, tr.transcriptID(), peerAdb, c.peerID), nil } @@ -114,6 +119,12 @@ func (c *responderCore) finish(peerTagC []byte) (*Session, error) { if !hmac.Equal(expectedA, peerTagC) { return nil, ErrConfirmationFailed } + // A correct tag is not peer confirmation if it is also our own tag. + // Equal shares and AD otherwise let B's tag be reflected as C (#308). + tagB := c.transcript.responderConfirmationTag(c.isk, c.sid) + if hmac.Equal(tagB, peerTagC) { + return nil, ErrConfirmationFailed + } return newSession(c.isk, c.transcript.transcriptID(), c.transcript.initiatorAD(), c.peerID), nil } diff --git a/docs/evidence-baseline.md b/docs/evidence-baseline.md index ef85608..9f4d45e 100644 --- a/docs/evidence-baseline.md +++ b/docs/evidence-baseline.md @@ -12,6 +12,8 @@ This document is the module for pinned evidence baselines. It names the commit, ## Current Release-Claim State +Issue #308 changes confirmation acceptance to reject equal local/remote tags in both Finish paths; see [the reflection analysis](key-confirmation-reflection.md). The older pinned dependency/SAST, Capslock, security/spec/vector, and paired long-fuzz lanes below do not cover this security-relevant candidate. Refresh those lanes against the exact selected source candidate before any stronger release claim. Local regression checks are implementation validation only, and no new release candidate or production-readiness claim is selected here. Historical `v0.1.3` evidence remains valid only for its recorded candidate contract. + The frozen `v0.1.3` candidate source baseline is `1f19e278112fa037890848ed6c086addeffdca4e`, selected from a clean worktree under Go 1.26.5 after the PR #227 disposition and toolchain update. The dependency, vulnerability, SAST/gosec, Capslock capability, security/spec audit, Go 1.26.4 to Go 1.26.5 vector-stability, paired long-fuzz, and pre-publication GitHub release-control lanes are current for that candidate contract and are assembled in `docs/evidence/1f19e27-20260801-candidate/`. Signed annotated tag `v0.1.3` points to documentation-only descendant `b4174c6bf4bae78f4081c3d6d6baebff5f1cbbf1`; [Release Validation run 30686073480](https://github.com/the-sarge/cpace/actions/runs/30686073480) and the consumer verification recorded on issue #238 complete the publication-only lane without making a production-readiness claim. The current dependency, vulnerability, and SAST/gosec evidence is the clean-worktree Go 1.26.5 capture in `docs/evidence/1f19e27-20260731/`. Module integrity passed, `govulncheck@v1.3.0` reported no vulnerabilities, and the test-inclusive pinned `gosec@v2.26.1` scan reported zero issues. The current protocol bundle is `docs/evidence/1f19e27-20260731-protocol/`: Capslock classes/counts are unchanged, the focused protocol audit passed, and the normalized 112-line vector event streams are bit-identical under Go 1.26.4 and Go 1.26.5 with shared SHA-256 `05e966e9b4cc8ea6883d5f8a750cc2d91098a3e59aee365ed852e717067efbd9`. These completed lanes are prerelease evidence, not a production-readiness claim. diff --git a/docs/evidence/308-reflection-20260923/README.md b/docs/evidence/308-reflection-20260923/README.md new file mode 100644 index 0000000..755deb5 --- /dev/null +++ b/docs/evidence/308-reflection-20260923/README.md @@ -0,0 +1,34 @@ +# Issue #308 reflection validation + +Source candidate: `d2db6eed5923395bf0dfb57e73275ea0a1506c44` (`fix: reject reflected confirmation tags in both roles (#308)`). Base: `87528342d68d26db4a5a69386ae5647baf7d05ed`. Validation ran from a clean dedicated worktree on 2026-09-23 under Go 1.27.1 on Darwin/arm64. This bundle and the link from the analysis are a documentation-only descendant of that source candidate; the candidate contains all implementation, tests, and security-conclusion changes. + +## Results + +| Command | Result | +| --- | --- | +| `go test -run '^(TestReflection\|TestRistrettoDraft21Vectors\|TestIRTranscriptDraftVectorFlow)' -count=1 -v .` | PASS; 22 reflection subtests, IR transcript flow, and draft-21 vector test with its six generator sid mutation subtests | +| `task --silent check` | PASS; full normal/race suites, Markdown/whitespace, release helper/policy checks, CI classifier/tool/fuzz-input checks, evidence checks, formatting/imports, pinned GolangCI-Lint, ast-grep, and govulncheck | +| `go mod verify` | PASS; all modules verified | + +`validation.log` preserves the exact commands, candidate SHA, worktree state, UTC command-start timestamps, environment, raw output, and return codes. `--silent` suppresses Task command echo only; check output is preserved. Go's build/test cache was not globally disabled; the focused regression/vector command and the evidence checker explicitly use `-count=1`, and the root normal/race results in this transcript are uncached (no cached marker). The definition of every constituent `task check` command is pinned by the candidate's `Taskfile.yml`. + +Verify the raw transcript from this directory with: + +```sh +shasum -a 256 -c SHA256SUMS +``` + +No detached signature is attached to this local implementation-validation packet; it is not a signed release packet. + +## Review + +The code-review skill ran independent read-only Standards and Spec agents against the pinned diff `87528342d68d26db4a5a69386ae5647baf7d05ed...d2db6eed5923395bf0dfb57e73275ea0a1506c44`. + +- Standards: no material actionable findings. The checks preserve the CPace core boundary and terminal cleanup, use `hmac.Equal`, and add no persistent secret fields. Tests follow helper, assertion, and snapshot conventions; documentation records the narrow finding-driven exception and evidence limits. No actionable baseline smells were reported. +- Spec: no findings against issue #308 and its triage brief. Both roles, the AD matrix, correctly framed authentic-tag reflection, forced equality, honest controls, replay, nil outputs, consumption, and cleanup were covered. The review found no scope creep or incorrect implementation. Its pending validation-capture caveat is satisfied by this bundle. + +## Limitations + +The [analysis](../../key-confirmation-reflection.md) bounds the security conclusion: the forced equal-message false confirmation was reproduced, but no feasible way for a network attacker to force that condition with fresh production randomness was established. Randomized regression observations do not estimate collision probability, and replay tests do not exhaust concurrent schedules. + +These checks and automated reviews are not independent cryptographic review or a refreshed release-evidence packet. Historical dependency/SAST, Capslock, security/spec/vector, and paired long-fuzz evidence remains pinned to its older candidates. Refresh the affected lanes against the exact selected release candidate before any stronger release claim, as recorded in the [evidence baseline](../../evidence-baseline.md). No hosted CI, cross-platform campaign, paired long-fuzz run, or new Capslock capture is claimed here. diff --git a/docs/evidence/308-reflection-20260923/SHA256SUMS b/docs/evidence/308-reflection-20260923/SHA256SUMS new file mode 100644 index 0000000..2f9bdc6 --- /dev/null +++ b/docs/evidence/308-reflection-20260923/SHA256SUMS @@ -0,0 +1 @@ +b98367e6a6f9afe9046509668c3f7d41743b96a166413f8b0d1a53dbab70466c validation.log diff --git a/docs/evidence/308-reflection-20260923/validation.log b/docs/evidence/308-reflection-20260923/validation.log new file mode 100644 index 0000000..da86c68 --- /dev/null +++ b/docs/evidence/308-reflection-20260923/validation.log @@ -0,0 +1,122 @@ + +UTC 2026-09-23T18:14:58.755539+00:00 +$ git rev-parse HEAD +d2db6eed5923395bf0dfb57e73275ea0a1506c44 +exit_code=0 + +UTC 2026-09-23T18:14:58.766847+00:00 +$ git status --short --branch +## codex/308-reflection-regressions +exit_code=0 + +UTC 2026-09-23T18:14:58.779543+00:00 +$ uname -srm +Darwin 27.0.0 arm64 +exit_code=0 + +UTC 2026-09-23T18:14:58.783040+00:00 +$ go version +go version go1.27.1 darwin/arm64 +exit_code=0 + +UTC 2026-09-23T18:14:58.790186+00:00 +$ task --version +3.53.1 +exit_code=0 + +UTC 2026-09-23T18:14:58.821730+00:00 +$ go test -run ^(TestReflection|TestRistrettoDraft21Vectors|TestIRTranscriptDraftVectorFlow) -count=1 -v . +=== RUN TestReflectionResponderRejectsOwnTag +=== RUN TestReflectionResponderRejectsOwnTag/empty/production_randomness +=== RUN TestReflectionResponderRejectsOwnTag/empty/forced_equal_shares +=== RUN TestReflectionResponderRejectsOwnTag/equal_nonempty/production_randomness +=== RUN TestReflectionResponderRejectsOwnTag/equal_nonempty/forced_equal_shares +=== RUN TestReflectionResponderRejectsOwnTag/different/production_randomness +=== RUN TestReflectionResponderRejectsOwnTag/different/forced_equal_shares +--- PASS: TestReflectionResponderRejectsOwnTag (0.00s) + --- PASS: TestReflectionResponderRejectsOwnTag/empty/production_randomness (0.00s) + --- PASS: TestReflectionResponderRejectsOwnTag/empty/forced_equal_shares (0.00s) + --- PASS: TestReflectionResponderRejectsOwnTag/equal_nonempty/production_randomness (0.00s) + --- PASS: TestReflectionResponderRejectsOwnTag/equal_nonempty/forced_equal_shares (0.00s) + --- PASS: TestReflectionResponderRejectsOwnTag/different/production_randomness (0.00s) + --- PASS: TestReflectionResponderRejectsOwnTag/different/forced_equal_shares (0.00s) +=== RUN TestReflectionInitiatorRejectsReflectedShareAndAuthenticTag +=== RUN TestReflectionInitiatorRejectsReflectedShareAndAuthenticTag/empty/production_randomness +=== RUN TestReflectionInitiatorRejectsReflectedShareAndAuthenticTag/empty/forced_equal_shares +=== RUN TestReflectionInitiatorRejectsReflectedShareAndAuthenticTag/equal_nonempty/production_randomness +=== RUN TestReflectionInitiatorRejectsReflectedShareAndAuthenticTag/equal_nonempty/forced_equal_shares +=== RUN TestReflectionInitiatorRejectsReflectedShareAndAuthenticTag/different/production_randomness +=== RUN TestReflectionInitiatorRejectsReflectedShareAndAuthenticTag/different/forced_equal_shares +--- PASS: TestReflectionInitiatorRejectsReflectedShareAndAuthenticTag (0.00s) + --- PASS: TestReflectionInitiatorRejectsReflectedShareAndAuthenticTag/empty/production_randomness (0.00s) + --- PASS: TestReflectionInitiatorRejectsReflectedShareAndAuthenticTag/empty/forced_equal_shares (0.00s) + --- PASS: TestReflectionInitiatorRejectsReflectedShareAndAuthenticTag/equal_nonempty/production_randomness (0.00s) + --- PASS: TestReflectionInitiatorRejectsReflectedShareAndAuthenticTag/equal_nonempty/forced_equal_shares (0.00s) + --- PASS: TestReflectionInitiatorRejectsReflectedShareAndAuthenticTag/different/production_randomness (0.00s) + --- PASS: TestReflectionInitiatorRejectsReflectedShareAndAuthenticTag/different/forced_equal_shares (0.00s) +=== RUN TestReflectionInitiatorWithoutTagOracle +=== RUN TestReflectionInitiatorWithoutTagOracle/empty +=== RUN TestReflectionInitiatorWithoutTagOracle/equal_nonempty +=== RUN TestReflectionInitiatorWithoutTagOracle/different +--- PASS: TestReflectionInitiatorWithoutTagOracle (0.00s) + --- PASS: TestReflectionInitiatorWithoutTagOracle/empty (0.00s) + --- PASS: TestReflectionInitiatorWithoutTagOracle/equal_nonempty (0.00s) + --- PASS: TestReflectionInitiatorWithoutTagOracle/different (0.00s) +=== RUN TestReflectionHonestExchangeControls +=== RUN TestReflectionHonestExchangeControls/empty +=== RUN TestReflectionHonestExchangeControls/equal_nonempty +=== RUN TestReflectionHonestExchangeControls/different +=== RUN TestReflectionHonestExchangeControls/forced_equal_shares_different_ad +--- PASS: TestReflectionHonestExchangeControls (0.00s) + --- PASS: TestReflectionHonestExchangeControls/empty (0.00s) + --- PASS: TestReflectionHonestExchangeControls/equal_nonempty (0.00s) + --- PASS: TestReflectionHonestExchangeControls/different (0.00s) + --- PASS: TestReflectionHonestExchangeControls/forced_equal_shares_different_ad (0.00s) +=== RUN TestReflectionRejectsTagsFromAnotherExchange +=== RUN TestReflectionRejectsTagsFromAnotherExchange/empty +=== RUN TestReflectionRejectsTagsFromAnotherExchange/equal_nonempty +=== RUN TestReflectionRejectsTagsFromAnotherExchange/different +--- PASS: TestReflectionRejectsTagsFromAnotherExchange (0.00s) + --- PASS: TestReflectionRejectsTagsFromAnotherExchange/empty (0.00s) + --- PASS: TestReflectionRejectsTagsFromAnotherExchange/equal_nonempty (0.00s) + --- PASS: TestReflectionRejectsTagsFromAnotherExchange/different (0.00s) +=== RUN TestIRTranscriptDraftVectorFlow +--- PASS: TestIRTranscriptDraftVectorFlow (0.00s) +=== RUN TestRistrettoDraft21Vectors +=== RUN TestRistrettoDraft21Vectors/generator_sid_mutation_first_byte +=== RUN TestRistrettoDraft21Vectors/generator_sid_mutation_last_byte +=== RUN TestRistrettoDraft21Vectors/generator_sid_mutation_nil +=== RUN TestRistrettoDraft21Vectors/generator_sid_mutation_empty +=== RUN TestRistrettoDraft21Vectors/generator_sid_mutation_appended +=== RUN TestRistrettoDraft21Vectors/generator_sid_mutation_truncated +--- PASS: TestRistrettoDraft21Vectors (0.00s) + --- PASS: TestRistrettoDraft21Vectors/generator_sid_mutation_first_byte (0.00s) + --- PASS: TestRistrettoDraft21Vectors/generator_sid_mutation_last_byte (0.00s) + --- PASS: TestRistrettoDraft21Vectors/generator_sid_mutation_nil (0.00s) + --- PASS: TestRistrettoDraft21Vectors/generator_sid_mutation_empty (0.00s) + --- PASS: TestRistrettoDraft21Vectors/generator_sid_mutation_appended (0.00s) + --- PASS: TestRistrettoDraft21Vectors/generator_sid_mutation_truncated (0.00s) +PASS +ok github.com/the-sarge/cpace 0.203s +exit_code=0 + +UTC 2026-09-23T18:14:59.205312+00:00 +$ task --silent check +release helper smoke tests passed +ok github.com/the-sarge/cpace/tools/releasepolicy 0.225s +release policy checker passed +CI change classifier tests passed +Go tool helper tests passed +Fuzz input validator tests passed +ok github.com/the-sarge/cpace/tools/evidencebaseline 0.338s +evidence baseline checker passed +ok github.com/the-sarge/cpace 0.125s +ok github.com/the-sarge/cpace 1.462s +0 issues. +No vulnerabilities found. +exit_code=0 + +UTC 2026-09-23T18:15:16.583100+00:00 +$ go mod verify +all modules verified +exit_code=0 diff --git a/docs/key-confirmation-reflection.md b/docs/key-confirmation-reflection.md new file mode 100644 index 0000000..bd6af8f --- /dev/null +++ b/docs/key-confirmation-reflection.md @@ -0,0 +1,70 @@ +# Key-confirmation reflection review (#308) + +Date: 2026-09-23. Baseline: `87528342d68d26db4a5a69386ae5647baf7d05ed`. Scope: this package's ordered Ristretto255/SHA-512 A/B/C exchange. This is a bounded implementation review and regression investigation, not independent cryptographic review or a production-readiness claim. + +## Conclusion and disposition + +The baseline has a conditional false-confirmation path: with equal public shares and equal associated data, the responder accepts its own B tag as C before the initiator has called Finish. Deterministic internal fixtures reproduce this with distinct configured identities and a nonempty SessionID. No feasible network attack producing that condition with fresh production randomness was established. The upstream single-run initiator attack does not transfer directly because this package withholds the initiator's tag until responder verification succeeds. + +Nevertheless, confirmation must distinguish a peer's tag from the local tag. The concrete equal-message acceptance finding justifies the narrow security-fix exception to the repository's behavior freeze: both Finish paths now reject an otherwise correct remote tag that equals the local confirmation tag, using `hmac.Equal` and the existing `ErrConfirmationFailed`. This closes the demonstrated conditional path rather than relying on share inequality as an implicit precondition. No public API, MAC role label, dependency, randomness interface, or wire encoding is added. This is a finding-driven correction within the existing confirmation boundary, not a broader architectural or package-policy reopen; no new ADR is introduced. + +## Upstream status + +On the review date, [upstream PR #21](https://github.com/cfrg/draft-irtf-cfrg-cpace/pull/21) remained open at `87b1aef16ee176511948f1b629424bca910732b1`, and [Datatracker](https://datatracker.ietf.org/doc/draft-irtf-cfrg-cpace/) still listed draft-21 as the latest revision. The PR patch rejects identical protocol messages; the [September 16 editor proposal](https://github.com/cfrg/draft-irtf-cfrg-cpace/pull/21#issuecomment-5700964252) instead describes rejecting equal reflected tags. The [September 22 update](https://github.com/cfrg/draft-irtf-cfrg-cpace/pull/21#issuecomment-5774999640) reports shepherd/chair approval to incorporate a clarification during RFC Editor review. There is not yet final published replacement wording to claim conformance with. Recheck it when updating the draft/RFC target. + +[Draft-21 §10.4](https://datatracker.ietf.org/doc/html/draft-irtf-cfrg-cpace-21#section-10.4) offers optional confirmation guidance that also permits parallel tag emission. This package mandates confirmation with a fixed order. The remediation follows the editor's equal-tag condition, but its justification is the reproduced package behavior, not the upstream editorial classification. + +## Attacker and exchange ordering + +The attacker can observe, replace, reframe, replay, reorder, and relay wire messages, including their public shares, AD, and tags. It does not initially know the password, ephemeral scalars, shared secret K, ISK, or MAC key, and cannot control `crypto/rand.Reader` or read process memory. Password guessing, compromised randomness, and compromise of a password-bearing peer are not made safe by this check. + +| Stage | Package action | Tag available to a network attacker | +| --- | --- | --- | +| `Start` | Samples the initiator scalar and sends A containing sid, Ya, ADa | None | +| `Respond` | Validates A, samples its own scalar, derives K/ISK, sends B containing Yb, ADb, Tb | Tb; no authenticated Session | +| `Initiator.Finish` | Derives K/ISK, verifies Tb, rejects equal Ta/Tb, then returns C containing Ta and a Session | Ta only after successful verification and reflection check | +| `Responder.Finish` | Verifies Ta and rejects equality with its own Tb before returning a Session | No new tag | + +For the initiator attack, replacing Yb with Ya is easy: the point is valid. But the resulting K is the initiator's scalar multiplied by its own share, and no tag for that resulting session has been released by `Start`. A fabricated B tag fails cryptographic confirmation. Copying a real Tb from an ordinary responder also fails after replacing B's share/AD, because that Tb belongs to the responder's original K and ordered transcript. The initiator exposes no tag oracle on failure: C and Session are nil, its scalar is cleared, and a second Finish returns `ErrStateUsed`. + +For the responder attack, Tb is available before C and can be copied into a correctly framed C without knowing a secret. At baseline this succeeds exactly when it is also the expected Ta. Equal shares and equal AD guarantee that equality: both tags use the same MAC key and identical length-value input. With distinct messages, equality would require a MAC collision; ordinary reflected Tb fails. The fix rejects equality even when the copied tag is otherwise correct. + +## Why the bindings do and do not help + +`confirmationTag` in `crypto.go` hashes `CPaceMac || sid || ISK` for its key and authenticates `lvCat(y, ad)`. `newIRTranscript` in `transcript.go` binds Ya, ADa, Yb, ADb in fixed order into ISK. There is no role-specific prefix in the tag input. Thus, within one session, equal `(y, ad)` pairs necessarily yield equal tags; distinct CI identities or a fresh nonempty sid cannot change that fact. + +`buildCI` binds ordered, role-labelled identities, context, suite, and draft version into the common generator. This protects agreement on those configured facts, including role-local identity reversal. It does not turn identical within-session MAC inputs into different ones. Service-specific identities and appropriate application role assignment remain necessary for avoiding confused relays. + +AD is authenticated as transmitted, not required to differ or to equal an application's expected remote value. Empty and equal nonempty AD are valid inputs. Differing AD distinguishes the tag inputs even for equal shares, and an honest exchange in that case remains supported. Applications must still validate expected peer AD as described in [integration guidance](integration-guidance.md). + +Framing checks reject verbatim A-as-B or B-as-C wire bytes, but an attacker can copy fields into the proper role envelope. The tests deliberately use valid B/C framing and require `ErrConfirmationFailed`, rather than treating `ErrMessage` as reflection protection. The wire role byte is not a MAC role label. + +SessionID is checked against A at the responder and bound into generator derivation, ISK, and confirmation-key derivation. It separates sessions when used freshly but does not distinguish the two tags inside one session. The package does not maintain a global replay cache or enforce cross-session sid uniqueness; the explicit empty-sid compatibility option does not change the equal-tag check. + +## Forced equality and concurrent sessions + +The equal-share fixture supplies the same nonzero scalar stream to both existing unexported randomness seams with matching password/CI/sid. It proves the conditional behavior, not that an attacker controls the responder scalar in production. A is fixed before the responder draws its scalar. For a nonidentity generator, multiplication by the scalar is injective, and this implementation samples uniformly from nonzero 252-bit values under ideal independent randomness. For a fixed A in that set, the per-attempt equal-share probability is `1/(2^252 - 1)`. This is a reasoning bound under the stated model, not a measurement from the production-randomness tests or a full security proof. + +An attacker can collect authentic responder tags across concurrent sessions. Relaying Yb from one response as A to another responder does not make the next responder reuse Yb: it draws another scalar and binds the new ordered transcript. Likewise, copying B or C tags across independently sampled exchanges does not normally preserve K/ISK and their transcript binding. A regression replays authentic tags between two exchanges with identical caller inputs, deliberately including the same sid, and observes rejection. That bounded test does not authorize sid reuse or exhaust concurrent-session schedules. + +Transparent relay of an actual exchange between password holders is different from manufacturing confirmation without a matching peer. This patch is not channel authentication or a general relay defense. [Draft-21 §10.1.2](https://datatracker.ietf.org/doc/html/draft-irtf-cfrg-cpace-21#section-10.1.2) discusses application measures for concurrent loopback; [§10.9](https://datatracker.ietf.org/doc/html/draft-irtf-cfrg-cpace-21#section-10.9) requires fresh scalars and recommends unique sid values. Applications must preserve those assumptions and bind their intended roles and outer protocol. + +## Compatibility and tests + +The new observable result is `ErrConfirmationFailed` for otherwise valid equal-tag exchanges, including the negligible honest equal-share/equal-AD collision case. It is not `ErrAbort` (the share can be valid) or `ErrMessage` (the framing can be valid). The existing error identity and text are reused. `Respond` still emits B without claiming authentication; neither Finish returns a Session on rejection, and the initiator releases no C. Both states are consumed and persistent secrets cleared by the existing terminal lifecycle. MAC derivation, transcript bytes, draft vectors, public signatures, and the ordering of invalid-peer-share validation remain unchanged. The responder recomputes one local tag after successful peer-tag verification rather than storing another value in its core. + +`reflection_test.go` covers: + +- `TestReflectionResponderRejectsOwnTag`: authentic Tb copied into C before initiator Finish; empty/equal/different AD, production randomness and forced equal shares, nil Session, state consumption and cleanup. +- `TestReflectionInitiatorRejectsReflectedShareAndAuthenticTag`: A fields reframed as B with a genuine Tb; the forced equal-message case is also the genuine B, demonstrating that no ambiguous confirmation escapes. +- `TestReflectionInitiatorWithoutTagOracle`: correctly framed reflected share with a guessed mandatory tag; no C or Session escapes and state is spent. +- `TestReflectionHonestExchangeControls`: all three AD cases with production randomness, plus forced equal shares with differing AD; both sides export matching keys. +- `TestReflectionRejectsTagsFromAnotherExchange`: authentic B/C tags replayed across independent exchanges, with failure and cleanup assertions. + +The responder and initiator tests were each run before their corresponding check was added. Both failed only in the empty/equal-nonempty AD cases with forced equal shares, returning a Session (and C on the initiator). They pass with the checks. These are durable rejection regressions, not assertions that baseline acceptance should remain supported. Existing honest-exchange and draft-vector tests remain part of validation. + +## Evidence limits + +This security-relevant change invalidates the applicability of the older commit-pinned dependency/SAST, Capslock, security/spec/vector, and paired long-fuzz release evidence to this candidate. [Evidence baseline](evidence-baseline.md) preserves the historical records and the refresh requirement. Local tests and automated code review are not an independent cryptographic assessment, a multi-host long-fuzz campaign, or a release-evidence refresh. No stronger release claim is made by this change. Exact source-candidate validation results are recorded separately from the source commit to avoid a self-referential SHA. + +[The validation bundle](evidence/308-reflection-20260923/README.md) records source candidate `d2db6eed5923395bf0dfb57e73275ea0a1506c44`, successful focused regressions/vectors, full `task check`, module verification, and the Standards/Spec reviews. It includes raw command output and checksums; the follow-up evidence commit changes documentation only. diff --git a/docs/spec-matrix.md b/docs/spec-matrix.md index 6b2dd2b..ce623e3 100644 --- a/docs/spec-matrix.md +++ b/docs/spec-matrix.md @@ -15,6 +15,7 @@ Audit: the original matrix was reviewed against implementation commit `2e09774f1 | `scalar_mult_vfy` aborts on decode failure or neutral output | `scalarMultVFY` keeps the draft-shaped encoded-byte helper for vector/spec traceability; `scalarMultVFYElement` applies the same multiplication and neutral-output defense after responder prevalidation has already decoded `Ya`. Both paths return nil plus an `ErrAbort`-wrapped error instead of the draft's function-level neutral-element return — an intentional internal-only divergence with identical abort behavior (ADR-0003). | `TestScalarMultVFYDraftInvalidVectors`, `TestScalarMultVFYElementMatchesEncodedPeerShare`, `TestProtocolAbortsOnInvalidRistrettoEncoding`, `TestScalarMultVFYPostMultiplyIdentityDefense`, embedded B.3.11 JSON fixture | | Compute ISK from `lv_cat(DSI_ISK,sid,K)||transcript_ir(...)` | `deriveISK` | `TestRistrettoDraft21Vectors`; embedded B.3.9 JSON fixture pinned to the draft-decoded SHA-256 | | Add explicit key confirmation with MAC key derived from ISK | `confirmationTag`, `Initiator.Finish`, `Responder.Finish`; tags remain draft-compatible with no package-added role labels | confirmed exchange and mismatch tests | +| Reject reflected equal confirmation tags (issue #308 hardening informed by the pending upstream §10.4 clarification) | Both core Finish paths verify the remote tag and reject equality with the local tag before returning a Session; initiator also withholds C. This is a package correction, not a claim that new RFC wording is already published; see [analysis](key-confirmation-reflection.md). | `TestReflectionResponderRejectsOwnTag`, `TestReflectionInitiatorRejectsReflectedShareAndAuthenticTag`, `TestReflectionInitiatorWithoutTagOracle`, `TestReflectionHonestExchangeControls`, `TestReflectionRejectsTagsFromAnotherExchange` | | Integrate initiator and responder identifiers into CI with role binding | `buildCI` | mismatch tests; CI format documented as package-owned | | Abort on invalid/weak points | `Respond` prevalidates message A shares before responder scalar sampling as implementation hardening, then reuses that decoded non-identity element for responder Diffie-Hellman; `Initiator.Finish` keeps the encoded-byte `scalarMultVFY` check for responder shares. Rejections wrap `ErrAbort` plus `ErrPeerShareEncoding` or `ErrPeerShareIdentity` with role context. | invalid Ristretto tests, `TestPeerShareErrorsWrapErrAbort`, `TestPeerShareRoleDecodeSharedSecretAddsRoleContext`, `TestPeerShareErrorsWrapErrAbort/respond_non-canonical`, `TestPeerShareErrorsWrapErrAbort/respond_identity` | @@ -34,5 +35,6 @@ Package-owned profile and extensions: Known gaps before a production release: +- Exact-candidate evidence refresh after issue #308's security-relevant confirmation change; the historical audit SHA above does not cover this correction (see [evidence baseline](evidence-baseline.md)). - independent cryptographic review - external review of package-owned message framing, CI, and profile choices diff --git a/docs/threat-model.md b/docs/threat-model.md index a05a62c..a3ef880 100644 --- a/docs/threat-model.md +++ b/docs/threat-model.md @@ -75,6 +75,10 @@ length-value fields, exact-sized public shares and tags, and per-field size caps. Decoders reject malformed, non-canonical, oversized, trailing, wrong-role, wrong-suite, and invalid-length inputs. +### Key-Confirmation Reflection + +Both Finish paths reject an otherwise valid remote confirmation tag equal to the local tag with `ErrConfirmationFailed`. This addresses issue #308's forced equal-share/equal-AD case, where the baseline responder accepted its own B tag as C. No feasible way to force that condition with fresh production randomness was established, but identities, CI, and SessionID do not distinguish identical tag inputs within one session. Initiator confirmation is released only after peer verification and the equality check; all rejected Finish calls return no Session and consume state. See [the reflection analysis](key-confirmation-reflection.md) for the attacker model, A/B/C ordering, correctly framed regression attempts, compatibility change, concurrent-session limits, and stale release-evidence lanes. + ### Session Outputs `Session.TranscriptID` is the draft `CPaceSidOutput` for the confirmed CPace diff --git a/reflection_test.go b/reflection_test.go new file mode 100644 index 0000000..5b21612 --- /dev/null +++ b/reflection_test.go @@ -0,0 +1,230 @@ +package cpace + +import ( + "bytes" + "errors" + "testing" +) + +func reflectionADCases() []struct { + name string + a, b []byte +} { + return []struct { + name string + a, b []byte + }{ + {name: "empty"}, + {name: "equal_nonempty", a: []byte("same"), b: []byte("same")}, + {name: "different", a: []byte("first"), b: []byte("second")}, + } +} + +// Forced equality is a test-only condition: the attacker cannot select the +// responder's fresh scalar through Respond. Both inputs retain distinct party +// identities and a nonempty shared SessionID. +func mustReflectionExchange(t *testing.T, ada, adb []byte, equalShares bool) *exchangeFixture { + t.Helper() + iInput, rInput := testInitiatorInput(), testResponderInput() + iInput.LocalAssociatedData, rInput.LocalAssociatedData = ada, adb + start, respond := Start, Respond + if equalShares { + start = startTestInitiator + respond = func(input Input, msgA []byte) (*Responder, []byte, error) { + return respondWithRandom(input, msgA, repeatingRand(0x11)) + } + } + i, msgA, err := start(iInput) + if err != nil { + t.Fatal(err) + } + t.Cleanup(func() { _ = i.Close() }) + r, msgB, err := respond(rInput, msgA) + if err != nil { + t.Fatal(err) + } + t.Cleanup(func() { _ = r.Close() }) + a, err := decodeMessageA(msgA) + if err != nil { + t.Fatal(err) + } + b, err := decodeMessageB(msgB) + if err != nil { + t.Fatal(err) + } + if got := bytes.Equal(a.ya, b.yb); got != equalShares { + t.Fatalf("share equality got %v want %v", got, equalShares) + } + return &exchangeFixture{tb: t, initiator: i, responder: r, msgA: msgA, msgB: msgB} +} + +func TestReflectionResponderRejectsOwnTag(t *testing.T) { + for _, ad := range reflectionADCases() { + for _, mode := range []struct { + name string + equalShares bool + }{{name: "production_randomness"}, {name: "forced_equal_shares", equalShares: true}} { + t.Run(ad.name+"/"+mode.name, func(t *testing.T) { + x := mustReflectionExchange(t, ad.a, ad.b, mode.equalShares) + b, err := decodeMessageB(x.msgB) + if err != nil { + t.Fatal(err) + } + // Copy only the authentic wire tag. The initiator has never + // called Finish, and the attacker reads no secret material. + msgC := encodeMessageC(b.tag) + secrets := snapshotResponderSecrets(t, x.responder) + session, err := x.responder.Finish(msgC) + if session != nil || !errors.Is(err, ErrConfirmationFailed) { + t.Fatalf("reflected C got session=%v err=%v want nil ErrConfirmationFailed", session != nil, err) + } + secrets.assertCleared() + if session, err := x.responder.Finish(msgC); session != nil || !errors.Is(err, ErrStateUsed) { + t.Fatalf("second Finish got session=%v err=%v want nil ErrStateUsed", session != nil, err) + } + }) + } + } +} + +func TestReflectionInitiatorRejectsReflectedShareAndAuthenticTag(t *testing.T) { + for _, ad := range reflectionADCases() { + for _, mode := range []struct { + name string + equalShares bool + }{{name: "production_randomness"}, {name: "forced_equal_shares", equalShares: true}} { + t.Run(ad.name+"/"+mode.name, func(t *testing.T) { + x := mustReflectionExchange(t, ad.a, ad.b, mode.equalShares) + a, err := decodeMessageA(x.msgA) + if err != nil { + t.Fatal(err) + } + b, err := decodeMessageB(x.msgB) + if err != nil { + t.Fatal(err) + } + // Reframe A's share and AD as B and copy an authentic B tag. + // For forced equal shares and equal AD this is the genuine B. + reflectedB := encodeMessageB(a.ya, a.ada, b.tag) + assertReflectionInitiatorRejected(t, x.initiator, reflectedB) + }) + } + } +} + +func assertReflectionInitiatorRejected(t *testing.T, i *Initiator, msgB []byte) { + t.Helper() + secrets := snapshotInitiatorSecrets(t, i) + msgC, session, err := i.Finish(msgB) + if msgC != nil || session != nil || !errors.Is(err, ErrConfirmationFailed) { + t.Fatalf("reflected B got C=%x session=%v err=%v want nil nil ErrConfirmationFailed", msgC, session != nil, err) + } + secrets.assertCleared() + if msgC, session, err := i.Finish(msgB); msgC != nil || session != nil || !errors.Is(err, ErrStateUsed) { + t.Fatalf("second Finish got C=%x session=%v err=%v want nil nil ErrStateUsed", msgC, session != nil, err) + } +} + +func TestReflectionInitiatorWithoutTagOracle(t *testing.T) { + for _, ad := range reflectionADCases() { + t.Run(ad.name, func(t *testing.T) { + input := testInitiatorInput() + input.LocalAssociatedData = ad.a + i, msgA, err := Start(input) + if err != nil { + t.Fatal(err) + } + t.Cleanup(func() { _ = i.Close() }) + a, err := decodeMessageA(msgA) + if err != nil { + t.Fatal(err) + } + // A has no tag to reflect. A network attacker must guess B's + // mandatory tag before the initiator will release C. + msgB := encodeMessageB(a.ya, ad.b, make([]byte, tagSize)) + assertReflectionInitiatorRejected(t, i, msgB) + }) + } +} + +func TestReflectionHonestExchangeControls(t *testing.T) { + for _, ad := range reflectionADCases() { + t.Run(ad.name, func(t *testing.T) { + x := mustReflectionExchange(t, ad.a, ad.b, false) + iSession, rSession := x.complete() + defer iSession.Close() + defer rSession.Close() + assertReflectionSessionKeysMatch(t, iSession, rSession) + }) + } + // Rejecting equal tags must not become a blanket equal-share ban: + // differing AD produces distinct tags even with forced equal shares. + t.Run("forced_equal_shares_different_ad", func(t *testing.T) { + x := mustReflectionExchange(t, []byte("first"), []byte("second"), true) + msgC, iSession := x.finishInitiator() + defer iSession.Close() + b, err := decodeMessageB(x.msgB) + if err != nil { + t.Fatal(err) + } + c, err := decodeMessageC(msgC) + if err != nil { + t.Fatal(err) + } + if bytes.Equal(b.tag, c.tag) { + t.Fatal("confirmation tags got equal want distinct with differing AD") + } + rSession := x.finishResponder(msgC) + defer rSession.Close() + assertReflectionSessionKeysMatch(t, iSession, rSession) + }) +} + +func assertReflectionSessionKeysMatch(t *testing.T, iSession, rSession *Session) { + t.Helper() + iKey, err := iSession.Export([]byte("reflection-control"), nil, 32) + if err != nil { + t.Fatal(err) + } + defer clearBytes(iKey) + rKey, err := rSession.Export([]byte("reflection-control"), nil, 32) + if err != nil { + t.Fatal(err) + } + defer clearBytes(rKey) + if !bytes.Equal(iKey, rKey) { + t.Fatal("exported keys got unequal want equal") + } +} + +func TestReflectionRejectsTagsFromAnotherExchange(t *testing.T) { + for _, ad := range reflectionADCases() { + t.Run(ad.name, func(t *testing.T) { + // Deliberately reuse all caller inputs, including SessionID. + // Fresh shares still bind each tag to its own exchange. + first := mustReflectionExchange(t, ad.a, ad.b, false) + second := mustReflectionExchange(t, ad.a, ad.b, false) + firstB, err := decodeMessageB(first.msgB) + if err != nil { + t.Fatal(err) + } + secondB, err := decodeMessageB(second.msgB) + if err != nil { + t.Fatal(err) + } + msgC, iSession := first.finishInitiator() + defer iSession.Close() + rSession := first.finishResponder(msgC) + defer rSession.Close() + assertReflectionInitiatorRejected(t, second.initiator, encodeMessageB(secondB.yb, secondB.adb, firstB.tag)) + secrets := snapshotResponderSecrets(t, second.responder) + if session, err := second.responder.Finish(msgC); session != nil || !errors.Is(err, ErrConfirmationFailed) { + t.Fatalf("replayed C got session=%v err=%v want nil ErrConfirmationFailed", session != nil, err) + } + secrets.assertCleared() + if session, err := second.responder.Finish(msgC); session != nil || !errors.Is(err, ErrStateUsed) { + t.Fatalf("second Finish got session=%v err=%v want nil ErrStateUsed", session != nil, err) + } + }) + } +}