ncry v3: latch session failure on transport errors, cap response length - #64
Merged
Merged
Conversation
- send_recv: any exception after encrypt_request (USB write failure, read timeout, over-long response) now latches _v3_failed for v3 sessions. Previously a caller catching the error could reuse the object and receive a stale (authentic, sequence-valid) response misattributed to the next command, contradicting the documented guarantee that any v3 failure is terminal. - send_recv: abort with CCFramingError once the accumulated response exceeds USB_V3_MAX_WIRE_MSG_LEN (v3) / MAX_MSG_LEN (legacy) instead of buffering unbounded data from a malfunctioning device. - PROTOCOL.md: state HMAC key/message argument order and HKDF key layout explicitly; document that transport failures are terminal. - tests: regression coverage for timeout/write-failure latching, stale-response rejection, and overlong responses.
Locks in the boundary of the transport-failure latching: a complete, validly-tagged b'err_' reply raises CCProtoError but leaves the v3 session usable (streams stay synchronized), per review triage.
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
2 participants
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.
Follow-up fixes from the cross-repository review of the ncry v3 protocol change (firmware PR Coldcard/firmware#759 x ckcc-protocol PR #63), incl. findings confirmed by two independent reviewers.
Changes
send_recv: latch_v3_failedon any post-encryption transport failure (Medium). A failed HID write, disconnect, or response timeout previously escaped as a plainAssertionErrorwithout poisoning the v3 session. Sinceencrypt_requestalready advancedtx_seqand the CTR stream, a caller catching the error and reusing the object could receive a stale — but authentic, sequence-valid — response misattributed to the next command, contradicting the documented "any v3 failure is terminal" guarantee. Now any exception after request encryption latches_v3_failed. Application-level error replies (err_/CCProtoError) deliberately do not poison the session: they arrive as complete, validly-tagged responses and both sides' streams stay synchronized. Verified live against the firmware simulator. v1/v2 behavior unchanged.send_recv: cap accumulated response length (Low). Abort withCCFramingErroronce the reassembled response exceedsUSB_V3_MAX_WIRE_MSG_LEN(v3) /MAX_MSG_LEN(legacy), instead of buffering unbounded data from a malfunctioning or malicious device.PROTOCOL.md: state HMAC key/message argument order and the HKDF key layout explicitly; document that transport failures are terminal.Verification
ncry_tests.py) run against this client: 12/12