fix(nonce): sliding-window replay protection; stop NACKing ordinary packet loss - #136
Open
davelee98 wants to merge 8 commits into
Open
fix(nonce): sliding-window replay protection; stop NACKing ordinary packet loss#136davelee98 wants to merge 8 commits into
davelee98 wants to merge 8 commits into
Conversation
Step 1 of docs/PLAN_PHASE1_NONCE_REPLAY_2026-07-26.md.
New dependency-free src/nonce_window.h holds the whole window state machine as
static inline functions over plain values: no Arduino, no mbedtls, no millis(),
no logging, no session. That is what tools/test_nonce_window.cpp targets
(Decision D), and it keeps nonceCheck/nonceCommit file-static in encryption.cpp
(Decision C) at no cost to testability.
Representation is IPsec/DTLS shifting style (RFC 4303 / RFC 6347), not the
circular RFC 6479 / WireGuard form (Decision B): bit i == "counter
(last_seen - i) consumed", bit 0 == last_seen. Eviction and falling out of
window become the same event, so the hand-maintained D >= 2W coupling between
constants in two files ceases to exist.
That representation deletes two defects outright rather than patching them:
D3 - no reserved sentinel, so "not seen" is a clear bit and the counter_diff
!= 0 exemption that made the highest-seen frame replayable is gone.
[H3]: that exemption also flushed the ring, unlocking the last 32
genuine counters, not just the last one.
D4 - a bitmap has no insertion point, so the function-static
replay_window_index cannot recur.
OD_NONCE_BACKWARD_BITS 256 (uint64_t[4], 32 B) is kept strictly greater than
OD_NONCE_FORWARD_CAP 128 so a legal forward slide can never exceed the bitmap
width, keeping the wholesale-clear branch off the normal path.
Cap is 128, not 64 [C1]: the old derivation (PIPE_MAX_W + MAX_PTO = 35, asserted
as a hard bound) missed the client's selective-repair transmit site, which spends
no window credit; the reachable gap is ~96 at blocks_per_ack = 1. Under a bitmap
the cap is a comparison, not storage, so the width is free.
encryptionSession shrinks by exactly 480 B (verified: sizeof 0x118 on esp32-N4).
…pering Steps 2-4 of docs/PLAN_PHASE1_NONCE_REPLAY_2026-07-26.md. verifyNonceReplay() is deleted outright (Decision C - no compatibility wrapper), along with its declarations in encryption.h and the duplicate in main.h. D1 - a nonce rejection no longer touches integrity_failures. The rule in one line: only a CCM tag failure is evidence of tampering; a nonce failure is evidence of a lossy link. Previously a lost window put the next frame out of range and each such frame counted toward session destruction, after which the device answered RESP_AUTH_REQUIRED to everything until reconnect. D2 - nonceCheck() is pure. It writes nothing to encryptionSession on any path, so replay state is advanced only by nonceCommit(), which runs as the FIRST statement of decryptCommand's success arm - after aes_ccm_decrypt. Placement is load-bearing [L2]: there is an early return in that arm for a decrypted-but-malformed payload_length, and that frame is authentic (it passed the tag). Today's unconditional commit does record it; committing after the early return would silently leave an authentic frame replayable. M1 - unsigned wrapping deltas only, no signed arithmetic. The 8 counter bytes are parsed off the wire before the tag is verified, so an unauthenticated attacker controls both operands: (int64_t)counter for >= 2^63 is implementation-defined pre-C++20, the subtraction can overflow, and negating INT64_MIN is UB. Unsigned overflow is defined as modular arithmetic, making the expression total over all 2^64 inputs. The four tests in od_nonce_check are ordered and the order is load-bearing - fwd and back are complements mod 2^64 and cannot both be small. M3 - resetNonceState() names all four shared fields explicitly. The two callers share only these four and are opposite on everything else, so a helper described loosely as "the bitmap and last_seen_counter" would invite dropping nonce_counter = 0 - which would carry the device's outbound counter across a re-auth while the client restarts at 0, walking into the [H2] keystream reuse against itself. L7 - both nonce-rejection logs demoted from ERROR to WARN and rate-limited to one per 5 s (one shared budget, so alternating between them cannot bypass it). The out-of-window log now fires routinely on a lossy link, and with counting removed nothing else throttles a peer driving the session-id line, which also no longer dumps two full session IDs. Routing NONCE_BAD_SESSION to "integrity_failures untouched" is a deliberate policy change, not a consequence of D1: a mismatched session id is usually a stale client talking to a device that re-authenticated. decryptCommand gains a NonceResult* reason out-param (internal signature only, nothing on the wire) so its single caller can distinguish nonce rejection from tag failure. Step 4b uses it.
Step 4b / [H1] of docs/PLAN_PHASE1_NONCE_REPLAY_2026-07-26.md. Without this, Phase 1 saves the device but still loses the transfer. Every decryptCommand failure - nonce and tag alike - produced the same unencrypted 3-byte RESP_NACK, and the client turns that shape into IntegrityCheckError before it ever reaches pipe-frame classification (device.py:833-838). The pipe send loop's only except is BLETimeoutError, so one out-of-window frame aborts the whole upload - and the frame the client would have repaired is the one whose NACK killed the transfer. No forward cap is wide enough to fix that. Now: NONCE_OUT_OF_WINDOW / NONCE_REPLAY on CMD_PIPE_WRITE_DATA sends NOTHING. Silence is already a first-class signal on the pipe path - it means "lost", the seq is absent from the next SACK mask, the client retransmits, and the transfer continues. This is a conformance fix, not a protocol change. pipe-write-protocol.md 5.2 already reserves NACKs for unrecoverable conditions, "not ordinary packet loss", and 5.1 makes an 0x81 NACK unconditionally fatal - so today's firmware violates the pipe spec as written. No opcode, response code, or envelope changes; no canonical-header edit. Deliberately narrow: tag failures keep the NACK (tamper evidence, not loss), and 0x0071 legacy DIRECT_WRITE_DATA is left alone - different ACK discipline, not analysed here, and the field failure lives on the pipe path. Step 6 - the stale comment at the 0x0081 case rewritten. Per [C1] it now states the MECHANISM rather than a number: the forward gap is bounded by the client's retransmit budget max_retx = max(3*W, n/2) and by blocks_per_ack, which live in another repo and one of which is a user-facing Home Assistant option, so OD_NONCE_FORWARD_CAP is a heuristic with headroom, not an invariant firmware can prove. A number written here would be falsified silently by a client-side config change.
This was referenced Aug 1, 2026
Step 5 / Decision D of docs/PLAN_PHASE1_NONCE_REPLAY_2026-07-26.md. Standalone,
no framework, no PlatformIO env and no test/ directory - so the 11-env matrix
and a bare `pio run` are untouched. Includes ONLY src/nonce_window.h.
g++ -std=c++17 -Wall -Wextra -Werror -O1 -fsanitize=undefined,address \
tools/test_nonce_window.cpp -o /tmp/test_nonce_window && /tmp/test_nonce_window
-> PASSED 38199 checks
Coverage:
- Purity of od_nonce_check (D2) - the single most valuable assertion here.
Snapshot the state, call every result class, memcmp byte-for-byte after.
"The tag is the only thing that may advance replay state" rests on it.
- D3 at fwd == 0: re-presenting a committed counter is REPLAY, including the
case today's firmware exempts.
- Fresh session: counter 0 accepted exactly once from a virgin state, with no
has_seen_counter sentinel anywhere.
- Shift edges: fwd = 0, 1, 63, 64, 65, 127, 128 through od_nonce_check (the
reachable range), plus 129, 191, 192, 255, 256, 257 driven directly against
od_nonce_commit for the word boundaries and the wholesale-clear guard.
- Wholesale slide asserts the EXACT enum: previously-seen counters come back
OUT_OF_WINDOW, not REPLAY. Both reject, so conflating them would be
invisible in behaviour and would hide a genuine slide bug.
- Bit-index invariant stated directly: the bit denoting (L - i) must sit at
i + d under L' = L + d.
- [M1] counter arithmetic at counter = 2^63 / last_seen = 1 and around
UINT64_MAX including the wrap - the inputs that make the signed form UB.
UBSan makes this self-checking.
- Differential test against a std::set oracle, fixed mt19937_64 seed, with a
full backward-window sweep at the end of each sequence. The oracle prunes
counters that fall out of the window so it agrees on REPLAY vs
OUT_OF_WINDOW, not merely on accept vs reject.
__ubsan_on_report is overridden so a UBSan report exits nonzero: UBSan defaults
to print-and-continue, which would let a reintroduced signed-arithmetic
regression pass CI with a runtime-error line nobody reads.
Mutation-checked: breaking the backward bit test, the cap comparison, the
fwd == 0 bit test, or the cross-word shift carry all fail the test.
…g, honest comments Findings from an independent adversarial review of the Phase 1 diff against docs/PLAN_PHASE1_NONCE_REPLAY_2026-07-26.md. No behaviour change to the window state machine; all 38199 host-test checks and all 11 firmware envs still pass. 1. One rate-limit budget PER LOG SITE, not one shared. A stale client spamming session-id mismatches could otherwise silence the out-of-window line for 5 s at a time - and out-of-window is precisely the condition Step 5's hardware tests 0-2 exist to observe. The shared budget would have masked the measurement the plan depends on. 2. The rejection log now prints `back` for a replay and `fwd` for out-of-window. A replayed counter is normally BEHIND last_seen, where the (correctly) wrapping fwd delta printed as a 20-digit number - unreadable in the case the line fires in most. 3. Decision C's "linkage enforces that only decryptCommand may commit state" is softened to what is actually true. nonceCheck/nonceCommit are file-static, so it holds for them - but encryption_state.h must include nonce_window.h for OD_NONCE_BITMAP_WORDS and main.h includes encryption_state.h, so the raw od_nonce_commit() primitive is visible in every TU next to the extern encryptionSession. Unavoidable while the struct needs the width macro; recorded rather than left as a claim the code does not support. 4. od_nonce_commit's totality comment now states its sharp edge instead of implying it away: a counter more than OD_NONCE_BACKWARD_BITS *behind* last_seen also lands in the forward branch (fwd and back are complements), so it clears the bitmap and REWINDS last_seen, un-seeing everything. Unreachable through od_nonce_check - which returns OUT_OF_WINDOW so it is never committed - but it is the edge to watch if a caller ever commits without checking. 5. nonce_window.h includes <stdbool.h>, so its "zero dependencies" claim also holds for a C translation unit.
Two comments claimed the silent drop was repaired by the normal SACK path. That holds for a rejection inside the window; it is false past OD_NONCE_FORWARD_CAP. Once a frame is rejected for fwd > cap, nothing commits, so last_seen never advances. Every retransmission re-encrypts with a fresh, higher counter (py-opendisplay _write_pipe_frame never resends the original ciphertext), so each one is rejected at a greater distance than the last. The session cannot recover without re-authenticating, and the transfer stalls until the stuck-transfer timeout releases the panel. The cap's job is to put that state out of reach, not to make it recoverable. Dropping silently still beats a 0x81 NACK, which is unconditionally fatal and kills the upload immediately -- but it does not rescue the transfer, and the comments should not have implied it did. Comments only; no behaviour change. nrf52840custom builds.
OD_NONCE_FORWARD_CAP turned a transient link fault into a permanent session fault. A counter more than 128 ahead was rejected, and because nothing commits until the CCM tag verifies, last_seen never advanced. The client re-encrypts every retransmission with a fresh, higher counter and never resends the original ciphertext, so each subsequent frame was rejected at a greater distance than the last. The session could not recover without re-authenticating; the transfer stalled until the 15-minute watchdog released the panel. It is reachable with supported settings. With W=32 and blocks_per_ack=1, 16 queued gap-ACKs at PIPE_RETX_ACK_SPACING=2 burn 128 counters on repairs alone, and the gap accumulates across aborted attempts because the client deliberately does not re-authenticate mid-transfer. max_retx = max(3*W, n/2) is order thousands for a full-panel upload, so the budget is nowhere near spent when the cap is crossed. The cap bought nothing. All 8 counter bytes sit inside the CCM nonce, so a tampered counter fails the tag; commit runs only on the success arm; and passing the check mutates nothing. An attacker who cannot forge a tag could not advance last_seen at any distance, with or without a cap -- and the session id is cleartext in every frame, so anyone who could flood CCM with a capped window could flood it without one. So: no forward bound. A counter ahead of last_seen is accepted at any distance and gated by the tag, exactly as RFC 4303 Appendix A2 does. Comparison also moves from modular to numeric. Modular arithmetic made a counter far behind indistinguishable from one far ahead, which is what let an ancient counter present as an enormous forward jump -- the "sharp edge" the header admitted, where committing one would rewind last_seen and clear the bitmap. That is now impossible by construction rather than by the caller's contract. For the same reason, do not reintroduce a bound as cap=UINT64_MAX or as "not-backward implies forward": either restores the overlap. Consequences: - Counters no longer wrap. Reaching UINT64_MAX requires re-authentication, per RFC 4303 3.3.3; wrapping would reuse a (key, nonce) pair, which is the exact failure this file prevents. 2^64 counters in one session is unreachable. - NONCE_OUT_OF_WINDOW now means only "too far behind", so the rejection log computes direction from the counters instead of inferring it from the reason, which would have printed an underflowed 20-digit distance. - OD_NONCE_BACKWARD_BITS keeps its value but not its old justification, which was stated in terms of the cap. It is now purely out-of-order tolerance, and its exact value is not load-bearing: a backward rejection is self-healing, because the retransmit carries a higher counter that is accepted unconditionally. Tests: the oracle no longer prunes its seen set -- that pruning encoded the implementation's forgetting, so it could only ever agree with it. It now keeps every counter ever committed and states the property directly: a consumed counter is never returned NONCE_OK, at any distance, by any route. Added the cliff as a sequence (not a point, which cap=UINT64_MAX would also pass), the escalating-retransmit pattern, and far-behind-is-never-forward, which pins both forbidden shortcuts. Shift edges now run through od_nonce_check, so the wholesale-clear path is exercised the way production reaches it. Verified: 47445 checks pass under -Werror with ASan+UBSan; the same suite fails 1635 checks against the pre-change implementation. nrf52840custom and esp32-c3-N16 build. No wire change: nonce format, response codes and framing are untouched, and the accept set only grows, so no peer needs updating in lockstep. # Conflicts: # .github/workflows/main.yaml
Every 0x81 NACK is terminal: it sets pipeState.error, releases the panel, and makes the client raise. None of that left a trace in the device log, so a failed upload could not be told apart from a stall or a link drop without a sniffer or the client's traceback. Logged before the send, at ERROR, with the state a diagnosis needs: the error code, expected_seq, highest_seen, reorder-queue occupancy and the negotiated window. It cannot flood -- pipeState.error is set immediately after and makes every later 0x0081 frame discard, so at most one fires per session. The comment singles out err 0x04. A conforming client cannot produce it: it only transmits seq within W of its own window_base (all four transmit sites in py-opendisplay's _pipe_stream are bounded by that rule), and the device's expected_seq is provably within W of that same base, so fwd < W or back <= W always holds and the "out of window on both sides" arm is unreachable. Client and device agree on W because the device advertises PIPE_MAX_W in the START ACK and both apply the same min-rule -- including on the PIPE_SMALL_DRAM_WINDOW envs where PIPE_MAX_W is 16 rather than 32. That proof rests on a window rule enforced in another repo and on nothing in this firmware, which is exactly why the line is worth having: if the assumption ever drifts, this is the only thing that would say so. nrf52840custom, esp32-c3-N16 and esp32-N4 build.
davelee98
force-pushed
the
fix/nonce-replay-window
branch
from
August 2, 2026 11:53
19335e6 to
e83251e
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.
Nonce/replay rework for the encrypted BLE command path. 8 commits, independent of the freeze-hardening work in #135.
The field-visible fix
A pipe DATA frame (
0x0081) rejected for a nonce reason is now answered with silence instead of a fatal0x81NACK.Nonce rejection on that path is ordinary packet loss — it is the direct consequence of frames having been dropped — and
pipe-write-protocol.md§5.2 already reserves NACKs for unrecoverable conditions, with §5.1 making a0x81NACK unconditionally fatal. Answering one violated the spec as written and made py-opendisplay raiseIntegrityCheckErrorfrom a path its pipe send loop does not catch, killing the entire upload on the first rejected frame. Silence is a first-class signal here: the seq is absent from the next SACK mask, the client retransmits under a fresh higher counter, and that counter is accepted unconditionally.Deliberately narrow — tag failures keep the NACK (tamper evidence, not loss), and legacy
0x0071is left alone because its ACK discipline differs and hasn't been analysed.The rest
integrity_failures. Only a CCM tag failure is a tamper oracle; counting packet loss there was the D1 defect.Verification
tools/test_nonce_window.cpp, built-Werrorunder ASan+UBSan. UBSan is load-bearing, not decoration: it catches thex << 64shift UB automatically rather than relying on the test author to predict it.nrf52840custombuilds clean.include/opendisplay_protocol.huntouched.