Repository navigation
Conversation
|
@codex review |
|
bugbot run |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
💡 Codex Review
This is a libFuzzer-generated crash-<sha1> artifact and is byte-for-byte identical to the committed test/fuzz/qbit_corpora/p2p_v2_pq_malicious_peer/initiator-noncompliant-responder seed. Nothing references the root copy, so every checkout unnecessarily carries duplicate generated output that also misleadingly suggests an unresolved crash; retain the named corpus seed and delete this file.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 8f7ece3. Configure here.
Close the test gaps the #230 deep review's mutants exposed; each mutant below survived bip324_tests and net_tests before and is now caught. - The switch wipes the retained ECDH secret and the transcript (hybrid_api_order, v2_pq_pair, v2_pq_legacy_matrix), and a legacy outcome wipes them and the decapsulation key (v2_pq_legacy_matrix, in every arm and for every pair of modes). - v2_pq_close_wipes: a missing garbage terminator, an oversized version packet length and a damaged version packet tag close with both secrets wiped at once, in both roles. These closes come before the peer's version packet authenticated: no PQ failure, pending, and a responder's offer counts as sent. - The status is pending from the start of the handshake until the peer's version packet is processed, not legacy_peer (v2_pq_pair, v2_pq_legacy_matrix, v2_pq_close_wipes). - A decapsulation failure closes on the initiator's version packet itself, with no confirmation queued, instead of one packet later. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@codex review |
|
Codex Review: Didn't find any major issues. 👍 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Part 4 now wipes the hybrid secrets whenever the peer's version is processed (#230, L5). pq_shed_wipes_secrets already checks right after the initiator's key, before any version, so a shed branch that doesn't wipe is still caught. With the merged part 4, a variant of the test that checks after the version lets that mutant survive. Make the precondition explicit, so the check can't drift past the version. Follow-up to work item F4 of #206 (#184 part 5). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Add the hybrid ML-KEM-1024 key recipe of #184 to BIP324Cipher. Nothing calls it from the transport yet; E3 does. - Initialize(..., retain_for_hybrid) keeps the raw BIP324 ECDH secret, starts a running SHA256 transcript with ellswift_I || ellswift_R, and snapshots the network magic and cipher direction. The default (false) keeps today's secret lifetime and the existing vectors. - AddVersionContents hashes CompactSize(len) || contents, responder first, through a small stream adapter without copying the contents. - SwitchToHybrid derives HKDF-SHA256 with salt "qbit_v2_hybrid_mlkem1024" || magic over ss_ECDH || ss_MLKEM || TH, re-emplaces all four packet ciphers with counters at 0 and replaces the session id; garbage terminators stay. Misuse (not initialized, not retained, wrong contents count, a second switch) returns false and changes nothing. - The retained secret, IKM, HKDF state and derived keys are wiped on switch, in DiscardHybridSecret and in the destructor. Copying is deleted. The same recipe goes into test_framework/crypto/bip324_cipher.py, and contrib/devtools/generate-pq-transport-vectors.py uses it, the Python ML-KEM-1024 and the BIP324 ECDH helpers (imported by path, never the C++ code) to write src/test/data/pq_transport_vectors.json: inputs, every intermediate, both confirmations and first packets, and negative cases. Its --check mode runs in lint through test/lint/lint-pq-transport-vectors.py. bip324_tests gains hybrid_vectors (both roles and self-decrypt mode, with every intermediate recomputed from qbit's own primitives), hybrid_transcript_binding, hybrid_api_order and hybrid_rekey_boundaries. The bip324_cipher_roundtrip fuzz target gains an optional switch, transcript and secret mutations and discards. The existing BIP324 vectors are unchanged. Part of #184 (#199, E1). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Add the specification of the hybrid ML-KEM-1024 v2 transport from #184's design, written for interoperating implementations and outside review: the record grammar and header registry (0x00 reserved, 0xF0 for the ML-KEM-1024 offer and accept, qbit's values from 0xF0 upward), byte layouts, parse rules (unparseable contents are ignored, unlike the Transport Feature Negotiation draft), the key recipe byte by byte, the responder and initiator message rules and why progress still holds, the switch boundary, key confirmation MUSTs, failure handling, security notes, an interop walk-through of the vectors file, the convergence and retirement policy, and a contributor section. The spec quotes vector 0 in a marked block, and generate-pq-transport-vectors.py now fails (in --check mode too, so in lint) when a quoted value differs from the vectors. Part of #184 (#199, H2). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Add src/bip324_pq.{h,cpp} with PQHandshake, the version-contents parser
and one connection's ML-KEM-1024 state for the hybrid v2 transport of
#184. Nothing calls it from V2Transport yet; E3 does, so behavior is
unchanged.
- ParseContents is a single noexcept pass over a SpanReader. The whole
contents must parse as records CompactSize(len) || header || payload
(len >= 1, canonical, within the contents) before anything else is
looked at; any framing error means no features. Each length is
checked against what is left before SpanReader::ignore(). The first
0xF0 record wins and is returned as a span into the contents.
- CheckRecordLength applies the role's length rule to that record
(EK_LENGTH or CT_LENGTH); AcceptOffer runs the modulus check
(EK_MODULUS) and encapsulates; DecapsulateAccept decapsulates and
wipes the single-use key. SerializeRecord writes fd 21 06 f0 ||
payload.
- Key generation draws two independent 32-byte values and encapsulation
one, from GetStrongRandBytes in production or the test constructor's
PQRandomSource; the ML-KEM operations come through PQKemOps. Seeds
and coins are wiped.
- Internal ML-KEM errors return mlkem::Error::INTERNAL and record which
operation failed; nothing asserts. The first recorded failure wins,
and any failure, ClearSecrets() and the destructor wipe the
decapsulation key.
net_tests gains pq_records_grammar, pq_records_first_wins,
pq_record_semantics, pq_records_max_contents and
pq_handshake_negotiation (vector 0's ek, ct and shared secret from
deterministic entropy, plus injected keygen, check, encaps and decaps
failures). The new pq_records fuzz target checks the parser against an
independent reading of the grammar; it joins the required qbit seed
corpora with 15 generated seeds, including the nightly mutation list
in ci/test/03_test_script.sh.
Part of #184 (#199, E2).
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Wire PQHandshake into V2Transport per #184 and #199 (E3). Production construction stays PQ-off (V2PQOptions{} in MakeTransport); part 6 adds the option, and with the switch off the wire bytes are today's. - New states: SendState::AWAITING_VERSION and RecvState::CONFIRM, with the transition checks and diagrams updated. - With PQMode::NEGOTIATE the ciphers retain the ECDH secret and start the transcript. A responder generates its key only once the initiator's whole public key has arrived, queues its terminator and the offer, and then sends nothing until the initiator's version packet is processed. An initiator queues only its terminator and holds its version packet. While holding, SetMessageToSend refuses and GetBytesToSend reports more=false. - ProcessReceivedPacketBytes takes the send lock around ProcessVersionContents, so the switch of all four ciphers and the session id is atomic under both locks; application packets take no extra lock. A legacy peer (no own record, or contents that do not parse) keeps the ECDH keys. A valid offer is answered with the accept under the ECDH keys, then both sides switch and append a 20-byte key confirmation. Packets are only ever appended to the send buffer. - In CONFIRM the peer's first hybrid packet must decrypt to length 0, checked as soon as its 3 length bytes arrive, then verify its tag with empty AAD and carry the ignore bit. It never becomes a message. - Off and fallback neither parse nor validate the peer's contents; a responder that could not offer never decapsulates. - Internal ML-KEM failures never assert. Key generation, key check and encapsulation failures continue as plain v2 with an empty version packet; a decapsulation failure after the initiator committed closes. The decapsulation key and the retained ECDH secret are wiped on switch, legacy outcome, every failure and in destructors. - Transport::Info gains transport_pq and transport_pq_status; until the confirmation the status is pending, the classification stays detecting and no session id is published. V2Transport::GetPQSnapshot exposes the negotiation's progress and first failure for part 5. - V2PQOptions::corrupt_shared_secret inverts byte 0 of the local ML-KEM secret before derivation, as the vectors' negative case models. net_tests gains v2_pq_pair, v2_pq_legacy_matrix (including two real transports in every mode pair), v2_pq_v1_detection, v2_pq_send_hold, v2_pq_switch_boundary, v2_pq_confirmation_length, _tag and _not_decoy, v2_pq_corrupt_ct, v2_pq_internal_errors and v2_pq_rekey, and the existing random-contents scenarios also run with the negotiation on against a legacy peer. The bidirectional v2 and v1v2 transport fuzz targets take a PQ mode and an ML-KEM entropy seed per side from the input and check that the keys are hybrid exactly when both sides negotiate; both become required qbit corpus targets with generated seeds, including in the nightly mutation list. Part of #184 (#199, E3). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Add the p2p_v2_pq_malicious_peer target (#199, G4): a peer that completes the ECDH part of BIP324 honestly, so every packet it sends authenticates, and drives the hybrid negotiation of a V2Transport from the attacker's side. The tested transport runs in either role and in each mode (a responder never falls back), built with E3's deterministic test constructor and counting ML-KEM operations. The fuzzer scripts the peer step by step: its key, terminator, decoys (also carrying own records), version packets (honest, duplicate and unknown records, trailing corruption, malformed lengths, a key that fails the modulus check, blind, damaged and unsolicited ciphertexts, an accept without the peer's own switch), confirmation variants, application packets, unauthenticated bytes, partial delivery and collection, messages for the transport to send, and a disconnect at any point. The peer reads everything the transport sends and switches to hybrid keys itself when it played its part honestly. The variant generator follows the E3 verifier's malpeer harness. Checked after every step: no assertion or hang; GetBytesToSend stays append-only and its 'more' predictions hold; refused messages stay untouched and a holding initiator takes none; nothing is delivered between the switch and the peer's confirmation; the transport sends no decoy but its confirmation, which decrypts to length 0 under the peer's keys when both switched; only the same keys confirm, with equal session ids, and an honest exchange delivered in full confirms; off and fallback make no ML-KEM call, and a responder decapsulates only an accept to its own offer; once the peer's version packet is processed, or the transport failed, no hybrid secret remains; a close under the hybrid keys before confirmation is a confirmation failure. The target joins the required qbit seed corpora with 16 generated seeds, each a script whose outcome is documented in the manifest: honest exchanges in both roles (with maximum garbage, extra records and fragmented delivery), every malformed record and confirmation failure, a damaged ciphertext, a legacy peer, a fallback initiator, an unsolicited accept and a disconnect while the offer is pending. Part of #184 (#199, G4). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The G4 commit added crash-ef5ec86784262f7cd94f46933e5f6760bee654b5 at the repository root by accident. It is an input for p2p_v2_pq_malicious_peer left over from one of the harness false positives fixed during G4's development, not a transport defect: it replays cleanly on that target and on pq_records, bip324_cipher_roundtrip and both bidirectional transport targets. libFuzzer writes its crash, leak, out-of-memory, slow-unit and timeout artifacts to the working directory, so ignore those prefixes at the root. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Close the test gaps the #230 deep review's mutants exposed; each mutant below survived bip324_tests and net_tests before and is now caught. - The switch wipes the retained ECDH secret and the transcript (hybrid_api_order, v2_pq_pair, v2_pq_legacy_matrix), and a legacy outcome wipes them and the decapsulation key (v2_pq_legacy_matrix, in every arm and for every pair of modes). - v2_pq_close_wipes: a missing garbage terminator, an oversized version packet length and a damaged version packet tag close with both secrets wiped at once, in both roles. These closes come before the peer's version packet authenticated: no PQ failure, pending, and a responder's offer counts as sent. - The status is pending from the start of the handshake until the peer's version packet is processed, not legacy_peer (v2_pq_pair, v2_pq_legacy_matrix, v2_pq_close_wipes). - A decapsulation failure closes on the initiator's version packet itself, with no confirmation queued, instead of one packet later. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
ProcessVersionContents returned early, without wiping, when this side was not awaiting the peer's version packet: the negotiation off, in fallback, or a responder that could not offer. Every such path in this PR wipes (or never retains) before it sends its version packet, so this is correct today. But a later path that declines the negotiation after Initialize(..., retain_for_hybrid=true) would keep the ECDH secret for the whole connection; part 5's load shedding first had exactly that bug. Wipe in that branch too, a no-op unless such a path kept a secret, so that no hybrid secret outlives the peer's version packet by construction. A test-only option, V2PQOptions::retain_without_negotiation, retains the secret with the negotiation off to stand in for such a path; production passes default options. v2_pq_version_wipes checks the wipe in both roles and that plain v2 continues. v2_pq_internal_errors now also checks that the keygen failure wipes at once, which the new wipe would otherwise mask until the initiator's version packet arrives. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
96cecd3 to
ac16fb6
Compare
Part 4 now wipes the hybrid secrets whenever the peer's version is processed (#230, L5). pq_shed_wipes_secrets already checks right after the initiator's key, before any version, so a shed branch that doesn't wipe is still caught. With the merged part 4, a variant of the test that checks after the version lets that mutant survive. Make the precondition explicit, so the check can't drift past the version. Follow-up to work item F4 of #206 (#184 part 5). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Summary
Part 4 of #184. Closes #199. Stacked on #225, the ML-KEM library.
This adds the hybrid v2 handshake: the key recipe, the record parser, negotiation inside
V2Transportwith key confirmation, the protocol spec, and a malicious-peer fuzzer. Production construction stays PQ-off, so nothing changes for a running node; the-v2pqtransportoption comes in part 6.bip324derives hybrid session keys with HKDF over an ordered transcript, mixing the ML-KEM-1024 and ECDH secrets. The salt isqbit_v2_hybrid_mlkem1024plus the network magic. Test vectors are included.doc/design/pq-transport.mddescribes the wire format (CompactSize(len)‖header‖payloadrecords in the version packet, header0xF0), message flow, key schedule, key confirmation, failure handling and status values.PQHandshakeparses the whole version-contents grammar before acting, holds the per-connection keys, generates keys lazily, and wipes secrets on every exit path. It has apq_recordsfuzz target with seeds in the required corpus.V2Transport.pendinguntil the peer's confirmation verifies, thenhybrid.off); any other failure closes the connection and reportspendinguntil the peer is removed.p2p_v2_pq_malicious_peerfuzz target. A stateful attacker completes ECDH honestly, then sends any authenticated negotiation bytes: malformed lengths, unknown and duplicate records, damaged keys and ciphertexts, decoys around the switch, partial writes and disconnects. It runs against both roles and all modes, and checks that output stays append-only,morepredictions hold, keys agree, and nothing is delivered before confirmation. Its seeds reach confirmation handling and are in the required corpus.Testing
test_qbit,qbitdand the fuzz binary, with every commit built and tested on its own.bip324_tests,net_testsandmlkem_tests, including the transport-level malformed-record cases (both roles, with KEM call counting and wipe checks) and the internal-error cases;bip324_testsandnet_testsare clean, and a two-thread send/receive stress harness ran 12,000 times with no TSan report and nomoreviolation;-Wthread-safety.qbitdandtest_qbitkeep their GNU property notes (IBT, SHSTK, x86-64-baseline) before and after.Two independent adversarial reviews, of E2 and of E3, each accepted the code with no blocking defect. They walked every row of #199's state table, diffed an 8.2 MB PQ-off transcript byte for byte, and ran a 20,000-iteration malicious peer. All their findings and nits are fixed here.
Target Branch
1.x.x, stacked on crypto: vendor mlkem-native and build ML-KEM-1024 with runtime backend selection #225.Risk / Review Notes
Notes: P2P transport crypto, off in production until part 6 adds the option and part 9 turns it on. No consensus change. With PQ off, the transport's bytes are unchanged; the PQ-off transcript diff is byte-identical.
Docs / Process Impact
doc/design/pq-transport.mdanddoc/development/fuzzing.md.Closes #199 · Part of #184
🤖 Generated with Claude Code
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.