usb: ncry v3 - #759
Conversation
|
client part Coldcard/ckcc-protocol#63 |
|
Adversarial cross-repository review: ncry v3 (firmware #759 × ckcc-protocol #63)Reviewed heads (fetched live):
Method: both full diffs read line-by-line; an independent reference implementation written from the documented construction only (stdlib Findings (severity-ordered)Medium — v3 client does not go terminal on transport failures; stale authentic responses get misattributed to later commands (also flagged by another reviewer; reproduced and confirmed here)
Low 1 — TX-sequence exhaustion kills the firmware USB task via unhandled exception — Low 2 — Doc example import path doesn't exist in the published package — Low 3 — Ambiguous HMAC argument-order notation — Low 4 — Client has no cap on accumulated HID response length — Cross-repo byte-level agreement (all verified by execution)
Compatibility matrix (each cell executed)
Cryptographic assessmentPer-session/direction keys make counter-0 CTR reset safe; fresh ECDH both sides gives session-key uniqueness; transcript binding blocks UKS/reflection/cross-version confusion; Endpoint-authentication boundaryDocs are accurate: v3 does not authenticate the endpoint, and Tests run
VerdictCompatible; merge after fixing the Medium transport-failure latching bug (small client-side change + regression test; follow-up branch on ckcc-protocol), and consider the four Low items. The core protocol — KDF, key mapping, wire format, sequence semantics, terminal behavior on auth/framing failures, cross-repo byte agreement, and v1/v2 compatibility — is fully verified and sound. |
|
Codex: I reviewed this specifically for compatibility with deployed protocol clients, including older Compatibility verdictI found no backward-compatibility break for deployed ncry v1/v2 clients. The legacy AES-CTR wire format, 2060-byte message limit, v1 mixed plaintext/encrypted traffic, v1 rekeying, and v2 bound behavior remain intact. I also built the PR simulator and directly tested the exact pre-PR ckcc 1.5.0 client:
Released-client audit:
Findings to address1. V3 client state is not poisoned after transport failures
I reproduced this with a simulated HID write failure: For v3, mark the session failed after any transport/framing exception occurring after request encryption, until a complete authenticated response has been received. Application-level 2. Update the ckcc-protocol submodule pointerFirmware currently pins 3. Legacy CI does not use a legacy clientThe new v1/v2 tests instantiate the updated ckcc client. They test legacy mode in new code, but do not lock compatibility with the released ckcc 1.5.0 implementation. Please add an exact 1.5.0 fixture or frozen interoperability coverage for v1, v2, v1 plaintext mixing, v1 rekeying, and the 2060-byte boundary. 4. Documentation imports the wrong package nameThe new example imports The public example should use: Overall: the legacy compatibility story looks sound, but I recommend resolving the v3 transport-failure handling and submodule pin before merge. — Codex |
|
Review feedback implemented. All findings from the two reviews above (cross-repo review, Codex compatibility review) are now addressed, split across the two repos as they belong: This PR — commit
Client side — ckcc-protocol PR #64:
Not addressed, deliberately: the suggestion to add a frozen ckcc 1.5.0 client fixture to CI — hardening rather than a defect, given the real 1.5.0 client was already validated against this PR's simulator manually. Happy to add it if you'd like it locked in. One follow-up after merge order: once ckcc-protocol#64 lands, I'll bump the submodule pin here once more so it carries those client fixes (one-line change). |
Submodule now points to |
6821a41 to
7fa3ebd
Compare
3a596e6 to
8ae7a65
Compare
Implements opt-in USB ncry v3 authenticated encryption. V3 derives independent AES-CTR and HMAC-SHA256 keys for each direction and authenticates every frame over its direction, sequence number, length, and ciphertext. This rejects tampering, replay, reordering, and cross-direction reflection; authentication or framing failures terminate the session.
The default remains ncry v1, with v1/v2 behavior covered by regression tests. V3 protects the encrypted channel but does not authenticate the COLDCARD endpoint, so clients requiring endpoint authentication must use
check_mitm()with a previously trusted xpub.Includes matching ckcc-protocol
1.6.0support, fixed test vectors, and protocol documentation. First supported in Mk4/Mk5 firmware5.6.1and Q firmware1.5.1Q.