Phase 1: nonce/replay correctness + unauthenticated-command guard#126
Closed
davelee98 wants to merge 12 commits into
Closed
Phase 1: nonce/replay correctness + unauthenticated-command guard#126davelee98 wants to merge 12 commits into
davelee98 wants to merge 12 commits into
Conversation
Distinct plan for Phase 1 of the freeze-proofing effort, with all five decisions resolved: - A: forward cap 64, derived from PIPE_MAX_W + MAX_PTO = 35 (the client's credit limit), not the parent plan's undrived 4*PIPE_MAX_W = 128. Forward width is the [M1] jam-forward DoS magnitude, so tight is correct. - B: sliding bitmap (uint64_t[2], 16 B) replacing the 512 B value ring, IPsec/DTLS shifting style rather than RFC 6479/WireGuard. Net struct change -496 B. Defects D3 and D4 cease to exist rather than being patched. - C: delete verifyNonceReplay(); nonceCheck/nonceCommit are file-static so linkage enforces that only decryptCommand may commit session state. - D: standalone tools/test_nonce_window.cpp against a new dependency-free src/nonce_window.h, under UBSan/ASan, wired into CI. - E: no wire change; recorded for a future protocol revision only. No code changes yet.
Adversarial review of the Phase 1 plan (1 Critical, 3 High, 3 Medium, 7 Low; verdict: safe to ship alone, but not as written). All 14 findings addressed: - C1: forward cap 64 -> 128. The 'PIPE_MAX_W + MAX_PTO = 35 hard bound' derivation was wrong -- selective repair (device.py:2789) spends no window credit and the client hoards ACKs (drain_stale=False), reaching ~96 at blocks_per_ack=1, which is user-settable in HA. Firmware cannot bound this from its own constants; the cap is now documented as a heuristic. - H1: new Step 4b -- stop answering a nonce-rejected pipe frame with a fatal NACK. The client raises IntegrityCheckError on the first one, so the transfer died regardless of cap width. This is a conformance fix: pipe-write-protocol section 5.2 already reserves NACKs for unrecoverable conditions, 'not ordinary packet loss'. - H2: device and client share one nonce space (same key, same session_id, no direction bit, both counters from 0) -> CCM keystream reuse. Shipping defect since b04a22b (2026-03-10), present in all four firmware repos; the canonical header never specified the nonce layout. Recorded, deferred to a protocol revision. - H3: D3 is worse than stated -- the exempted re-accept writes the ring, so 64 replays flush it and unlock the last 32 counters. - M1: unsigned wrapping deltas; the signed form is UB on attacker-controlled counters parsed before tag verification. - M2/M3/L1-L7: jam-forward argument withdrawn, resetNonceState field list, D4 demoted to latent, commit placement, storage table, CI job, line refs. Also flags that D1's mechanism narrative ('MAX_PTO=3 produces exactly 3 rejections') does not survive L4+H1 -- integrity_failures plausibly reaches 1, not 3, so Step 4b may be the highest-value change in Phase 1. Added a baseline hardware test to settle it before trusting the before/after story. Includes the parent plan's new 'NO wire protocol changes' hard constraint, and a compliance table showing every Phase 1 change is in bounds.
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.
[L5] of docs/PLAN_PHASE1_NONCE_REPLAY_2026-07-26.md. A top-level job, NOT a step inside the existing `build` job - that is an 11-entry matrix and would run the host test eleven times. Needs no toolchain beyond the runner's stock g++. -fsanitize=undefined,address is the point of the gate, not decoration: it is what catches the `x << 64` UB in the bitmap shift automatically rather than relying on the test author to predict it. tools/ is invisible to every firmware build (build_src_filter is relative to src_dir and no env adds tools/), so the test file cannot perturb the matrix.
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.
…l NACK Regression from Phase 1, reported from hardware: endless 'Decryption failed' + 00 71 FF on every write, with no escalation and no recovery. Diagnosis from the device log: - len=232 = 2 + CHUNK_SIZE(230), the *unencrypted* chunk budget. An encrypted 0x0071 frame cannot exceed 185 B (cmd2+nonce16+len1+data154+tag12). So the client had dropped its session state mid-connection and fallen back to plaintext uploads. - The all-0x11 'nonce' is therefore image payload sitting where the nonce would be, so the session-id check fails -> NONCE_BAD_SESSION. - Repeating forever with no session clear rules out tag failures, which still increment integrity_failures and clear at 3. Root cause: [L7] routed NONCE_BAD_SESSION to 'does not touch integrity_failures' alongside the packet-loss cases. Refusing to treat it as tamper evidence was right, but three mismatches previously cleared the session, after which every command answered 0xFE -- and THAT is what made the client re-authenticate. Phase 1 removed the escalation without providing a resync path, so the device NACKed a desynced client forever. Retrying can never fix a session-state disagreement; only re-authentication can. Fix: answer NONCE_BAD_SESSION with RESP_AUTH_REQUIRED, which states exactly that condition. The client raises AuthenticationRequiredError and re-authenticates; the mismatch clears in one round trip instead of three strikes plus a session teardown. No session state destroyed, so a peer cannot force a teardown by spraying bad session ids. In bounds under the parent plan's NO-wire-changes constraint: an existing RESP_* code used in its documented meaning, same 3-byte shape already sent by the isAuthenticated() gate above. Underlying trigger is unaddressed and belongs to Phase 5: nothing clears encryptionSession on BLE disconnect, so a session outlives the connection that created it. All 12 envs build; host test unaffected.
…mmands The encryption gate answers every gated frame sent without a usable session with RESP_AUTH_REQUIRED and has no counter of its own, so a client that ignores 0xFE drives one response plus one unrate-limited error line per frame, indefinitely. Seen in the field: a client took 00 70 FE for a DIRECT_WRITE_START and streamed its 0x0071 image data anyway, ~100 ms apart, until it ran out of image. Count consecutive 0xFE answers and, at ten, drop the link, forcing a fresh connect + CMD_AUTHENTICATE handshake instead of letting the client spin. Ten, not three: a legitimate client may probe several gated commands before it authenticates (the trace above shows 0x0040, 0x0044, 0x0070 first), and ten matches the existing 10-per-60s cap on CMD_AUTHENTICATE, which covers the same confused-client case. It is a count, not a rate. Consecutive: any frame that clears the gate resets it, as does a successful handshake. BLE only -- encryptionSession is one global shared with LAN, so counting a LAN peer's rejections would let it cost an unrelated BLE client its link, the same cross-transport bleed auth_attempts already has. The drop is deferred to loop() and runs after the response queue drains: the counter is bumped on the NimBLE host task / in a SoftDevice callback, where tearing down a link from inside the stack's own write callback risks a use-after-free, and dropping before the flush would strand the final 0xFE. The offending handle is captured when the drop is armed and re-verified before disconnecting, so a peer that leaves and is replaced in between does not cost the innocent client its link. The handle is read from the stack (Bluefruit.connHandle() / NimBLE getPeerInfo) rather than mirrored from the connect callbacks, so no per-target callback plumbing is needed. Guard declarations live in communication.h, which stays free of any BLE-stack include; ble_init.h pulls in NimBLEDevice.h on ESP32 and cannot be included from shared code. Builds clean on all twelve environments.
…nsfer
The guard tripped but never dropped anything on nRF while a client flooded
gated frames inside a direct write. Confirmed on hardware: the threshold's
"drop pending" line printed, and then NEITHER the drop nor its skip
diagnostic appeared until the flood stopped. loop() was not being scheduled
at all, so the deferred drop could not land in the window the guard exists to
close.
On nRF the dispatch IS the Bluefruit event-task write callback, and that task
runs at a higher priority than the Arduino loop task. Deferring to loop() is
therefore the wrong strategy there, so do the disconnect inline.
Safe on nRF, and specifically not on ESP32:
- The 0xFE has already gone out. nRF notifies from sendResponseUnencrypted
directly (imageCharacteristic.notify) with no response ring to drain, so
nothing is left to strand. ESP32 queues the reply, which is why that path
still defers until after flushResponseQueueToBle().
- Bluefruit.disconnect() only asks the SoftDevice to tear the link down; the
disconnect event arrives afterwards on the same event task rather than
unwinding the callback we are inside.
The inline call consumes the pending flag, so loop()'s call is a no-op on
nRF; it stays for ESP32 and as a fallback for anything arming the guard from
outside a write callback. All existing safety checks still apply -- it is the
same serviceBleAuthAbuseDisconnect().
Also make the no-live-link early return log instead of returning silently.
That silence is what made this indistinguishable from the guard not running,
and it cost a diagnostic round trip. The line reports both the armed handle
and what the stack says now, so "peer already left" (benign) and "stack never
reported a live link" (not benign) can be told apart without a rebuild.
Verified on nRF hardware: the link now drops mid-burst. ESP32 unchanged and
untested on hardware. Builds clean on all twelve environments.
Contributor
Author
|
Superseded by #127, which carries the same nine code commits rebased onto |
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.
Phase 1 of the freeze-proofing work: nonce/replay correctness on the BLE session layer, plus a guard against a client that floods gated commands into a dead session.
Nonce / replay
src/nonce_window.h). Same coverage, 16x less RAM, and it no longer degrades into false rejections when frames arrive out of order.nonceCheck()is pure and writes nothing to the session; only a frame that passes the CCM tag advances replay state. Previously an unverified frame could move the window.integrity_failures. Only a CCM tag failure is tamper evidence. On a lossy link the old behaviour cleared the session after three dropped frames.0x0081pipe frame is answered with silence, not a fatal NACK. The pipe spec reserves NACKs for unrecoverable conditions and makes an0x0081NACK unconditionally fatal; answering one for ordinary loss killed the whole upload on the first dropped frame. The seq simply falls out of the next SACK mask and the client retransmits.RESP_AUTH_REQUIRED, not a fatal NACK. A stale client and a device that re-authenticated cannot resolve the disagreement by retrying — only by re-authenticating. Previously the device answered0xFFforever while the client fell back to plaintext uploads.Host test for the window state machine (
tools/test_nonce_window.cpp) plus a separate CI job to run it.Unauthenticated-command guard
The encryption gate answers every gated frame sent without a usable session with
RESP_AUTH_REQUIREDand had no counter of its own, so a client that ignores0xFEdrove one response plus one unrate-limited error line per frame, indefinitely. Observed in the field: a client took00 70 FEfor aDIRECT_WRITE_STARTand streamed its0x0071image data anyway, ~100 ms apart, until it ran out of image.Ten consecutive
0xFEanswers now drop the BLE link, forcing a fresh connect +CMD_AUTHENTICATEhandshake.0x0040,0x0044,0x0070first), and ten matches the existing 10-per-60s cap onCMD_AUTHENTICATE. It is a count, not a rate.encryptionSessionis one global shared with LAN, so counting a LAN peer's rejections would let it cost an unrelated BLE client its link.loop()and runs after the response queue drains, so the final0xFEis not stranded. On nRF the dispatch is the Bluefruit event-task write callback, which outranks the Arduino loop task — measured on hardware,loop()is not scheduled at all while a client floods frames mid-transfer, so the drop happens inline there. nRF notifies responses directly with no ring to drain, andBluefruit.disconnect()only asks the SoftDevice to tear down, so the callback is not unwound underneath us.Testing
Note
PR #84 (
balloob:fix/aes-ccm-nonce-direction-and-replay) touches the same subsystem. These will need to be reconciled — this branch does not include the nonce direction-bit change.