diff --git a/.github/workflows/main.yaml b/.github/workflows/main.yaml index 4c8d889..d3b1cb2 100644 --- a/.github/workflows/main.yaml +++ b/.github/workflows/main.yaml @@ -5,6 +5,26 @@ on: pull_request: jobs: + # Separate top-level job on purpose ([L5]): the `build` job below is an + # 11-entry matrix, so a step added there would run the host test eleven times. + # This needs no toolchain beyond the runner's stock g++ and gates every push + # alongside the firmware builds. 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 affect the matrix. + host-tests: + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + - name: Build and run the nonce-window host test + # -fsanitize=undefined is 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. See Decision D in + # docs/PLAN_PHASE1_NONCE_REPLAY_2026-07-26.md. + run: | + 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 + build: runs-on: ubuntu-latest strategy: diff --git a/docs/FINDINGS_FREEZE_PROOFING_PLAN_REVIEW_2026-07-26.md b/docs/FINDINGS_FREEZE_PROOFING_PLAN_REVIEW_2026-07-26.md new file mode 100644 index 0000000..b3b85a5 --- /dev/null +++ b/docs/FINDINGS_FREEZE_PROOFING_PLAN_REVIEW_2026-07-26.md @@ -0,0 +1,706 @@ +# Adversarial Review — PLAN_FREEZE_PROOFING_2026-07-26 + +**Date:** 2026-07-26 +**Reviewing:** `docs/PLAN_FREEZE_PROOFING_2026-07-26.md` (branch `debug/ble-hardening`) +**Scope:** `Firmware` repo — nRF52840 (Bluefruit) + ESP32-S3/C6/C3/classic (NimBLE-Arduino 2.5.0) + +> All Critical/High/Medium corrections below have been folded back into +> `PLAN_FREEZE_PROOFING_2026-07-26.md` (revised 2026-07-26), tagged `[C1]`…`[X7]`. +> This document is retained as the rationale record — read it before re-litigating any +> plan decision, especially the phase ordering and the progress-stamp definition. + +--- + +## Verdict + +**Sound in shape, wrong in several load-bearing details — ship only with the Critical +corrections below.** The diagnosis (§Context items 1–5) is accurate and I verified four of +the five wedge mechanisms against source. The phase decomposition is sensible and most of +the platform facts the author claims to have "verified this session" hold up. + +But the plan has **four Critical defects**, two of which mean the headline deliverable +does not work: + +- **The 10-minute supervisor cannot fire in the exact wedge it was designed for**, because + `markSessionProgress()` is stamped on *command dispatch* and *successful notify* — both + of which keep ticking while the device answers every command `RESP_AUTH_REQUIRED`. +- **Phase 4's second-central refusal is a no-op on the wire**: `BLE_ERR_CONN_LIMIT` (0x09) + is not a legal `HCI_Disconnect` reason, so `ble_gap_terminate` is rejected by the + controller. +- **The `pwrmgmLock` 10 s steal is shorter than a legitimate lock hold** and destroys the + lock's mutual exclusion when it fires. +- **Phase 4's refusal path re-enters the shared disconnect-cleanup flag**, which on the two + non-WiFi envs has no owner guard at all. + +None of these require restructuring the plan. All four are surgical. + +--- + +## CRITICAL + +### C1 — Progress stamps on "command dispatch" and "successful notify" make the supervisor blind to the primary wedge + +**Plan element:** Phase 6 — *"Progress stamps (`markSessionProgress()`): command dispatch, +pipe frame accept, successful notify, LAN frame dispatch, refresh completion."* + +**Why it breaks:** The wedge the plan opens with (§Context item 1) is: `integrity_failures +>= 3` → `clearEncryptionSession()` mid-transfer, after which +`src/communication.cpp:664-670` answers **every** command with +`{RESP_ACK, cmd, RESP_AUTH_REQUIRED}` via `sendResponseUnencrypted()`. That is a *command +dispatch* and a *successful notify* on every single client retry. + +**Concrete sequence** (ESP32, W=16 pipe transfer, HA push): + +| t | event | state | +|---|---|---| +| 0 s | `0x0080` START accepted | `directWriteActive=true`, `pipeState.active=true` | +| 41 s | window loss burst; 3 nonce rejections | `encryption.cpp:691-696` → `clearEncryptionSession()` | +| 41 s+ | client retransmits `0x0081` frames | each reaches `imageDataWritten` → **stamp**; each is answered `RESP_AUTH_REQUIRED` → notify → **stamp** | +| 56 s | py-opendisplay hits `MAX_PTO=3 × TIMEOUT_PIPE_DATA_COMPRESSED=5.0 s` (`device.py:465`, `commands.py:93`) and raises `ProtocolError` | client stops sending; **but does not necessarily drop the link** | +| 56 s → ∞ | HA keeps the `Device` object; any subsequent poll/telemetry command re-stamps | `now - g_lastProgressMs` never exceeds 600 000 | + +The wedge predicate `(transferActive() || chunkedWriteState.active || directWriteActive)` +is **true** the whole time (`directWriteActive` is only cleared by +`cleanupDirectWriteState`, `display_service.cpp:2011`). The supervisor therefore *sees* the +wedge and *never times it out*. Phase 6 delivers nothing for the case it was written for. + +The same defect hits Phase 5: `g_lastLinkRxMs` is stamped in `onWrite` +(`esp32_ble_callbacks.h:81`), so a client that keeps writing doomed commands defeats the +5-minute idle disconnect too. With Phase 6 subsuming the 15-min watchdogs (below), the +device ends up with **strictly less** recovery than today. + +**Correction:** `markSessionProgress()` must mean *the state machine advanced*, not *bytes +moved*. Stamp only at: +- `pipeState.expected_seq` actually advancing (inside the in-order accept in + `handlePipeWriteData`, not at frame receipt), +- `directWriteBytesWritten` increasing (`display_service.cpp:2005`), +- `chunkedWriteState.receivedChunks` incrementing (`communication.cpp:565`), +- `partialCtx` byte counter advancing, +- refresh completion (`display_service.cpp:2436`). + +Do **not** stamp on command dispatch, on notify, or on LAN frame dispatch. Add one extra +stamp on `handleAuthenticate` success so a legitimate re-auth resets the clock. + +--- + +### C2 — `pwrmgmLock` 10 s steal is shorter than legitimate holds, and stealing corrupts the lock + +**Plan element:** Phase 0 — *"`pwrmgmLockTake`: 10 s deadline +(`OD_PWRMGM_LOCK_TIMEOUT_MS = 10000`); on expiry log ERROR and force-take (steal)."* + +**Legitimate holds already exceed 10 s.** The lock is held across panel I/O in three +places (`display_service.cpp:439`, `:496`, `:513`), and the bb_epaper busy wait inside +those paths is bounded at **30 000 ms for 3-/4-/7-colour panels**: + +``` +.pio/libdeps/esp32-s3-N16R8/bb_epaper/src/bb_ep.inl:3959-3975 + int iMaxTime = 5000; // B/W + if (pBBEP->iFlags & (BBEP_3COLOR|BBEP_4COLOR|BBEP_7COLOR)) iMaxTime = 30000; +``` + +- `epdSessionForceOffLocked()` (`display_service.cpp:417-433`) calls `bbepSleep(&bbep,1)`, + which for UC81xx 3-/4-colour panels calls `bbepWaitBusy()` (`bb_ep.inl:4122`, `:4129`) → + **up to 30 s under the lock**, plus `delay(50)`. +- `epdSessionAcquire()` (`:437-487`) holds the lock across `bbepWakeUp` + + `bbepSendCMDSequence` + `epdAlignCustomPartialRamMode`; `bbepSetAddrWindow` alone ends in + a `bbepWaitBusy` (`bb_ep.inl:4104`). + +So on any Spectra/ACeP tag, a **normal** `cleanupDirectWriteState(true)` → `epdSessionForceOff()` +can hold `pwrmgmLock` for ~30 s. On nRF the transfer runs on the Bluefruit Callback task +while `epdSessionTick()` runs on loop — the plan's own rationale for the lock existing. +A 10 s steal fires on healthy hardware. + +**What the steal does when it fires** (`display_service.cpp:401-413`): + +```c +static void pwrmgmLockTake(void) { while (__atomic_exchange_n(&pwrmgmLock,1,ACQUIRE)) delay(1); } +static void pwrmgmLockGive(void) { __atomic_store_n(&pwrmgmLock, 0, RELEASE); } +``` + +`pwrmgmLock` is a plain 0/1 flag with no owner field. After a steal there are two threads +that each believe they hold it: + +1. Stealer runs `epdSessionForceOffLocked()` → `bbepSleep` + rail cut while the true holder + is mid-`bbepSendCMDSequence` → **two tasks driving the same SPI bus and CS line**. +2. True holder finishes and calls `pwrmgmLockGive()` → flag = 0 **while the stealer still + holds it**. Mutual exclusion is now gone for every subsequent operation, permanently. + `epdSessionTick()`'s `pwrmgmLockTryTake()` (`:520`) will now succeed mid-transfer and + rail-cut a live push — the precise failure the lock was introduced to prevent. +3. `pwrmgmState` ends up whatever the *loser* wrote last: the holder's `pwrmgmState = + PWR_ACTIVE` (`:467`) lands after the stealer's `PWR_OFF`, leaving the firmware convinced + the panel is powered when the rail is down. `epdSessionTick()` then never re-arms + (`:520` early-returns unless `PWR_WARM`), so the panel stays permanently mis-tracked. + +**Correction:** do not steal. Make `pwrmgmLockTake` return `bool` with a **60 s** deadline +(≥ 2× the worst-case `bbepWaitBusy`), and on expiry **fail the operation** — log ERROR, +return false, let the caller skip its panel work and set a "panel state unknown" flag that +`abortToKnownState` reports. If a forced recovery is genuinely required, add an owner field +(`volatile TaskHandle_t pwrmgmOwner`) and have the stealer set `pwrmgmOwner = self` so the +original holder's `Give` becomes a detectable no-op instead of a silent unlock. + +--- + +### C3 — `BLE_ERR_CONN_LIMIT` is not a legal `HCI_Disconnect` reason; the exclusivity refusal silently no-ops + +**Plan element:** §Key platform facts and Phase 4 — *"refuse via `pServer->disconnect(connInfo, +BLE_ERR_CONN_LIMIT)`"*. + +`NimBLEServer::disconnect(connInfo, reason)` forwards straight to `ble_gap_terminate`: + +``` +.pio/libdeps/esp32-s3-N16R8/NimBLE-Arduino/src/NimBLEServer.cpp:321-332 +bool NimBLEServer::disconnect(uint16_t connHandle, uint8_t reason) const { + int rc = ble_gap_terminate(connHandle, reason); + ... NIMBLE_LOGE("ble_gap_terminate failed: rc=%d ..."); return false; +``` + +The Core Spec restricts the `HCI_Disconnect` Reason parameter to a small allowlist — +`0x05` Authentication Failure, `0x13`/`0x14`/`0x15` (remote/local user or low-resources +termination), `0x1A` Unsupported Remote Feature, `0x29` Pairing with Unit Key Not Supported, +`0x3B` Unacceptable Connection Parameters. **`0x09` (Connection Limit Exceeded) is not in +that set**; the controller answers `Invalid HCI Command Parameters (0x12)`, +`ble_gap_terminate` returns non-zero, and `disconnect()` returns `false` after logging. + +Result: the gatecrasher stays connected. Phase 4's central promise — single-owner +exclusivity, and the "completes the earlier BLE-max-connections task" claim — does not hold, +while the code *looks* like it worked. This one is easy to miss on the bench because the +second central often disconnects on its own. + +**Correction:** use `BLE_ERR_REM_USER_CONN_TERM` (0x13). Check the `bool` return and log +WARN on failure. Add the same fix to the LAN side (`incoming.stop()` needs no reason code, +so LAN is unaffected). + +--- + +### C4 — Refusing the second central re-enters the shared disconnect-cleanup flag, which has no owner guard on `esp32-N4` + +**Plan element:** Phase 4 — *"ESP32 `onConnect`: refuse second central … `disconnect(connInfo, …)`"*. + +Terminating the refused link fires `MyBLEServerCallbacks::onDisconnect` +(`esp32_ble_callbacks.h:57-70`), which is a blind flag-setter with **no conn-handle +discrimination**: + +```c +bleDisconnectCleanupPending = true; +bleRestartAdvertisingPending = true; +``` + +`serviceBleDisconnectCleanup()` (`main.cpp:321-348`) then decides whether to tear down. Its +only protection is the `ownerStillUp` guard — and that guard is inside +`#ifdef OPENDISPLAY_HAS_WIFI` (`main.cpp:328-338`). + +`OPENDISPLAY_HAS_WIFI` is `TARGET_ESP32 && OPENDISPLAY_ENABLE_WIFI` (`wifi_service.h:13-15`). +Expanding `extends` in `platformio.ini`, exactly two of the eleven envs lack it: +`nrf52840custom` (no NimBLE at all) and **`esp32-N4`** (`platformio.ini:284-299`). + +**Concrete failure on `esp32-N4`:** client A is mid-pipe-transfer at chunk 90/300. A phone +running the opendisplay.org web client scans and connects. `onConnect` sees +`getConnectedCount() == 2`, refuses B. B's `onDisconnect` sets the flag. Next loop pass, +`serviceBleDisconnectCleanup` runs with the guard compiled out: + +``` +main.cpp:343 if (directWriteActive) cleanupDirectWriteState(true); // panel torn down +main.cpp:346 cleanupPartialWriteOnDisconnect(); +main.cpp:347 resetPipeWriteState(); // A's transfer destroyed +``` + +Client A's transfer is killed by a stranger walking past. **This is a new remote-DoS +introduced by the plan**, on the one env that already has the least headroom. + +**Correction:** two changes, both required. +1. Give the refusal its own path: set a `bleRefusedGatecrasherPending` flag in `onConnect` + and have `onDisconnect` skip raising `bleDisconnectCleanupPending` when the departing + handle is the refused one (capture the conn handle in `onConnect` and compare + `connInfo.getConnHandle()` in `onDisconnect`). +2. Move the `ownerStillUp` early-return **out** of `#ifdef OPENDISPLAY_HAS_WIFI` — the + `pServer->getConnectedCount() > 0` half of it is unconditionally correct and costs + nothing on a no-WiFi build. + +--- + +## HIGH + +### H1 — Phase 5 escalates a recoverable command drop into a forced disconnect, and the ring holds 32, not 33 + +**Plan element:** Phase 5 — *"Command ring full: set `commandQueueOverflowAbort` (host task); +loop services → `abortToKnownState("command queue overflow", dropLink=true)`."* + +Today a full ring drops the newest frame (`esp32_ble_callbacks.h:127-129`). The pipe +protocol is *designed* for that: the missing seq shows up as a zero bit in the next SACK +mask and the client retransmits exactly that chunk (`docs/pipe-write-protocol.md` §5.2; +`device.py:2771-2772`). Loss of one frame costs one round trip. Phase 5 turns it into a +link drop plus a full re-auth plus a restarted transfer. + +Worse, the ring's usable capacity is `COMMAND_QUEUE_SIZE - 1 = 32`, not 33 — the producer +refuses at `nextHead == tail` (`esp32_ble_callbacks.h:122`). The comment at `main.h:365-370` +("33 slots hold a full W=32 in-flight window + END") is off by one. With W=32 negotiated +(`PIPE_MAX_W = 32`, `structs.h:51`) and the loop task stalled in a Spectra SPI write, a full +window fills the ring exactly; **any** interleaved non-pipe command (a `0x0060` telemetry +poll, a CCCD write) overflows it and, under Phase 5, kills the link. + +**Correction:** keep the drop, do not drop the link. Set the flag, log one WARN with the +count, and gate the *abort* on the overflow recurring while `transferActive()` and +`g_lastProgressMs` is already stale — i.e. let the supervisor own the decision. If you want +belt-and-braces, bump `COMMAND_QUEUE_SIZE` to 34 on the envs with DRAM to spare (not +`esp32-N4`) so the documented W=32+END claim is actually true. + +### H2 — Phase 2, read literally, makes the cross-transport clobber *worse*, and it ships two phases before the fix + +**Plan element:** Phase 2 — *"Queue flush + session clear must run even when the +transfer-owner guard early-returns for the other transport."* + +`disconnectWiFiServer()` sets `bleDisconnectCleanupPending = true` (`wifi_service.cpp:812`). +If Phase 2 puts `clearEncryptionSession()` **before** the `ownerStillUp` guard at +`main.cpp:333`, then on every WiFi-lost tick (`main.cpp:452-457`) or LAN client close, a +live, authenticated, mid-transfer BLE session is destroyed — which is §Context item 4, the +bug Phase 4 exists to fix, now reachable from a *second* code path and shipping first. + +The guard as written happens to save you today: `transferSessionOrigin()` returns +`sessionOrigin`, which defaults to `0` (`ORIGIN_BLE`, `display_service.cpp:2114`) and is +only written at transfer START, so `lanOwnsSession` is false and `ownerStillUp` reads +`getConnectedCount() > 0`. Putting the clear before it throws that away. + +**Correction:** place `flushCommandQueue(); flushResponseQueue(); clearEncryptionSession();` +**after** the `ownerStillUp` early-return, and make the guard unconditional (see C4.2). +Better: land the Phase 4 owner token *first* and scope the clear to +`linkOwner() == OWNER_BLE`. The plan's own note ("until then clear when the departing +transport owned it") is the right instinct — make it a hard requirement, not a parenthetical. + +### H3 — Phase 6 deletes the only timeouts that currently work, before the supervisor is trustworthy + +**Plan element:** Phase 6 — *"Subsume the 15-min watchdogs: delete main.cpp:436-442 direct-write +block; retire `checkPartialWriteTimeout`'s 15-min path."* + +Given C1, the supervisor's `g_lastProgressMs` is refreshed by ordinary command traffic. +Deleting `main.cpp:436-442` (which keys on `directWriteStartTime`, a *wall-clock start* +stamp that nothing refreshes) replaces an unconditional 15-minute bound with a conditional +one that a chatty client defeats. Same for `checkPartialWriteTimeout` (`display_service.cpp:578-587`), +which keys on `partialCtx.start_time`. + +**Correction:** keep both wall-clock bounds as a backstop and simply raise them past the +supervisor (e.g. 20 min), or add a second supervisor arm that is *not* progress-based: +`transferActive() && now - g_transferStartMs > OD_TRANSFER_HARD_CAP_MS`. Delete the old +blocks only after hardware soak proves the progress arm fires. + +### H4 — nRF: the "single task, no race" premise has a documented hole, and `abortToKnownState` is reachable from the Bluefruit task + +**Plan element:** Phase 2 — *"nRF `disconnect_callback`: add `clearEncryptionSession()` (same +task as all crypto — no race)"*; Phase 1 — *"nRF variant … callable from Bluefruit task"*; +Phase 6 — *"nRF: `volatile bool g_commandInFlight` around `imageDataWritten`"*. + +The premise is *usually* right and I verified it: both `connect/disconnect_callback` +(`bluefruit.cpp:829`, `:849`) and the characteristic write callback +(`BLECharacteristic.cpp:538-542`) are dispatched through `ada_callback()`, i.e. serialized +on the single "Callback" task (`AdaCallback.c:45`, `:147`). + +The hole: `ada_callback_invoke()` allocates its item with `rtos_malloc` and +`VERIFY(cb_data)` returns **false** on failure — at which point `BLECharacteristic.cpp:541` +invokes `_wr_cb` **inline on the BLE event task**: + +```c +if ( !(_use_ada_cb.write && + ada_callback(request->data, request->len, _wr_cb, ...)) ) +{ + _wr_cb(conn_hdl, this, request->data, request->len); // BLE task, not Callback task +} +``` + +Heap exhaustion during a large nRF52840 transfer is exactly when this triggers. Then +`imageDataWritten` runs concurrently on two tasks, `clearEncryptionSession()`'s +`memset(session_key, 0, 16)` (`encryption.cpp:205`) can land mid-`aes_ccm_decrypt`, and +`g_commandInFlight` — a plain bool, not a counter — is cleared by whichever nests out +first, letting the supervisor abort under a live handler. + +**Correction:** make `g_commandInFlight` a `volatile uint8_t` **depth counter** +(increment/decrement, abort only at 0). On nRF, make `clearEncryptionSession()` from +`disconnect_callback` deferred: set `nrfSessionClearPending` and service it from `loop()` +when the depth counter is 0. Note the inline-fallback path explicitly in the plan so nobody +later relies on "single task" as an invariant. + +--- + +## MEDIUM + +### M1 — Asymmetric nonce window (+128 / −32): not a replay hole, but a 4× DoS amplifier + +**Plan element:** Phase 3 — *"backward stays −32; forward becomes `OD_NONCE_FORWARD_WINDOW = 4*PIPE_MAX_W = 128`"*. + +I worked the exact math on the 64-entry ring and the widening is **not** exploitable as a +replay: + +- `encryption.cpp:136` only consults `replay_window[]` when `counter <= last_seen_counter`. + A forward-accepted counter immediately becomes `last_seen_counter` (`:149-151`), so a + second copy of it takes the `<=` branch and is caught by the ring. +- Backward window (32) < ring depth (64), so every counter reachable via the backward branch + is still in the ring. Sound. + +What *is* real: the window is one-sided. An attacker who captures a single valid frame at +counter `N` and replays it can jam `last_seen_counter` forward. With `+32` today the damage +is 32 counters; with `+128` it is 128. The client's next 96 legitimate frames +(`N+1 … N+96`) then fall outside the `−32` backward window and are rejected. Under Phase 3 +those rejections no longer count as `integrity_failures` (correct), so the session survives +— and stalls, which is exactly the wedge the supervisor must catch. The plan makes the +recovery path load-bearing while widening the trigger. + +Ranking this Medium, not Critical: it requires a local attacker with a captured frame, and +the same class of attack exists today at ¼ the magnitude. + +**Correction:** widen **both** sides to `OD_NONCE_WINDOW = 128` and grow +`replay_window[]` from 64 to 256 entries (`encryption_state.h:21`; +1.5 KB `.bss`, check it +on `esp32-N4`). If the RAM is not there, keep backward at −32 but cap forward at +64 so a +single replay cannot open a gap wider than the ring can police. + +Also: the plan correctly moves `replay_window_index` out of the function-static +(`encryption.cpp:152`) into `encryptionSession`. That is a real bug today — +`clearEncryptionSession()` memsets the ring (`:217`) but leaves the index, so the new +session's first 64 accepts overwrite an arbitrary rotation. **Verified correct; keep it.** + +### M2 — Phase 4 exclusivity breaks reconnect after an abrupt client loss + +If the central's host crashes or its adapter is reset, the device does not learn the link is +gone until its **supervision timeout** expires (typically 4 s, up to 32 s; negotiated by the +central — the repo only requests PHY/DLE, `ble_init.cpp:` `ble_nrf_request_fast_link`, and +sets nothing on ESP32). During that window, `getConnectedCount()` still reads 1, so the +returning client's reconnect is refused. Today, with `CONFIG_BT_NIMBLE_MAX_CONNECTIONS = 3` +(`sdkconfig.h:613`), it just works. + +HA delivery is `async with Device(...)` per push (`device.py:655-697`), so a supervision-timeout +window that eats one delivery attempt will be retried next wake (`delivery.py:452`) — not +fatal, but a visible reliability regression on flaky links. + +**Correction:** on refusal, if the *existing* link has no transfer in flight +(`!transferActive()`) **and** its last RX is older than ~10 s, terminate the **old** link +with `BLE_ERR_REM_USER_CONN_TERM` and accept the new one. Refuse only when the incumbent is +actively transferring. + +### M3 — `touchForceResume()` and `directWriteTouchSuspended` drift out of sync + +**Plan element:** Phase 1 — *"`touchForceResume()` (zero the `s_epd_refresh_suspend` counter)"*. + +The counter has a paired bool guard: `handleDirectWriteStart` and the pipe full-frame +setup set `directWriteTouchSuspended = true` (`display_service.cpp:2137`, `:2800`), and +`cleanupDirectWriteState` consumes it (`:2035-2038`). If `abortToKnownState` zeroes the +counter *before* calling `cleanupDirectWriteState`, the subsequent +`touchResumeAfterEpdRefresh()` early-returns at `touch_input.cpp:418` — harmless — but +`directWriteTouchSuspended` is left `true` only if the abort ordering is different from the +plan's. As written (`cleanupDirectWriteState(true)` first, `touchForceResume()` later) the +ordering is fine. + +The genuine hazard the prompt flags — touch polling I2C concurrently with SPI streaming — +does **not** materialise on ESP32: `processTouchInput()` also gates on `transferActive()` +(`touch_input.cpp:584`), and `abortToKnownState` clears all three transfer flags before +`touchForceResume()`. On nRF there is no such gate at all (the block is `#ifdef TARGET_ESP32`), +but nRF also runs the transfer on a different task from `processTouchInput()`, so nothing +changes. + +**Correction:** have `touchForceResume()` also clear `directWriteTouchSuspended` (expose a +setter or move the bool next to the counter), and assert the counter is 0 afterwards. Low +effort, removes a future footgun. Keep the plan's ordering. + +### M4 — `esp32_set_ble_connectable()` has no failure path; a failed `start()` leaves the radio permanently dark + +**Plan element:** Phase 4 — *"stop → `setConnectableMode(NON/UND)` → re-push +`setAdvertisementData` → start"*. + +The manufacturer-data concern is **handled correctly** — see V4 below. The gap is failure +handling. `NimBLEAdvertising::start()` returns `bool`; `setAdvertisementData()` returns +`bool` and fails if the payload exceeds 31 bytes. If either fails after the `stop()`, the +device is not advertising, is not connected, and nothing retries: `bleRestartAdvertisingPending` +is only raised by `onDisconnect`. That is "unresponsive but not frozen" — a support ticket +that looks like a dead device. + +**Correction:** check both return values; on any failure, force +`setConnectableMode(BLE_GAP_CONN_MODE_UND)`, re-push, `start()` again, and set +`bleRestartAdvertisingPending = true` so the loop keeps retrying. + +### M5 — Drain-abort `break` placement relative to `commandQueue[tail].pending = false` + +**Plan element:** Phase 1 — *"after `imageDataWritten` returns, `if (commandDrainAbortPending) +{ clear; break; }` — break WITHOUT the tail store."* + +The index walk is correct as specified. `flushCommandQueue()` sets +`commandQueueTail := head`, the drain then breaks without executing `main.cpp:417`, so tail +stays at the flushed position and nothing is re-dispatched or leaked. **Verified sound.** + +The one hazard is placement: the check must go **between** `main.cpp:415` and `main.cpp:416`. +If it goes after `:416`, the `commandQueue[tail].pending = false` writes into a slot the +producer may already have re-filled post-flush. `pending` is write-only today (its only +readers are `main.cpp:291/309/416` — all writes), so the consequence is nil, but it is a +landmine for anyone who later gives `pending` meaning. + +**Correction:** state the exact insertion point in the plan, and delete the vestigial +`pending` field while you are in there — it has no readers in either ring. + +--- + +## LOW + +### L1 — Plan factual error: the response ring *is* flushed today + +§Context item 3 asserts *"neither ring is EVER flushed (heads/tails never reset anywhere)"*. +The response ring is drained to empty on every loop pass with no central connected: + +``` +main.cpp:307-312 +} else { + while (responseQueueTail != responseQueueHead) { + responseQueue[responseQueueTail].pending = false; + responseQueueTail = (responseQueueTail + 1) % RESPONSE_QUEUE_SIZE; + } +} +``` + +So Phase 2's `flushResponseQueue()` on BLE disconnect is redundant on ESP32 (it happens one +pass later anyway). Only the **command** ring genuinely survives a disconnect. Harmless to +add, but the plan should not claim it as a fix for a bug that does not exist — and the +finding matters because it means stale *responses* were never the wedge. + +### L2 — The 60 s pipe-error reset is safe but far outside client patience, and the doc needs a line + +The prompt's worry that Phase 2 breaks `docs/pipe-write-protocol.md` §5.1 does **not** hold. +§5.1 (lines 363-365) promises only that later `0x0081` frames are *silently discarded* until +the next `0x0080` or a disconnect. After `resetPipeWriteState()`, `handlePipeWriteData`'s +first line still discards them (`display_service.cpp:2811`: `if (!pipeState.active || +pipeState.error) return;`). Client-observable behaviour is identical. + +What is off is the plan's rationale: *"preserves the 60 s stable-ACK-position window for +client retries."* py-opendisplay treats every `0x81` NACK as fatal and raises immediately +(`device.py`, `ProtocolError` on fatal NACK); nothing re-reads the ACK position. 60 s is +~4× the client's entire `MAX_PTO` budget. The number is harmless but the justification is +fiction — pick 10 s and say it is a hardware-release deadline, not a client-retry window. + +Also: `PipeWriteState` (`structs.h:106-126`) has no timestamp field at all, so +`error_since_ms` is a genuine struct addition, not a rename. Fine on RAM (4 bytes). + +### L3 — "No hardware WDT" is true only because every long wait yields + +The plan's premise is defensible but the surrounding facts are not quite as stated: + +- The FreeRTOS Task WDT **is** enabled with panic on the pinned platform: + `CONFIG_ESP_TASK_WDT_INIT 1`, `CONFIG_ESP_TASK_WDT_PANIC 1`, + `CONFIG_ESP_TASK_WDT_TIMEOUT_S 5`, watching `IDLE_TASK_CPU0` + (`framework-arduinoespressif32-libs/esp32s3/qio_qspi/include/sdkconfig.h:906-910`). Plus + `CONFIG_ESP_INT_WDT` at 300 ms (`:903-904`). +- Every ESP env passes `-DCONFIG_FREERTOS_WATCHDOG_TIMEOUT_S=120` (e.g. `platformio.ini:189`). + That symbol does not exist in IDF 5.x (it is `CONFIG_ESP_TASK_WDT_TIMEOUT_S`), and the + precompiled `sdkconfig.h` wins anyway — the same trap the repo already documented for + `CONFIG_BT_NIMBLE_MAX_CONNECTIONS`. **The flag is inert.** Either delete it or fix the + name; leaving a dead knob that reads like a 120 s guarantee is how the next reader gets + the sizing wrong. +- On single-core C3/C6 the Arduino loop task shares CPU0 with IDLE0, so a genuinely + non-yielding `loop()` *would* panic-reboot in 5 s. It does not today because every long + wait yields (`waitforrefresh` `delay(10)` at `display_service.cpp:759`; `bbepWaitBusy` + `bbepLightSleep(20)` at `bb_ep.inl:3973`). Worth one sentence in the plan so a future + busy-spin does not get added casually. + +The plan's RTC rationale is **correct and already hardware-proven** — see V5. + +### L4 — Minor line-reference drift in the plan + +Not worth churn, but for the record: `communication.cpp:113-116` is actually `:117-121` +(the ring-full check); `sdkconfig.h:471` is `:613` on the S3 variant; `display_fastepd.cpp:222-231` +lands on `fastepd_full_update` at `:227-231`. Everything else I spot-checked +(`encryption.cpp:149-155`, `display_service.cpp:401-408`, `power_latch.cpp:87-90`, +`main.cpp:406-423`/`:321-348`/`:436-442`, `device_control.cpp:227-240`, +`esp32_ble_callbacks.h:128`, `touch_input.cpp:584`, `wifi_service.cpp:804`/`:879`) is exact. + +--- + +## What the plan MISSES — freeze vectors not addressed + +### X1 — I2C bus hang: `Wire.setTimeOut()` is never called anywhere + +`grep -n "setTimeOut" src/*.cpp` returns nothing. The firmware drives four I2C peripherals +(AXP2101 PMIC in `display_service.cpp`, GT911 touch in `touch_input.cpp`, SHT40, BQ27220) +through `Wire`, whose Arduino-ESP32 default timeout is 50 ms per transaction — survivable — +**but** `wireBeginForOpenDisplay()` (`display_service.cpp:785-800`) and +`invalidateOpenDisplayWire()` never re-assert it after a re-`begin()`. More importantly a +slave holding SDA low (classic GT911 wedge after a rail cut) is not recovered by a timeout: +it needs the nine-clock bus-recovery pulse train, which the firmware never issues. + +This is a real, common e-paper-tag freeze mode (touch controller and panel share the rail +that `epdSessionForceOff` cuts), and the supervisor cannot see it — `transferActive()` is +false while `processTouchInput()` spins. **Recommend:** add explicit `Wire.setTimeOut(25)` +after every `Wire.begin()`, and a nine-clock SDA-recovery routine invoked when +`rt->i2c_fail_streak` crosses a threshold (`touch_input.cpp:400` already tracks it). + +### X2 — The boot refresh is invisible to every `epdRefreshInProgress` gate + +`refreshBootScreenFull()` (`display_service.cpp:533-542`) calls `touchSuspendForEpdRefresh()` ++ `bbepRefresh` + `waitforrefresh(60)` and **never sets `epdRefreshInProgress`**. Neither +does the FastEPD boot path (`:1588-1594`). Consequences: `serviceBleDisconnectCleanup` +(`main.cpp:322`), `esp32_restart_ble_advertising` (`ble_init.cpp:236`), the `workInFlight` +gate (`main.cpp:478`) and the plan's new `idleLinkTick()` / supervisor "never interrupt a +refresh" rule all mis-read a boot refresh as idle. On a Spectra panel that is a 30–60 s +blind spot on every cold boot, and the retry path (`:1624-1633`) can double it. + +**Recommend:** set/clear `epdRefreshInProgress` around both boot-refresh paths as a Phase 0 +one-liner. It is free and it makes the supervisor's refresh exemption honest. + +### X3 — `fastepd_wait_refresh()` is a stub — the IT8951 path has *zero* firmware-side bound + +``` +src/display_fastepd.cpp:228-231 +bool fastepd_wait_refresh(int timeout_sec) { + (void)timeout_sec; + return !s_init_failed; +} +``` + +`waitforrefresh(60)` short-circuits to this on FastEPD builds (`display_service.cpp:749-751`), +so the "60 s cap" the plan cites does not exist on IT8951/E1004. The plan's Phase 0 mitigation +(log if `fullUpdate` exceeds 120 s) wraps `fastepd_full_update` (`:227-231`) but **not** +`fastepd_direct_refresh`, which is the path a real transfer takes +(`display_service.cpp:2422-2423`). Post-hoc logging on the wrong function is not the +"documented residual risk" the plan claims to have accepted. + +**Recommend:** wrap `fastepd_direct_refresh` too, and implement `fastepd_wait_refresh` as a +real busy poll against the IT8951 LUT-busy register with the passed `timeout_sec`. + +### X4 — Config chunked-write has no timer of its own + +`chunkedWriteState.active` is set at `communication.cpp:496` and cleared only on completion +(`:574`), on a malformed chunk (`:558`), or on an auth failure (`:550`). A client that sends +`0x0040` START and vanishes leaves it latched forever — and, unlike the transfer flags, it +is not covered by any existing watchdog. The plan adds `resetChunkedWriteState()` and puts +`chunkedWriteState.active` in the supervisor predicate, which is correct — but per C1 the +supervisor never fires. Once C1 is fixed this is covered; flagging it so the dependency is +explicit. + +### X5 — Deep-sleep entry with a live transfer + +`enterDeepSleep()` (`main.cpp:577`) is reachable from the advertising-window branch +(`main.cpp:388`) and the idle branch (`:498`). The idle branch is gated by `workInFlight`, +which includes `epdRefreshInProgress` and `getConnectedCount() > 0` — but **not** +`transferActive()`. A pipe transfer whose client link has already dropped (so +`getConnectedCount() == 0`) but whose `pipeState.active` is still latched, with +`bleDisconnectCleanupPending` deferred behind `epdRefreshInProgress`, can reach +`enterDeepSleep()` with the panel rail up. Rare, but the plan's `abortToKnownState` is the +natural place to close it. + +**Recommend:** add `transferActive()` to the `workInFlight` disjunction at `main.cpp:474-479`. +One term, no behaviour change in the normal case. + +### X6 — Buzzer and LED timers are outside every bound + +`buzzerService()` / `processLedFlash()` run unbounded from `loop()`. The plan's +`abortToKnownState` lists "buzzer/LED stop" — good — but nothing bounds a stuck +`buzzer_control` sequence in the first place. Low probability; noting for completeness. + +### X7 — Nothing detects "advertising stopped and never restarted" + +`esp32_restart_ble_advertising()` early-returns without starting when +`epdRefreshInProgress` (`ble_init.cpp:236-239`, re-arming the flag) or when +`getConnectedCount() > 0` (`:232-235`, **clearing** the flag). The second is the hole: if +the count is stale-nonzero (refused gatecrasher, C3/C4) the flag is cleared and advertising +never resumes. **Recommend:** a cheap `advertisingHealthTick()` — if no peer, not advertising +(`BLEDevice::getAdvertising()->isAdvertising()`), and no pending flag for > 30 s, force a +restart and log WARN. + +--- + +## Phase-ordering hazards + +| Phase shipped alone | Verdict | +|---|---| +| **0** | Net **regression** as written, because of the `pwrmgmLockTake` steal (C2). The `powerOff` and drain-cap bounds are fine. Ship Phase 0 only after C2's correction (fail-closed instead of steal). | +| **1** | Safe alone. The drain-trap fix (M5) and the queue flushes are self-contained and correct. `abortToKnownState` has no callers until Phase 5/6, so it is dead code — which is fine, but means Phase 1 provides no field benefit on its own. | +| **2** | **Must not ship before Phase 4** if implemented literally (H2). With the clear placed *after* an unconditional owner guard, it is safe and beneficial alone. The nRF half needs H4's deferral. | +| **3** | Safe alone and the highest value-per-risk phase — it removes the actual root cause of the field failures. Ship it **first**. | +| **4** | Blocked on C3 (wrong reason code) and C4 (cleanup re-entry). Once fixed, safe alone. | +| **5** | Net **regression** alone: the command-ring abort (H1) and the RX/TX-keyed idle timer (C1's twin) each make things worse without the corrected progress accounting from Phase 6. | +| **6** | Net **regression** alone as written: it deletes two working wall-clock watchdogs (H3) and replaces them with a predicate that command traffic defeats (C1). | + +**Recommended order:** 3 → 0(corrected) → 1 → 4(corrected) → 2 → 6(corrected, keeping the +old watchdogs) → 5(corrected). That front-loads the phase that fixes the reported field +failure and puts the owner token in place before anything depends on it. + +--- + +## Verified CORRECT — do not churn on these + +1. **`verifyNonceReplay` commits before tag verification.** `encryption.cpp:149-155` writes + `last_seen_counter` and the ring *before* `aes_ccm_decrypt` runs at `:714`. Real bug, + correctly diagnosed, and the `nonceCheck`/`nonceCommit` split is the right shape. +2. **Nonce failures should not touch `integrity_failures`.** `encryption.cpp:691-697` + currently increments on *any* `verifyNonceReplay` failure — including plain packet loss. + Confirmed as the mechanism behind §Context item 1. Fix is correct. +3. **The pipe fatal-NACK latch has no timeout.** `sendPipeNack` (`display_service.cpp:2562-2578`) + sets `pipeState.error = true` and calls `cleanupDirectWriteState(true)`, which clears + `directWriteActive` (`:2011`) — so `main.cpp:436-442`'s watchdog no longer applies, and + `checkPartialWriteTimeout` (`:578`) only covers `partialCtx`. For a non-partial pipe + transfer there is genuinely **no** bound. Diagnosis exact. +4. **`setConnectableMode(NON)` calls `setFlags(0)` and the re-push is required.** Confirmed + at `NimBLEAdvertising.cpp:82-84`; `setFlags(0)` is one-way (UND does not restore it), and + `setAdvertisementData(*advertisementData)` copies the app object which carries + `setFlags(0x06)` from `ble_init.cpp:302`. The plan's mitigation, and the + "`setAdvertisementData` must be LAST" trap at `ble_init.cpp:307-312`, are both correct. +5. **RTC memory does not survive a non-deep-sleep reset.** `main.cpp:96-100` documents it + from hardware (`FINDINGS_DEEP_SLEEP_WAKE_BOOT_SCREEN_2026-07-07.md`), and + `displayed_etag` is `RTC_DATA_ATTR` (`main.h:294`). The "reset state, never reboot" + decision is well-founded. +6. **NimBLE adds the peer before `onConnect`.** `NimBLEServer.cpp:464-471` fills + `m_connectedPeers` then calls `onConnect`, so `getConnectedCount() > 1` is a valid + gatecrasher test. (The refusal *mechanism* is still wrong — C3.) +7. **`-DCONFIG_BT_NIMBLE_MAX_CONNECTIONS=1` is inert.** `sdkconfig.h:613` defines it to 3. + Correct to enforce in code. +8. **nRF `Bluefruit.begin(1, 0)` already caps at one link.** `ble_init.cpp:152`. Correct. +9. **Clearing the session on disconnect is client-compatible.** py-opendisplay authenticates + inside `__aenter__` on every connection (`device.py:670`) and calls `_clear_session()` in + `__aexit__` (`:697`) and on any setup failure (`:680-683`). HA's delivery path is one + `async with Device(...)` per push. No client assumes session persistence across a link + drop. **No compatibility risk.** +10. **The 10-minute supervisor timeout is not too short for legitimate work.** Worst-case + legitimate blocks: `waitforrefresh(60)` = 60 s (`display_service.cpp:2423`), + `bbepWaitBusy` 30 s, and the E1004 ~960 KB upload — but the client itself gives up far + sooner (`TIMEOUT_PIPE_DATA_COMPRESSED = 5.0` × `MAX_PTO = 3` ≈ 15 s; + `TIMEOUT_REFRESH = 90.0`, `device.py:457-473`). Provided the stamps are moved per C1, no + legitimate transfer comes near 600 s of no progress. The number is fine. +11. **The 2 s drain cap is safe.** The drain already caps at `COMMAND_QUEUE_SIZE` iterations + and flushes responses between commands (`main.cpp:419-421`); a wall-clock cap on top + changes nothing in the normal case. +12. **The queue-flush SPSC reasoning is right.** `commandQueueTail` is written only by the + consumer, and every call path the plan names (`serviceBleDisconnectCleanup`, the drain + loop, LAN dispatch, `abortToKnownState`) is on the loop task. `tail := head` snapshot is + the correct flush. `responseQueue` head/tail are both loop-task-only, so its flush is + trivially safe. +13. **The OTA exception is genuinely satisfied.** ESP32 `0x0051` is `esp_restart`; nRF DFU + jumps to the bootloader. Nothing to special-case. +14. **Phase 2's 60 s pipe-error reset does not break `pipe-write-protocol.md` §5.1** — see + L2. The doc's contract is about client-observable discard behaviour, which is unchanged. + Update §5.1 to mention the reset, but no protocol change is needed. + +--- + +## Summary of required corrections + +| # | Phase | Correction | +|---|-------|-----------| +| C1 | 6, 5 | Stamp progress only on state-machine advancement; never on command dispatch or notify. Key the idle timer the same way. | +| C2 | 0 | Replace the 10 s lock steal with a 60 s fail-closed take; add an owner field if a steal is ever genuinely needed. | +| C3 | 4 | Use `BLE_ERR_REM_USER_CONN_TERM` (0x13), not `BLE_ERR_CONN_LIMIT` (0x09). Check the return value. | +| C4 | 4 | Discriminate the refused conn handle in `onDisconnect`; move the `ownerStillUp` guard out of `#ifdef OPENDISPLAY_HAS_WIFI`. | +| H1 | 5 | Command-ring overflow logs and drops; it does not drop the link. Fix the off-by-one in the `main.h:365` comment. | +| H2 | 2 | Place `clearEncryptionSession()` after the owner guard, or land Phase 4's token first. | +| H3 | 6 | Keep the wall-clock watchdogs as a backstop until the progress arm is soak-proven. | +| H4 | 1, 2, 6 | `g_commandInFlight` becomes a depth counter; defer the nRF disconnect-time session clear to `loop()`. | +| M1 | 3 | Make the nonce window symmetric (±128 with a 256-entry ring, or ±64). | +| M2 | 4 | Prefer evicting an idle incumbent over refusing a reconnect. | +| M4 | 4 | Check `setAdvertisementData`/`start()` returns; fall back to connectable. | +| X1 | new | `Wire.setTimeOut()` + a nine-clock I2C recovery routine. | +| X2 | 0 | Set `epdRefreshInProgress` around both boot-refresh paths. | +| X3 | 0 | Wrap `fastepd_direct_refresh`, and implement `fastepd_wait_refresh` for real. | +| X5 | 6 | Add `transferActive()` to the `workInFlight` disjunction. | +| X7 | new | `advertisingHealthTick()` — detect a permanently dark radio. | + +## Build/portability check + +New `src/session_guard.cpp` compiles into **all eleven** envs. Guards needed: +`pServer` / `responseQueue` / `commandQueue` under `#ifdef TARGET_ESP32`; LAN calls under +`#ifdef OPENDISPLAY_HAS_WIFI` (**not** `TARGET_ESP32` — `esp32-N4` is ESP32 without WiFi, +`platformio.ini:284`). `lib_ignore = NimBLE-Arduino` on nRF (`platformio.ini:36`) means +`session_guard.cpp` must not include `ble_init.h`'s NimBLE surface unguarded. RAM cost is a +handful of scalars plus `PipeWriteState::error_since_ms` (+4 B) — fine even on `esp32-N4`, +which is DRAM-tight enough to need `PIPE_SMALL_DRAM_WINDOW` (`structs.h:45-48`). If M1's +256-entry `replay_window` is adopted (+1.5 KB `.bss`), verify `esp32-N4` links before +committing to it. diff --git a/docs/FINDINGS_PHASE1_PLAN_REVIEW_2026-07-26.md b/docs/FINDINGS_PHASE1_PLAN_REVIEW_2026-07-26.md new file mode 100644 index 0000000..0169586 --- /dev/null +++ b/docs/FINDINGS_PHASE1_PLAN_REVIEW_2026-07-26.md @@ -0,0 +1,431 @@ +# Adversarial Review — PLAN_PHASE1_NONCE_REPLAY_2026-07-26 + +**Date:** 2026-07-26 · **Reviewing:** [`PLAN_PHASE1_NONCE_REPLAY_2026-07-26.md`](PLAN_PHASE1_NONCE_REPLAY_2026-07-26.md) (branch `debug/ble-hardening`) +**Scope:** `src/encryption.cpp`, `src/encryption_state.h`, their callers, and the two clients that +drive them (`py-opendisplay`, the web client `ble-common.js`) + +> Companion to [`FINDINGS_FREEZE_PROOFING_PLAN_REVIEW_2026-07-26.md`](FINDINGS_FREEZE_PROOFING_PLAN_REVIEW_2026-07-26.md). +> Where the two disagree, this document is the later and more specific analysis — in particular +> it **retracts** part of that review's `[M1]` (see `M2` and `L1`). + +**Independently re-verified after the review was produced** (not taken on trust): `C1`'s +selective-repair transmit site and `max_retx` formula; `H1`'s `IntegrityCheckError` path and the +pipe loop's single `except`; `H2`'s shared nonce space in `encryptResponse`; `H3`'s +unconditional ring write; `L3`'s arithmetic; `L5`'s 11-leg CI matrix. All confirmed as +described. + +--- + +## Verdict + +**Phase 1 is safe to ship alone — no new freeze mode, no new remote DoS, no regression on either +target — but not as written.** Two edits are mandatory (`C1`, `M1`), one is strongly recommended +(`H1`), two need a paragraph each (`H2`, `H3`). + +D1–D4 are real (three exactly as described, one overstated), the check/commit split is the +correct shape, and the sliding-bitmap state machine is correct as specified. `−496 B`, +`+1,536 B`, `PIPE_MAX_W` 32/16, `MAX_PTO = 3`, the `protocol.h:384` citation and the "one counter +burned per transmission" premise all check out. Three things do not: `C1`, `H1`, `H2`. + +**Findings: 1 Critical, 3 High, 3 Medium, 7 Low.** + +--- + +## CRITICAL + +### C1 — `OD_NONCE_FORWARD_CAP = 64` is derived from a bound that does not hold + +**Plan element:** Decision A — *"worst-case unseen run = `PIPE_MAX_W + MAX_PTO` = 32 + 3 = 35 … +This is a **hard bound, not an estimate**"* — plus Step 6, which instructs writing that sentence +into `communication.cpp` as the invariant future changes are checked against. + +**What the derivation assumes:** *"After the window is exhausted it blocks for an ACK; on timeout +it resends exactly one chunk (`_send(window_base)`), never another window."* Accurate for the +**PTO** path ([device.py:2715-2726](../../py-opendisplay/src/opendisplay/device.py)) and for +**new** sends (`:2688-2694`, gated on `(next_to_send - window_base) < window`). It ignores the +third transmit site. + +**The site it ignores** — selective repair, which spends no window credit at all: + +```python +device.py:2789-2796 + for m in missing: # oldest first + ... + if do_retx: + await _send(m) + pending_retx[m] = 0 + retx_count += 1 +``` + +`missing` is every hole below `highest_recv` (`device.py:2771-2772`), up to `W-1` entries. Each +hole is re-sent again every `PIPE_RETX_ACK_SPACING = 2` ACKs (`commands.py:99`, +`device.py:2782-2788`). The only cap on the total is +`max_retx = max(3*window, ceil(n * 0.5))` — **96** for `W = 32`, and far larger for a +multi-thousand-chunk upload (`device.py:2672`, `commands.py:96`). + +**And the client deliberately hoards ACKs to spend on those rounds:** + +``` +device.py:781 Passes ``drain_stale=False`` so queued sliding-window ACKs are preserved. +device.py:786 await self._conn.write_command(self._encrypt_frame(data), response=response, drain_stale=False) +``` + +So `B` backlogged ACKs, processed while the device is not consuming anything (mid-`bbepWaitBusy`, +or with the 33-slot command ring full — [esp32_ble_callbacks.h:119-129](../src/esp32_ble_callbacks.h), +which drops on the NimBLE host task *before* decrypt), buy `⌈B/2⌉` repair rounds of up to `H` +holes each. Firmware supplies the backlog: it ACKs every `ack_every` accepted frames +([display_service.cpp:2854](../src/display_service.cpp)) **and** once per `ack_every` +out-of-order arrivals plus one immediately when a gap opens (`:2875-2892`). + +**The arithmetic.** With `W` in flight, `H` holes and `N = ack_every`, the device emits about +`(W−H)/N` ACKs before it stalls, and the client can spend them on `⌈(W−H)/(2N)⌉` repair rounds: + +| `W` | `N` | worst `H` | repair transmissions | + PTO probes | total gap | +|---|---|---|---|---|---| +| 32 | 8 (py default, `device.py:497`) | 16 | 16 | 2 | ~18 | +| 32 | 4 (HA default, `const.py:51`) | 20 | 40 | 2 | ~42 | +| 32 | **2** | 16 | 16 × 4 = **64** | 2 | **66** ✗ | +| 32 | **1** | 16 | 16 × 8 = 128, capped at `max_retx` = 96 | — | **~96** ✗ | +| 16 | 4 (web client, `ble-common.js:38-39`) | 8 | 8 | 2 | ~10 | + +`N = 1` and `N = 2` are not hypothetical: `blocks_per_ack` is a Home Assistant options-flow field +with `min=1, max=32` (`config_flow.py:104-113`), `py-opendisplay` documents it as "1..32" +(`device.py:525`), firmware clamps only at the *top* +([display_service.cpp:2723-2725](../src/display_service.cpp)), and +[main.cpp:269](../src/main.cpp) explicitly sizes the response-flush path for *"small negotiated +ack_every (N_eff 1-2)"*. + +**Why it matters.** Not a freeze — after the D1 fix an over-cap gap drops frames and keeps the +session. It matters because (a) the plan instructs writing "35 < 64" into the source as a +permanent invariant, and (b) it converts a *recoverable* transfer into a failed one exactly in +the lossy conditions Phase 1 exists to survive: at cap 128 the device re-syncs when it drains; at +cap 64 it rejects and the client burns its remaining `max_retx` budget rediscovering that by +timeout. + +**Correction:** (1) set `OD_NONCE_FORWARD_CAP = 128` — under the bitmap the cap costs nothing, +and the DoS argument for keeping it tight does not survive `M2`; (2) re-derive from the client's +**retransmit budget** `max_retx = max(3·W, n/2)`, stating plainly that firmware cannot bound it +from its own constants; (3) write the *mechanism* into the Step 6 comment, not a number that a +`blocks_per_ack` change in another repo silently invalidates. + +--- + +## HIGH + +### H1 — Decision E's "the client recovers on its own" is false + +`decryptCommand` returning false produces an unencrypted 3-byte NACK for **every** rejection +reason: + +```c +communication.cpp:698-703 + if (!decryptCommand(...)) { + od_log_error("ERROR: Decryption failed"); + uint8_t response[] = {RESP_ACK, (uint8_t)(command & 0xFF), RESP_NACK}; + sendResponseUnencrypted(response, sizeof(response)); +``` + +`RESP_NACK = 0xFF` ([opendisplay_protocol.h:698](../include/opendisplay_protocol.h)). The +client's `_read` intercepts that shape before any pipe-frame classification: + +``` +device.py:833-838 + if len(raw) == 3 and raw[2] == 0xFF: + raise IntegrityCheckError(...) +``` + +and the pipe send loop's only `except` is `BLETimeoutError` (`device.py:2716`). +`IntegrityCheckError` propagates straight out of `_stream_pipe_chunks`; the upload fails on the +**first** out-of-window frame. There is no retransmit machinery to recover with — the frame the +client would have repaired is the one whose NACK aborted it. + +Phase 1's field benefit is real but narrower than claimed: the device stays clean and ready for +the *next* connection instead of answering `0xFE` to everything. The in-progress transfer still +dies. + +**Correction:** on `NONCE_OUT_OF_WINDOW` / `NONCE_REPLAY` for `0x0081`, send **nothing** (let +PTO/SACK treat it as the plain frame loss it is) or send the normal pipe ACK. Keep `RESP_NACK` +for tag failures. ~5 lines in `communication.cpp:698-703` behind a reason code out of +`nonceCheck`; it converts Phase 1 from "the device survives" into "the transfer survives". +Decision E's recorded wire change remains right for non-pipe opcodes. + +### H2 — Device and client share one nonce space: CCM keystream reuse (shipping defect, unaddressed) + +```c +encryption.cpp:158-174 +void getCurrentNonce(uint8_t* nonce) { + memcpy(nonce, encryptionSession.session_id, 8); + uint64_t counter = encryptionSession.nonce_counter; // starts at 0 (:210, :655) + for (int i = 0; i < 8; i++) nonce[8 + i] = (counter >> (56 - i * 8)) & 0xFF; +} +``` + +```python +crypto.py:92-113 +def get_nonce(session_id: bytes, counter: int) -> bytes: + return session_id + counter.to_bytes(8, "big") +... + ccm_nonce = nonce_full[3:] # 13 bytes +``` + +`encryptResponse` ([encryption.cpp:740-743](../src/encryption.cpp)) and `decryptCommand` +(`:704-705`) both take `nonce_full[3:16]`. Same `session_key`, same `session_id`, **no direction +separator**, both counters reset to 0 at session start (`encryption.cpp:210`/`:655`; +`device.py:735`; `ble-common.js:1259`). Response #*k* and command #*k* therefore encrypt under an +identical (key, nonce). CCM is CTR underneath: the keystream depends only on key and nonce, not +the AD (which does differ), so `C_resp ⊕ C_cmd = P_resp ⊕ P_cmd`. Responses are short and highly +predictable (`{RESP_ACK, cmd, status}`, pipe ACKs), so a passive eavesdropper recovers the +leading plaintext bytes of the matching command. No authenticity break, but a textbook +nonce-reuse confidentiality failure. + +**Correction:** the fix is a direction bit — a wire change of the same cost class as Decision E, +and correctly out of Phase 1 scope. What *is* in scope: **record it** alongside Decision E so one +future wire revision fixes both. Doing nothing and not writing it down is the only unacceptable +outcome, because Phase 1 is the change that makes a future reader believe this layer has been +audited. + +### H3 — D3 understated: the exemption lets an attacker flush the entire replay ring + +The core claim is verified ([encryption.cpp:136](../src/encryption.cpp)). What the plan misses is +that the accept path writes the ring unconditionally, *including* on the exempted `diff == 0` +re-accept: + +```c +encryption.cpp:152-154 + static uint8_t replay_window_index = 0; + encryptionSession.replay_window[replay_window_index] = nonce_counter; + replay_window_index = (replay_window_index + 1) % 64; +``` + +Replaying the highest-seen frame **64 times** overwrites all 64 slots with that one value, +evicting every genuine entry, while `last_seen_counter` never moves (`:149-151` is +`>`-conditional) so the ±32 check at `:131` still admits `[L−32, L]`. **Every one of the last 32 +genuine commands then becomes replayable** — config writes, power-off, buzzer, LED. + +Reachable on ESP32: `CONFIG_BT_NIMBLE_MAX_CONNECTIONS` is **3** (`sdkconfig.h:613`; the `-D…=1` +override is inert), the write callback does not discriminate conn handles +([esp32_ble_callbacks.h:119-129](../src/esp32_ble_callbacks.h)), and `isAuthenticated()` +(`:195-199`) is a global flag with no peer binding — a second central can write captured frames +into a live session. On nRF `Bluefruit.begin(1, 0)` ([ble_init.cpp:152](../src/ble_init.cpp)) +caps at one link. + +**Correction:** no design change (the bitmap closes it). Restate D3's consequence and add a +Step 5 hardware test: replay the final frame 64× then replay an *older* in-window counter — must +be `NONCE_REPLAY`. + +--- + +## MEDIUM + +### M1 — Step 2 and Decision D prescribe different deltas, and Step 2's is UB on attacker-controlled input + +Step 2: `diff = (int64_t)counter - (int64_t)last_seen_counter`. Decision D: *"compute the delta as +`(int64_t)(counter - last_seen)` rather than subtracting two casted `int64_t`s."* Not the same +expression; Step 2's is wrong. The 8 counter bytes are plaintext and parsed **before** tag +verification ([encryption.cpp:119-121](../src/encryption.cpp), called from `:691` ahead of +`aes_ccm_decrypt` at `:714`), so an unauthenticated attacker controls the delta. +`(int64_t)nonce_counter` for `≥ 2^63` is an out-of-range conversion and the subtraction overflows +— UB that the plan's own `-fsanitize=undefined` gate will flag against the expression Step 2 +mandates. Today's `:130` has exactly this construct. + +Decision D's form is not sufficient either: the Step 2 table uses `-diff` twice, and `-diff` is +UB when `diff == INT64_MIN` (reachable with `counter = last_seen + 2^63`). + +**Correction:** drop signed arithmetic entirely: + +```c +const uint64_t fwd = counter - last_seen; /* wraps; 0 when equal */ +const uint64_t back = last_seen - counter; /* wraps; fwd + back == 0 mod 2^64 */ +if (fwd == 0) return bit_test(bm, 0) ? NONCE_REPLAY : NONCE_OK; +if (fwd <= OD_NONCE_FORWARD_CAP) return NONCE_OK; +if (back < OD_NONCE_BACKWARD_BITS) return bit_test(bm, back) ? NONCE_REPLAY : NONCE_OK; +return NONCE_OUT_OF_WINDOW; +``` + +Make Step 2's table match so `nonce_window.h` and the plan cannot diverge. + +### M2 — "Tight is correct here" rests on a jam-forward DoS that cannot happen + +Decision A inherits `[M1]` from the parent review. Under Phase 1's own design it does not hold: + +1. **A replay cannot push `last_seen_counter` past the client's high-water mark.** After the D2 + fix only a CCM-verified frame commits (Step 3), so the only counters an attacker can commit are + ones the client actually transmitted. No *future* client frame is ever below the window. +2. **Repairs carry fresh, higher counters.** `_encrypt_frame` increments on every transmission + including repairs (`device.py:747-760`, `:759`) — the plan cites this itself. The "stranded" + frames do not exist. + +The only frames a jam can strand are ones already in flight at lower counters, and those land in +the 127-wide **backward** window, where they are accepted as unseen. Net damage: zero. + +**Correction:** delete the "Why not more" paragraph; it is the only argument that produced 64 +instead of 128. Note that `[M1]`'s concern was an artifact of the value-ring design and does not +survive commit-after-verify + bitmap. + +### M3 — The `resetNonceState()` fold merges two blocks that are not equivalent + +| Field | `clearEncryptionSession` (`:205-217`) | `handleAuthenticate` (`:654-662`) | +|---|---|---| +| `nonce_counter` | `= 0` (`:210`) | `= 0` (`:655`) | +| `last_seen_counter` | `= 0` (`:211`) | `= 0` (`:656`) | +| `replay_window` | `memset` (`:217`) | `memset` (`:660`) | +| `integrity_failures` | `= 0` (`:212`) | `= 0` (`:657`) | +| `authenticated` | `= false` (`:209`) | `= true` (`:654`) | +| `session_start_time`/`last_activity` | `= 0` (`:213-214`) | `= currentTime` (`:658-659`) | +| keys/nonces/`ccm_ctx` | wiped (`:203-208`) | populated | +| `auth_attempts`, `server_nonce_time` | `= 0` (`:215-216`) | `server_nonce_time = 0` only (`:662`) | + +Only four fields are common; the rest are opposite. A fold worded as "bitmap + +`last_seen_counter`" silently drops `nonce_counter = 0` the first time someone tidies the +surrounding lines — and a device that keeps its outbound counter across a re-auth while the client +restarts at 0 walks into `H2`'s keystream reuse with *itself*. + +**Correction:** define `resetNonceState()` = +`{ nonce_counter = 0; last_seen_counter = 0; memset(replay_bitmap); integrity_failures = 0; }`, +naming all four fields. Leave every other field where it is. + +--- + +## LOW + +**L1 — D4 is not a live bug.** The facts are right (`replay_window_index` is a function static at +`:152`, reset by neither `:217` nor `:660`); the consequence is not. Both reset sites `memset` the +ring to **all zeros**, so writing from an arbitrary offset into a uniformly-empty 64-slot ring +with a +1 index produces exactly the same strict-FIFO eviction order as starting from 0. No +counter's accept/reject differs. The one real artifact is the `0`-as-empty sentinel: counter 0 can +never be accepted through the backward branch — masked today by D3's exemption, i.e. **D3 is +load-bearing for D4's representation**. Demote D4 to "latent"; keep the fix. + +**L2 — `nonceCommit()` placement is under-specified.** The success arm has an early return between +`:717` and `integrity_failures = 0`: + +```c +encryption.cpp:717-725 + if (success) { + uint8_t payload_length = decrypted_with_length[0]; + if (payload_length > encrypted_len - 1) { ...; return false; } // authentic frame + ... + encryptionSession.integrity_failures = 0; +``` + +If the commit lands after `:722`, an authentic-but-malformed frame is left replayable — and +today's code *does* commit it (`:149-153` is unconditional), so this is a behaviour change. Say +"first statement of the success arm, `:718`". + +**L3 — Decision B's storage table contradicts its own formula.** `2W × 8 B` = 16 B per unit ✓ for +the 32-row (512 B). Rows 2–3 are exactly 2× too large: 127 × 16 = **2,032 B** not 4,064; +255 × 16 = **4,080 B** not 8,160. Conclusion unaffected. + +**L4 — `PIPE_MAX_W + MAX_PTO` over-counts by one.** `MAX_PTO = 3` yields **two** probe sends: the +client increments then raises at the threshold before sending (`device.py:2721-2726`). 35 → 34 +(18 on `esp32-N4`). Conservative direction; moot under `C1`. + +**L5 — CI step runs 11×.** `.github/workflows/main.yaml` is one `build` job with an 11-entry +`matrix.environment` (`:10-23`). Use a separate top-level `host-tests` job. Confirmed safe: +`tools/test_nonce_window.cpp` is invisible to every firmware build — +`build_src_filter = +<*> -` (`platformio.ini:34`) is relative to the default +`src_dir` and no env adds `tools/`. + +**L6 — Line-reference drift.** The ring drop is `esp32_ble_callbacks.h:127-128` (`:126` is the +RELEASE store on the *success* path), not `:126-127`; the NACK is `communication.cpp:698-703`, not +`:700-701`. Everything else spot-checked is exact. + +**L7 — `NONCE_BAD_SESSION` silently stops being tamper evidence.** Today a `session_id` mismatch +counts (`:122-128` → `:691-696`); the plan routes it to "untouched". Probably right, but it is a +decision, not a consequence of D1. Also `:123-127` logs both full session IDs at `od_log_error` — +with counting removed an attacker can drive that line indefinitely. State the change; +demote/rate-limit the log as Step 4 already does for the out-of-window log. + +--- + +## Verified CORRECT — do not churn on these + +1. **D1 is real and is the wedge.** `:691-696` increments `integrity_failures` on *any* + `verifyNonceReplay` failure including plain packet loss, clearing at 3, after which + `communication.cpp:665-670` answers everything `RESP_AUTH_REQUIRED`. +2. **D2 is real.** `last_seen_counter` (`:149-151`) and the ring write (`:153`) both land before + `aes_ccm_decrypt` at `:714`. Step 3's "called from exactly one place" is achievable — + `decryptCommand` is the sole caller. +3. **D3 is real and exploitable** (`:136`). See `H3` — worse than the plan says. +4. **The bitmap state machine is correct as specified.** Worked by hand: fresh session + (`last_seen=0`, bitmap 0) accepts counter 0 exactly once via `fwd==0` + clear bit 0, needing no + `has_seen_counter`; forward commit by `d` moves old bit *i* (counter `L−i`) to bit `i+d`, which + under `L'=L+d` denotes `L−i` ✓ for d = 1, 63, 64, 65, 127; `d ≥ 128` clears wholesale and every + discarded counter is then ≥128 behind so it is rejected on width, never mis-reported unseen; + backward bit indices 1…127 valid with `back ≥ 128` rejected, so the backward window is exactly + `OD_NONCE_BACKWARD_BITS − 1 = 127` as stated; a forward cap < 128 means a legal slide never + needs a wholesale clear, so the two sides genuinely decouple, and the plan's claim that a cap + *wider* than the backward window would still be harmless is also true. Only the arithmetic form + (`M1`) and the cap value (`C1`) are wrong. +5. **"The ring is not buggy" — correct conclusion, off-by-one proof.** Attempts to construct a + sequence evicting an accepted counter `d` from the 64-slot ring while `d` is still inside ±32 + all fail. The tight count is `2W − 1 = 63`, not `2W`: all counters acceptable while `d` stays + in-window lie in `[L₀−32, d+32]`, and at least one member is provably already seen (the + `last_seen_counter` value preceding `d`, or — in the fresh-session corner — counter 0, blocked + by the zeroed ring's sentinel). 63 writes leave the index one short of `d`'s slot. So `D=64` is + sound **with exactly one slot of margin**, and the parent plan's 256/128 likewise. `D ≥ 2W` is + conservative and its conclusion holds; the real argument for the bitmap — non-obvious + combinatorics maintained by hand across two files — is strengthened by how tight the margin + actually is. +6. **Decision C is clean.** `verifyNonceReplay` has exactly three occurrences outside its + definition: `encryption.h:17`, `main.h:276`, and the sole call at `encryption.cpp:691`. Nothing + in `src/`, `tools/`, or the nRF build path references it. (`Firmware_NRF54` has its own private + copy at `opendisplay_pipe.c:427-455` — separate repo, unaffected, but it carries all four + defects and should be scheduled.) +7. **Decision D's factoring is sound and build-safe** — see `L5`. +8. **Sizing arithmetic.** 512 B → 16 B = **−496 B** ✓. 256-entry ring = 2,048 B → **+1,536 B** ✓. + No `platformio.ini` change; the `esp32-N4` link-headroom question is genuinely moot. +9. **Client premises.** One counter burned per transmission including repairs + (`device.py:747-760`) ✓; window blocks for an ACK and PTO resends exactly one chunk + (`:2688-2694`, `:2715-2726`) ✓; `MAX_PTO = 3` at `commands.py:93` ✓; `PIPE_MAX_W` 32 / + 16-on-`esp32-N4` (`structs.h:45-53`) ✓. Missing from the premise set: the SACK repair site + (`C1`). +10. **The `communication.cpp:772-775` invariant survives.** Commit still happens for every frame + that decrypts, including duplicates `handlePipeWriteData` discards. Only the name and ordering + clause need rewriting, as Step 6 says. +11. **Scope boundaries are right.** Leaving the 30 s auth-challenge window alone is correct (wire + contract at `include/opendisplay_protocol.h:384`, cannot wedge anything). Deferring + `session_timeout_seconds` and the link-drop guard to Phase 5 is correct, and the plan is + admirably explicit that Phase 1 closes only the nonce arm. +12. **No new failure mode on either target.** nRF has no command ring — `imageDataWritten` runs + inline on the Bluefruit callback task ([ble_init.cpp:157](../src/ble_init.cpp)), + single-threaded and in order — so its only gap source is air loss, which the link layer + repairs; a wider forward cap is inert there. On ESP32 nothing in Phase 1 touches the ring, the + loop watchdogs, or advertising. The LAN TLS path bypasses `decryptCommand` entirely + ([communication.cpp:663](../src/communication.cpp), origin-gated on `ORIGIN_LAN_TLS`) and is + untouched by every Phase 1 change; the plain-LAN path shares the one `encryptionSession` + exactly as today. `handleAuthenticate`'s re-auth path (`:583-585` → `clearEncryptionSession()` + → `:654-662`) resets both counters and the ring on both sides, unchanged in shape. + +--- + +## Summary of findings + +| # | Sev | Class | Finding | Correction | +|---|-----|-------|---------|-----------| +| C1 | Critical | plan | Forward cap 64 derived from a non-bound; SACK repair (`device.py:2792`) + preserved ACK backlog (`:781`) reach ~66 at `ack_every=2`, ~96 at `ack_every=1`, both user-selectable (`config_flow.py:104-113`) | Cap **128**; re-derive from `max_retx`; rewrite the Step 6 comment as a mechanism | +| H1 | High | plan | Decision E's "client recovers on its own" is false — 3-byte `0xFF` NACK raises `IntegrityCheckError` (`device.py:834`) uncaught by the pipe loop | Send no NACK (or a pipe ACK) for nonce rejections on `0x0081`; firmware-only | +| H2 | High | shipping | Device/client counters share one nonce space → CCM keystream reuse (`encryption.cpp:158-174` vs `crypto.py:92-113`) | Out of scope, but **record it** next to Decision E | +| H3 | High | shipping | D3 understated: exempted re-accept writes the ring (`:152-154`), so 64 replays flush it and unlock the last 32 counters | Restate D3; add the ring-flush hardware test | +| M1 | Medium | plan | Step 2 vs Decision D disagree; both, plus `-diff`, are UB on attacker-controlled counters | Two wrapping `uint64_t` deltas; no signed arithmetic | +| M2 | Medium | plan | "Tight is correct" rests on a jam-forward DoS that cannot occur post-D2 | Delete "Why not more"; it is the only argument against C1 | +| M3 | Medium | plan | `resetNonceState()` folds blocks sharing only four fields; drops `nonce_counter = 0` | Define the helper by naming all four fields | +| L1 | Low | plan | D4 is latent, not live | Demote the claim; keep the fix | +| L2 | Low | plan | `nonceCommit()` vs the `payload_length` early return at `:719-722` | "First statement of the success arm, `:718`" | +| L3 | Low | plan | Storage table rows 2–3 are 2× its own formula | 2,032 B and 4,080 B | +| L4 | Low | plan | `MAX_PTO=3` yields 2 probe sends | 34 / 18 (moot under C1) | +| L5 | Low | plan | CI step inside an 11-leg matrix | Separate `host-tests` job | +| L6 | Low | plan | Two line refs drift | Fix in place | +| L7 | Low | plan | `NONCE_BAD_SESSION` policy change unremarked; log attacker-drivable | State it; demote/rate-limit | + +## Is Phase 1 safe to implement as written? + +**Safe: yes.** No new freeze mode, no new remote DoS, no regression on either target. Every change +is confined to a path whose current behaviour is strictly worse. It is genuinely independent of +Phases 2–6 and genuinely deliverable alone. + +**As written: no.** Mandatory before implementation: `OD_NONCE_FORWARD_CAP = 128` with the +derivation replaced (`C1` + `M2`), and the unsigned delta form (`M1`) so the code and the UBSan +gate agree. One more (`H1`) decides whether Phase 1 saves the *transfer* or only the *device*; it +is five lines and should be in scope. `H2` and `H3` need a paragraph each, not code. With those, +Phase 1 is the right first change and should ship ahead of the rest of the program. diff --git a/docs/PLAN_FREEZE_PROOFING_2026-07-26.md b/docs/PLAN_FREEZE_PROOFING_2026-07-26.md new file mode 100644 index 0000000..98fd489 --- /dev/null +++ b/docs/PLAN_FREEZE_PROOFING_2026-07-26.md @@ -0,0 +1,186 @@ +# Freeze-Proofing the OpenDisplay Firmware (branch: debug/ble-hardening) + +> **Revised 2026-07-26** after adversarial review. The review found 4 Critical defects — two of +> which meant the headline deliverable did not work. All corrections are folded in below and +> tagged `[C1]`…`[X7]`. Full review: `docs/FINDINGS_FREEZE_PROOFING_PLAN_REVIEW_2026-07-26.md` +> (**first action on implementation: move it there from +> `~/.claude/plans/create-a-comprehensive-plan-majestic-hamster-agent-a0d75f8717fc20eb0.md`** — +> plan mode blocked writing it into the repo). + +## Context + +Field failures: during PIPE_WRITE uploads from Home Assistant, lost ACKs / blind retransmits leave the device **frozen or unresponsive**. Investigation traced four wedge mechanisms plus several unbounded waits: + +1. **Nonce replay-window overrun** — the client burns a nonce per transmission (incl. retransmits); losing a full 32-frame window puts the next frame >32 ahead of `last_seen_counter`; 3 rejections (= client's `MAX_PTO`) trip `integrity_failures >= 3` → `clearEncryptionSession()` mid-transfer. The device then answers everything `0xFE` while `directWriteActive` keeps the panel powered 15 min. Bonus bug: `verifyNonceReplay` commits `last_seen_counter` **before** CCM tag verification ([encryption.cpp:149-155](../src/encryption.cpp)). +2. **Pipe fatal-NACK latch** — `sendPipeNack` leaves `pipeState.active=true` forever ([display_service.cpp:2562-2578](../src/display_service.cpp)); `transferActive()` latches → touch dead ([touch_input.cpp:584](../src/touch_input.cpp)), WiFi roam dead; the 15-min watchdog keys on `directWriteActive`, just cleared → nothing bounds it. +3. **Queue overflow** — response ring (10) drops newest on full ([communication.cpp:117-121](../src/communication.cpp)); command ring drops on full ([esp32_ble_callbacks.h:128](../src/esp32_ble_callbacks.h)). The **command** ring is never flushed on disconnect, so stale commands survive. *(The response ring IS drained whenever no central is connected — [main.cpp:307-312](../src/main.cpp) — so stale responses were never the wedge* `[L1]`*.)* +4. **Cross-transport session clobber** — LAN accept/close unconditionally `clearEncryptionSession()` ([wifi_service.cpp:879, :804](../src/wifi_service.cpp)), killing a live BLE session. BLE+LAN can be connected simultaneously today. +5. **Unbounded waits** — `pwrmgmLockTake` infinite spin ([display_service.cpp:401-408](../src/display_service.cpp)); `powerOff` stuck-button loop ([power_latch.cpp:87-90](../src/power_latch.cpp)); FastEPD refresh has no firmware-side bound. +6. **Session lifetime is unbounded and unscoped** — nothing clears `encryptionSession` on a BLE disconnect (only LAN does, [wifi_service.cpp:804](../src/wifi_service.cpp)), so the key and `last_seen_counter` survive into the next connection. The survival is unusable for resumption (a reconnecting client resets its counter to 0 → rejected as out-of-window or as a ring replay) and it enables a real attack: `verifyNonceReplay` exempts `counter_diff == 0` from the replay-ring check ([encryption.cpp:136](../src/encryption.cpp)), so a captured last-frame replayed after the owner disconnects is **accepted and re-executed**. Separately, `session_timeout_seconds` expiry is evaluated inside `isAuthenticated()` on every command ([encryption.cpp:195-199](../src/encryption.cpp)) and so can fire mid-transfer — a deterministic wedge on a long upload, since the client's proactive re-auth is deliberately skipped for the whole pipe stream ([device.py:778-786](../../py-opendisplay/src/opendisplay/device.py)). + +**User decisions (confirmed):** software-only supervisor (no hardware WDT — reset state, never reboot; a reboot wipes RTC incl. `displayed_etag` and forces a boot-screen redraw); clear encryption session on BLE disconnect; **encryption is always scoped to the life of the connection — `session_timeout_seconds` expiry is disabled**; BLE idle timeout = 5 minutes, kept firmware-local (not promoted to the canonical protocol header). + +**Platform facts (verified):** +- ESP32: commands queue via SPSC ring (NimBLE host task → loop task, [main.cpp:406-423](../src/main.cpp)); responses via a 10-slot ring. nRF: NO queues — `imageDataWritten` runs inline on the Bluefruit *Callback* task; `Bluefruit.begin(1,0)` already caps BLE at 1 link. +- `-DCONFIG_BT_NIMBLE_MAX_CONNECTIONS=1` does NOT work: precompiled `sdkconfig.h:613` redefines it to 3 and wins (empirically verified — do not re-add). Enforce in `onConnect` (NimBLE fills `m_connectedPeers` BEFORE `onConnect`, so `getConnectedCount() > 1` is a valid gatecrasher test). +- OTA exception satisfied by design: ESP32 has no OTA (0x0051 = `esp_restart`); nRF DFU jumps to the bootloader with the app gone. Comment it for future OTA work. +- Drain-loop trap: [main.cpp:409-417](../src/main.cpp) caches `tail` before dispatch, stores `tail+1` after — a flush from handler context gets clobbered. +- `setConnectableMode(NON)` internally calls `setFlags(0)` (one-way), so re-push `setAdvertisementData(*advertisementData)` before `start()` — same trap already documented at [ble_init.cpp:307-312](../src/ble_init.cpp). + +--- + +## Hard constraint — NO wire protocol changes + +**Nothing in this plan may change the BLE/LAN wire protocol.** This is a firmware-internal robustness effort; every fix must be observably compatible with today's clients (`py-opendisplay`, the HA integration, the web configurator) and with the other three firmware repos. + +Concretely, the following are **out of bounds** for every phase: +- Editing `include/opendisplay_protocol.h` — it is a byte-for-byte vendored copy of `../opendisplay-protocol/src/opendisplay_protocol.h`. No local edit, and no change pushed through the canonical repo either. +- Adding, removing, or renumbering any `CMD_*` opcode or `RESP_*` code; changing the meaning of an existing one. +- Changing frame layout, framing, header/field sizes, nonce or CCM parameters, or the auth handshake sequence. +- Changing the config-packet layout in `include/opendisplay_structs.h` (field add/remove/resize/reorder), which is the same contract by another name. +- Changing any value the protocol header specifies — notably the **30 s auth-challenge window** (already recorded under *Deliberately NOT changed*) and the **LAN 30 s idle timeout** (`opendisplay_protocol.h:984`). +- Changing client-observable behaviour documented in `docs/pipe-write-protocol.md` (SACK semantics, discard rules, NACK meaning). + +What **is** in bounds, and why each stays inside the constraint: +- Firmware-local constants that no client reads: `OD_NONCE_WINDOW`, `OD_BLE_IDLE_DISCONNECT_MS`, the pipe error-release deadline, supervisor/backstop timeouts, `COMMAND_QUEUE_SIZE`. None appear on the wire; a client cannot observe their value, only the (already-legal) behaviour they produce. +- Widening the replay window and fixing the `counter_diff == 0` hole — accepting *more* legitimate frames and rejecting a replay are both already-permitted outcomes of the existing nonce rules. +- Disabling `session_timeout_seconds` expiry — the field's own spec already defines `0 = no timeout (persists until disconnect)` ([opendisplay_structs.h:916](../include/opendisplay_structs.h)); the firmware simply behaves as if the field is always 0. The struct field stays, unchanged in size and position, and becomes advisory-only. +- Dropping a link (idle timeout, supervisor abort, connection-exclusivity refusal) — disconnect is always a legal outcome; clients already handle it and reconnect. +- Sending an existing NACK/`RESP_*` code in a new situation, as long as the code's documented meaning is unchanged. + +If any phase appears to require a protocol change to work, **stop and escalate** — do not push a header change through `../opendisplay-protocol` as part of this work. Documentation-only additions (a note in `docs/pipe-write-protocol.md` §5.1, field notes in `tools/od-device-cli.py`) are permitted and expected, provided they describe behaviour the current spec already allows. + +Verification of the constraint itself, run before any phase is called done: +```bash +cd ../opendisplay-protocol && tools/sync_protocol_header.py --check --only Firmware # must pass, unchanged +cd ../Firmware && git diff main --stat -- include/opendisplay_protocol.h include/opendisplay_structs.h # must be empty +``` + +--- + +## Phase order + +Reordered per review: **root cause first, owner token before anything depends on it, supervisor before the escalations that rely on its accounting.** + +### Phase 1 — Nonce/replay correctness `(was Phase 3 — highest value-per-risk, ship first)` +[encryption.cpp](../src/encryption.cpp) / [encryption_state.h](../src/encryption_state.h) +- Split `verifyNonceReplay` → pure `nonceCheck()` (OK / BAD_SESSION / OUT_OF_WINDOW / REPLAY, **no state writes**) + `nonceCommit(counter)` (advance `last_seen_counter` + seen-set). +- `decryptCommand`: `nonceCheck` → nonce failures return false **without** touching `integrity_failures` (loss ≠ tampering; only a CCM tag failure is tamper evidence) → CCM decrypt → on success `nonceCommit` + reset counter; on tag failure increment (≥3 → clear session, unchanged). +- `[M1]` Make the window **symmetric**: `OD_NONCE_WINDOW = ±128` with `replay_window[]` grown 64 → 256 ([encryption_state.h:21](../src/encryption_state.h), +1.5 KB `.bss`). An asymmetric +128/−32 lets one replayed frame jam `last_seen_counter` forward and strand the next 96 legitimate frames. **If `esp32-N4` won't link with +1.5 KB, fall back to ±64 with the existing 64-entry ring** — never forward-wider than the ring can police. +- Move `replay_window_index` out of the function static ([encryption.cpp:152](../src/encryption.cpp)) into `encryptionSession` so `clearEncryptionSession()` resets it (real bug today). +- **Close the `counter_diff == 0` replay hole.** The ring check is guarded by `nonce_counter <= last_seen_counter && counter_diff != 0` ([encryption.cpp:136](../src/encryption.cpp)); the `!= 0` term exists only so a fresh session's first frame (client counter 0 vs `last_seen_counter` initialised to 0) isn't flagged, but it also exempts **replay of the current highest-seen counter** from the ring entirely — the CCM tag validates because the frame is genuine, and the command re-executes. Replace the special case with an explicit `has_seen_counter` bool (or a sentinel initial value) so counter 0 is handled without exempting equality, then apply the ring check uniformly for `nonce_counter <= last_seen_counter`. Harmless for pipe DATA (duplicate seq is discarded) but not for a config write, power-off, or buzzer/LED command, which is typically what the last frame of a session is. +- Log: distinguish `nonce out-of-window (diff=%lld) — frame dropped, session kept` from `CCM tag failure %u/3`. + +### Phase 2 — Bound every unbounded wait `(was Phase 0, corrected)` +- `[C2]` **`pwrmgmLockTake` — do NOT steal.** Legitimate holds already exceed 10 s: `bbepWaitBusy` caps at **30 000 ms** for 3/4/7-colour panels (`bb_ep.inl:3959-3975`) and `epdSessionForceOffLocked` holds the lock across `bbepSleep` → `bbepWaitBusy` (`bb_ep.inl:4122`). A steal on a bare 0/1 flag with no owner means two tasks drive the same SPI/CS, the true holder's later `Give` unlocks it under the stealer (mutual exclusion permanently dead), and `pwrmgmState` ends up `PWR_ACTIVE` on a dead rail. **Instead:** `pwrmgmLockTake` returns `bool` with a **60 s** deadline (≥2× worst-case busy wait); on expiry log ERROR, return false, caller skips its panel work and sets a "panel state unknown" flag that `abortToKnownState` reports. If a forced take is ever genuinely needed, add `volatile TaskHandle_t pwrmgmOwner` so the original holder's `Give` becomes a detectable no-op. +- `powerOff` button wait ([power_latch.cpp:87-90](../src/power_latch.cpp)): bound at 10 s, then drop the latch anyway. +- `[X2]` Set/clear `epdRefreshInProgress` around **both boot-refresh paths** (`refreshBootScreenFull` [display_service.cpp:533-542](../src/display_service.cpp) and the FastEPD boot path at `:1588-1594`). Today a 30–60 s Spectra boot refresh is invisible to every `epdRefreshInProgress` gate — including the supervisor's "never interrupt a refresh" rule. +- `[X3]` **FastEPD really is unbounded.** `fastepd_wait_refresh()` is a stub that ignores its timeout ([display_fastepd.cpp:228-231](../src/display_fastepd.cpp)) and `waitforrefresh(60)` short-circuits to it, so the "60 s cap" does not exist on IT8951/E1004. Implement `fastepd_wait_refresh` as a real busy poll against the IT8951 LUT-busy register honouring `timeout_sec`, and wrap **`fastepd_direct_refresh`** (the path a real transfer takes, [display_service.cpp:2422-2423](../src/display_service.cpp)) — not just `fastepd_full_update`. +- `[X1]` **I2C — DOWNGRADED after verification; the original finding was wrong.** The review claimed a wedged GT911 spins unbounded and that touch polls too rarely to notice. Neither holds: + - The driver **already gives up**: 5 consecutive read failures (`TOUCH_I2C_FAIL_DISABLE_THRESHOLD`, [touch_input.cpp:39](../src/touch_input.cpp)) trigger `touch_disable_controller(..., "too many I2C read failures")` ([:642](../src/touch_input.cpp), [:677](../src/touch_input.cpp)), and `TOUCH_I2C_FAIL_BACKOFF_MS` suppresses INT-driven re-reads while failing ([:609-611](../src/touch_input.cpp)). A wedged controller is dropped, not retried forever. + - The poll rate is fine — the floor is 100 ms (`TOUCH_PROCESS_MIN_INTERVAL_MS`, [:38](../src/touch_input.cpp)), enforced globally at [:589](../src/touch_input.cpp) **regardless of the configured `poll_interval_ms`**, whose per-controller value at [:600](../src/touch_input.cpp) can only ever slow polling further. (Note: [opendisplay_structs.h](../include/opendisplay_structs.h) documents `0 = 25 ms default`; the firmware uses 100 and cannot go below it. Header/behaviour divergence, documentation-only, not fixed here.) + - **So: no nine-clock SDA recovery, no new state machine.** The only residual is that each failing transaction blocks for the Arduino default (~50 ms on ESP32) since `Wire.setTimeOut()` is never called — worst case ~250 ms of blocked `loop()` before the controller is disabled. Bounded and acceptable. **Optional**: add `Wire.setTimeOut(25)` after each `Wire.begin()` (incl. `wireBeginForOpenDisplay`, [display_service.cpp:785-800](../src/display_service.cpp)) to halve that window. A wedged GT911 costs touch until reboot; it cannot freeze the device. +- Loop command drain: 2 s wall-clock cap alongside the count cap. +- `[L3]` Delete or rename the inert `-DCONFIG_FREERTOS_WATCHDOG_TIMEOUT_S=120` in every ESP env — the IDF 5.x symbol is `CONFIG_ESP_TASK_WDT_TIMEOUT_S` and the precompiled `sdkconfig.h` wins regardless. Leaving a dead knob that reads like a 120 s guarantee misleads the next reader. Add a comment that the real TWDT is 5 s/panic on IDLE0 and that today's long waits survive only because they all yield. + +### Phase 3 — `abortToKnownState()` + queue flushes + drain-trap fix `(was Phase 1)` +New `src/session_guard.h/.cpp` (both targets; ESP32 parts `#ifdef TARGET_ESP32`, LAN parts `#ifdef OPENDISPLAY_HAS_WIFI` — **not** `TARGET_ESP32`, since `esp32-N4` is ESP32 without WiFi). +- Flags: `commandDrainAbortPending`, `commandQueueOverflowAbort`, `responseQueueOverflowAbort`; `g_lastProgressMs` + `markSessionProgress()`. +- `flushCommandQueue()` / `flushResponseQueue()` in main.cpp; loop-task only; `tail := head` snapshot (SPSC-safe — tail has a single writer). +- `abortToKnownState(reason, dropLink)`: log first → optional client NACK (skip when dropping link) → set drain-abort flag → flush command ring → `cleanupDirectWriteState(true)` → `cleanupPartialWriteOnDisconnect()` → `resetPipeWriteState()` → **new** `resetChunkedWriteState()` → **new** `touchForceResume()` → buzzer/LED stop → `epdSessionForceOff()` **only if** `!epdRefreshInProgress` → `clearEncryptionSession()` → flush response ring → if dropLink: disconnect → release owner token → `markSessionProgress()`. +- `[M3]` `touchForceResume()` must also clear `directWriteTouchSuspended` ([display_service.cpp:2035-2038](../src/display_service.cpp)) and assert the counter reached 0. Keep the stated ordering (`cleanupDirectWriteState` first). +- `[M5]` **Drain-trap fix — exact placement**: the check goes **between** [main.cpp:415](../src/main.cpp) and `:416`, i.e. immediately after `imageDataWritten` returns and *before* `commandQueue[tail].pending = false`, breaking without the tail store. Placed after `:416` it writes into a slot the producer may have re-filled. While there, delete the vestigial `pending` field — it has no readers in either ring. +- `[H4]` `g_commandInFlight` is a `volatile uint8_t` **depth counter**, not a bool. Bluefruit's `ada_callback_invoke` falls back to invoking the write callback **inline on the BLE task** when `rtos_malloc` fails (`BLECharacteristic.cpp:538-542`), so "single task on nRF" is not an invariant — heap pressure during a large transfer is exactly when it breaks. + +### Phase 4 — Connection exclusivity `(was Phase 4, corrected)` +- Owner token: `OWNER_NONE/BLE/LAN`, `linkClaim()`/`linkRelease()`. +- `[C3]` **Refuse with `BLE_ERR_REM_USER_CONN_TERM` (0x13), NOT `BLE_ERR_CONN_LIMIT` (0x09).** `NimBLEServer::disconnect` forwards to `ble_gap_terminate` (`NimBLEServer.cpp:321-332`) and 0x09 is not in the Core Spec's legal `HCI_Disconnect` reason allowlist — the controller rejects it with 0x12 and the gatecrasher stays connected while the code looks like it worked. Check the `bool` return; log WARN on failure. +- `[C4]` **Do not let the refusal re-enter the shared cleanup.** `onDisconnect` ([esp32_ble_callbacks.h:57-70](../src/esp32_ble_callbacks.h)) is a blind flag-setter, and the `ownerStillUp` guard is inside `#ifdef OPENDISPLAY_HAS_WIFI` ([main.cpp:328-338](../src/main.cpp)) — so on **`esp32-N4`** a refused stranger's disconnect tears down the incumbent's live transfer (a new remote DoS). Two required changes: (a) capture the refused conn handle in `onConnect` and skip raising `bleDisconnectCleanupPending` for it in `onDisconnect`; (b) move the `ownerStillUp` early-return **out** of the `#ifdef` — its `getConnectedCount() > 0` half is unconditionally correct. +- `[M2]` Prefer **evicting an idle incumbent** over refusing a reconnect: after an abrupt client loss the link lingers until supervision timeout (4–32 s) and a returning client would be refused. If the incumbent has `!transferActive()` and last RX older than ~10 s, terminate the old link and accept the new one. Refuse only when the incumbent is actively transferring. +- LAN accept ([wifi_service.cpp:874-900](../src/wifi_service.cpp)): `linkClaim(OWNER_LAN)` or `incoming.stop()`; scope both `clearEncryptionSession()` sites to `OWNER_LAN`. +- Advertising while LAN owns: `esp32_set_ble_connectable(bool)` — stop → `setConnectableMode(NON/UND)` → re-push `setAdvertisementData` → start. Skip during the post-deep-sleep-wake window. `[M4]` **Check the return of both `setAdvertisementData()` and `start()`**; on failure force connectable mode, re-push, retry, and set `bleRestartAdvertisingPending` — otherwise a failed `start()` after a `stop()` leaves the radio permanently dark with nothing retrying. +- `[X7]` Add `advertisingHealthTick()`: no peer + not advertising (`getAdvertising()->isAdvertising()`) + no pending flag for >30 s → force restart, log WARN. Closes the "unresponsive but not frozen" hole at [ble_init.cpp:232-235](../src/ble_init.cpp), where a stale nonzero connection count *clears* the pending flag. + +### Phase 5 — Disconnect hardening `(was Phase 2, corrected — now lands after the owner token)` +- `[H2]` ESP32 `serviceBleDisconnectCleanup`: place `flushCommandQueue(); flushResponseQueue(); clearEncryptionSession();` **after** the (now unconditional) `ownerStillUp` early-return, scoped to `linkOwner() == OWNER_BLE`. Putting the clear before the guard would let every WiFi-lost tick ([main.cpp:452-457](../src/main.cpp)) destroy a live BLE session — the very bug Phase 4 exists to fix, from a second code path. +- `[H4]` nRF `disconnect_callback`: **defer** the session clear — set `nrfSessionClearPending`, service from `loop()` when the in-flight depth counter is 0. A `memset(session_key)` landing mid-`aes_ccm_decrypt` on the inline-fallback path is a real (if rare) race. +- Pipe NACK latch: add `error_since_ms` to `PipeWriteState` (genuine +4 B addition — the struct has no timestamp today) and a `pipeErrorTick()` in loop. `[L2]` Use **10 s**, not 60 s, and describe it honestly as a **hardware-release deadline** — py-opendisplay treats every `0x81` NACK as immediately fatal and never re-reads the ACK position, so the "client-retry window" rationale was fiction. Confirmed this does **not** break `docs/pipe-write-protocol.md` §5.1 (client-observable discard behaviour is unchanged) — add a line to §5.1 noting the reset. + +#### Session lifetime := connection lifetime + +**Disable `session_timeout_seconds` expiry.** `checkEncryptionSessionTimeout()` ([encryption.cpp:221-232](../src/encryption.cpp)) always returns true for an authenticated session; age-based expiry is removed. Encryption is scoped to the life of the connection and nothing else. + +- **No protocol change and no header edit.** The canonical field already documents `0 = no timeout (persists until disconnect)` ([opendisplay_structs.h:916](../include/opendisplay_structs.h)) — the firmware now behaves as if the field is always 0, which is an already-specified, already-supported value. The struct field stays (removing it is a cross-repo change); it simply becomes advisory-only on this firmware. Document it as ignored in `tools/od-device-cli.py`'s field notes. +- **No client breakage.** py-opendisplay's `_reauthenticate_if_needed` returns immediately when the value is 0 ([device.py:795-796](../../py-opendisplay/src/opendisplay/device.py)); with a legacy nonzero config it performs one unnecessary but harmless re-auth at 90% — that path is only reached from `_write`, never from `_write_pipe_frame`, so it cannot land mid-stream. Provision new units with 0 to skip the pointless handshake. +- **This removes a whole freeze class.** Expiry was evaluated inside `isAuthenticated()` on *every* command dispatch, so it could fire mid-transfer — deterministically wedging any upload longer than the configured timeout, since the client's proactive re-auth is skipped for the entire pipe stream. It also removes a query-with-side-effects: `isAuthenticated()` currently mutates session state as a side effect of being asked a question. +- With expiry gone, call site 1 of `clearEncryptionSession()` disappears, and site 2 (`handleAuthenticate`'s re-auth path, [encryption.cpp:583-585](../src/encryption.cpp)) simplifies to "authenticated → clear and re-challenge", which is the correct behaviour for a client-initiated re-auth. + +**Make a dead session with a live link impossible.** Add the guard inside `clearEncryptionSession()` itself ([encryption.cpp:201](../src/encryption.cpp)) rather than at each call site, so future callers cannot regress it: if a client is connected and the clear was not client-initiated, raise a flag that `loop()` services by dropping the link. A cleared session under a live link is invisible to the client — it keeps sending encrypted frames that all bounce `0xFE` and never re-authenticates mid-stream — so this is the difference between a recoverable error and a wedge. Surviving call sites and their disposition: + +| Site | Disposition | +|---|---| +| [encryption.cpp:585](../src/encryption.cpp) re-auth challenge | **Exempt** — client-initiated, expects a new session | +| [encryption.cpp:670](../src/encryption.cpp) `aes_cmac` failure | Exempt — aborts a session being born | +| [encryption.cpp:695](../src/encryption.cpp) / [:732](../src/encryption.cpp) `integrity_failures >= 3` | Must drop the link (Phase 1 removes the nonce trigger; the CCM-tag trigger remains) | +| [communication.cpp:67](../src/communication.cpp) `reloadConfigAfterSave` | Must drop the link — a config write always arrives over a live link, and security settings may have changed | +| [wifi_service.cpp:804](../src/wifi_service.cpp) / [:879](../src/wifi_service.cpp) LAN | Scope to `OWNER_LAN` (Phase 4) | +| [config_parser.cpp:883](../src/config_parser.cpp) boot load | Exempt — not connected | + +**Note in code that deep sleep already wipes the session.** `encryptionSession` is plain `.bss` ([main.h:289](../src/main.h)), not `RTC_DATA_ATTR`, so a deep-sleep cycle destroys it regardless. Comment this so nobody later "optimises" it into RTC memory — persisting the key and `last_seen_counter` across sleeps would reintroduce the replay vector Phase 1 closes. + +### Phase 6 — The 10-minute supervisor `(was Phase 6, corrected)` +- `[C1]` **Progress means the state machine advanced — never "a command arrived" or "a notify succeeded."** After `clearEncryptionSession()`, [communication.cpp:664-670](../src/communication.cpp) answers every retry with `RESP_AUTH_REQUIRED` — a dispatch *and* a notify per retry — so dispatch/notify stamps keep `g_lastProgressMs` fresh forever and the supervisor never fires in the exact wedge it was built for. Stamp **only** at: `pipeState.expected_seq` advancing (inside the in-order accept), `directWriteBytesWritten` increasing, `chunkedWriteState.receivedChunks` incrementing, `partialCtx` byte counter advancing, refresh completion, and `handleAuthenticate` success. **Not** on command dispatch, notify, or LAN frame dispatch. +- Wedge: `(transferActive() || chunkedWriteState.active || directWriteActive) && now - g_lastProgressMs > 600000` → if `epdRefreshInProgress`, log and retry next pass; else `abortToKnownState("supervisor", true)`. +- `[H3]` **Keep the existing wall-clock watchdogs** ([main.cpp:436-442](../src/main.cpp), `checkPartialWriteTimeout`) as a backstop, raised to 20 min. They key on *start* stamps nothing refreshes, so they bound cases a progress predicate can't. Delete them only after hardware soak proves the progress arm fires. +- **nRF has NO transfer watchdog today — H3 is "keep" on ESP32 but "ADD" on nRF.** Both 900 s bounds live inside the `#ifdef TARGET_ESP32` arm of `loop()`: the direct-write check at [main.cpp:438](../src/main.cpp) and the `checkPartialWriteTimeout()` call beside it. The nRF `loop()` body is the `#else` arm and evaluates neither, so on nRF a stalled transfer is bounded by **nothing** — not today, and not by H3's "backstop" unless it is explicitly added there. Neither the original plan nor the adversarial review caught this. The supervisor and the wall-clock backstop must both be wired into the nRF `loop()` path, and the nRF hardware soak must cover a stalled transfer explicitly rather than assuming ESP32 parity. +- `[X5]` Add `transferActive()` to the `workInFlight` disjunction ([main.cpp:474-479](../src/main.cpp)) so a latched transfer with a dropped link can't reach `enterDeepSleep()` with the panel rail up. +- `[H4]` Abort only when the in-flight depth counter is 0. If stale >10 min while in-flight, log an ERROR heartbeat — Phase 2's bounds are the recovery story. +- No hardware WDT (user decision). Comment the OTA-exception rationale. +- `[X4]` Config chunked-write has no timer of its own ([communication.cpp:496](../src/communication.cpp) set, cleared only on completion/malformed/auth-fail) — it is covered by the supervisor predicate *once C1 is fixed*. Dependency noted deliberately. + +### Phase 7 — Queue-full handling + BLE idle timeout `(was Phase 5, corrected — must land after Phase 6)` +- `[H1]` **Command-ring overflow logs and drops; it does NOT drop the link.** The pipe protocol is designed for a dropped frame (zero bit in the next SACK → client retransmits that chunk, `docs/pipe-write-protocol.md` §5.2) — one round trip vs. a link drop + re-auth + restarted transfer. Escalate to `abortToKnownState` only if overflow recurs while `transferActive()` **and** `g_lastProgressMs` is already stale, i.e. let the supervisor own the decision. Also fix the off-by-one in the [main.h:365-370](../src/main.h) comment: usable capacity is `COMMAND_QUEUE_SIZE - 1 = 32`, not 33 (producer refuses at `nextHead == tail`), so the documented "W=32 window + END" claim is false — bump `COMMAND_QUEUE_SIZE` to 34 on envs with DRAM to spare (**not** `esp32-N4`). +- Response-ring overflow: flag, serviced in loop, gated on `transferActive()`. +- **BLE idle disconnect.** `OD_BLE_IDLE_DISCONNECT_MS = 300000` (5 min; 0 disables). Gate on peer connected && `!epdRefreshInProgress`, then drop the link. + + **Definition of activity (authoritative):** *a chunk arriving in the command queue carrying a **valid command**, or a **continuation of a data upload**.* Nothing else stamps `g_lastLinkActivityMs`. + + `[C1]` "Valid" is the load-bearing word and it is what makes this timer resistant to the defect that killed the original RX-keyed design. Validity is not knowable at queue-insert time — `onWrite` runs on the NimBLE host task before decryption — so the stamp goes in `imageDataWritten` **after** the decrypt/auth gate passes ([communication.cpp:663-711](../src/communication.cpp)) **and** the opcode resolves to a known command (`commandName(command) != nullptr`, i.e. not the `default:` unknown-opcode branch). A post-`clearEncryptionSession()` retry flood therefore never stamps: every frame short-circuits to `RESP_AUTH_REQUIRED` at [communication.cpp:664-670](../src/communication.cpp) before reaching the stamp, and the link is dropped at 5 min. + + "Continuation of a data upload" means a `0x0071`/`0x0081` frame that was **accepted** (consumed in order, or queued in the reorder window) — **not** one silently discarded because `pipeState.error` is latched ([display_service.cpp:2811](../src/display_service.cpp)). Counting discarded frames would let a client retransmitting into a dead pipe hold the link forever, which is the same defect one layer down. + + **Relationship to the supervisor's `g_lastProgressMs` (Phase 6):** two distinct signals, deliberately. Idle activity answers *"is the client still talking sense?"*; supervisor progress answers *"is the state machine advancing?"* They compose: a client politely polling battery status every minute is active (valid commands) and correctly not dropped, while making no transfer progress — and the supervisor doesn't fire either, because its wedge predicate requires `transferActive()`. A client flooding undecryptable frames trips the idle timer at 5 min; a client sending valid frames into a stalled transfer trips the supervisor at 10 min. + + **LAN keeps its 30 s ([opendisplay_protocol.h:984](../include/opendisplay_protocol.h)); the asymmetry is a deliberate design choice, not an oversight.** LAN is a machine-to-machine push transport: a client connects, pushes, and closes, so 30 s of silence means it is gone or broken — drop it fast and free the socket. **Only BLE carries interactive sessions**, where a human using the web configurator or the HA UI legitimately pauses between commands (reading config, composing a change, waiting on a slow refresh). A 30 s BLE timeout would break interactive use; 5 min tolerates human latency while still bounding the hostage window. BLE reconnect is also far more expensive than a TCP reconnect — advertising, connection setup, re-auth, and possibly a deep-sleep wake. + + **The constant stays firmware-local.** Mirroring LAN by promoting it to the canonical header would be a cross-repo change through `../opendisplay-protocol` plus a `--push` to all four firmware repos; not warranted until the behaviour is proven on hardware. Revisit only if a client ever needs to read the value. + + **Interaction with deep sleep is cooperative, not adversarial.** `pollActivity()` refreshes `lastActivityMs` every pass while a client is connected ([main.cpp:243-258](../src/main.cpp): *"A live link … is activity in itself"*), so today a silent client pins the device out of deep sleep indefinitely — the hostage case. Dropping the link makes `connCount` fall to 0, `lastActivityMs` stops being refreshed, and the existing `sleep_timeout_ms` hold elapses normally. No change to the sleep gates is required. + +--- + +## Files touched +`src/session_guard.h/.cpp` (new), `src/main.cpp`/`main.h`, `src/encryption.cpp`/`encryption_state.h`, `src/display_service.cpp`, `src/wifi_service.cpp`, `src/esp32_ble_callbacks.h`, `src/ble_init.cpp`, `src/device_control.cpp`, `src/power_latch.cpp`, `src/display_fastepd.cpp`, `src/touch_input.cpp`, `src/communication.cpp`, `src/structs.h`, `platformio.ini`, `docs/pipe-write-protocol.md`. + +## Verification +- Session lifetime: verify a transfer longer than any legacy `session_timeout_seconds` completes untouched; verify a client-initiated re-auth mid-connection still works; verify a captured last-frame replayed after reconnect is now REJECTED (the `counter_diff == 0` hole). +- Per phase: `pio run -e nrf52840custom -e esp32-s3-N16R8 -e esp32-c3-N16 -e esp32-c6-N4 -e esp32-N4`. CI builds all 11 on push. **`esp32-N4` is the gate** for Phase 1's `replay_window[256]` (+1.5 KB `.bss`) — it already needs `PIPE_SMALL_DRAM_WINDOW` to fit; if it won't link, drop to ±64. +- Hardware (py-opendisplay CLI): forward-gap nonce test (skip 100 counters) → session survives; true replay → rejected, session survives; kill client mid-pipe-window → reconnect, re-auth, clean push; forced pipe NACK → touch recovers ≤10 s; second BLE central → **verify on-air with a sniffer or `nRF Connect` that the refusal actually terminates** (this is the C3 trap); LAN connect during BLE session → accept-then-close; BLE connect while LAN owns → refused, telemetry still advertising; silent client 5+ min → dropped, advertising resumes, deep sleep reachable; stalled pipe with live connection **that keeps sending doomed frames** → supervisor still cleans at 10 min (this is the C1 regression test); regression: full Spectra transfer (60 s+ refresh) and an E1004 ~960 KB upload complete untouched. +- Every recovery action logs one ERROR/WARN line with reason + counters. +- Final soak: 24 h cron'd pushes alternating BLE/LAN with ~10% induced client kills. + +## Deliberately NOT changed + +Recorded so implementation does not "fix" these and review does not re-litigate them. Both were surfaced by `TIMER_AND_WATCHDOG_INVENTORY_2026-07-26.md`; both are decided. + +- **The 30 s auth-challenge validity window stays exactly as-is.** Step 2 of `CMD_AUTHENTICATE` must arrive within 30 s of the challenge — stamped at [encryption.cpp:587-588](../src/encryption.cpp), enforced at [:604-608](../src/encryption.cpp). This is a **cross-repo wire contract**, not a firmware-local knob: [opendisplay_protocol.h:384](../include/opendisplay_protocol.h) specifies *"STEP 2 must arrive within 30 s of the challenge or it is rejected"* with `@targets: Firmware | NRF54 | Silabs | NRF52811`. Changing it would require an edit in `../opendisplay-protocol` plus a `--push` to four firmware repos. It is also **not** a liveness timer and cannot wedge anything — a late step 2 gets `AUTH_STATUS_ERROR` and the client restarts the handshake. Phase 1 restructures `encryption.cpp` but **must not touch this check**, including the known boot-window quirk (`server_nonce_time = 0` from `clearEncryptionSession` means a step 2 with no preceding step 1 passes the freshness test during the first 30 s of uptime; not exploitable, since the MAC still requires the master key). Correct Phase 5's "scoped to the connection **and nothing else**" phrasing to acknowledge this second, independent encryption time bound — wording only, no code. + +- **No new bounding timer for the buzzer or the LED.** `abortToKnownState` stops both as part of teardown (Phase 3) — that is a teardown action, not a timer, and it stays. Nothing further is added. For the record, and correcting review finding `[X6]` which lumped them together: + - **Buzzer is already bounded** — `kBuzzerMaxTotalMs = 30000` ([buzzer_control.cpp:17](../src/buzzer_control.cpp), enforced at `:168`). No gap. *(The source comment there claims a "5 s cap" against a 30 s constant; comment-only defect, left alone.)* + - **LED flash is genuinely unbounded** — `processLedFlash` has no global cap equivalent. **Accepted as-is.** A stuck LED sequence wastes power but cannot freeze the device: it holds no lock, blocks no task, and is cleared by `abortToKnownState`, by disconnect teardown, and by an explicit `0x0075` LED_STOP. + +## Explicit non-goals / residual risk +- A true CPU/peripheral hard hang remains detect-and-log only — accepted with the software-only decision. **But "recoverable by power cycle" is weaker than it sounds on latching devices.** The button *press* is captured by an ISR ([device_control.cpp:679-693](../src/device_control.cpp)), but the hold-duration evaluation and the power-off action run in `processButtonEvents()`, which is called **only** from `loop()`/`idleDelay()` ([main.cpp:481](../src/main.cpp), [:514](../src/main.cpp), [:527](../src/main.cpp), [:542](../src/main.cpp)) — and the power-off hold test itself is at [device_control.cpp:78](../src/device_control.cpp). So while `loop()` is blocked (e.g. inside a 60 s `waitforrefresh`, whose `delay(10)` yields to FreeRTOS but never services buttons), a long-press does not trigger power-off. On a `DEVICE_FLAG_BATTERY_LATCH` unit with no way to interrupt the rail, the user's fallback is unavailable for the duration of the block. This does not change the software-only decision, but it means the residual risk is "wait out the block or remove the battery", not "hold the button". +- No config-schema changes (timeouts are compile-time constants in v1). +- The client-side nonce burn (py-opendisplay) is untouched; firmware-side widening makes it safe. +- `[M1]` A local attacker with a captured frame can still jam `last_seen_counter` forward within the window and stall a session; the supervisor is the recovery path. Symmetric windowing keeps the stall bounded by the ring. diff --git a/docs/PLAN_PHASE1_NONCE_REPLAY_2026-07-26.md b/docs/PLAN_PHASE1_NONCE_REPLAY_2026-07-26.md new file mode 100644 index 0000000..3a3f7f8 --- /dev/null +++ b/docs/PLAN_PHASE1_NONCE_REPLAY_2026-07-26.md @@ -0,0 +1,699 @@ +# Phase 1 Implementation Plan — Nonce / Replay Correctness + +**Branch:** `debug/ble-hardening` · **Date:** 2026-07-26 +**Parent plan:** [`PLAN_FREEZE_PROOFING_2026-07-26.md`](PLAN_FREEZE_PROOFING_2026-07-26.md) § "Phase 1" +**Review that shaped it:** [`FINDINGS_FREEZE_PROOFING_PLAN_REVIEW_2026-07-26.md`](FINDINGS_FREEZE_PROOFING_PLAN_REVIEW_2026-07-26.md) `[M1]` + +> **Revised 2026-07-26 after adversarial review** — +> [`FINDINGS_PHASE1_PLAN_REVIEW_2026-07-26.md`](FINDINGS_PHASE1_PLAN_REVIEW_2026-07-26.md) +> (1 Critical, 3 High, 3 Medium, 7 Low; verdict: safe to ship alone, but not as written). +> +> **Applied here:** `C1` — forward cap 64 → **128**, and the "hard bound" derivation replaced +> (Decision A); `M2` — the jam-forward DoS argument withdrawn, since it cannot occur once D2 is +> fixed; `H1` — **new Step 4b**, stop answering a nonce-rejected pipe frame with a fatal NACK. +> Bitmap widened to `uint64_t[4]` so the cap stays inside the window. `M1` — Step 2 now +> specifies **unsigned wrapping deltas**, resolving both the plan's internal contradiction and +> the UB on attacker-controlled counters. +> +> `H2` recorded under Decision E (shipping defect, cross-repo wire fix — no Phase 1 code); +> `H3` folded into the D3 row and a new hardware test 4b; `M3` `resetNonceState()` now names all +> four fields; `L1`-`L7` corrected in place. +> +> **All 14 findings are now addressed.** The only ones carrying no code change are `H2` +> (recorded, deferred to a protocol revision) and `L1` (D4 demoted to latent — the fix stays). + +Phase 1 is the root-cause fix and ships first. It is self-contained: it has no dependency on any +later phase and delivers field benefit on its own — unlike Phase 3, which is dead code until +Phase 5/6 call it. Scope is the encryption layer (`encryption.cpp`, `encryption_state.h`, a new +`nonce_window.h`) plus a small, deliberate change in `communication.cpp` (Step 4b); the full list +is in "Files touched". + +--- + +## What is actually wrong today + +Four distinct defects live in `verifyNonceReplay()` ([encryption.cpp:114-156](../src/encryption.cpp)) +and its one caller `decryptCommand()` ([:688-735](../src/encryption.cpp)): + +| # | Defect | Evidence | Consequence | +|---|---|---|---| +| **D1** | Nonce rejection is counted as **tamper evidence** | [:691-696](../src/encryption.cpp) — `verifyNonceReplay` false → `integrity_failures++` → 3 ⇒ `clearEncryptionSession()` | Packet loss ≠ attack. A lost window puts the next frame out of range, and each such frame counts toward session destruction; at 3 the session is destroyed mid-transfer and everything then answers `0xFE`. **See the mechanism caveat below — how the count reaches 3 is not what this plan originally claimed.** | +| **D2** | State is committed **before** the CCM tag is verified | `last_seen_counter` at [:149-151](../src/encryption.cpp), ring write at [:153](../src/encryption.cpp), all *before* `aes_ccm_decrypt` at [:714](../src/encryption.cpp) | An unauthenticated attacker (or corrupt frame) advances the replay state of a live session. Forged counter `last_seen + 32` sticks even though the frame is discarded. | +| **D3** | `counter_diff == 0` is exempted from the replay-set check | [:136](../src/encryption.cpp) `nonce_counter <= last_seen && counter_diff != 0` | **Replay of the highest-seen frame is accepted and re-executed** — the tag is valid because the frame is genuine. Harmless for a pipe DATA frame (duplicate seq is discarded) but not for `CMD_CONFIG_WRITE`, `CMD_POWER_OFF`, or a buzzer/LED command, which is typically what the last frame of a session is. The `!= 0` term exists only so a fresh session's first frame (client counter 0 vs `last_seen_counter` initialised to 0 at [:211](../src/encryption.cpp)) isn't flagged. **`[H3]` — worse than one replayed command:** the accept path writes the ring unconditionally ([:152-154](../src/encryption.cpp)), *including* on this exempted re-accept, while `last_seen_counter` never moves (`:149` is `>`-conditional). So replaying the highest-seen frame **64 times flushes every genuine entry out of the ring**, after which the whole `[L−32, L]` backward window is replayable — the last 32 genuine commands, not just the last one. Reachable on ESP32: `CONFIG_BT_NIMBLE_MAX_CONNECTIONS` is really 3 (`sdkconfig.h:613`), the write callback does not discriminate connection handles, and `isAuthenticated()` ([:195-199](../src/encryption.cpp)) is a global flag with no peer binding, so a *second* central can feed captured frames into a live session. nRF is capped at one link by `Bluefruit.begin(1, 0)`. | +| **D4** | `replay_window_index` is a **function static** | [:152](../src/encryption.cpp) | `clearEncryptionSession()` memsets the ring ([:217](../src/encryption.cpp)) but cannot reset the index. **`[L1]` — latent, not live:** both reset sites zero the *whole* ring, and writing from an arbitrary offset into a uniformly-empty 64-slot ring with a +1 index gives the same strict-FIFO eviction order as starting from 0, so no counter's accept/reject decision differs today. It becomes a real bug the moment the ring stops being uniformly reset. Fix it anyway — the bitmap removes the field entirely. | + +### ⚠ Mechanism caveat on D1 — verify on hardware before trusting the narrative + +Earlier versions of this plan (and the parent plan's context section) asserted that a lost window +produces *"exactly 3 rejections, because the client's `MAX_PTO` is 3"*. **That coincidence is not +real, and the chain it describes may not be how the field failure actually happens.** Two +corrections compound: + +1. **`MAX_PTO = 3` yields only two probe sends** (`[L4]`; `device.py:2721-2726` increments and + raises at the threshold *before* sending). Two rejections do not reach a threshold of 3. +2. **The client aborts on the *first* rejection, not the third** (`[H1]`). The `RESP_NACK` for + rejection #1 raises `IntegrityCheckError` (`device.py:833-838`), uncaught by the pipe loop — + so the transfer is already dead and the client sends nothing further on that path. + +So within a single transfer, `integrity_failures` plausibly reaches **1**, not 3. Reaching 3 +requires *repeated* attempts on the same session — an HA retry of the whole upload, or unrelated +commands, each rejected because the device's `last_seen_counter` is stranded far below the +client's. That is plausible but **unverified**. + +**Why this matters, and why it does not weaken Phase 1:** + +- **It re-weights the fix.** If the client aborts at rejection #1, the observed field symptom — + latched `pipeState.active`, dead touch, powered panel — needs no session destruction at all; + the abort alone leaves the device latched. That makes **Step 4b (`[H1]`) potentially the + highest-value change in Phase 1**, not a refinement of it, and it reinforces that the latch + itself is only cleared by Phase 3's `abortToKnownState()`. +- **D1 is still a genuine defect and still worth fixing first.** Counting packet loss as tamper + evidence is wrong on its own terms, and it is what turns a recoverable transfer failure into a + device that answers `0xFE` to everything until reconnect. The fix does not depend on how the + count reaches 3. + +**Action:** Step 5 test 1-2 must **capture the baseline on unmodified firmware first** — log the +actual `integrity_failures` trajectory and whether the session is cleared during a real +window-loss event — before asserting the before/after story anywhere. If the session is never +actually cleared in the field, say so and re-rank the phases accordingly rather than defending +this plan's original framing. + +Plus the structural issue `[M1]`: the window is **±32 symmetric today** +([:131](../src/encryption.cpp)) and one replayed frame can jam `last_seen_counter` forward, +stranding every legitimate frame between the old and new positions. How wide the forward side +should be is Decision A; how the seen-set is represented is Decision B. **Both are now +resolved below** — D3 and D4 disappear entirely under the chosen representation rather than +being patched. + +--- + +## Design: split check from commit + +``` +decryptCommand(...) + ├─ isAuthenticated() unchanged (Phase 5 removes the timeout side-effect) + ├─ NonceResult r = nonceCheck(nonce) PURE — no writes to encryptionSession + │ ├─ OK → continue + │ ├─ BAD_SESSION → return false, integrity_failures UNTOUCHED + │ ├─ OUT_OF_WINDOW → return false, integrity_failures UNTOUCHED ← D1 fix + │ └─ REPLAY → return false, integrity_failures UNTOUCHED + ├─ aes_ccm_decrypt(...) the ONLY tamper oracle + │ ├─ tag fail → integrity_failures++ ; >=3 ⇒ clearEncryptionSession() (unchanged) + │ └─ tag OK → nonceCommit(counter) ← D2 fix: commit AFTER authentication + └─ integrity_failures = 0 ; updateEncryptionSessionActivity() ; return true +``` + +The rule in one line: **only a CCM tag failure is evidence of tampering; a nonce failure is +evidence of a lossy link.** Nonce failures drop the frame and keep the session. + +--- + +## Steps + +### Step 1 — `encryption_state.h`: replace the value ring with a sliding bitmap + +```c +// Anti-replay: bit i == "counter (last_seen_counter - i) has been consumed". +// Bit 0 is last_seen_counter itself. Backward window is implicitly +// OD_NONCE_BACKWARD_BITS - 1; there is no separate window constant to keep in +// step, and no index to reset. +#define OD_NONCE_BACKWARD_BITS 256 // uint64_t[4], 32 B +#define OD_NONCE_FORWARD_CAP 128 // see Decision A + uint64_t replay_bitmap[OD_NONCE_BACKWARD_BITS / 64]; +``` + +- **Delete** `uint64_t replay_window[64]` (512 B) and the function-static + `replay_window_index` ([:152](../src/encryption.cpp)). Net struct change: **−480 B**. +- **Why the backward width is 256, not 128:** keeping `OD_NONCE_FORWARD_CAP < + OD_NONCE_BACKWARD_BITS` means a legal forward slide can never exceed the bitmap width, so the + wholesale-clear branch in Step 3 is unreachable on any legitimate input. At width 128 with a + 128 cap the two are equal and a maximal slide clears the whole bitmap — still *correct* (every + discarded counter is then out-of-window and rejected on width), but it puts the hardest branch + on the normal path for the sake of 16 bytes. Keep the margin. +- **No `replay_window_index` field** — a bitmap has no insertion point, so **D4 cannot recur**. +- **No `has_seen_counter` field** — "not seen" is a clear bit, not a reserved value, so **D3 + cannot recur**. A fresh session is `last_seen_counter = 0` with an all-zero bitmap; the + first frame at counter 0 has `fwd == 0`, finds bit 0 clear, and is accepted exactly once. +- `clearEncryptionSession()` ([:201-219](../src/encryption.cpp)) and the fresh-session block in + `handleAuthenticate` ([:654-660](../src/encryption.cpp)) must **both** reset the nonce state. + Fold that into one `resetNonceState()` helper so a third caller cannot drift — but **`[M3]` + define it by naming all four fields**, because the two blocks share only these four and are + *opposite* on everything else (`authenticated` false vs true, timestamps zeroed vs stamped, + keys wiped vs populated): + + ```c + static void resetNonceState(void) { + encryptionSession.nonce_counter = 0; /* device's OWN outbound counter */ + encryptionSession.last_seen_counter = 0; + encryptionSession.integrity_failures = 0; + memset(encryptionSession.replay_bitmap, 0, sizeof(encryptionSession.replay_bitmap)); + } + ``` + + A helper described loosely as "reset the bitmap and `last_seen_counter`" invites someone + tidying the surrounding lines to drop `nonce_counter = 0`. That would leave the device's + **outbound** counter running across a re-auth while the client restarts at 0 — walking + straight into the keystream reuse described under Decision E `[H2]`, against itself. + +### Step 2 — `encryption.cpp`: `nonceCheck()`, pure +```c +/* enum lives in src/nonce_window.h — the TYPE is shared (Step 4b needs it in + encryption.h to carry a reason out of decryptCommand); the FUNCTIONS stay + file-static (Decision C). Sharing a type grants no ability to commit state. */ +enum NonceResult { NONCE_OK, NONCE_BAD_SESSION, NONCE_OUT_OF_WINDOW, NONCE_REPLAY }; + +static NonceResult nonceCheck(const uint8_t* nonce, uint64_t* counter_out); /* encryption.cpp */ +``` +- Keep the existing `constantTimeCompare` session-id check ([:122](../src/encryption.cpp)) → + `NONCE_BAD_SESSION`. +- Then, **unsigned arithmetic only** (`[M1]` — see below for why this is not a style choice): + +```c +const uint64_t fwd = counter - last_seen; /* wraps; 0 when equal */ +const uint64_t back = last_seen - counter; /* wraps; fwd + back == 0 mod 2^64 */ + +if (fwd == 0) return bit_test(bm, 0) ? NONCE_REPLAY : NONCE_OK; +if (fwd <= OD_NONCE_FORWARD_CAP) return NONCE_OK; /* ahead: cannot have been seen */ +if (back < OD_NONCE_BACKWARD_BITS) return bit_test(bm, back) ? NONCE_REPLAY : NONCE_OK; +return NONCE_OUT_OF_WINDOW; +``` + + The `fwd == 0` case is where **D3 closes**: no `!= 0` exemption, the bit is simply tested like + any other. + +- **The four tests are ordered, and the order is load-bearing.** `fwd` and `back` sum to zero + mod 2^64, so they cannot both be small: with the current constants `fwd <= 128 && back < 256` + would need `fwd + back <= 384 ≡ 0 (mod 2^64)`, true only when both are zero — the case already + consumed by the first test. A counter far from the window in either direction leaves both huge + and falls through to `NONCE_OUT_OF_WINDOW`. Do not reorder. + +- **Why unsigned, not `int64_t` deltas.** The 8 counter bytes are parsed off the wire at + [:119-121](../src/encryption.cpp) and reach this function *before* `aes_ccm_decrypt` + ([:714](../src/encryption.cpp)), so an **unauthenticated attacker controls both operands**. + Converting a `uint64_t >= 2^63` to `int64_t` is implementation-defined before C++20, the + subtraction can overflow outright (`counter = 2^63`, `last_seen = 1` → `INT64_MIN - 1`), and + negating `INT64_MIN` is UB as well — so a table keyed on `diff`/`-diff` has three separate + ways to be undefined on attacker-chosen input. Unsigned overflow is defined as modular + arithmetic, making the expression total over all 2^64 inputs with no range precondition. This + is the standard formulation in IPsec/DTLS implementations. Today's [:130](../src/encryption.cpp) + uses the signed form; it is one of the things being replaced, not preserved. + *(Practical note: on Xtensa and ARM the signed form almost certainly compiles to the wrapping + behaviour anyway — no known live miscompilation. It is worth fixing because the correct form + is simpler than the buggy one, the input is attacker-controlled in a security check, and + Decision D's `-fsanitize=undefined` gate would otherwise fail against the plan's own code.)* + +- **No writes to `encryptionSession` on any path.** This is the property Step 5 tests. + +### Step 3 — `encryption.cpp`: `nonceCommit(uint64_t counter)` + +Same `fwd`/`back` unsigned deltas as Step 2 — do not reintroduce a signed `diff` here. + +- **Forward** (`fwd != 0`, i.e. `counter > last_seen_counter` in window terms): shift the bitmap + left by `fwd`, clearing the vacated low bits; `last_seen_counter = counter`; set bit 0. +- **Backward/equal**: set bit `back`. `last_seen_counter` does not move. +- **Keep a `fwd >= OD_NONCE_BACKWARD_BITS` guard that zeroes the bitmap wholesale, but know that + it is unreachable in practice.** `nonceCheck` rejects anything with `fwd > OD_NONCE_FORWARD_CAP` + (128) before commit is ever called, and the bitmap is 256 wide — that margin is deliberate + (Step 1). The guard exists so the function is total if called directly (the host test does + exactly that) and so a future cap increase cannot silently produce an over-wide shift. It is + still *correct* when it does fire: every counter it discards is then ≥256 behind and gets + rejected on width. +- Shifting across a `uint64_t[4]` must handle `shift == 0` and `shift >= 64` explicitly — + `x << 64` is undefined behaviour in C, and it is the classic bug in this pattern. +- Step 5's host test drives `fwd` = 0, 1, 63, 64, 65, 127, 128 (**the reachable range**, capped by + `OD_NONCE_FORWARD_CAP`) and additionally 129, 191, 192, 255, 256, 257 **directly against + `nonce_window.h`** to exercise the guard and the word-boundary shifts that the reachable range + alone would leave untested. +- Called from exactly one place: after a successful `aes_ccm_decrypt`. + +### Step 4 — `decryptCommand()` rewiring +- Replace the `verifyNonceReplay` block ([:691-698](../src/encryption.cpp)) with the `nonceCheck` + call and its result handling from Step 2; **delete** the `integrity_failures++` on that path. +- Insert `nonceCommit()` as the **first statement of the `if (success)` arm**, i.e. at + [:718](../src/encryption.cpp) — `[L2]` **not** merely "before `integrity_failures = 0`". + There is an early `return false` in between, for a decrypted-but-malformed `payload_length` + ([:719-722](../src/encryption.cpp)). That frame is *authentic* — it passed the CCM tag — and + today's unconditional commit at `:149-153` does record it. Placing the commit after the early + return would leave an authentic frame replayable, i.e. a silent behaviour change dressed as a + refactor. +- Leave the tag-failure arm ([:729-733](../src/encryption.cpp)) exactly as-is. +- Logging: `nonce out-of-window (counter=%llu last_seen=%llu fwd=%llu) — frame dropped, session + kept` at WARN vs. `CCM tag failure %u/3` at ERROR. The current out-of-window log is + `od_log_error` ([:132](../src/encryption.cpp)) and will now fire routinely on a lossy link — + demote it or it becomes noise that masks real errors. **`[L7]`** applies the same treatment to + the session-id mismatch log at [:123-127](../src/encryption.cpp), which prints two full session + IDs at ERROR: once nonce failures stop counting toward `integrity_failures`, nothing rate-limits + an attacker driving that line. Demote and/or rate-limit it. +- **`[L7]` — state the `NONCE_BAD_SESSION` policy change explicitly.** Today a session-id + mismatch *does* count as tamper evidence ([:122-128](../src/encryption.cpp) → `:691-696`); + routing it to "`integrity_failures` untouched" alongside the loss cases is a deliberate + decision, not a consequence of D1. It is the right call — a mismatched session id is what a + stale client sends after the device re-authenticated, i.e. usually confusion rather than + attack, and the CCM tag remains the tamper oracle — but it must be written down rather than + arrived at silently. +- **Delete `verifyNonceReplay()`** and both of its declarations (Decision C: the body at + [:114-156](../src/encryption.cpp), [encryption.h:17](../src/encryption.h), + [main.h:276](../src/main.h)). The build is the check here — the compiler will name any caller + we missed. + +### Step 4b — Stop answering a dropped pipe frame with a fatal NACK `[H1]` + +**Without this, Phase 1 saves the device but still loses the transfer.** Today every +`decryptCommand` failure — nonce *and* tag alike — produces the same unencrypted 3-byte +`RESP_NACK` ([communication.cpp:698-703](../src/communication.cpp)), and the client turns that +shape into a fatal exception before it ever reaches pipe-frame classification: + +``` +device.py:833-838 if len(raw) == 3 and raw[2] == 0xFF: raise IntegrityCheckError(...) +device.py:2716 the pipe send loop's ONLY except is BLETimeoutError +``` + +So one out-of-window frame aborts the whole upload, no matter how wide the cap is. The client +cannot repair the hole, because the frame it would have repaired is the one whose NACK killed +the transfer. + +**Change:** + +1. Give `decryptCommand` a reason out-param (or an enum return) so the caller can distinguish + nonce rejection from tag failure. It has exactly one caller, so this is mechanical — but it + touches the declarations in [encryption.h:21](../src/encryption.h) and + [main.h:274](../src/main.h) as well. +2. At [communication.cpp:698-703](../src/communication.cpp): when the reason is + `NONCE_OUT_OF_WINDOW` or `NONCE_REPLAY` **and** the opcode is `CMD_PIPE_WRITE_DATA` + (`0x0081`), **send nothing at all**. Every other combination keeps today's `RESP_NACK`. + +**Why silence is the correct answer and not a hack.** A pipe DATA frame is not +request/response — the client never blocks on a per-frame reply; it blocks on sliding-window +ACK reads. Dropping the frame silently is therefore *exactly* the signal "this frame was lost", +which is the one condition the pipe protocol is built to repair: the seq is absent from the next +SACK mask, the client retransmits it, and the transfer continues. Answering instead with a fatal +NACK converts recoverable loss into an aborted upload. + +**This is a conformance fix, not a protocol change.** `docs/pipe-write-protocol.md` §5.2 already +specifies the rule: + +> *"NACKs are reserved for unrecoverable conditions (bad payload, protocol violation), **not +> ordinary packet loss**."* + +A frame rejected because its nonce fell outside the window **is** ordinary packet loss — it is +the direct consequence of frames having been dropped. Today's firmware answers it with a fatal +`0x81` NACK, which §5.1 defines as unconditionally fatal. **Today's behaviour therefore violates +the pipe spec as written; Step 4b restores conformance.** That also settles it against the parent +plan's *"NO wire protocol changes"* constraint: no documented client-observable behaviour +changes, because silence-on-loss is what the document already prescribes. No `.md` edit is +required — at most a clarifying sentence in §5.2 that a nonce-rejected data frame is classed as +loss, not as an unrecoverable condition. + +**Deliberately narrow:** +- **Tag failures keep the NACK.** They are tamper evidence, not loss, and §5.2's "unrecoverable + condition" is exactly right for them. +- **`0x0071` (legacy DIRECT_WRITE_DATA) is left alone.** It has a different ACK discipline that + has not been analysed here, and the field failure lives on the pipe path. Note it as a + deliberate exclusion so the next reader does not assume it was an oversight. +- **No canonical-header change** (Decision E still stands): no opcode, response code, or envelope + changes. + +### Step 5 — Verification +- **Host test** (Decision D): `tools/test_nonce_window.cpp` against `src/nonce_window.h`, run + under UBSan/ASan. Full case list in Decision D — it covers the window state machine, *not* + the `integrity_failures` behaviour, which is what hardware tests 1-2 below are for. +- **Build gate:** `pio run -e nrf52840custom -e esp32-s3-N16R8 -e esp32-c3-N16 -e esp32-c6-N4 + -e esp32-N4`. CI builds all 11. +- **Hardware** (py-opendisplay CLI + `tools/od-device-cli.py`). **Test 0 comes first:** + 0. **Baseline on unmodified firmware.** Induce a real window-loss event and record the + `integrity_failures` trajectory, whether `clearEncryptionSession()` actually fires, and + whether the client aborts at the first NACK. This settles the D1 mechanism caveat above. + Without it, the before/after claims for tests 1-2 rest on an unverified model. + 1. Forward-gap **within** the cap: skip 100 counters mid-session → next frame accepted, + transfer continues, session survives (today: session destroyed after 3). + 2. Forward-gap **beyond** the cap: skip 200 (> `OD_NONCE_FORWARD_CAP`) → frames rejected as + out-of-window, but **`integrity_failures` stays 0 and the session survives**. This is the + D1 regression test. + 2b. **`[H1]` — the gap must not kill the transfer.** Run test 2 *during a live pipe upload* + and assert the upload **completes**: the rejected frames are silently dropped (Step 4b), + absent from the next SACK mask, retransmitted, and repaired. Today, and in the pre-review + version of this plan, the client raises `IntegrityCheckError` and aborts. This is the test + that distinguishes "the device survived" from "the transfer survived". + 2c. **Worst-case client settings.** Repeat the pipe regression with + `blocks_per_ack = 1` and `W = 32` — the configuration that makes the gap widest + (Decision A). Confirms the 128 cap in the conditions that motivated it. + 3. True replay of an old counter → rejected, session survives. + 4. **Replay of the last frame of a session** (D3) → now REJECTED. Use a + non-idempotent command (buzzer) so acceptance is observable. + 4b. **`[H3]` — ring-flush replay.** Replay the final frame **64 times**, then replay an + *older* counter that is still inside the backward window. Must be `NONCE_REPLAY`. On + today's firmware the 64 re-accepts flush the value ring and the older frame is **accepted + and re-executed**; this test fails before the change and passes after, which is the only + way to demonstrate the bitmap closed the widened hole rather than just the narrow one. + 5. Forged/corrupt tag ×3 → session still cleared (unchanged behaviour, deliberately). + 6. Regression: full Spectra transfer and an E1004 ~960 KB upload complete untouched. + +### Step 6 — Comment hygiene +- [communication.cpp:772-775](../src/communication.cpp) documents that the replay counter + "already advanced at decrypt time … so drops/dupes never desync it". The invariant still + holds (commit happens for every frame that *decrypts*, including ones the pipe handler + discards) but the function name and the ordering claim are now wrong. Rewrite it in the + same change. **Write the mechanism, not a number** (Decision A, `[C1]`): the gap is driven by + the client's *retransmit budget* `max_retx = max(3·W, n/2)` and by `blocks_per_ack`, both of + which live in another repo and one of which is a user-facing Home Assistant setting — so + `OD_NONCE_FORWARD_CAP` is a heuristic with headroom, **not** an invariant firmware can prove. + A comment asserting a specific bound would be falsified silently by a client-side config + change. Say that, and point at Decision A. + +--- + +## Decisions + +**All five are settled.** Nothing blocks implementation. + +### Decision A — RESOLVED: forward cap **128** + +> **Revised after adversarial review** (`C1`, `M2` in +> [`FINDINGS_PHASE1_PLAN_REVIEW_2026-07-26.md`](FINDINGS_PHASE1_PLAN_REVIEW_2026-07-26.md)). +> This decision previously said 64, derived from `PIPE_MAX_W + MAX_PTO = 35` and asserted as a +> **hard bound**. That derivation was incomplete and the assertion was wrong. Both are corrected +> below; the earlier reasoning is retained only where it is still valid. (`[L4]`: even that +> figure was one too many — `MAX_PTO = 3` yields **two** probe sends, since the client increments +> and raises at the threshold *before* sending, `device.py:2721-2726`. Moot now, but noted so the +> arithmetic is not re-derived wrongly later.) + +**What the gap actually is** (unchanged, and still the right framing): not the in-flight chunk +depth, but the run of consecutive transmissions that never reach `decryptCommand` — the client +burns a counter per transmission whether or not it lands +([device.py:747-760](../../py-opendisplay/src/opendisplay/device.py)). Sources: frames lost on +air; frames dropped at the command ring +([esp32_ble_callbacks.h:127-128](../src/esp32_ble_callbacks.h), on the NimBLE host task *before* +decrypt); and retransmissions. + +**Why there is no firmware-side hard bound.** The earlier derivation counted two of the client's +three transmit sites. New sends are window-credit-limited +([device.py:2688-2694](../../py-opendisplay/src/opendisplay/device.py)) and PTO probes resend +exactly one chunk (`:2715-2726`) — but **selective repair spends no window credit at all**: + +```python +device.py:2789-2796 + for m in missing: # every hole below highest_recv, up to W-1 of them + if do_retx: + await _send(m) # fresh counter each; no credit consumed + retx_count += 1 +``` + +and the client deliberately preserves queued ACKs to spend on repeat repair rounds +(`drain_stale=False`, `device.py:781-786`). The only ceiling is the client's retransmit budget: + +``` +max_retx = max(3 * W, ceil(n * 0.5)) device.py:2672, commands.py:96 +``` + +which is 96 for `W = 32` and scales with *chunk count* for large uploads. Worked through, the +reachable gap is ~66 at `blocks_per_ack = 2` and ~96 at `blocks_per_ack = 1` — and +`blocks_per_ack` is a **user-settable Home Assistant option** (`min=1, max=32`), which firmware +clamps only at the top ([display_service.cpp:2723-2725](../src/display_service.cpp)). + +**So the honest statement is: firmware cannot bound this from its own constants.** The bound +lives in a client in another repo, behind a user-facing setting. Any cap is a heuristic; the +question is only how much headroom it buys and what it costs. + +**`OD_NONCE_FORWARD_CAP = 128`.** It covers the realistic worst case (~96) with margin, and +under the bitmap it costs **nothing** — the cap is a comparison, not storage. The old "tight is +correct" argument is withdrawn: it rested on the `[M1]` jam-forward DoS, which **cannot occur +once D2 is fixed.** After commit-after-verify, only a CCM-authenticated frame advances +`last_seen_counter`, so an attacker can only commit counters the client genuinely transmitted — +never past the client's own high-water mark. Repairs then carry *fresh, higher* counters +(`device.py:759`), so no future client frame ever falls below the window. The frames a jam could +strand do not exist. `[M1]` was an artifact of the value-ring design and does not survive +commit-after-verify plus a bitmap. + +**Step 6's source comment must record the mechanism, not the number.** A `blocks_per_ack` change +in Home Assistant, or a larger `max_retx`, silently invalidates any figure written into +`communication.cpp`. The comment should say *why* the cap exists and *where* the real bound +lives, so the next reader knows it is a heuristic to be re-checked rather than an invariant that +has been proven. + +**Residual risk, stated plainly.** A gap beyond 128 is still possible on a pathological link with +`blocks_per_ack = 1` and a multi-thousand-chunk upload. With Step 4b in place that is no longer +fatal — the frames are silently dropped and repaired by the normal SACK path — which is +precisely why `[H1]` is treated as in-scope rather than deferred. + +**Backward width is a separate constant** — see Decision B. Under a bitmap the two sides are +independent: forward acceptance stores nothing. + +### Decision B — RESOLVED: sliding bitmap, **IPsec/DTLS shifting style**, `uint64_t[4]` = 32 B + +**Bitmap over value ring.** To be precise about why, because the ring is *not* buggy: a ring of +depth `D` policing a backward window `W` is sound iff `D >= 2W`, since at most `2W` distinct +counters can be accepted while any given one remains in-window. Today's 64/32 and the parent +plan's 256/128 both satisfy it. The objection is not correctness but that `D >= 2W` is a +hand-maintained coupling between constants in two files, provable only by non-obvious +combinatorics, in a function that already shipped one undetected state bug (D4). The bitmap +makes eviction and falling-out-of-window *the same event*, so the coupling ceases to exist — +and takes D3 and D4 with it (Step 1). + +Storage scales at **16 B per unit of backward window** for a ring (`2W` entries × 8 B) versus +**1 bit** for a bitmap — a fixed 128:1 ratio at any width: + +| Backward window | Ring (`2W × 8 B`) | Bitmap | +|---|---|---| +| 32 (today) | 512 B | 8 B | +| 127 | 2,032 B | 16 B | +| 255 (**chosen**) | 4,080 B | **32 B** | + +`OD_NONCE_BACKWARD_BITS = 256` (`uint64_t[4]`, backward window 255) is generous for a tolerance +that is **never exercised in normal operation** — the client's counters are strictly increasing +and both transports preserve ordering — but at 32 B there is no reason to economize, and the +margin over `OD_NONCE_FORWARD_CAP = 128` keeps the wholesale-clear branch off the normal path +(Step 1). Net struct change is **−480 B** against today, versus **+1,536 B** for the ring plan. +The `esp32-N4` link headroom measured for the ring (81,940 → 83,476 B of 327,680, SUCCESS both +ways) is therefore moot; recorded only so the fallback question never gets reopened. + +**Shifting (RFC 4303 / RFC 6347) over non-shifting (RFC 6479 / WireGuard).** Both are the same +algorithm; they differ only in how the window advances: + +- **Shifting** — the bitmap is a plain integer shifted left by `diff` on advance. This is what + IPsec ESP §3.4.3 and DTLS §4.1.2.6 describe. +- **RFC 6479 / WireGuard** — a *circular* bit array indexed by `counter mod size`, where + advancing clears the blocks between the old and new positions instead of shifting. It also + requires the array to be strictly larger than the window ("redundant bits") for the clearing + to be safe. + +**Pick shifting.** RFC 6479 exists to avoid the cost of shifting a *large* window — WireGuard +carries ~8,192 bits because it is a high-throughput VPN over UDP with genuine reordering. +Ours is 128 bits over two words at roughly 40 frames/s, where the whole operation is a handful +of instructions and is dwarfed by the AES-CCM decrypt of the same frame. Choosing RFC 6479 +would buy nothing measurable and would add modular indexing plus the redundant-bits invariant — +more subtlety, in exactly the function where subtlety has already cost us. The one real hazard +in the shifting form is UB on `x << 64`, which Step 3 calls out and Step 5 tests directly. + +Both styles are equally standard; this is a sizing call, not a security one. + +### Decision C — RESOLVED: delete `verifyNonceReplay()` outright + +No compatibility wrapper. Nothing outside `decryptCommand` should ever be able to commit nonce +state, and a surviving wrapper is an invitation to re-introduce exactly the commit-before-verify +bug (D2) that Phase 1 exists to remove. Three deletions, all in Step 4: + +| Location | Action | +|---|---| +| [encryption.cpp:114-156](../src/encryption.cpp) | Delete the function body; `nonceCheck` + `nonceCommit` replace it | +| [encryption.h:17](../src/encryption.h) | Delete the declaration | +| [main.h:276](../src/main.h) | Delete the duplicate declaration | + +`nonceCheck` and `nonceCommit` are **file-static** in `encryption.cpp` — they get no header +declaration at all, so the "only `decryptCommand` may commit session state" rule is enforced by +linkage rather than by convention. The pure logic they wrap lives in `src/nonce_window.h` and +is what the host test targets (Decision D), so staying static costs no testability. + +**The `NonceResult` *type* is shared, and that is not a loophole.** Step 4b needs +`decryptCommand` to report *why* it failed, so the enum is declared in `nonce_window.h` and +reaches `encryption.h`. A visible type conveys no ability to read or mutate `encryptionSession`; +the functions that can are still unreachable outside `encryption.cpp`. + +**Sequencing note:** this is not a standalone edit — `decryptCommand` is the sole caller, so the +deletion only compiles as part of Steps 1-4 landing together. + +### Decision D — RESOLVED: standalone `tools/test_nonce_window.cpp`, no PlatformIO env + +No `[env:native]` and no `test/` directory, so the 11-env matrix and a bare `pio run` are +untouched. (Note `.cpp`, not `.c` as first written — it includes a header shared with C++ +firmware code.) + +**This forces one structural refinement, and it is a good one.** The pure window logic must +compile with no Arduino, no mbedtls, and no `millis()` in its translation path — so it moves +into a dependency-free header operating on plain values: + +``` +src/nonce_window.h static inline, zero dependencies: (bitmap*, last_seen, counter) + -> NonceResult / updated state. No session, no logging, no crypto. +src/encryption.cpp static nonceCheck()/nonceCommit() wrap it with encryptionSession + + od_log_*. Still file-static (Decision C) — linkage still enforces + "only decryptCommand may commit session state". +tools/test_nonce_window.cpp includes ONLY src/nonce_window.h +``` + +This keeps Decision C's guarantee intact while making the part worth testing reachable: the +bit-shifting state machine is exactly the code with edge cases, and it has no business knowing +about sessions or logging anyway. + +```bash +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 +``` + +`-fsanitize=undefined` is not decoration — it is what catches the `x << 64` UB in Step 3 +automatically rather than relying on the test author to predict it. + +**Coverage (all against `nonce_window.h` directly):** +- **Shift edges:** `fwd` = 0, 1, 63, 64, 65, 127, 128 (reachable), plus 129, 191, 192, 255, 256, + 257 driven directly against `nonce_window.h` to cover the word boundaries and the + wholesale-clear guard (Step 3). +- **Purity of `nonceCheck` (D2):** snapshot the state, call `nonceCheck` on every result class, + `memcmp` the state afterwards. This is the single most valuable assertion in the file — it is + the property that "the tag is the only thing that may advance replay state" rests on. +- **D3:** commit a counter, re-present the same counter → `NONCE_REPLAY`. Cover `fwd == 0` + specifically, which is the exempted case today. +- **Fresh session:** `last_seen = 0`, empty bitmap → counter 0 accepted exactly once, rejected + on re-presentation. No `has_seen_counter` involved. +- **Wholesale slide:** forward jump ≥ `OD_NONCE_BACKWARD_BITS` → bitmap cleared; previously + seen counters now return `OUT_OF_WINDOW`, **not** `REPLAY` (both reject, but conflating them + would hide a genuine slide bug). +- **Differential/property test:** run a few thousand pseudo-random accept/replay/gap sequences + against a naive `std::set` oracle that models "seen, within window". Cheap, and it covers + the interleavings hand-written cases miss. +- **Counter arithmetic (`[M1]`):** assert the unsigned form from Step 2 behaves correctly at + `counter = 2^63`, `last_seen = 1` and near `UINT64_MAX` — the inputs that make the signed form + undefined. Unreachable in normal operation (2^63 frames) but attacker-reachable, and free to + get right. UBSan makes this test self-checking. + +**Not covered here, deliberately:** that a nonce failure leaves `integrity_failures` untouched +(D1) lives in `decryptCommand`, not in the window logic. That assertion belongs to Step 5's +hardware tests 1-2. + +**CI `[L5]`:** add a **separate top-level `host-tests` job** in `.github/workflows/main.yaml` — +**not** a step inside the existing `build` job, which is an 11-entry `matrix.environment` +(`:10-23`) and would run the host test eleven times. It needs no toolchain beyond the runner's +stock `g++` and gates every push alongside the firmware builds. + +Confirmed build-safe: `tools/test_nonce_window.cpp` is invisible to every firmware build — +`build_src_filter = +<*> -` (`platformio.ini:34`) is relative to the default +`src_dir`, and no env adds `tools/`. + +### Decision E — RESOLVED: no wire change. Recorded for the future, **not actioned** + +`decryptCommand` returning false yields an unencrypted `RESP_NACK` +([communication.cpp:698-703](../src/communication.cpp)) for both "lost your window" and +"tag failed". Phase 1 leaves that exactly as it is. + +**Why it is acceptable to leave — corrected.** This decision originally claimed the client's +"existing retransmit/PTO machinery recovers on its own". **That was false** (`[H1]`): the 3-byte +`0xFF` NACK is intercepted at `device.py:833-838` and raised as `IntegrityCheckError`, which the +pipe send loop does not catch, so the transfer dies on the first rejected frame. + +What makes leaving the *wire* alone acceptable is **Step 4b**, which fixes this firmware-side by +sending nothing at all for a nonce-rejected pipe DATA frame. Silence is already a first-class +signal in the pipe protocol — it means "lost", and the SACK path repairs it. No new response +code is required to get correct recovery; the client needs no change. + +**Recorded for a future protocol revision** (do not implement in Phase 1, and do not let a +reviewer re-open it here): a distinct response code for "nonce out of window" would let a +client re-sync deliberately — abandon the in-flight window and re-authenticate — instead of +burning its `MAX_PTO` budget discovering the same thing by timeout. That is a strictly better +recovery, and worth doing *if* the wire is being revised for other reasons. It is not worth +doing on its own: it is a cross-repo change through `../opendisplay-protocol`, a `--push` to +all four firmware repos, and a coordinated py-opendisplay release, to save a few seconds on a +path Phase 1 already makes non-fatal. + +#### `[H2]` Also recorded here: device and client share one nonce space (shipping defect) + +**Not a Phase 1 change — recorded so one future wire revision fixes it together with the +response code above.** Phase 1 is the change that will make a future reader believe this layer +has been audited, so an unrecorded defect here is worse than one nobody has looked for. + +The outbound nonce is built as `session_id || nonce_counter` +([encryption.cpp:158-174](../src/encryption.cpp)) from the device's **own** counter, and the +inbound nonce is `session_id || client_counter` off the wire. `encryptResponse` +([:740-743](../src/encryption.cpp)) and `decryptCommand` ([:704-705](../src/encryption.cpp)) +both then take `nonce_full[3..15]` as the CCM nonce, under the **same `session_key`**, with +**no direction separator**, and both counters reset to 0 at session start +([:210](../src/encryption.cpp)/[:655](../src/encryption.cpp); `device.py:735`). + +So device response #*k* and client command #*k* encrypt under an identical (key, nonce). CCM is +CTR underneath and the keystream depends only on key and nonce — the AAD differs but feeds only +the tag — so `C_resp ⊕ C_cmd = P_resp ⊕ P_cmd`. Responses are short and highly predictable +(`{RESP_ACK, cmd, status}`, pipe ACKs), so a passive eavesdropper recovers the leading plaintext +of the matching command. Authenticity is unaffected; this is a confidentiality failure. + +**Age and blast radius (verified from history):** introduced in `b04a22b` *"Add encryption"* +(2026-03-10) — the construction is byte-identical today, and `fd0d73a` merely moved it from +`main.cpp` into `encryption.cpp`. It is **not** a regression from this branch. The same +construction is in all four firmware repos (`Firmware_NRF54/src/opendisplay_pipe.c:518-522`, +`Firmware_Silabs/opendisplay_pipe.c:476`, `Firmware_NRF/encryption.c:197`) and both clients, +because [opendisplay_protocol.h:203-209](../include/opendisplay_protocol.h) specifies the +envelope and `CCM nonce = nonce[3..15]` but says **nothing about the nonce's internal +structure** — there is no single place where a reviewer would have seen the missing direction +separator. + +**Therefore the fix is a spec change first, not four patches:** define the nonce layout in the +canonical header *including* a direction bit, then `--push`. Whoever schedules it must also plan +the compatibility story — an old client and a new device would disagree on the nonce. See +`[M3]` in Step 1 for the in-scope consequence: `resetNonceState()` must keep zeroing +`nonce_counter`, or the device reproduces this reuse against itself across a re-auth. + +**Action for Phase 1: none in code.** This paragraph, plus a note in +`../opendisplay-protocol/agents/` where cross-repo design issues live — this repo is not where +the next person will look for a protocol-level defect. + +--- + +## Scope boundaries (do not drift) + +- **Do not touch the 30 s auth-challenge freshness window** ([:587-588](../src/encryption.cpp) + stamp, [:604-608](../src/encryption.cpp) enforcement). It is a cross-repo wire contract + specified at [opendisplay_protocol.h:384](../include/opendisplay_protocol.h), it is not a + liveness timer, and it cannot wedge anything — see the parent plan's "Deliberately NOT + changed". This includes leaving the known boot-window quirk (`server_nonce_time == 0`) alone. +- **Do not disable `session_timeout_seconds` here.** That is Phase 5. Consequence to state + plainly: Phase 1 alone does **not** close the mid-transfer freeze caused by expiry firing + inside `isAuthenticated()` ([:195-199](../src/encryption.cpp)) — it closes the *nonce* arm + only. Both arms must land before the field failure is fully addressed. +- **Do not add link-drop behaviour to `clearEncryptionSession()`.** That guard is Phase 5. + After Phase 1 the CCM-tag path can still clear a session under a live link, leaving the + client talking to a device that answers `0xFE` — a known, accepted gap until Phase 5. +- **Do not shrink the backward window to zero.** TLS over TCP accepts no out-of-order records + at all and lets the AEAD tag enforce ordering for free (RFC 8446 §5.3), and OpenDisplay's + situation is arguably TLS's rather than DTLS's — ordered transports, a strictly increasing + client counter. But narrowing the accepted range is a larger behavioural change than + widening it, other firmware repos share this protocol, and the field failure is a *forward* + gap problem. Out of scope for Phase 1; noted so the option is not lost. +- No protocol-header edits, no config-schema changes, no client-side changes. + +**Compliance with the parent plan's "NO wire protocol changes" constraint** — every Phase 1 +change is inside its in-bounds list: + +| Change | Why it is in bounds | +|---|---| +| Widened forward cap, bitmap replay set | Firmware-local constants no client reads; accepting more legitimate frames and rejecting a replay are both already-legal outcomes of the existing nonce rules | +| D3 fix (replay of the highest-seen counter now rejected) | Rejecting a replay is what the nonce rules already prescribe; the exemption was the deviation | +| Step 4b (no fatal NACK for a nonce-rejected `0x0081`) | **Conformance**, not change — `pipe-write-protocol.md` §5.2 already reserves NACKs for unrecoverable conditions, "not ordinary packet loss" | +| `decryptCommand` reason out-param | Internal signature; nothing on the wire | + +Nothing here requires an edit to `include/opendisplay_protocol.h` or +`include/opendisplay_structs.h`. Run the constraint check from the parent plan before calling +Phase 1 done. + +## Files touched + +| File | Change | +|---|---| +| `src/nonce_window.h` | **new** — dependency-free window state machine (Decision D) | +| `src/encryption_state.h` | ring → bitmap, **−480 B** | +| `src/encryption.cpp` | the substance: `nonceCheck`/`nonceCommit`, `decryptCommand` rewiring, `verifyNonceReplay` deleted | +| `src/encryption.h` | declaration removal (Decision C); `decryptCommand` reason out-param (Step 4b) | +| `src/main.h` | duplicate declaration removal (Decision C); same signature change (Step 4b) | +| `src/communication.cpp` | **Step 4b**: suppress the fatal NACK for nonce-rejected `0x0081` — plus the comment at [:772-775](../src/communication.cpp) | +| `docs/pipe-write-protocol.md` | *optional* — one clarifying sentence in §5.2 that a nonce-rejected data frame is classed as loss. No behaviour change to document: Step 4b makes the firmware conform to what §5.2 already says. | +| `tools/test_nonce_window.cpp` | **new** — host test (Decision D) | +| `.github/workflows/main.yaml` | separate `host-tests` job: compile + run the host test | + +**No `platformio.ini` change**: the bitmap is smaller than what it replaces, so the `esp32-N4` +link headroom that gated the original proposal is not a consideration on any target. +**No protocol-header change** (Decision E). **No client-side change.** diff --git a/docs/TIMER_AND_WATCHDOG_INVENTORY_2026-07-26.md b/docs/TIMER_AND_WATCHDOG_INVENTORY_2026-07-26.md new file mode 100644 index 0000000..ab45de9 --- /dev/null +++ b/docs/TIMER_AND_WATCHDOG_INVENTORY_2026-07-26.md @@ -0,0 +1,1182 @@ +# Timer, Watchdog and Timeout Inventory — `Firmware` + +**Date:** 2026-07-26 +**Scope:** everything tracked in this repo — `src/`, `include/`, `platformio.ini`, `scripts/`. +**Explicitly out of scope:** `.pio/libdeps/**` (bb_epaper, NimBLE-Arduino, FastEPD, Adafruit +Bluefruit), `~/.platformio/packages/**`, the precompiled `sdkconfig.h`, mbedTLS, FreeRTOS/IDF, +and every sibling repo (`py-opendisplay`, …). Where firmware hands control to one of those, the +entry is recorded as **bounded by library — not analyzed (out of scope)** with the firmware-side +call site and the argument the firmware passes. + +Written as the pre-implementation map for +[PLAN_FREEZE_PROOFING_2026-07-26.md](PLAN_FREEZE_PROOFING_2026-07-26.md) and its review, +[FINDINGS_FREEZE_PROOFING_PLAN_REVIEW_2026-07-26.md](FINDINGS_FREEZE_PROOFING_PLAN_REVIEW_2026-07-26.md). +Every claim below was verified against source; nothing is carried over from those documents +without re-checking. Corrections to them are in the final section. + +**Target shorthand.** `nRF` = `TARGET_NRF` (nRF52840, Bluefruit; commands run inline on the +Bluefruit Callback task, no queues). `ESP32` = `TARGET_ESP32` (S3/C6/C3/classic; SPSC command +ring drained by `loop()`). `OPENDISPLAY_HAS_WIFI` is `TARGET_ESP32 && OPENDISPLAY_ENABLE_WIFI` — +`esp32-N4` is ESP32 **without** WiFi, `nrf52840custom` has neither. + +--- + +## 1. Watchdog-related build flags and code in this repo + +### 1.1 `-DCONFIG_FREERTOS_WATCHDOG_TIMEOUT_S=120` + +- **Mechanism:** PlatformIO build flag, present in every ESP env. +- **file:line:** [platformio.ini:53](../platformio.ini), [:83](../platformio.ini), + [:112](../platformio.ini), [:140](../platformio.ini), [:189](../platformio.ini), + [:209](../platformio.ini), [:229](../platformio.ini), [:253](../platformio.ini), + [:295](../platformio.ini). `esp32-s3-E1004` ([:156](../platformio.ini)) and + `esp32-s3-N16R8-extuart-debug` ([:278](../platformio.ini)) inherit it through + `${env:.build_flags}`, so **all ten ESP envs carry it**; `nrf52840custom` does not. +- **Target:** ESP32 only. +- **Duration / bound:** nominally 120 s. +- **What it monitors:** nothing. **The flag is inert.** Two independent reasons, both verifiable + from inside this repo: (a) it is not an IDF 5.x symbol — the real one is + `CONFIG_ESP_TASK_WDT_TIMEOUT_S` — and (b) the firmware never calls any WDT API, so nothing in + this codebase reads it. Verified firmware-side: `grep -rn "esp_task_wdt\|rtc_wdt\|NRF_WDT" src/` + returns **zero** hits. +- **Action on expiry:** none. +- **Context:** n/a. +- **Can it fire while connected / transferring / mid-refresh?** It cannot fire at all. + +### 1.2 Firmware-armed watchdogs + +**There are none, on either target.** No `esp_task_wdt_add/init/reset`, no `NRF_WDT`, no +`rtc_wdt_*` anywhere in `src/`. Whatever hardware watchdog the platform enables by default is +outside this repo's control and out of scope. The only "watchdogs" this firmware owns are the +`millis()` deadlines in §2. + +### 1.3 Reset-reason handling (observability only) + +- **Mechanism:** `resetReasonName()` + `esp_reset_reason()` logging at boot. +- **file:line:** [main.cpp:29-44](../src/main.cpp) (name table), + [main.cpp:82-84](../src/main.cpp) (read + log). +- **Target:** ESP32 (`#ifdef TARGET_ESP32`). +- **Bound:** n/a — one-shot at boot. +- **What it monitors:** whether the previous boot ended in `PANIC` / `INT_WDT` / `TASK_WDT` / + `WDT` / `BROWNOUT`. Purely a log line; nothing branches on it. +- **Action:** logs. The adjacent comment at [main.cpp:95-100](../src/main.cpp) records the + hardware-proven consequence that a non-deep-sleep reset wipes RTC memory (so `deep_sleep_count` + reads 0 and the boot screen redraws). +- **Context:** `setup()`. + +--- + +## 2. `millis()`-based deadlines in `src/` + +Every one, grouped by subsystem. + +### 2.1 Direct-write 15-minute watchdog + +- **file:line:** [main.cpp:436-442](../src/main.cpp); stamp set at + [display_service.cpp:2077](../src/display_service.cpp) (`directWriteActivatePanel`), cleared at + [display_service.cpp:2022](../src/display_service.cpp) (`cleanupDirectWriteState`). +- **Target:** ESP32 only — the block sits inside the `#ifdef TARGET_ESP32` region of `loop()`. + **nRF has no equivalent**; its `loop()` body is the `#else` arm + ([main.cpp:518-530](../src/main.cpp)). +- **Duration:** `900000UL`, an inline literal at [main.cpp:438](../src/main.cpp). No named + constant. +- **Monitors:** wall-clock age of a direct-write session, measured from START. **Nothing + refreshes the stamp** — it is a hard cap on the whole upload + refresh window, not an + inactivity timer. +- **Action on expiry:** `od_log_error` + `cleanupDirectWriteState(true)` → clears all + `directWrite*` state and force-powers the panel off. +- **Context:** loop task, once per pass. +- **Fires while connected?** Yes. **During a transfer?** That is its only purpose. **Mid-EPD + refresh?** No — `directWriteFinishAndRefresh` blocks the loop task across + `bbepRefresh`/`waitforrefresh` ([display_service.cpp:2415-2436](../src/display_service.cpp)), + so the check simply does not run until the refresh returns. +- **Gap:** keys on `directWriteActive`. `sendPipeNack` clears that flag via + `cleanupDirectWriteState(true)` ([display_service.cpp:2564-2578](../src/display_service.cpp)) + while leaving `pipeState.active = true` — after a fatal pipe NACK this watchdog no longer + applies to the latched pipe. + +### 2.2 `checkPartialWriteTimeout()` — partial-write 15-minute watchdog + +- **file:line:** [display_service.cpp:578-587](../src/display_service.cpp); called from + [main.cpp:443](../src/main.cpp). Stamp set at + [display_service.cpp:2249](../src/display_service.cpp) (legacy `0x76` START) and + [display_service.cpp:2762](../src/display_service.cpp) (pipe-partial START). +- **Target:** ESP32 only (same `#ifdef` region as §2.1). +- **Duration:** `900000UL`, inline literal at [display_service.cpp:580](../src/display_service.cpp). +- **Monitors:** `partialCtx.active` age from START. Again a start-stamp, never refreshed. +- **Action:** `cleanup_partial_write_state()`, plus `resetPipeWriteState()` when + `pipeState.partial` — so a pipe-partial transfer *is* cleared here. +- **Context:** loop task, once per pass. +- **Fires while connected/transferring?** Yes. Mid-refresh: same answer as §2.1 — the loop task + is inside the refresh, so the check is deferred, not suppressed. + +### 2.3 `waitforrefresh()` — panel busy-wait bound + +- **file:line:** [display_service.cpp:747-775](../src/display_service.cpp); the loop bound is + [display_service.cpp:760](../src/display_service.cpp). +- **Target:** both. +- **Duration:** `timeout * 100` iterations of `delay(10)`. Every call site passes `60` → + **60 s**: [display_service.cpp:541](../src/display_service.cpp) (boot), + [:1592](../src/display_service.cpp) (FastEPD boot), [:2423](../src/display_service.cpp) + (FastEPD direct refresh), [:2432](../src/display_service.cpp) (bb_epaper direct refresh), + [:3241](../src/display_service.cpp) and [:3244](../src/display_service.cpp) (partial refresh). +- **Monitors:** the panel BUSY line via `bbepIsBusy()`. +- **Action:** `od_log_warn("Refresh timed out")`, returns `false`. Callers treat `false` as + refresh failure → `epdSessionRelease(false)` powers the panel down. +- **Context:** whichever task ran the refresh — loop task on ESP32, Bluefruit Callback task on + nRF for a client-driven refresh. +- **Two short-circuits that make the 60 s cap not apply:** + 1. **FastEPD (IT8951 / E1004 driver builds):** + [display_service.cpp:749](../src/display_service.cpp) returns `fastepd_wait_refresh(timeout)`, + which is a stub — [display_fastepd.cpp:228-231](../src/display_fastepd.cpp) does + `(void)timeout_sec; return !s_init_failed;`. **No wait, no bound.** The actual blocking + happens inside the library (`fastepd_full_update` / `fastepd_direct_refresh`) — + *bounded by library — not analyzed (out of scope)*, firmware call sites + [display_service.cpp:1592](../src/display_service.cpp) and + [:2422](../src/display_service.cpp), no timeout argument passed. + 2. **E1004 panel:** [display_service.cpp:752-756](../src/display_service.cpp) returns `true` + immediately when the panel is already idle, on the grounds that `bbepRefresh` already waited + — *bounded by library — not analyzed (out of scope)*. +- The 60 s loop itself yields (`delay(10)`), which is why a long refresh does not trip whatever + platform WDT exists. + +### 2.4 EPD keep-alive (`pwrmgmOffDeadlineMs` / `epdSessionTick`) + +- **file:line:** deadline armed [display_service.cpp:505](../src/display_service.cpp) + (`epdSessionRelease`); expiry checked [display_service.cpp:520-527](../src/display_service.cpp) + (`epdSessionTick`); window computed [display_service.cpp:383-395](../src/display_service.cpp). +- **Target:** both. +- **Duration — config-driven:** `power_option.screen_timeout_seconds` + ([opendisplay_structs.h:504](../include/opendisplay_structs.h), `uint8_t`, documented `@min 0 + @max 30`), clamped to `EPD_KEEPALIVE_MAX_S = 30` + ([display_service.h:16](../src/display_service.h)) at + [display_service.cpp:393](../src/display_service.cpp). **0 = power off immediately** (also the + factory/old-blob default). **Forced to 0 unconditionally when an AXP2101 PMIC is in the sensor + list** ([display_service.cpp:385-391](../src/display_service.cpp)), logging once per call when + it overrides a nonzero value. +- **Monitors:** how long a `PWR_WARM` (post-successful-refresh) panel keeps its rail up. +- **Action:** `epdSessionForceOffLocked()` — `bbepSleep` + `delay(50)` + rail cut, or + `fastepd_direct_sleep()` on FastEPD builds. +- **Context:** loop task via [main.cpp:353](../src/main.cpp), **and** inside `idleDelay()` at + [main.cpp:545](../src/main.cpp) so a long `idleDelay` still expires it. +- **Fires while connected?** Yes — WARM survives disconnect by design. **During a transfer?** No: + the tick early-returns unless `pwrmgmState == PWR_WARM` + ([display_service.cpp:521](../src/display_service.cpp)) and a transfer is `PWR_ACTIVE`, and it + additionally `TryTake`s the lock and skips the pass if held. **Mid-refresh?** No, same reasons. +- On battery ESP32 the effective window is `min(configured window, idle-hold)`, because + `enterDeepSleep()` calls `epdSessionForceOff()` unconditionally at + [main.cpp:609](../src/main.cpp). + +### 2.5 Encryption session timeout + +- **file:line:** [encryption.cpp:221-232](../src/encryption.cpp) + (`checkEncryptionSessionTimeout`), reached from `isAuthenticated()` + [encryption.cpp:194-198](../src/encryption.cpp). +- **Target:** both. +- **Duration — config-driven:** `securityConfig.session_timeout_seconds` + ([opendisplay_structs.h:916](../include/opendisplay_structs.h), `uint16_t` LE, documented + `0 = no timeout (persists until disconnect)`). **0 short-circuits to "still valid"** at + [encryption.cpp:223](../src/encryption.cpp) — there is no firmware fallback constant. +- **Monitors:** age since `session_start_time`, in whole seconds + ([encryption.cpp:224-225](../src/encryption.cpp)). +- **Action:** `clearEncryptionSession()` + return false → the command that triggered the check is + answered `RESP_AUTH_REQUIRED` ([communication.cpp:664-670](../src/communication.cpp)). +- **Context:** whichever task dispatches the command — loop task (ESP32) or Bluefruit Callback + task (nRF). It is a *query with side effects*: `isAuthenticated()` mutates session state. +- **Fires while connected?** Yes, by construction. **During a transfer?** **Yes** — it is + evaluated on every command dispatch, including every pipe DATA frame, so on a nonzero + configured value it deterministically fires mid-upload once the transfer outlives the window. + **Mid-refresh?** No — no commands dispatch while the loop task is inside the refresh. + +### 2.6 Encryption auth challenge validity — 30 s + +- **file:line:** stamped [encryption.cpp:588](../src/encryption.cpp) + (`server_nonce_time = currentTime`), checked + [encryption.cpp:604](../src/encryption.cpp). +- **Target:** both. +- **Duration:** `30000` ms, inline literal at [encryption.cpp:604](../src/encryption.cpp). +- **Monitors:** how long an issued `AUTH_STATUS_CHALLENGE` server nonce stays acceptable. +- **Action:** the 32-byte response arm rejects the stale challenge (client must re-request). +- **Context:** command dispatch task. +- **Fires mid-transfer?** Only during the authenticate handshake, which precedes any transfer. + +### 2.7 LAN read / idle timeout — 30 s + +- **file:line:** [wifi_service.cpp:957-960](../src/wifi_service.cpp). Stamps at + [:891](../src/wifi_service.cpp) (accept), [:915](../src/wifi_service.cpp) (TLS handshake + complete), [:951](../src/wifi_service.cpp) (bytes read), [:977](../src/wifi_service.cpp) + (valid frame about to dispatch). +- **Target:** ESP32 with `OPENDISPLAY_HAS_WIFI` only. +- **Duration:** `OD_LAN_READ_TIMEOUT_S = 30u`, from the canonical protocol header + [opendisplay_protocol.h:984](../include/opendisplay_protocol.h) — the **only** timeout in this + firmware that comes from the wire protocol rather than being firmware-local. +- **Monitors:** silence on an accepted LAN TCP/TLS session. Note the guard is + `else if (drainedBytes == 0)` — the check only runs on a tick where **nothing** was read. +- **Action:** `disconnectWiFiServer()`. +- **Context:** loop task, inside `handleWiFiServer()`. +- **Fires while a BLE client is connected?** Yes — the two transports are independent, and + `disconnectWiFiServer()` raises `bleDisconnectCleanupPending` + ([wifi_service.cpp:812](../src/wifi_service.cpp)), which is why the `ownerStillUp` guard in + [main.cpp:328-338](../src/main.cpp) exists. **During a LAN transfer?** Only if the client has + been silent 30 s. **Mid-refresh?** No — `handleWiFiServer()` runs on the loop task, which is + inside the refresh. + +### 2.8 `wifiClient.setTimeout(30000)` + +- **file:line:** [wifi_service.cpp:888](../src/wifi_service.cpp). +- **Target:** ESP32 + WiFi. +- **Bound:** 30 000 ms passed to the Arduino `WiFiClient` — *bounded by library — not analyzed + (out of scope)*. Recorded because the firmware chooses the value. + +### 2.9 WiFi link supervisor poll — 10 s + +- **file:line:** [main.cpp:448-464](../src/main.cpp). +- **Target:** ESP32 + WiFi. +- **Duration:** `10000` ms, inline literal at [main.cpp:449](../src/main.cpp). +- **Monitors:** `WiFi.status()` vs. the cached `wifiConnected` flag. +- **Action:** on loss → `disconnectWiFiServer()`; on regain → `restartWiFiLanAfterReconnect()`. +- **Context:** loop task. +- **Fires while a BLE client is connected / transferring?** Yes to both — and via + `disconnectWiFiServer()` it raises the shared BLE-disconnect-cleanup flag. + +### 2.10 Initial blocking WiFi connect — 3 × 10 s + 2 × 2 s + +- **file:line:** [wifi_service.cpp:762-787](../src/wifi_service.cpp). +- **Target:** ESP32 + WiFi, and only on the `waitForConnection == true` path (setup). +- **Duration:** `maxRetries = 3` ([:762](../src/wifi_service.cpp)) × + `timeoutPerRetry = 10000` ms ([:763](../src/wifi_service.cpp)), with `delay(2000)` between + attempts ([:786](../src/wifi_service.cpp)) → **worst case ~34 s of blocking**. Inner poll is + `delay(500)` ([:769](../src/wifi_service.cpp)); early-aborts on `WL_CONNECT_FAILED` / + `WL_NO_SSID_AVAIL`. +- **Action on expiry:** `wifiConnected = false`, log, continue booting. +- **Context:** whichever task called `initWiFi(true)`. + +### 2.11 mDNS TXT MSD update throttle — 400 ms + +- **file:line:** [wifi_service.cpp:377-383](../src/wifi_service.cpp). +- **Target:** ESP32 + WiFi. +- **Duration:** `400` ms, inline literal at [:378](../src/wifi_service.cpp). +- **Monitors:** re-publish rate of the `msd` TXT record when the payload is unchanged. +- **Action:** skip the update. + +### 2.12 Deep-sleep idle hold + +- **file:line:** [main.cpp:486-499](../src/main.cpp) (idle branch); + [main.cpp:374-390](../src/main.cpp) (post-wake advertising branch). `lastActivityMs` is + refreshed by `pollActivity()` [main.cpp:257](../src/main.cpp), at end of setup + [main.cpp:175](../src/main.cpp), and on an aborted sleep [main.cpp:590](../src/main.cpp). +- **Target:** ESP32 only. +- **Duration — config-driven:** `power_option.sleep_timeout_ms` + ([opendisplay_structs.h:490](../include/opendisplay_structs.h), `uint16_t` LE ms — so the + maximum expressible hold is 65 535 ms). **Fallback when 0: `DEFAULT_IDLE_HOLD_MS = 10000`** + ([main.h:322](../src/main.h)), applied at [main.cpp:488-490](../src/main.cpp) and + [main.cpp:375-377](../src/main.cpp). +- **Monitors:** quiet time since the last detected activity. +- **Action:** `enterDeepSleep()`. +- **Context:** loop task. Gated on `power_mode == 1` **and** `deep_sleep_time_seconds > 0`. +- **Fires while connected?** No — `pollActivity()` treats `connCount > 0` as activity in itself + ([main.cpp:254](../src/main.cpp)), so a connected client pins the device awake indefinitely. + `enterDeepSleep()` re-checks `getConnectedCount() > 0` at [main.cpp:588](../src/main.cpp). + **During a transfer with the link already dropped?** `workInFlight` + ([main.cpp:474-479](../src/main.cpp)) does **not** include `transferActive()`, so a latched + `pipeState.active` with `connCount == 0` does not block the idle branch. **Mid-refresh?** No — + `epdRefreshInProgress` is in `workInFlight` ([main.cpp:478](../src/main.cpp)). + +### 2.13 Minimum-wake hold + +- **file:line:** `minWakeTimeMs()` [main.cpp:197-200](../src/main.cpp); `minWakeHoldActive()` + [main.cpp:202-210](../src/main.cpp); armed at [main.cpp:162-163](../src/main.cpp) (button wake) + and [main.cpp:171-172](../src/main.cpp) (first boot / hidden reset). +- **Target:** ESP32 only. +- **Duration — config-driven:** `power_option.min_wake_time_seconds` + ([opendisplay_structs.h:503](../include/opendisplay_structs.h), `uint16_t` LE seconds). + **Fallback when 0: `DEFAULT_MIN_WAKE_TIME_SECONDS = 120`** ([main.h:312](../src/main.h)). +- **Monitors:** a floor on awake time after a button wake or a first boot, layered *under* the + idle hold — sleep requires both conditions. +- **Action on expiry:** clears `minWakeWindowActive`, logs, permits sleep. +- **Context:** loop task; also re-checked defensively inside `enterDeepSleep()` + ([main.cpp:598-601](../src/main.cpp)), which the comment notes must stay ahead of the + advertising stop. + +### 2.14 Post-wake advertising window + +- **file:line:** [main.cpp:374-390](../src/main.cpp); start stamp + [main.cpp:158](../src/main.cpp). +- **Target:** ESP32 only. +- **Duration:** same source as §2.12 (`sleep_timeout_ms`, fallback `DEFAULT_IDLE_HOLD_MS`). + Measured from `lastActivityMs`, not from `advertising_start_time` — a connect-then-drop re-arms + the whole window. `advertising_start_time` is used only for the log line + ([main.cpp:384](../src/main.cpp)). +- **Action:** `enterDeepSleep()`. +- **Context:** loop task, `woke_from_deep_sleep && advertising_timeout_active` branch; the branch + `return`s every pass after `idleDelay(50)` ([main.cpp:396](../src/main.cpp)), so the command + drain and everything below it are **not reached** until a client connects. + +### 2.15 MSD refresh cadence — 60 s + +- **file:line:** [main.cpp:509-513](../src/main.cpp). +- **Target:** ESP32 only. Note it sits in the **`else` (not-`workInFlight`) branch**, so it does + not run while a client is connected or a refresh is in flight. +- **Duration:** `60000` ms, inline literal at [main.cpp:510](../src/main.cpp). +- **Action:** `updatemsdata()` — re-polls sensors and re-pushes the BLE advertisement. +- **Interlock:** `MyBLEServerCallbacks::onConnect` sets `msdUpdatePending` + ([esp32_ble_callbacks.h:56](../src/esp32_ble_callbacks.h)) rather than calling `updatemsdata()` + inline, precisely because this 60 s path also drives it from the loop task. + +### 2.16 nRF advertising boost — 3 s + +- **file:line:** constant [ble_init.cpp:44](../src/ble_init.cpp) (`NRF_ADV_BOOST_MS = 3000`); + armed [:47](../src/ble_init.cpp); consulted [:51](../src/ble_init.cpp) and + [:61](../src/ble_init.cpp); serviced by `ble_nrf_advertising_tick()` + [:59-76](../src/ble_init.cpp). +- **Target:** nRF only. +- **Duration:** 3 000 ms. Boost interval 20–30 ms (`NRF_ADV_BOOST_MIN/MAX`, + [:42-43](../src/ble_init.cpp)); steady interval 160–1000 ms + (`NRF_ADV_INTERVAL_MIN/MAX`, [:40-41](../src/ble_init.cpp)). +- **Action on expiry:** restore the slow interval, `Advertising.stop()` + `start(0)`. +- **Context:** loop task via [main.cpp:526](../src/main.cpp), and inside `idleDelay()` at + [main.cpp:540](../src/main.cpp). + +### 2.17 nRF link-diagnostic one-shot + +- **file:line:** [ble_init.cpp:136-145](../src/ble_init.cpp). +- **Target:** nRF only. +- **Duration:** `s_link_diag_timer.begin(500, …, /*repeating=*/false)` + ([ble_init.cpp:141](../src/ble_init.cpp)); `reset()` restarts it from now + ([:144](../src/ble_init.cpp)). **The literal is 500; the adjacent comment says "fires ~2.5 s + later".** See §8. +- **Action:** logs negotiated PHY/MTU/DLE if still connected. Diagnostics only — no state change. +- **Context:** FreeRTOS timer task (not the Callback task). + +### 2.18 Buzzer sequencing + +- **file:line:** step deadline `s_buzzer.step_until_ms`, armed + [buzzer_control.cpp:203](../src/buzzer_control.cpp) and + [:209](../src/buzzer_control.cpp), consumed + [buzzer_control.cpp:243](../src/buzzer_control.cpp); global cap checked + [buzzer_control.cpp:168](../src/buzzer_control.cpp) against `play_start_ms` + ([:341](../src/buzzer_control.cpp)). +- **Target:** both. +- **Durations:** `kBuzzerMaxTotalMs = 30000u` + ([buzzer_control.cpp:17](../src/buzzer_control.cpp)) — a hard 30 s cap on any playback; + `kBuzzerDurationUnitMs = 5u` ([:15](../src/buzzer_control.cpp)) — note duration units; + `kBuzzerInterPatternGapMs = 20u` ([:16](../src/buzzer_control.cpp)). + Remaining budget is recomputed per step at [:182](../src/buzzer_control.cpp), so no single note + can overshoot the cap. +- **Action on expiry:** `buzzer_stop_internal()` — tone off, drive off, state zeroed. +- **Context:** `buzzerService()`, called from `loop()` at [main.cpp:354](../src/main.cpp), + [:483](../src/main.cpp), [:516](../src/main.cpp), [:529](../src/main.cpp), and from + `idleDelay()` [main.cpp:546](../src/main.cpp). +- **Fires while connected/transferring?** The cap is honest wall-clock and fires whenever + `buzzerService()` runs — but during a blocking refresh or SPI stream `buzzerService()` does not + run, so a tone can sound past 30 s until the loop task is released. Non-blocking design: the + handler ACKs immediately after starting ([:347-348](../src/buzzer_control.cpp)). +- **Blocking exception:** `passiveBuzzerPowerOffAlert()` uses three raw `delay(80)` + ([buzzer_control.cpp:369-373](../src/buzzer_control.cpp)) — ~240 ms of unconditional blocking, + reached from the power-off hold path. + +### 2.19 LED flash sequencing + +- **file:line:** `led_schedule_delay_ms()` [device_control.cpp:356-363](../src/device_control.cpp); + deadline consumed [device_control.cpp:529](../src/device_control.cpp) in `processLedFlash()`. +- **Target:** both. +- **Durations:** per-step, from the config's packed `LedFlashPattern`, scaled by + `LED_DELAY_FACTOR_MS = 100u` and floored at `LED_MIN_STEP_DELAY_MS = 1u` + ([device_control.cpp:279-280](../src/device_control.cpp)). +- **Bound:** **none.** There is no global cap equivalent to the buzzer's `kBuzzerMaxTotalMs`; a + long loop count in the pattern runs to completion. See §5.8. +- **Context:** `processLedFlash()` from `loop()` [main.cpp:352](../src/main.cpp) and `idleDelay()` + [main.cpp:544](../src/main.cpp). + +### 2.20 Touch poll interval / I²C backoff + +- **file:line:** global floor [touch_input.cpp:588-592](../src/touch_input.cpp); per-controller + interval [touch_input.cpp:600](../src/touch_input.cpp); timed-poll comparisons + [:612](../src/touch_input.cpp) and [:625](../src/touch_input.cpp); I²C-fail backoff + [:609-611](../src/touch_input.cpp). +- **Target:** both (the `transferActive()` skip at [:584-586](../src/touch_input.cpp) is + `#ifdef TARGET_ESP32` only). +- **Durations:** `TOUCH_PROCESS_MIN_INTERVAL_MS = 100` + ([touch_input.cpp:38](../src/touch_input.cpp)) — a hard floor on how often + `processTouchInput()` does any work at all; per-controller + `TouchController.poll_interval_ms` ([opendisplay_structs.h:951](../include/opendisplay_structs.h), + `uint8_t` ms) with **firmware fallback `TOUCH_PROCESS_MIN_INTERVAL_MS` (100)** when 0 — the + header documents the default as 25 ms (see §8); `TOUCH_I2C_FAIL_BACKOFF_MS = 100` + ([:37](../src/touch_input.cpp)) suppresses level-triggered INT-low reads after a failure. +- **Action:** skip this pass. +- **Context:** loop task and `idleDelay()`. +- **Fires mid-transfer?** On ESP32 the whole function early-returns while + `transferActive() || s_epd_refresh_suspend > 0` ([:584-586](../src/touch_input.cpp)). On nRF + there is no such gate, but the transfer runs on a different task. + +### 2.21 GT911 reset/settle delays + +- **file:line:** `GT911_PRE_RESET_DELAY_MS = 300` / `GT911_POST_RESET_SETTLE_MS = 200` + ([touch_input.cpp:29-30](../src/touch_input.cpp)), used at + [:317-321](../src/touch_input.cpp), [:331-333](../src/touch_input.cpp), + [:337-339](../src/touch_input.cpp), [:429](../src/touch_input.cpp). + The address-select pulse train at [:250-269](../src/touch_input.cpp) adds + `delay(10)+delay(60)+delay(1)+delay(11)+delayMicroseconds(110)+delay(6)+delay(51)`. +- **Target:** both. +- **Bound:** fixed, unconditional blocking — a full re-init can block ~1 s. +- **Context:** whichever task calls `touchResumeAfterEpdRefresh()` / init, i.e. the loop task. + +### 2.22 Power-off button hold — `power_latch` + +- **file:line:** `POWER_OFF_HOLD_MS = 3000` ([power_latch.cpp:21](../src/power_latch.cpp)); + stamped [:139](../src/power_latch.cpp); compared [:142](../src/power_latch.cpp). +- **Target:** ESP32 (the whole file's active body is ESP32; the nRF build gets the stubs at + [power_latch.cpp:196](../src/power_latch.cpp)). +- **Duration:** 3 000 ms, firmware constant. Not config-driven on this path. +- **Action:** `powerOff()` → latch release + `esp_deep_sleep_start()`. +- **Context:** `powerButtonPoll()`, called from `processButtonEvents()` + ([device_control.cpp:587](../src/device_control.cpp)). +- **Fires while connected / transferring / mid-refresh?** `processButtonEvents()` runs from the + loop task and `idleDelay()`, so it can fire while a client is connected and while a transfer is + latched — but not while the loop task is inside a blocking refresh or SPI stream. + +### 2.23 Power-off button hold — config-driven (`device_control`) + +- **file:line:** stamped [device_control.cpp:75](../src/device_control.cpp); compared + [device_control.cpp:78](../src/device_control.cpp). +- **Target:** both. +- **Duration — config-driven:** `BinaryInputs.power_off_hold_sec` + ([opendisplay_structs.h:862](../include/opendisplay_structs.h), `uint8_t` seconds). + **Fallback when 0: 3 000 ms**, computed at + [device_control.cpp:746](../src/device_control.cpp) into `ButtonState.power_off_hold_ms` + ([structs.h:181](../src/structs.h), `uint16_t` ms). +- **Action:** `passiveBuzzerPowerOffAlert()` (≈240 ms of blocking `delay`) then + `powerLatchTriggerOff()`. +- **Context:** `processButtonEvents()` on the loop task. +- **Note:** this is a *second, independent* power-off-hold mechanism from §2.22, with a different + default source. Both can be armed on the same device. See §6. + +### 2.24 ADC-ladder button poll and press-count window + +- **file:line:** [device_control.cpp:173-200](../src/device_control.cpp). +- **Target:** ESP32 only (`registerAdcLadder` is `#ifdef TARGET_ESP32`, + [device_control.cpp:738-742](../src/device_control.cpp)). +- **Durations:** `ADC_LADDER_POLL_MS = 5` ([:96](../src/device_control.cpp)) — poll floor; + `5000` ms inline at [:193](../src/device_control.cpp) — press-count reset window; + `ADC_LADDER_DEBOUNCE = 3` ([:97](../src/device_control.cpp)) — consecutive equal samples + required. +- **Context:** loop task via `processButtonEvents()`. + +### 2.25 Sensor / battery read caches (TTL, not timeouts) + +| Cache | file:line | TTL | Target | +|---|---|---|---| +| SHT40 MSD poll | [sensor_sht40.cpp:239](../src/sensor_sht40.cpp), used [:245](../src/sensor_sht40.cpp) | `kSht40MsdPollTtlMs = 30000u` | both | +| BQ27220 MSD poll | [sensor_bq27220.cpp:142](../src/sensor_bq27220.cpp), used [:151](../src/sensor_bq27220.cpp) | `kBq27220MsdPollTtlMs = 30000u` | both | +| Battery voltage | [display_service.cpp:1707](../src/display_service.cpp), used [:1712](../src/display_service.cpp) | `kBatteryVoltageTtlMs = 30000u` | both | + +These suppress I²C/ADC work rather than bounding it. Action on expiry is "do the read". The +uncached battery read itself blocks `delay(10) + 10 × delay(2)` ≈ 30 ms +([display_service.cpp:1690-1698](../src/display_service.cpp)). + +### 2.26 Transfer start stamps (inputs to §2.1/§2.2) + +| Stamp | Set | Cleared | Read by | +|---|---|---|---| +| `directWriteStartTime` | [display_service.cpp:2077](../src/display_service.cpp) | [:2022](../src/display_service.cpp), [:2375](../src/display_service.cpp) | [main.cpp:437](../src/main.cpp) | +| `partialCtx.start_time` | [display_service.cpp:2249](../src/display_service.cpp), [:2762](../src/display_service.cpp) | `cleanup_partial_write_state()` | [display_service.cpp:580](../src/display_service.cpp) | +| `imgLogStartMs` | [display_service.cpp:1862](../src/display_service.cpp) | — | [:1901](../src/display_service.cpp) — throughput logging only, no deadline | +| `partial_prepare_panel_ram` `t0` | [display_service.cpp:3249](../src/display_service.cpp) | — | debug logging only, no deadline | + +`pipeState` has **no timestamp field at all** ([structs.h:106-126](../src/structs.h)) — this is +the structural reason a latched pipe has no bound of its own. + +--- + +## 3. Retry / attempt bounds acting as implicit timeouts + +### 3.1 Authentication rate limit — 10 attempts per 60 s + +- **file:line:** [encryption.cpp:568-578](../src/encryption.cpp). +- **Target:** both. +- **Bound:** `auth_attempts >= 10` within `timeSinceLastAuth < 60` seconds (both inline literals, + [:570-571](../src/encryption.cpp)). The counter resets when a request arrives ≥60 s after the + previous one ([:576](../src/encryption.cpp)). +- **Action:** respond `AUTH_STATUS_RATE_LIMIT`, return false. The link is **not** dropped. +- **Context:** command dispatch task. +- **Note:** the counter increments on *every* authenticate attempt including the challenge + request ([:579](../src/encryption.cpp)), so a client that re-handshakes more than 10 times a + minute rate-limits itself. + +### 3.2 Integrity-failure threshold — 3 + +- **file:line:** [encryption.cpp:692-696](../src/encryption.cpp) (nonce/replay path) and + [:729-733](../src/encryption.cpp) (CCM tag path); reset on success + [:725](../src/encryption.cpp) and at session start [:657](../src/encryption.cpp). +- **Target:** both. +- **Bound:** 3 (inline literal, twice). +- **Action:** `clearEncryptionSession()`. The link stays up; every subsequent command is answered + `RESP_AUTH_REQUIRED`, unencrypted, forever. +- **Fires mid-transfer?** **Yes — this is the primary field wedge.** Ordinary packet loss trips + the nonce arm, which increments the same counter as genuine tamper evidence. + +### 3.3 Nonce replay window — ±32, 64-entry ring + +- **file:line:** window test [encryption.cpp:131](../src/encryption.cpp); ring scan + [:137-141](../src/encryption.cpp); ring store [:152-154](../src/encryption.cpp); ring + declaration `uint64_t replay_window[64]` + ([encryption_state.h:21](../src/encryption_state.h)). +- **Target:** both. +- **Bound:** `counter_diff < -32 || counter_diff > 32` → reject. The ring index is a **function + static** at [encryption.cpp:152](../src/encryption.cpp), so `clearEncryptionSession()` memsets + the ring ([:217](../src/encryption.cpp)) but leaves the index — a real bug today. +- **Action:** reject the frame; via §3.2 three rejections kill the session. +- **Ordering hazard:** `last_seen_counter` and the ring are committed at + [:149-154](../src/encryption.cpp) **before** `aes_ccm_decrypt` runs at + [:714](../src/encryption.cpp). + +### 3.4 nRF notify retry — 4 attempts × `delay(5)` + +- **file:line:** [communication.cpp:345-349](../src/communication.cpp). +- **Target:** nRF only. +- **Bound:** up to 4 retries, 5 ms apart → **≤20 ms of blocking** per response on backpressure. +- **Action:** give up silently (no error path if all 5 attempts fail). +- **Context:** Bluefruit Callback task (or, per the inline-fallback hazard, the BLE task). + +### 3.5 ESP32 BLE notify drain cap — 16 per call + +- **file:line:** [main.cpp:279](../src/main.cpp) (`bleDrain < 16`). +- **Target:** ESP32. +- **Bound:** at most 16 notifies per `flushResponseQueueToBle()` call; also breaks early when + `notify()` returns false (mbuf exhaustion), deliberately leaving the entry queued + ([main.cpp:288-290](../src/main.cpp)). +- **Action:** return; the remainder drains next call. Called once per loop pass **and between + every command in the drain** ([main.cpp:421](../src/main.cpp)) and between config-read chunks + ([communication.cpp:473](../src/communication.cpp)). + +### 3.6 Response ring — 10 slots, drop-newest + +- **file:line:** `RESPONSE_QUEUE_SIZE 10` ([structs.h:83](../src/structs.h)); full check + [communication.cpp:113-116](../src/communication.cpp). +- **Target:** ESP32. +- **Action on full:** `od_log_error("Response queue full, dropping response")` and drop **the + newest**. Backlog warning at depth ≥2 ([:126](../src/communication.cpp)). +- **Flush:** drained to empty every pass when no central is connected + ([main.cpp:307-312](../src/main.cpp)). + +### 3.7 Command ring — 33 slots, 32 usable, drop-newest + +- **file:line:** `COMMAND_QUEUE_SIZE 33` ([main.h:371](../src/main.h), mirrored + [esp32_ble_callbacks.h:19](../src/esp32_ble_callbacks.h)); producer refuses at + `nextHead == tail` ([esp32_ble_callbacks.h:122-129](../src/esp32_ble_callbacks.h)). +- **Target:** ESP32. +- **Bound:** usable capacity is `COMMAND_QUEUE_SIZE - 1 = 32`, not 33. `PIPE_MAX_W = 32` + ([structs.h:51](../src/structs.h); 16 under `PIPE_SMALL_DRAM_WINDOW`, + [structs.h:47](../src/structs.h) — `esp32-N4` only). +- **Action on full:** `od_log_error("Command queue full, dropping command")`, drop the newest. +- **Flush:** **never.** There is no code path anywhere that resets `commandQueueHead/Tail`; + stale commands survive a disconnect. + +### 3.8 Loop command drain cap — 33 iterations + +- **file:line:** [main.cpp:406-423](../src/main.cpp) (`while (drained < COMMAND_QUEUE_SIZE)`). +- **Target:** ESP32. +- **Bound:** a count cap only — **no wall-clock cap**. Each iteration can block for the full + duration of the command it dispatches (up to a 60 s refresh), so the drain's true worst case is + unbounded in time. +- **Drain-loop trap:** `tail` is cached at [main.cpp:409](../src/main.cpp) and the incremented + value stored at [:417](../src/main.cpp) *after* dispatch — a flush performed from handler + context would be clobbered by that store. + +### 3.9 Config chunked write + +- **file:line:** START [communication.cpp:495-517](../src/communication.cpp); chunks + [communication.cpp:541-581](../src/communication.cpp). +- **Target:** both. +- **Bounds:** `MAX_CONFIG_CHUNKS = 20u`, `CONFIG_CHUNK_SIZE = 200u`, + `CONFIG_CHUNK_SIZE_WITH_PREFIX = 202u` + ([opendisplay_protocol.h:882-884](../include/opendisplay_protocol.h)); enforced at + [communication.cpp:558](../src/communication.cpp). +- **Action on violation:** `chunkedWriteState.active = false` + NACK. +- **Gap:** `chunkedWriteState.active` is cleared **only** on completion + ([:574](../src/communication.cpp)), a malformed chunk ([:558](../src/communication.cpp)), or an + auth failure ([:550](../src/communication.cpp)). **No timer.** A client that sends `0x0040` + START and vanishes latches it forever — and it is not covered by §2.1 or §2.2. + +### 3.10 Config read chunk cap + +- **file:line:** [communication.cpp:447-448](../src/communication.cpp). +- **Bound:** `maxChunks = (MAX_CONFIG_SIZE + 93) / 94`, a derived compile-time bound guaranteeing + the loop terminates regardless of payload arithmetic. `MAX_RESPONSE_DATA_SIZE = 100u` + ([opendisplay_protocol.h:885](../include/opendisplay_protocol.h)). + +### 3.11 Pipe ACK cadence and reorder bounds + +- **file:line:** in-order cadence [display_service.cpp:2854](../src/display_service.cpp); + gap/duplicate rate limit [:2877-2881](../src/display_service.cpp) and + [:2888-2893](../src/display_service.cpp); reorder overflow guard + [:2874](../src/display_service.cpp); over-size frame guard + [:2817](../src/display_service.cpp); out-of-window NACK [:2896](../src/display_service.cpp). +- **Target:** both. +- **Bounds:** `pipeState.ack_every` (negotiated, `PIPE_MAX_N = 32` / 16 on `esp32-N4`, + [structs.h:52](../src/structs.h)/[:48](../src/structs.h)); `PIPE_REORDER_SLOTS = 33` / 17 + ([structs.h:50](../src/structs.h)/[:46](../src/structs.h)); `PIPE_REORDER_SLOT_SIZE = 248` + ([structs.h:54](../src/structs.h)); `PIPE_ACK_MASK_BITS` from the protocol header. +- **Action on violation:** `sendPipeNack()` → sets `pipeState.error = true` and calls + `cleanupDirectWriteState(true)` ([display_service.cpp:2564-2578](../src/display_service.cpp)). + **`pipeState.active` stays true**, so `transferActive()` latches and the §2.1 watchdog no + longer applies. This is the "pipe fatal-NACK latch". +- All of these are *count* bounds. There is no time bound anywhere in the pipe state machine. + +### 3.12 LAN drain byte budget + +- **file:line:** [wifi_service.cpp:936-992](../src/wifi_service.cpp), loop condition + [:992](../src/wifi_service.cpp). +- **Target:** ESP32 + WiFi. +- **Bound:** `drainedBytes < sizeof(tcpReceiveBuffer)` = **16 384 bytes** per `handleWiFiServer()` + tick ([wifi_service.cpp:40](../src/wifi_service.cpp)). Frame length is separately bounded by + `OD_LAN_MAX_PAYLOAD = 4094u` + ([opendisplay_protocol.h:982](../include/opendisplay_protocol.h)), enforced at + [wifi_service.cpp:966-970](../src/wifi_service.cpp). +- **Action:** return; resume next loop pass. Like §3.8 this is a byte cap, not a time cap — each + dispatched frame can block arbitrarily. + +### 3.13 GT911 I²C retries and disable threshold + +- **file:line:** `GT911_I2C_RETRIES = 3` ([touch_input.cpp:35](../src/touch_input.cpp)), used at + [:180](../src/touch_input.cpp) and [:202](../src/touch_input.cpp) with + `delayMicroseconds(GT911_I2C_RETRY_DELAY_US)` = 500 µs + ([:36](../src/touch_input.cpp), used [:196](../src/touch_input.cpp), [:207](../src/touch_input.cpp), + [:212](../src/touch_input.cpp)); `TOUCH_I2C_FAIL_DISABLE_THRESHOLD = 5` + ([:39](../src/touch_input.cpp)), enforced [:642](../src/touch_input.cpp) and + [:677](../src/touch_input.cpp). +- **Target:** both. +- **Action:** after 5 consecutive failures, `touch_disable_controller()` + ([touch_input.cpp:97-112](../src/touch_input.cpp)) permanently disables that controller for the + rest of the boot (cleared only by a re-init at [:378-379](../src/touch_input.cpp) / + [:519-520](../src/touch_input.cpp)). +- **Underlying `Wire` transaction bound:** *bounded by library — not analyzed (out of scope)*. + **The firmware never calls `Wire.setTimeOut()`** — verified: `grep -rn "setTimeOut" src/` + returns nothing. `wireBeginForOpenDisplay()` + ([display_service.cpp:785-805](../src/display_service.cpp)) sets only clock, with a 100 kHz + fallback on a failed `begin`. + +### 3.14 Boot-screen refresh retry — 1 retry, battery only + +- **file:line:** [display_service.cpp:1622-1636](../src/display_service.cpp). +- **Target:** both (`nrfVbusPresent()` gates it). +- **Bound:** exactly one retry, and only when `!nrfVbusPresent()`. Retry costs + `pwrmgm(false) + delay(200) + prepareEpdRailForBoot() + initBbepPanelSession()` plus a second + `refreshBootScreenFull()` → a second 60 s `waitforrefresh`. Worst case ≈ **2 × 60 s of + blocking in `setup()`**. +- **Action if both fail:** `od_log_warn("Boot screen refresh did not complete")` and continue. +- **Note:** neither boot-refresh path sets `epdRefreshInProgress` — see §4 and §5. + +### 3.15 Fixed-count hardware loops + +| Loop | file:line | Bound | +|---|---|---| +| External-flash bit-bang power-down (nRF) | [main.cpp:846-853](../src/main.cpp), [:872-882](../src/main.cpp) | 8 bits × `delayMicroseconds(1)`; JEDEC read 3 bytes | +| Battery ADC averaging | [display_service.cpp:1694-1698](../src/display_service.cpp) | `numSamples = 10`, `delay(2)` each | +| E1004 half-plane byte sink | [display_service.cpp:278-294](../src/display_service.cpp) | bounded by `e1004HalfPlaneBytes`; returns on `!e1004StreamOpen` | +| Buzzer note-index folding | [buzzer_control.cpp:87-91](../src/buzzer_control.cpp) | converges into `[kBuzzerMinNoteIdx, kBuzzerMaxNoteIdx]` = [117, 234] by ±1 octave steps, with a defensive clamp at [:93-95](../src/buzzer_control.cpp) | +| Boot-screen font autosizing | [boot_screen.cpp:424](../src/boot_screen.cpp), [:439](../src/boot_screen.cpp), [:448](../src/boot_screen.cpp) | monotonically decrement a scale bounded below by 1 | +| Secure-erase zero-fill | [encryption.cpp:817](../src/encryption.cpp), [:834](../src/encryption.cpp) | bounded by `fileSize` | +| Config block walk | [config_parser.cpp:314](../src/config_parser.cpp) | bounded by `configLen` | + +--- + +## 4. Periodic ticks + +Everything below runs from `loop()` and/or `idleDelay()`. **All of it starves whenever the loop +task blocks** — the two long blockers are `waitforrefresh` (up to 60 s) and the SPI/zlib streaming +inside a direct/pipe/partial write. On nRF the command handlers run on the Bluefruit Callback +task instead, so `loop()` keeps ticking during a client-driven transfer; the loop-blocking +analysis below is ESP32-specific except where noted. + +| Tick | file:line | Cadence | Target | Starves when loop blocks | +|---|---|---|---|---| +| `processLedFlash()` | [main.cpp:352](../src/main.cpp), [:544](../src/main.cpp) | every pass; internally gated by `delay_until_ms` | both | LED pattern freezes mid-sequence | +| `epdSessionTick()` | [main.cpp:353](../src/main.cpp), [:545](../src/main.cpp) | every pass; acts only in `PWR_WARM` past the deadline | both | keep-alive expiry deferred (benign — a transfer is `PWR_ACTIVE` anyway) | +| `buzzerService()` | [main.cpp:354](../src/main.cpp), [:483](../src/main.cpp), [:516](../src/main.cpp), [:529](../src/main.cpp), [:546](../src/main.cpp) | every pass; gated by `step_until_ms` | both | a tone sounds past its step, and past the 30 s cap | +| `pollActivity()` | [main.cpp:356](../src/main.cpp) → [main.cpp:218-265](../src/main.cpp) | every pass | ESP32 | `lastActivityMs` not refreshed — but sleep gates are downstream and also blocked | +| Command drain | [main.cpp:406-423](../src/main.cpp) | every pass, ≤33 commands | ESP32 | n/a (it *is* the blocker) | +| `flushResponseQueueToBle()` | [main.cpp:421](../src/main.cpp), [:424](../src/main.cpp) | between every drained command + once per pass | ESP32 | ACKs stall → the client's PTO fires. Explicitly pre-flushed before the refresh at [display_service.cpp:2412](../src/display_service.cpp) to mitigate | +| `serviceBleDisconnectCleanup()` | [main.cpp:370](../src/main.cpp), [:428](../src/main.cpp) | every pass; deferred while `epdRefreshInProgress` | ESP32 | disconnect teardown deferred | +| `esp32_restart_ble_advertising()` | [main.cpp:372](../src/main.cpp), [:434](../src/main.cpp), [display_service.cpp:2439](../src/display_service.cpp) | on `bleRestartAdvertisingPending` | ESP32 | radio stays dark. Contains a hard `delay(100)` at [ble_init.cpp:241](../src/ble_init.cpp) | +| §2.1 direct-write watchdog | [main.cpp:436](../src/main.cpp) | every pass | ESP32 | deferred | +| `checkPartialWriteTimeout()` | [main.cpp:443](../src/main.cpp) | every pass | ESP32 | deferred | +| `handleWiFiServer()` | [main.cpp:447](../src/main.cpp) | every pass | ESP32+WiFi | LAN idle timeout and TLS handshake progress both stall | +| `serviceLanRoam()` | [wifi_service.cpp:856](../src/wifi_service.cpp), inside `handleWiFiServer()` | every pass; self-gated on `!wifiClient.connected() && !transferActive()` | ESP32+WiFi | roam deferred (by design) | +| WiFi link supervisor | [main.cpp:448-464](../src/main.cpp) | 10 s | ESP32+WiFi | link-loss detection deferred | +| `processButtonEvents()` | [main.cpp:481](../src/main.cpp), [:514](../src/main.cpp), [:527](../src/main.cpp), [:542](../src/main.cpp) | every pass (workInFlight branch) / every `idleDelay` chunk | both | **power-off hold cannot complete** — the user's only manual recovery is dead during a wedged refresh | +| `processTouchInput()` | [main.cpp:482](../src/main.cpp), [:515](../src/main.cpp), [:528](../src/main.cpp), [:543](../src/main.cpp) | ≥100 ms floor | both | touch dead (already suppressed during transfers on ESP32 by design) | +| MSD refresh | [main.cpp:509-513](../src/main.cpp) | 60 s, **idle branch only** | ESP32 | deferred | +| `ble_nrf_advertising_tick()` | [main.cpp:526](../src/main.cpp), [:540](../src/main.cpp) | every pass / every `idleDelay` chunk | nRF | advertising stays at the boosted 20–30 ms interval | +| `idleDelay(ms)` | [main.cpp:535-551](../src/main.cpp) | services the above every `CHECK_INTERVAL_MS = 100` ([:536](../src/main.cpp)) | both | n/a | + +**`idleDelay` call sites and their arguments:** `idleDelay(50)` in the post-wake advertising +window ([main.cpp:396](../src/main.cpp)); `idleDelay(5)` on the battery idle-hold path +([:495](../src/main.cpp)) and the USB idle path ([:507](../src/main.cpp)); on nRF, +`idleDelay(globalConfig.power_option.sleep_timeout_ms)` or `idleDelay(500)` +([main.cpp:519-525](../src/main.cpp)) — i.e. **on nRF the loop cadence is itself config-driven**, +up to 65 535 ms, though the 100 ms internal chunking keeps every tick above serviced. + +--- + +## 5. Unbounded waits inside this project + +This section is the inventory's most important half: the absence of a bound. + +### 5.1 `pwrmgmLockTake()` — infinite spin + +- **file:line:** [display_service.cpp:401-408](../src/display_service.cpp); the spin is + [:407](../src/display_service.cpp). +- **Target:** both (the cross-task hazard it exists for is nRF-specific: Bluefruit Callback task + vs. loop task). +- **Bound:** **none.** `while (__atomic_exchange_n(&pwrmgmLock, 1, ACQUIRE)) { delay(1); }` — it + yields (deliberately, per the comment at [:402-406](../src/display_service.cpp), to avoid + priority-inversion livelock) but never gives up. +- **Callers:** `epdSessionAcquire` ([:438](../src/display_service.cpp)), `epdSessionRelease` + ([:496](../src/display_service.cpp)), `epdSessionForceOff` + ([:514](../src/display_service.cpp)). +- **Held across:** panel I/O including `bbepSleep`, `bbepWakeUp`, `bbepSendCMDSequence`, plus a + raw `delay(50)` at [:430](../src/display_service.cpp) — *the busy-wait inside those is bounded + by library — not analyzed (out of scope)*. +- **Note:** `pwrmgmLockTryTake()` ([:409-411](../src/display_service.cpp)) is the safe variant and + is used only by `epdSessionTick` ([:520](../src/display_service.cpp)). The lock is a bare 0/1 + flag with **no owner field**, so a hung holder is indistinguishable from a busy one. + +### 5.2 `powerOff()` stuck-button spin + +- **file:line:** [power_latch.cpp:87-90](../src/power_latch.cpp). +- **Target:** ESP32. +- **Bound:** **none.** `while (digitalRead(buttonPin()) == LOW) { delay(20); }` — a shorted or + stuck-low button pin holds the device here forever, with the latch still engaged. +- **Context:** called from `powerButtonPoll()` on the loop task, i.e. this hangs `loop()`. + +### 5.3 FastEPD refresh — no firmware-side bound at all + +- **file:line:** [display_fastepd.cpp:228-231](../src/display_fastepd.cpp). +- **Target:** ESP32 with `OPENDISPLAY_FASTEPD` (IT8951 / E1004 class). +- **Bound:** `fastepd_wait_refresh()` discards its `timeout_sec` argument entirely and returns + `!s_init_failed`. `waitforrefresh(60)` short-circuits to it at + [display_service.cpp:749](../src/display_service.cpp), so **the 60 s cap does not exist on + these builds**. The real blocking lives in `fastepd_full_update()` + ([display_fastepd.cpp:222-226](../src/display_fastepd.cpp)) and `fastepd_direct_refresh` — + *bounded by library — not analyzed (out of scope)*; the firmware passes **no** timeout to + either. +- **Firmware call sites of the blocking work:** [display_service.cpp:1592](../src/display_service.cpp) + (boot), [display_service.cpp:2422-2423](../src/display_service.cpp) (direct refresh — the path + a real transfer takes), [display_service.cpp:3288](../src/display_service.cpp) + (`fastepd_partial_refresh`). + +### 5.4 Boot refresh is invisible to every `epdRefreshInProgress` gate + +- **file:line:** `refreshBootScreenFull()` [display_service.cpp:533-542](../src/display_service.cpp); + FastEPD boot path [display_service.cpp:1588-1594](../src/display_service.cpp). +- **Target:** both. +- **Bound:** the refresh itself is bounded (60 s, §2.3) or not (§5.3) — but **neither path sets + `epdRefreshInProgress`**. Verified: the flag is written only at + [display_service.cpp:2415/2436](../src/display_service.cpp) and + [:3284/3294](../src/display_service.cpp). So every reader — + [main.cpp:322](../src/main.cpp), [main.cpp:478](../src/main.cpp), + [ble_init.cpp:236](../src/ble_init.cpp) — mis-reads a 30–60 s boot refresh as idle. With the + §3.14 retry that window can double. + +### 5.5 Blocking sequences with no escape + +| Sequence | file:line | Blocking cost | +|---|---|---| +| `pwrmgm(true)` rail bring-up | [main.cpp:717-753](../src/main.cpp) | `delay(800)` + `delay(100)` (or `delay(200)` on FastEPD) ≈ **900 ms**, unconditional | +| `initBbepPanelSession()` | [display_service.cpp:341-354](../src/display_service.cpp) | `delay(200)` plus library init | +| `prepareEpdRailForBoot()` (nRF, battery) | [display_service.cpp:172-183](../src/display_service.cpp) | a full extra `pwrmgm` off/on cycle ≈ 950 ms | +| GT911 address-select + reset | [touch_input.cpp:250-269](../src/touch_input.cpp), [:317-339](../src/touch_input.cpp) | ~150 ms + up to 3 × 500 ms reset cycles | +| `esp32_restart_ble_advertising()` | [ble_init.cpp:241](../src/ble_init.cpp) | `delay(100)` | +| `updatemsdata()` advertising re-push | [display_service.cpp:1811](../src/display_service.cpp) | `delay(50)` | +| `directWriteFinishAndRefresh` pre-refresh settle | [display_service.cpp:2414](../src/display_service.cpp) | `delay(20)` | +| `enterDeepSleep()` teardown | [main.cpp:618](../src/main.cpp), [:621](../src/main.cpp), [:637](../src/main.cpp) | `delay(200) + delay(100) + delay(100)` | +| `reboot()` | [device_control.cpp:259-269](../src/device_control.cpp), [communication.cpp:720](../src/communication.cpp) | `delay(200)`/`delay(100)` before reset — terminal, so harmless | +| `checkResetPin()` | [encryption.cpp:875](../src/encryption.cpp), [:881](../src/encryption.cpp) | `delay(100)` ×2 in `setup()` | +| Button init settle | [device_control.cpp:782](../src/device_control.cpp) | `delay(10)` **per configured pin** — up to 320 ms with 32 buttons | +| `od_log_flush()` | [od_log.cpp:64](../src/od_log.cpp) | `delay(5)` on ESP32, per call | + +### 5.6 Terminal `while (1) {}` + +- **file:line:** [device_control.cpp:866](../src/device_control.cpp). +- **Target:** nRF only, inside the DFU-bootloader jump. +- **Bound:** none by design — control never returns; `bootloader_util_app_start()` has already + transferred execution. Not a freeze vector. + +### 5.7 Latched state with no timer of its own + +These are not loops, but they are unbounded in exactly the way that matters: + +| Latch | Set | Cleared | Bounded by | +|---|---|---|---| +| `pipeState.active` after a fatal NACK | [display_service.cpp:2568](../src/display_service.cpp) | only `resetPipeWriteState()` — reachable from disconnect ([main.cpp:347](../src/main.cpp)) or a new `0x0080` | **nothing** for a non-partial pipe: §2.1 keys on `directWriteActive`, which `sendPipeNack`→`cleanupDirectWriteState(true)` just cleared | +| `chunkedWriteState.active` | [communication.cpp:496](../src/communication.cpp) | completion / malformed / auth-fail only | **nothing** | +| `encryptionSession` after a BLE disconnect | — | nothing on BLE; LAN clears it at [wifi_service.cpp:804](../src/wifi_service.cpp) and [:879](../src/wifi_service.cpp) | **nothing** (survives into the next connection) | +| `directWriteTouchSuspended` / `s_epd_refresh_suspend` | [display_service.cpp:2136](../src/display_service.cpp), [:2799](../src/display_service.cpp), [:539](../src/display_service.cpp), [:1590](../src/display_service.cpp) → `touchSuspendForEpdRefresh()` [touch_input.cpp:116-117](../src/touch_input.cpp) | paired `touchResumeAfterEpdRefresh()` [touch_input.cpp:418-421](../src/touch_input.cpp) | **nothing** — an unpaired suspend leaves touch dead for the rest of the boot | +| `roamPending` | [wifi_service.cpp:606](../src/wifi_service.cpp) | `serviceLanRoam()` when idle | **nothing** — a permanently busy device never roams (deliberate) | +| `touch` `rt->disabled` | [touch_input.cpp:100](../src/touch_input.cpp) | only a full re-init | **nothing** — permanent for the boot | + +### 5.8 LED pattern with no global cap + +`processLedFlash()` ([device_control.cpp:524-535](../src/device_control.cpp)) advances a state +machine whose loop counts come from config. Unlike the buzzer there is **no +`kBuzzerMaxTotalMs` equivalent**, so a pathological pattern runs indefinitely. It never blocks +the loop (each step schedules a deadline and returns), so it is a nuisance rather than a freeze — +but it is genuinely unbounded. + +--- + +## 6. Config-driven values — consolidated + +| Timeout | Struct field | Type / range | 0 means | Firmware constant backing it | Consumer | +|---|---|---|---|---|---| +| EPD keep-alive | `power_option.screen_timeout_seconds` [opendisplay_structs.h:504](../include/opendisplay_structs.h) | `uint8_t`, `@min 0 @max 30` | power off immediately | clamp `EPD_KEEPALIVE_MAX_S = 30` [display_service.h:16](../src/display_service.h) | [display_service.cpp:383-395](../src/display_service.cpp) | +| Idle hold / advertising window | `power_option.sleep_timeout_ms` [opendisplay_structs.h:490](../include/opendisplay_structs.h) | `uint16_t` LE ms (max 65 535) | fall back to default | `DEFAULT_IDLE_HOLD_MS = 10000` [main.h:322](../src/main.h) | [main.cpp:375-377](../src/main.cpp), [:487-490](../src/main.cpp); also the nRF `idleDelay` argument [main.cpp:519-520](../src/main.cpp) | +| Minimum wake hold | `power_option.min_wake_time_seconds` [opendisplay_structs.h:503](../include/opendisplay_structs.h) | `uint16_t` LE s | fall back to default | `DEFAULT_MIN_WAKE_TIME_SECONDS = 120` [main.h:312](../src/main.h) | [main.cpp:197-200](../src/main.cpp) | +| Deep-sleep duration | `power_option.deep_sleep_time_seconds` [opendisplay_structs.h:499](../include/opendisplay_structs.h) | `uint16_t` LE s | deep sleep disabled | none | [main.cpp:583-585](../src/main.cpp), [:625-628](../src/main.cpp); overridable for one cycle by command `0x0053` | +| Encryption session lifetime | `securityConfig.session_timeout_seconds` [opendisplay_structs.h:916](../include/opendisplay_structs.h) | `uint16_t` LE s | **no timeout** (persists until disconnect) | none — the 0 case returns `true` directly | [encryption.cpp:221-232](../src/encryption.cpp) | +| Power-off hold (binary inputs) | `BinaryInputs.power_off_hold_sec` [opendisplay_structs.h:862](../include/opendisplay_structs.h) | `uint8_t` s | default 3 s | literal `3000u` [device_control.cpp:746](../src/device_control.cpp) → `ButtonState.power_off_hold_ms` [structs.h:181](../src/structs.h) | [device_control.cpp:78](../src/device_control.cpp) | +| Touch poll interval | `TouchController.poll_interval_ms` [opendisplay_structs.h:951](../include/opendisplay_structs.h) | `uint8_t` ms | header says 25 ms; **firmware uses 100** | `TOUCH_PROCESS_MIN_INTERVAL_MS = 100` [touch_input.cpp:38](../src/touch_input.cpp) | [touch_input.cpp:600](../src/touch_input.cpp) | +| LAN listener idle drop | — (protocol constant, not config) | — | — | `OD_LAN_READ_TIMEOUT_S = 30u` [opendisplay_protocol.h:984](../include/opendisplay_protocol.h) | [wifi_service.cpp:957](../src/wifi_service.cpp) | + +Everything not in this table is a compile-time firmware constant. + +Note the **two independent power-off-hold mechanisms**: `power_latch`'s fixed +`POWER_OFF_HOLD_MS = 3000` ([power_latch.cpp:21](../src/power_latch.cpp)), keyed on +`SystemConfig.pwr_pin_3`, and `device_control`'s config-driven per-instance hold. They share the +3 s default but not the source, and both can be active on one device. + +--- + +## 7. Analysis + +### 7.1 Coverage map + +Rows are failure conditions; cells name the mechanism that actually bounds them today. + +| Failure | nRF | ESP32 | Covered? | +|---|---|---|---| +| **Stalled direct-write transfer** (client silent, `directWriteActive` set) | *nothing* | §2.1, 15 min | Partial — nRF has no watchdog at all | +| **Stalled pipe transfer, no fatal NACK** | *nothing* | §2.1 via `directWriteActive` (set by `directWriteActivatePanel` for both legacy and pipe), 15 min | Partial | +| **Stalled pipe transfer after a fatal NACK** | *nothing* | *nothing* — `directWriteActive` cleared, `pipeState.active` latched | **GAP** | +| **Stalled partial write** | *nothing* | §2.2, 15 min | Partial | +| **Stalled config chunked write** | *nothing* | *nothing* | **GAP** (§3.9) | +| **Idle authenticated connection** (client connected, sending nothing) | *nothing* | *nothing* — `pollActivity` treats a live link as activity ([main.cpp:254](../src/main.cpp)), pinning the device awake | **GAP** | +| **Client flooding undecryptable frames post-session-clear** | *nothing* | *nothing* | **GAP** — every frame is answered `RESP_AUTH_REQUIRED` ([communication.cpp:664-670](../src/communication.cpp)) indefinitely | +| **Wedged panel (bb_epaper BUSY stuck)** | §2.3, 60 s | §2.3, 60 s | Yes | +| **Wedged panel (FastEPD / IT8951)** | n/a | *nothing* (§5.3) | **GAP** | +| **Wedged panel during the boot refresh** | 60 s wait, but invisible to every gate (§5.4) | same | Partial | +| **Hung I²C (GT911 holding SDA low)** | §3.13 disables the controller after 5 failures — but only if the transaction *returns* | same | **GAP** — no `Wire.setTimeOut`, no bus recovery | +| **Hung I²C (AXP2101 / SHT40 / BQ27220)** | *nothing* | *nothing* | **GAP** | +| **Dead radio — advertising stopped, never restarted** | `ble_nrf_advertising_tick` only re-starts after a boost expiry | *nothing* — `esp32_restart_ble_advertising` **clears** the pending flag when `getConnectedCount() > 0` ([ble_init.cpp:232-235](../src/ble_init.cpp)) | **GAP** | +| **Lost WiFi link** | n/a | §2.9, 10 s poll | Yes | +| **Silent LAN client** | n/a | §2.7, 30 s | Yes | +| **Stuck `pwrmgmLock`** | *nothing* (§5.1) | *nothing* | **GAP** | +| **Stuck power button (latch path)** | n/a | *nothing* (§5.2) | **GAP** | +| **Command ring full** | n/a (no ring) | drop-newest + log (§3.7); never flushed | Partial — recoverable, but stale commands survive a disconnect | +| **Response ring full** | n/a | drop-newest + log (§3.6); drained when disconnected | Yes | +| **Dead encryption session with a live link** | *nothing* | *nothing* | **GAP** — invisible to the client | +| **Encryption session expiring mid-transfer** | fires (§2.5) | fires (§2.5) | *Inverted* — the timer is itself the wedge | +| **CPU/peripheral hard hang** | *nothing* (no WDT armed) | *nothing* firmware-side | **GAP** — accepted per the plan's software-only decision | + +The shape of the gap: this firmware bounds **wall-clock transfer age** and **panel BUSY**, and +nothing else. There is no inactivity timer, no link-liveness timer, no I²C bound, no lock bound, +and on nRF no transfer watchdog whatsoever. + +### 7.2 Conflicts and nesting + +1. **§2.1/§2.2 (900 s) nest inside the drain and the refresh, not the other way round.** Both are + evaluated once per `loop()` pass at [main.cpp:436](../src/main.cpp)/[:443](../src/main.cpp), + *after* the command drain at [:406-423](../src/main.cpp). A single dispatched command can block + for a 60 s refresh, so the effective resolution of both watchdogs is one refresh, not one loop + pass. They cannot fire mid-refresh. Correct ordering, but it means "15 minutes" is really + "15 minutes, rounded up to the next loop pass." + +2. **§2.5 (encryption expiry) nests inside §2.1 and can fire during it.** Session expiry runs on + every command dispatch, so on a nonzero `session_timeout_seconds` shorter than 900 s it fires + *inside* the direct-write watchdog's window — and its action (`clearEncryptionSession`) does + **not** clear `directWriteActive`. The result is a transfer that is simultaneously "active" per + §2.1 and unable to accept a single further frame, for up to 15 minutes. + +3. **§3.2 (integrity ≥ 3) and §2.1 fight.** Same shape as (2): the session dies, the transfer + flag lives. §2.1 is the only thing that eventually releases the panel. + +4. **§3.11 (`sendPipeNack`) actively *disables* §2.1.** `cleanupDirectWriteState(true)` clears the + very flag §2.1 keys on, while leaving `pipeState.active` set. A fatal NACK therefore trades a + bounded wedge for an unbounded one. This is the single worst ordering inversion in the + inventory. + +5. **§2.7 (LAN 30 s) and the BLE disconnect path share one flag.** `disconnectWiFiServer()` sets + `bleDisconnectCleanupPending` ([wifi_service.cpp:812](../src/wifi_service.cpp)), so a LAN idle + timeout enters `serviceBleDisconnectCleanup()`. The only thing preventing it from tearing down + a live BLE transfer is the `ownerStillUp` guard at [main.cpp:328-338](../src/main.cpp) — + which is inside `#ifdef OPENDISPLAY_HAS_WIFI`, so on `esp32-N4` the guard does not exist at + all (there is also no LAN on that env, so the flag has no second raiser there today; the + hazard is latent and becomes live the moment anything else raises the flag). + +6. **§2.4 (keep-alive) vs. §2.12 (idle hold).** On battery ESP32, `enterDeepSleep()` calls + `epdSessionForceOff()` unconditionally ([main.cpp:609](../src/main.cpp)), so the effective + keep-alive is `min(screen_timeout_seconds, sleep_timeout_ms)`. With the defaults (keep-alive up + to 30 s, idle hold 10 s) the idle hold usually wins. The code comments at + [main.cpp:602-609](../src/main.cpp) document this deliberately; noting it because "30 s + keep-alive" is not what a battery device actually does. + +7. **§2.13 (min-wake) is checked twice, and the second check has an ordering constraint.** + `minWakeHoldActive()` is evaluated in `loop()` ([main.cpp:383](../src/main.cpp), + [:494](../src/main.cpp)) **and** inside `enterDeepSleep()` ([main.cpp:598](../src/main.cpp)). + The second check must stay above the advertising stop at [main.cpp:611-616](../src/main.cpp), + because everything past that point commits to `esp_deep_sleep_start()` — a late abort would + leave the device awake with the radio dark. The comment at [:594-597](../src/main.cpp) says so; + it is a real constraint on any future reordering. + +8. **§2.13/§2.12 have a side effect: `minWakeHoldActive()` mutates.** It clears + `minWakeWindowActive` when the window elapses ([main.cpp:205](../src/main.cpp)). Because it is + called from three sites, whichever one runs first consumes the transition and logs it. Benign + today (all three are on the loop task), but it is a query with side effects, same class of + defect as `isAuthenticated()`. + +9. **Two power-off holds, one device.** §2.22 (fixed 3 s, `power_latch`) and §2.23 (config, + default 3 s, `device_control`) both run from `processButtonEvents()` + ([device_control.cpp:586-587](../src/device_control.cpp)). If a device configures a + `BinaryInputs` power-off pin that is also `SystemConfig.pwr_pin_3`, both arm on the same press + with independent thresholds. + +10. **§2.15 (60 s MSD) vs. `onConnect`'s `msdUpdatePending`.** Both drive `updatemsdata()`, which + mutates the shared advertisement vector and re-pushes it with a `delay(50)` + ([display_service.cpp:1811](../src/display_service.cpp)). They are serialised only because + both run on the loop task — the `onConnect` callback deliberately defers rather than calling + inline ([esp32_ble_callbacks.h:53-56](../src/esp32_ble_callbacks.h)). Any future producer on + another task breaks this. + +11. **§3.5 (16-notify drain cap) vs. §3.6 (10-slot ring).** The drain cap exceeds the ring size, + so it is never the binding constraint; the binding one is `notify()` returning false. Not a + conflict, but the 16 is dead weight. + +### 7.3 Impact of the freeze-proofing plan + +Per existing mechanism, with the plan phase that touches it. + +| Existing mechanism | Verdict | Plan reference | +|---|---|---| +| §1.1 `-DCONFIG_FREERTOS_WATCHDOG_TIMEOUT_S=120` | **DELETES** (or renames) | Phase 2 `[L3]` | +| §1.3 reset-reason logging | LEAVES | — | +| §2.1 direct-write 15-min watchdog | **MODIFIES** — kept as a backstop, raised to 20 min | Phase 6 `[H3]` | +| §2.2 `checkPartialWriteTimeout` | **MODIFIES** — same, 20 min | Phase 6 `[H3]` | +| §2.3 `waitforrefresh(60)` (bb_epaper) | LEAVES | — | +| §2.3 `waitforrefresh` → FastEPD stub | **MODIFIES** — implement a real IT8951 LUT-busy poll honouring `timeout_sec`; wrap `fastepd_direct_refresh` | Phase 2 `[X3]` | +| §2.4 EPD keep-alive | LEAVES | — | +| §2.5 encryption session timeout | **DELETES** — age-based expiry removed; session lifetime := connection lifetime | Phase 5 | +| §2.6 auth challenge 30 s | LEAVES | — | +| §2.7 LAN 30 s idle | LEAVES — the BLE/LAN asymmetry is called out as deliberate | Phase 7 | +| §2.8 `wifiClient.setTimeout(30000)` | LEAVES | — | +| §2.9 WiFi 10 s supervisor | LEAVES (but its `disconnectWiFiServer` path gains the owner guard) | Phase 4/5 `[H2]` | +| §2.10 blocking WiFi connect | LEAVES | — | +| §2.11 mDNS 400 ms throttle | LEAVES | — | +| §2.12 deep-sleep idle hold | LEAVES — explicitly stated as requiring no change; the 5-min BLE idle drop makes `connCount` fall so the existing hold elapses naturally | Phase 7 | +| §2.13 min-wake hold | LEAVES | — | +| §2.14 advertising window | LEAVES, but Phase 4 must skip `esp32_set_ble_connectable` during the post-wake window | Phase 4 | +| §2.15 60 s MSD cadence | LEAVES | — | +| §2.16 nRF advertising boost | LEAVES | — | +| §2.17 nRF link-diag one-shot | LEAVES | — | +| §2.18 buzzer 30 s cap + sequencing | LEAVES; `abortToKnownState` adds a stop call | Phase 3 | +| §2.19 LED sequencing | LEAVES; `abortToKnownState` adds a stop call. **The plan does not add a global LED cap** — `[X6]` notes it and defers | Phase 3 / review `X6` | +| §2.20 touch poll interval | LEAVES; `touchForceResume()` added alongside | Phase 3 `[M3]` | +| §2.22 `power_latch` 3 s hold | LEAVES (the *spin* it can reach is bounded — see §5.2 row) | Phase 2 | +| §2.23 config power-off hold | LEAVES | — | +| §2.24 ADC ladder poll | LEAVES | — | +| §2.25 sensor TTLs | LEAVES | — | +| §3.1 auth rate limit 10/60 s | LEAVES | — | +| §3.2 integrity ≥ 3 | **MODIFIES** — nonce failures no longer increment it; CCM-tag trigger stays and must now drop the link | Phase 1, Phase 5 | +| §3.3 nonce window ±32 / 64-ring | **MODIFIES** — symmetric ±128 with a 256-entry ring (fallback ±64); `replay_window_index` moved into the session; `counter_diff == 0` exemption removed; check/commit split around CCM | Phase 1 `[M1]` | +| §3.4 nRF notify retry ×4 | LEAVES | — | +| §3.5 16-notify drain cap | LEAVES | — | +| §3.6 response ring 10 | **MODIFIES** — adds an overflow flag serviced in loop, gated on `transferActive()`; adds a redundant `flushResponseQueue()` | Phase 7, Phase 3 (`[L1]`: the flush is redundant on ESP32) | +| §3.7 command ring 33/32 | **MODIFIES** — adds `flushCommandQueue()`, fixes the `main.h:365-370` off-by-one comment, optionally bumps to 34 off `esp32-N4`. Overflow **keeps** drop-newest; it does not drop the link | Phase 7 `[H1]` | +| §3.8 drain count cap | **MODIFIES** — adds a 2 s wall-clock cap alongside; adds the drain-abort check between [main.cpp:415](../src/main.cpp) and [:416](../src/main.cpp) | Phase 2, Phase 3 `[M5]` | +| §3.9 config chunked write | **SUBSUMES** — no timer of its own; covered by the supervisor predicate, and `resetChunkedWriteState()` is added | Phase 6 `[X4]` | +| §3.11 pipe NACK latch | **MODIFIES** — `error_since_ms` added to `PipeWriteState` + a 10 s `pipeErrorTick()` hardware-release deadline | Phase 5 `[L2]` | +| §3.12 LAN 16 KB drain budget | LEAVES | — | +| §3.13 GT911 retries / disable | **MODIFIES** — `Wire.setTimeOut(25)` after every `Wire.begin()` plus a nine-clock SDA recovery keyed on `i2c_fail_streak` | Phase 2 `[X1]` | +| §3.14 boot-refresh retry | LEAVES, but §5.4's flag fix makes it visible | Phase 2 `[X2]` | +| §5.1 `pwrmgmLockTake` infinite spin | **MODIFIES** — returns `bool` with a **60 s** deadline, fail-closed, no steal | Phase 2 `[C2]` | +| §5.2 `powerOff` stuck-button spin | **MODIFIES** — 10 s bound, then drop the latch anyway | Phase 2 | +| §5.3 FastEPD unbounded refresh | **MODIFIES** — see §2.3 row | Phase 2 `[X3]` | +| §5.4 boot refresh invisible | **MODIFIES** — set/clear `epdRefreshInProgress` around both boot paths | Phase 2 `[X2]` | +| §5.7 `encryptionSession` surviving disconnect | **MODIFIES** — cleared on BLE disconnect (deferred on nRF behind the in-flight depth counter) | Phase 5 `[H4]` | +| §5.7 `directWriteTouchSuspended` unpaired | **MODIFIES** — `touchForceResume()` clears it and asserts the counter reached 0 | Phase 3 `[M3]` | +| §5.7 `roamPending`, `rt->disabled` | LEAVES | — | +| §5.8 LED unbounded pattern | LEAVES | review `X6` | +| §2.12 `workInFlight` gate | **MODIFIES** — `transferActive()` added to the disjunction | Phase 6 `[X5]` | +| `esp32_restart_ble_advertising` clearing the flag on a stale count | **MODIFIES** — `advertisingHealthTick()` at 30 s | Phase 4 `[X7]` | + +**New mechanisms the plan introduces** (for completeness, so a future reader can tell them from +the existing set): the 10-minute progress supervisor (`g_lastProgressMs`, 600 000 ms, Phase 6); +the 5-minute BLE idle disconnect (`OD_BLE_IDLE_DISCONNECT_MS = 300000`, Phase 7); the 10 s pipe +error-release deadline (Phase 5); the 60 s `pwrmgmLock` deadline and the 10 s `powerOff` bound +(Phase 2); the 2 s drain cap (Phase 2); the 30 s advertising health tick (Phase 4); the ~10 s +idle-incumbent eviction threshold (Phase 4 `[M2]`). + +**Silent conflicts to watch:** + +- **The plan's 20-minute backstop must sit above the supervisor's 10 minutes, and both above the + 60 s lock deadline.** As written the hierarchy is 10 s (pipe error) < 60 s (lock, refresh) < + 300 s (BLE idle) < 600 s (supervisor) < 1200 s (backstops). That is consistent — but note the + 20-minute backstops key on a *start* stamp while the supervisor keys on *progress*, so on a slow + legitimate transfer the backstop can fire first. A 20-minute upload is not achievable on any + current panel, so this is theoretical; it stops being theoretical if E1004 payload sizes grow. +- **The 5-minute BLE idle timer and the 60 s `pwrmgmLock` deadline can overlap.** A client + connected but silent for 5 minutes while another context holds `pwrmgmLock` will be dropped by + the idle timer; `abortToKnownState`'s `epdSessionForceOff()` then needs the lock it cannot get, + and must honour the new fail-closed return rather than spinning. +- **Phase 5 disables §2.5 but §2.6 (the 30 s challenge validity) stays.** They are different + timers on the same subsystem; the plan does not mention §2.6 at all. It is correct to leave it, + but the plan's "encryption is scoped to the life of the connection and nothing else" sentence + reads as if no encryption-side timer remains. One does. +- **The plan touches `esp32_restart_ble_advertising` (Phase 4 `[X7]`) but not the `delay(100)` at + [ble_init.cpp:241](../src/ble_init.cpp)**, which now runs on every health-tick-forced restart. +- **`abortToKnownState`'s buzzer/LED stop has no counterpart bound.** §5.8 stays unbounded; the + abort only stops a sequence that is already running when the abort fires. + +### 7.4 Numbers summary — shortest to longest + +Every firmware-controlled duration, sorted. Blocking `delay()`s under 10 ms are omitted. + +| Duration | Mechanism | Where | +|---|---|---| +| 500 µs | GT911 I²C retry gap | [touch_input.cpp:36](../src/touch_input.cpp) | +| 1 ms | `pwrmgmLockTake` spin yield | [display_service.cpp:407](../src/display_service.cpp) | +| 1 ms | `workInFlight` loop yield | [main.cpp:484](../src/main.cpp) | +| 5 ms | ADC ladder poll floor | [device_control.cpp:96](../src/device_control.cpp) | +| 5 ms | nRF notify retry gap | [communication.cpp:347](../src/communication.cpp) | +| 5 ms | `od_log_flush` settle (ESP32) | [od_log.cpp:64](../src/od_log.cpp) | +| 5 ms | buzzer duration unit | [buzzer_control.cpp:15](../src/buzzer_control.cpp) | +| 5 ms | `idleDelay` argument, idle paths | [main.cpp:495](../src/main.cpp), [:507](../src/main.cpp) | +| 10 ms | `waitforrefresh` poll interval | [display_service.cpp:761](../src/display_service.cpp) | +| 10 ms | per-button init settle | [device_control.cpp:782](../src/device_control.cpp) | +| 12 ms | SHT40 measurement wait | [sensor_sht40.cpp:16](../src/sensor_sht40.cpp) | +| 20 ms | buzzer inter-pattern gap | [buzzer_control.cpp:16](../src/buzzer_control.cpp) | +| 20 ms | pre-refresh settle | [display_service.cpp:2414](../src/display_service.cpp) | +| 20 ms | `powerOff` stuck-button poll | [power_latch.cpp:88](../src/power_latch.cpp) | +| 20–30 ms | nRF boosted advertising interval | [ble_init.cpp:42-43](../src/ble_init.cpp) | +| 50 ms | `idleDelay` argument, post-wake window | [main.cpp:396](../src/main.cpp) | +| 50 ms | advertising re-push settle | [display_service.cpp:1811](../src/display_service.cpp) | +| 50 ms | `epdSessionForceOffLocked` post-sleep | [display_service.cpp:430](../src/display_service.cpp) | +| 80 ms ×3 | power-off buzzer alert | [buzzer_control.cpp:369-373](../src/buzzer_control.cpp) | +| 100 ms | `idleDelay` internal chunk | [main.cpp:536](../src/main.cpp) | +| 100 ms | touch process floor / poll fallback | [touch_input.cpp:38](../src/touch_input.cpp) | +| 100 ms | touch I²C fail backoff | [touch_input.cpp:37](../src/touch_input.cpp) | +| 100 ms | LED delay factor | [device_control.cpp:279](../src/device_control.cpp) | +| 100 ms | advertising restart settle | [ble_init.cpp:241](../src/ble_init.cpp) | +| 160–1000 ms | nRF steady advertising interval | [ble_init.cpp:40-41](../src/ble_init.cpp) | +| 200 ms | GT911 post-reset settle | [touch_input.cpp:29](../src/touch_input.cpp) | +| 200 ms | panel init settle | [display_service.cpp:346](../src/display_service.cpp), [:354](../src/display_service.cpp) | +| 300 ms | GT911 pre-reset delay | [touch_input.cpp:30](../src/touch_input.cpp) | +| 400 ms | mDNS MSD TXT throttle | [wifi_service.cpp:378](../src/wifi_service.cpp) | +| 500 ms | nRF link-diag one-shot | [ble_init.cpp:141](../src/ble_init.cpp) | +| 500 ms | WiFi connect inner poll | [wifi_service.cpp:769](../src/wifi_service.cpp) | +| ~900 ms | `pwrmgm(true)` rail bring-up | [main.cpp:721](../src/main.cpp) + [:749](../src/main.cpp) | +| 2 000 ms | WiFi inter-retry delay | [wifi_service.cpp:786](../src/wifi_service.cpp) | +| 3 000 ms | nRF advertising boost | [ble_init.cpp:44](../src/ble_init.cpp) | +| 3 000 ms | power-off hold (latch, fixed) | [power_latch.cpp:21](../src/power_latch.cpp) | +| 3 000 ms | power-off hold default (config) | [device_control.cpp:746](../src/device_control.cpp) | +| 5 000 ms | ADC ladder press-count window | [device_control.cpp:193](../src/device_control.cpp) | +| 10 000 ms | WiFi link supervisor poll | [main.cpp:449](../src/main.cpp) | +| 10 000 ms | WiFi connect timeout per retry | [wifi_service.cpp:763](../src/wifi_service.cpp) | +| 10 000 ms | `DEFAULT_IDLE_HOLD_MS` | [main.h:322](../src/main.h) | +| ≤30 000 ms | EPD keep-alive (`EPD_KEEPALIVE_MAX_S`) | [display_service.h:16](../src/display_service.h) | +| 30 000 ms | buzzer total playback cap | [buzzer_control.cpp:17](../src/buzzer_control.cpp) | +| 30 000 ms | auth challenge validity | [encryption.cpp:604](../src/encryption.cpp) | +| 30 000 ms | LAN idle drop (`OD_LAN_READ_TIMEOUT_S`) | [opendisplay_protocol.h:984](../include/opendisplay_protocol.h) | +| 30 000 ms | `wifiClient.setTimeout` | [wifi_service.cpp:888](../src/wifi_service.cpp) | +| 30 000 ms | sensor / battery read TTLs | [sensor_sht40.cpp:239](../src/sensor_sht40.cpp), [sensor_bq27220.cpp:142](../src/sensor_bq27220.cpp), [display_service.cpp:1707](../src/display_service.cpp) | +| 60 000 ms | auth rate-limit window (10 attempts) | [encryption.cpp:570](../src/encryption.cpp) | +| 60 000 ms | MSD refresh cadence | [main.cpp:510](../src/main.cpp) | +| 60 000 ms | `waitforrefresh` cap (bb_epaper only) | [display_service.cpp:760](../src/display_service.cpp) with `timeout = 60` | +| ≤65 535 ms | `sleep_timeout_ms` idle hold / advertising window; nRF `idleDelay` argument | [opendisplay_structs.h:490](../include/opendisplay_structs.h) | +| 120 000 ms | `DEFAULT_MIN_WAKE_TIME_SECONDS` | [main.h:312](../src/main.h) | +| ≤65 535 s | `min_wake_time_seconds`, `deep_sleep_time_seconds`, `session_timeout_seconds` | config | +| 900 000 ms | direct-write watchdog | [main.cpp:438](../src/main.cpp) | +| 900 000 ms | partial-write watchdog | [display_service.cpp:580](../src/display_service.cpp) | +| ∞ | `pwrmgmLockTake`, `powerOff` button spin, FastEPD refresh, pipe NACK latch, chunked-write latch, encryption session across a BLE disconnect | §5 | + +The visible hierarchy: everything the firmware bounds today is either **under a minute** (panel, +LAN, radio, sensors) or **at fifteen minutes** (transfers). There is nothing in between — which +is precisely the band the plan's 5-minute idle and 10-minute supervisor are meant to fill. + +--- + +## 8. Corrections to existing docs + +Verified against source; each of these is wrong or imprecise in the plan or the review. + +1. **"Every ESP env passes `-DCONFIG_FREERTOS_WATCHDOG_TIMEOUT_S=120`" — correct, but only via + inheritance.** Review `L3` cites `platformio.ini:189`. Two envs + (`esp32-s3-E1004` [:156](../platformio.ini), `esp32-s3-N16R8-extuart-debug` + [:278](../platformio.ini)) do not list the flag; they inherit it through + `${env:.build_flags}`. The conclusion (all ten ESP envs, flag inert) holds — but a + `grep` for the flag returns nine lines, not ten, and anyone deleting it must check the + inheriting envs still compile. + +2. **The plan's `[L3]` says "the real TWDT is 5 s/panic on IDLE0."** That claim is sourced from + the precompiled `sdkconfig.h`, which is out of scope for this inventory. What is verifiable + from inside this repo: **the firmware arms nothing** (`grep -rn "esp_task_wdt" src/` → no + hits), so whatever the platform default is, this codebase neither sets it nor feeds it. State + it that way rather than quoting a framework value. + +3. **The buzzer's global cap is 30 s, not 5 s.** `kBuzzerMaxTotalMs = 30000u` + ([buzzer_control.cpp:17](../src/buzzer_control.cpp)), but the in-code comments at + [buzzer_control.cpp:167](../src/buzzer_control.cpp) ("Global 5 s cap") and + [:144](../src/buzzer_control.cpp) ("for the 5 s total cap") both say 5 s. Neither the plan nor + the review mentions the buzzer cap, so this is a source-comment defect rather than a doc + defect — flagged here because `[X6]` claims "nothing bounds a stuck `buzzer_control` + sequence," which is **wrong**: a 30 s cap exists. What is genuinely unbounded is the **LED** + sequence (§5.8), not the buzzer. + +4. **The nRF link-diag timer literal is 500, not 2500.** [ble_init.cpp:141](../src/ble_init.cpp) + passes `500`; the comment on the next line ([:144](../src/ble_init.cpp)) and the block comment + at [:82](../src/ble_init.cpp) both say "~2.5 s later". One of the two is wrong. Neither + plan nor review covers this; it is diagnostics-only, so the consequence is a misleading log + phase label, not a freeze. + +5. **`TouchController.poll_interval_ms`'s documented default is not the firmware's fallback.** + The header says `0 = default 25 ms` + ([opendisplay_structs.h:951](../include/opendisplay_structs.h)); the firmware falls back to + `TOUCH_PROCESS_MIN_INTERVAL_MS` = **100 ms** ([touch_input.cpp:600](../src/touch_input.cpp)). + Even an explicit 25 is floored to 100 by the global gate at + [touch_input.cpp:589-592](../src/touch_input.cpp), so **the config field cannot produce a poll + faster than 100 ms on this firmware**. Not mentioned anywhere; relevant to `[X1]`, which + assumes touch polling is frequent enough to detect an I²C wedge quickly. + +6. **Review `L4`'s line-drift note is itself slightly off.** It corrects + `communication.cpp:113-116` → `:117-121` for "the ring-full check". The ring-full *check* is + [communication.cpp:113-116](../src/communication.cpp) (`nextHead == responseQueueTail` at `:113`, the + `od_log_error` at `:114`); `:117-121` is the memcpy/enqueue that follows. Both the plan's + original and the review's correction point at roughly the right block; neither is exact. + +7. **Review `L4` says `display_fastepd.cpp:222-231` "lands on `fastepd_full_update` at + `:227-231`".** `fastepd_full_update` is [display_fastepd.cpp:222-226](../src/display_fastepd.cpp); + `fastepd_wait_refresh` is [:228-231](../src/display_fastepd.cpp). The two are transposed. The + substantive finding (`X3`) is correct. + +8. **Plan `[H3]` describes `checkPartialWriteTimeout` as living at + `display_service.cpp:578-587`** — correct — **but the plan's §Context item 2 implies it is the + only bound on a pipe transfer.** It bounds `partialCtx` only; for a *non-partial* pipe transfer + after a fatal NACK there is genuinely nothing, which the review states correctly under + "Verified CORRECT" item 3. The plan text should not be read as offering partial coverage there. + +9. **Neither document inventories the second power-off hold.** `power_latch`'s fixed + `POWER_OFF_HOLD_MS = 3000` ([power_latch.cpp:21](../src/power_latch.cpp)) is entirely separate + from `BinaryInputs.power_off_hold_sec`. Phase 2 bounds the `powerOff()` spin + ([power_latch.cpp:87-90](../src/power_latch.cpp)) but says nothing about the fact that the + `device_control` path reaches `powerLatchTriggerOff()` + ([device_control.cpp:80](../src/device_control.cpp)) rather than `powerOff()`, so the two + shutdown routes have different blocking profiles. + +10. **Neither document mentions that the *entire* recovery surface depends on `loop()`.** Every + bound in §2 and §3 except `waitforrefresh` and the nRF notify retry is evaluated from + `loop()` or `idleDelay()`. On ESP32 that includes `processButtonEvents()`, so during a wedged + blocking refresh **even the physical power-off hold does not work**. This matters for the + plan's "residual risk: recoverable by power cycle" claim — on a latching device, the power + button is itself software-mediated ([device_control.cpp:60-84](../src/device_control.cpp), + [power_latch.cpp:124-144](../src/power_latch.cpp)) and is not a recovery path while the loop + task is blocked. diff --git a/src/communication.cpp b/src/communication.cpp index eb39673..ee288cd 100644 --- a/src/communication.cpp +++ b/src/communication.cpp @@ -52,6 +52,155 @@ static const char* originTag(void) { } } +// ------------------------------------------- unauthenticated-command guard --- +// 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 can drive one response + one unrate-limited error line per frame +// indefinitely. Observed in the field: a client that got 00 70 FE and streamed +// its 0x0071 image data anyway, ~100 ms apart, until it ran out of image. +// +// Count consecutive 0xFE answers; at the threshold, drop the link. That forces +// the client through a fresh connect + CMD_AUTHENTICATE (0x0050) handshake +// instead of letting it spin. +// +// TEN, not three. Two reasons it cannot be tighter: +// - A legitimate client may probe several gated commands before it +// authenticates. The field trace shows 0x0040, 0x0044, 0x0070 and then data +// frames -- a threshold of 3 disconnects that client mid-connect, before it +// ever gets a chance to run the handshake. +// - It matches the only other auth-abuse limit in the firmware, the +// 10-attempts-per-60s cap on CMD_AUTHENTICATE (encryption.cpp), which exists +// for the same "confused client, not attacker" case. +// It is a count, not a rate: 10 rejections spread over an hour still drop the +// link, because nothing in between cleared the counter. +// +// CONSECUTIVE. Any frame that clears the gate resets it (see the reset below the +// decrypt), as does a successful authentication (encryption.cpp calls +// resetAuthGateRejects when it sets authenticated = true). A client that gets one +// command wrong and then authenticates never trips it. +// +// BLE ONLY. encryptionSession is a single global shared by BLE and LAN, so +// counting a LAN peer's rejections here would let it cost an unrelated BLE +// client its link -- the same cross-transport bleed auth_attempts already has. +// LAN-TLS frames bypass the gate entirely; LAN-plaintext ones are answered 0xFE +// as before, just not counted. +#define AUTH_GATE_MAX_CONSECUTIVE_REJECTS 10 + +// 0xFFFF is BLE_CONN_HANDLE_INVALID on the nRF SoftDevice and is never a valid +// NimBLE handle either, so it means "no link" on both stacks. +#define AUTH_GUARD_NO_CONN_HANDLE 0xFFFFu + +static uint8_t authGateRejects = 0; +static uint16_t authGateLastHandle = AUTH_GUARD_NO_CONN_HANDLE; +static volatile bool authAbuseDisconnectPending = false; +static volatile uint16_t authAbuseDisconnectHandle = AUTH_GUARD_NO_CONN_HANDLE; + +void resetAuthGateRejects(void) { authGateRejects = 0; } + +// The live central's handle, asked of the stack rather than cached from the +// connect callbacks: both stacks already track it, and mirroring it here would +// mean a second copy to keep in sync across every connect/disconnect path +// (including the ones that fire on reconnect during a refresh). +static uint16_t authGuardLiveConnHandle(void) { +#ifdef TARGET_NRF + return Bluefruit.connHandle(); // BLE_CONN_HANDLE_INVALID when idle +#endif +#ifdef TARGET_ESP32 + if (pServer == nullptr || pServer->getConnectedCount() == 0) return AUTH_GUARD_NO_CONN_HANDLE; + return pServer->getPeerInfo(0).getConnHandle(); +#endif +} + +void rejectUnauthenticated(uint16_t command) { + uint8_t response[] = {RESP_ACK, (uint8_t)(command & 0xFF), RESP_AUTH_REQUIRED}; + sendResponseUnencrypted(response, sizeof(response)); + + if (g_commandOrigin != ORIGIN_BLE) return; + + // A different central than the one that accrued the count gets a clean slate: + // a client must never inherit its predecessor's rejections and be dropped + // early for them. + const uint16_t handle = authGuardLiveConnHandle(); + if (handle != authGateLastHandle) { + authGateLastHandle = handle; + authGateRejects = 0; + } + + if (authGateRejects < 0xFF) authGateRejects++; + if (authGateRejects < AUTH_GATE_MAX_CONSECUTIVE_REJECTS) return; + + od_log_error("ERROR: %u consecutive unauthenticated commands (last 0x%04X) -- BLE link drop pending", + (unsigned)authGateRejects, (unsigned)command); + authGateRejects = 0; + // Flag-only: this runs on the BLE host task (NimBLE) or in a SoftDevice + // callback (nRF), and tearing a link down from inside the stack's own write + // callback is how you get a use-after-free. loop() does the work, after the + // response queue drains, so the client still receives this last 0xFE. Same + // deferral bleDisconnectCleanupPending already uses. + // + // The offender's handle rides along with the request so the deferred drop + // can verify it is still dropping the same link. + authAbuseDisconnectHandle = handle; + authAbuseDisconnectPending = true; + +#ifdef TARGET_NRF + // ...except on nRF, where deferring does not work during a transfer. This + // dispatch IS the Bluefruit event-task write callback, and that task runs at a + // higher priority than the Arduino loop task. Measured: while a client floods + // gated frames inside a direct/pipe write, loop() is not scheduled at all -- + // neither the drop nor its skip diagnostic printed until the flood stopped, + // which is precisely the window the guard exists to close. + // + // Safe inline here, unlike on ESP32: + // - The 0xFE above has already gone out. nRF notifies from + // sendResponseUnencrypted directly (imageCharacteristic.notify), with no + // response ring to drain first, so there is nothing left to strand. + // - Bluefruit.disconnect() only asks the SoftDevice to tear the link down. + // The BLE_GAP_EVT_DISCONNECTED event arrives afterwards on this same event + // task rather than unwinding the callback we are currently inside. + // This consumes the flag set above (the service clears it on every path), so + // loop()'s own call becomes a no-op -- it stays as the ESP32 path and as the + // fallback for anything that arms the guard from outside a write callback. + serviceBleAuthAbuseDisconnect(); +#endif +} + +void serviceBleAuthAbuseDisconnect(void) { + if (!authAbuseDisconnectPending) return; + const uint16_t handle = authAbuseDisconnectHandle; + // Clear before disconnecting: the nRF disconnect callback runs synchronously + // from Bluefruit.disconnect(), and would otherwise leave a stale request + // armed against whatever link comes next. + authAbuseDisconnectPending = false; + if (handle == AUTH_GUARD_NO_CONN_HANDLE) { + // Normally means the offender left before this pass -- benign. But it is + // also what a stack that fails to report a live link looks like, and that + // case is NOT benign: the guard arms against 0xFFFF and then quietly + // declines to drop anything, which reads exactly like the guard not + // running at all. Log both the armed handle and what the stack says right + // now, so the two are distinguishable without a rebuild. + od_log_warn("Auth-abuse drop skipped: no live link (armed handle 0x%04X, stack now 0x%04X)", + (unsigned)handle, (unsigned)authGuardLiveConnHandle()); + return; + } + // Between the rejection and this pass the offender may have disconnected and + // another central taken its place. Drop only the link that actually earned + // it -- never the innocent client that replaced it. + if (handle != authGuardLiveConnHandle()) { + od_log_info("Auth-abuse drop skipped: offending link %u is already gone", (unsigned)handle); + return; + } + + od_log_error("ERROR: Dropping BLE link (handle %u) after repeated unauthenticated commands", + (unsigned)handle); +#ifdef TARGET_NRF + Bluefruit.disconnect(handle); +#endif +#ifdef TARGET_ESP32 + if (pServer != nullptr) pServer->disconnect(handle); +#endif +} + static void reloadConfigAfterSave(void) { if (!loadGlobalConfig()) { od_log_warn("WARNING: Config was saved but reload from storage failed (see errors above). " @@ -663,15 +812,13 @@ void imageDataWritten(BLEConnHandle conn_hdl, BLECharPtr chr, uint8_t* data, uin if (isEncryptionEnabled() && g_commandOrigin != ORIGIN_LAN_TLS) { if (!isAuthenticated()) { od_log_error("ERROR: [%s] Command requires authentication (encryption enabled)", originTag()); - uint8_t response[] = {RESP_ACK, (uint8_t)(command & 0xFF), RESP_AUTH_REQUIRED}; - sendResponseUnencrypted(response, sizeof(response)); + rejectUnauthenticated(command); return; } if (len < BLE_CMD_HEADER_SIZE + ENCRYPTION_NONCE_SIZE + ENCRYPTION_TAG_SIZE) { od_log_error("ERROR: [%s] Unencrypted command received when encryption is enabled", originTag()); - uint8_t response[] = {RESP_ACK, (uint8_t)(command & 0xFF), RESP_AUTH_REQUIRED}; - sendResponseUnencrypted(response, sizeof(response)); + rejectUnauthenticated(command); return; } @@ -695,13 +842,71 @@ void imageDataWritten(BLEConnHandle conn_hdl, BLECharPtr chr, uint8_t* data, uin nonce_full[12], nonce_full[13], nonce_full[14], nonce_full[15]); } - if (!decryptCommand(data + BLE_CMD_HEADER_SIZE + ENCRYPTION_NONCE_SIZE, encrypted_data_len, plaintext, &plaintext_len, nonce_full, auth_tag, command)) { + NonceResult decrypt_reason = NONCE_OK; + if (!decryptCommand(data + BLE_CMD_HEADER_SIZE + ENCRYPTION_NONCE_SIZE, encrypted_data_len, plaintext, &plaintext_len, nonce_full, auth_tag, command, &decrypt_reason)) { + // Step 4b / [H1]: a pipe DATA frame rejected because its nonce fell + // outside the window IS ordinary packet loss - it is the direct + // consequence of frames having been dropped. pipe-write-protocol.md + // §5.2 already reserves NACKs for unrecoverable conditions, "not + // ordinary packet loss", and §5.1 makes a 0x81 NACK unconditionally + // fatal, so answering one here violates the spec as written. Send + // NOTHING: silence is a first-class signal on the pipe path. The seq + // is absent from the next SACK mask, the client retransmits it, and + // the transfer continues. Answering with the 3-byte NACK instead + // makes the client raise IntegrityCheckError, which its pipe send + // loop does not catch, killing the whole upload on the first + // rejected frame. + // + // Deliberately narrow: + // - TAG failures keep the NACK. They are tamper evidence, not loss. + // - 0x0071 (legacy DIRECT_WRITE_DATA) is left alone on purpose: it + // has a different ACK discipline that has not been analysed, and + // the field failure lives on the pipe path. Not an oversight. + const bool nonce_loss = (decrypt_reason == NONCE_OUT_OF_WINDOW || decrypt_reason == NONCE_REPLAY); + if (nonce_loss && command == CMD_PIPE_WRITE_DATA) { + return; + } + + // NONCE_BAD_SESSION is NOT loss and NOT tampering — it means the two + // sides disagree about which session is live, and no amount of client + // retrying can resolve that. Only a re-authentication can. Answer + // RESP_AUTH_REQUIRED, which says exactly that; the client raises + // AuthenticationRequiredError and re-authenticates, and the mismatch + // clears in one round trip. + // + // This restores a recovery path Phase 1 removed. Before Phase 1 a + // session-id mismatch counted toward integrity_failures, so three of + // them cleared the session and every later command answered 0xFE — + // which is what actually made the client re-authenticate. Routing + // BAD_SESSION to "does not count" ([L7]) was correct in refusing to + // treat it as tamper evidence, but it left the device answering a + // fatal 0xFF forever to a client that had lost its session and fallen + // back to plaintext uploads (observable as len=232 = 2 + CHUNK_SIZE + // 230, the *unencrypted* chunk budget; an encrypted 0x0071 frame + // cannot exceed 185 B). The device must tell the client to + // re-authenticate instead of NACKing it forever. + // + // In bounds under the parent plan's wire constraint: RESP_AUTH_REQUIRED + // is an existing response code used in its documented meaning ("this + // command requires a live authenticated session"), which is precisely + // the condition here. + if (decrypt_reason == NONCE_BAD_SESSION) { + od_log_warn("Nonce session_id mismatch on 0x%04X - answering AUTH_REQUIRED so the client re-authenticates", + (unsigned)command); + rejectUnauthenticated(command); + return; + } + od_log_error("ERROR: Decryption failed"); uint8_t response[] = {RESP_ACK, (uint8_t)(command & 0xFF), RESP_NACK}; sendResponseUnencrypted(response, sizeof(response)); return; } + // Cleared the gate: the peer is holding a live session and talking to it + // correctly, so the consecutive-rejection count starts over. + resetAuthGateRejects(); + static uint8_t decrypted_data[512]; decrypted_data[0] = data[0]; decrypted_data[1] = data[1]; @@ -769,10 +974,25 @@ void imageDataWritten(BLEConnHandle conn_hdl, BLECharPtr chr, uint8_t* data, uin handlePipeWriteStart(data + 2, len - 2); break; case CMD_PIPE_WRITE_DATA: // 0x0081 - // The replay counter (verifyNonceReplay) already advanced at decrypt time, - // above this switch, for every 0x0081 frame — including ones the handler - // then queues or discards — so drops/dupes never desync it and the counter - // delta stays within in-flight <= W <= 32 <= the +-32 replay window. + // Replay state is committed at decrypt time (nonceCommit, first + // statement of decryptCommand's success arm) for every 0x0081 frame + // that authenticates — including ones this handler then queues or + // discards — so drops/dupes never desync it. + // + // What bounds the forward gap is NOT a firmware constant. The client + // burns a nonce counter per *transmission*, landed or not, and its + // three transmit sites are new sends (window-credit-limited), PTO + // probes, and selective repair — and selective repair spends no + // window credit at all. The only ceiling is the client's retransmit + // budget max_retx = max(3*W, n/2), scaled by blocks_per_ack, which + // is a user-facing Home Assistant option (1..32) in another repo. + // OD_NONCE_FORWARD_CAP (src/nonce_window.h) is therefore a HEURISTIC + // with headroom over the realistic worst case, not an invariant this + // firmware can prove; a client-side blocks_per_ack change would + // silently falsify any specific number written here. A gap beyond + // the cap is no longer fatal: the frame is dropped silently (Step 4b + // above) and repaired by the normal SACK path. See Decision A / [C1] + // in docs/PLAN_PHASE1_NONCE_REPLAY_2026-07-26.md. handlePipeWriteData(data + 2, len - 2); break; case CMD_PIPE_WRITE_END: // 0x0082 diff --git a/src/communication.h b/src/communication.h index 7703584..f84f156 100644 --- a/src/communication.h +++ b/src/communication.h @@ -26,4 +26,29 @@ enum CommandOrigin { ORIGIN_BLE = 0, ORIGIN_LAN_PLAIN = 1, ORIGIN_LAN_TLS = 2 }; /// Origin of the command currently being dispatched (a CommandOrigin value). uint8_t commandOrigin(void); +// ------------------------------------------- unauthenticated-command guard --- +// Guard against a client that keeps sending gated commands into a dead session. +// Rationale and the choice of threshold are at the implementation in +// communication.cpp. +// +// Declared HERE, not in ble_init.h, because loop() calls into the guard on both +// targets. ble_init.h includes NimBLEDevice.h on ESP32, so including it from +// shared code drags the whole NimBLE surface into translation units that must +// also compile for the nRF SoftDevice. This header stays free of any BLE-stack +// include -- keep it that way; everything stack-specific is #ifdef'd inside +// communication.cpp, which already has both stacks available. + +// Answers a gated frame with RESP_AUTH_REQUIRED (0xFE) and, on BLE, counts it +// toward the disconnect threshold. Every 0xFE the encryption gate emits must go +// through here rather than building the response inline, or the count is wrong. +void rejectUnauthenticated(uint16_t command); + +// Clears the consecutive-rejection count. Called whenever a frame clears the +// encryption gate, and on a successful authentication. +void resetAuthGateRejects(void); + +// Performs a drop requested by the guard. Serviced from loop() on both targets; +// no-op unless one is pending. +void serviceBleAuthAbuseDisconnect(void); + #endif diff --git a/src/encryption.cpp b/src/encryption.cpp index 1508e6e..11fd4dd 100644 --- a/src/encryption.cpp +++ b/src/encryption.cpp @@ -29,6 +29,7 @@ using namespace Adafruit_LittleFS_Namespace; #endif void sendResponse(uint8_t* response, uint16_t len); +void resetAuthGateRejects(void); // communication.cpp: unauthenticated-command guard bool aes_cmac(const uint8_t* key, const uint8_t* message, size_t message_len, uint8_t* mac); bool aes_ecb_encrypt(const uint8_t* key, const uint8_t* input, uint8_t* output); bool aes_ccm_encrypt(const uint8_t* key, const uint8_t* nonce, size_t nonce_len, @@ -111,48 +112,84 @@ void deriveSessionId(const uint8_t* session_key, const uint8_t* client_nonce, } } -bool verifyNonceReplay(uint8_t* nonce) { - if (!encryptionSession.authenticated) return false; +// Reset ONLY the four nonce/replay fields. Named explicitly rather than +// described loosely ([M3]): clearEncryptionSession() and handleAuthenticate()'s +// fresh-session block share exactly these four and are *opposite* on everything +// else (authenticated false vs true, timestamps zeroed vs stamped, keys wiped vs +// populated). In particular nonce_counter — the device's OWN outbound counter — +// must keep being zeroed here: a device that carried it across a re-auth while +// the client restarts at 0 would reproduce the [H2] keystream reuse against +// itself. See docs/PLAN_PHASE1_NONCE_REPLAY_2026-07-26.md Step 1. +static void resetNonceState(void) { + encryptionSession.nonce_counter = 0; /* device's OWN outbound counter */ + encryptionSession.last_seen_counter = 0; + encryptionSession.integrity_failures = 0; + memset(encryptionSession.replay_bitmap, 0, sizeof(encryptionSession.replay_bitmap)); +} + +// Rate limiters for the two nonce-rejection logs. Once nonce failures stop +// counting toward integrity_failures ([L7]) nothing else throttles a peer that +// drives these lines, and out-of-window now fires routinely on a lossy link. +// +// One budget PER SITE, deliberately, not one shared budget: a stale client +// spamming session-id mismatches must not be able to silence the out-of-window +// line, which is the condition Step 5's hardware tests 0-2 exist to observe. +static uint32_t nonce_log_badsession_ms = 0; +static uint32_t nonce_log_window_ms = 0; +static bool nonceLogAllowed(uint32_t* last_ms) { + uint32_t now = millis(); + if (*last_ms != 0 && (uint32_t)(now - *last_ms) < 5000u) return false; + *last_ms = now; + return true; +} + +// PURE. Parses the counter out of the 16-byte nonce and decides whether the +// frame may be accepted. Writes NOTHING to encryptionSession on any path — that +// is the property the host test asserts by memcmp, and it is what "only a +// CCM-verified frame may advance replay state" (D2) rests on. +// +// File-static by design (Decision C): nothing outside decryptCommand should be +// able to reach the commit side. The pure logic lives in src/nonce_window.h. +// +// Caveat on how strong that is. Decision C says linkage enforces the rule, and +// for nonceCheck/nonceCommit it does. But encryption_state.h must include +// nonce_window.h for OD_NONCE_BITMAP_WORDS, and main.h includes +// encryption_state.h — so od_nonce_commit(), a static-inline header primitive, +// is visible in every TU alongside the extern encryptionSession. Nothing stops +// a determined caller from committing state directly. That is unavoidable while +// the struct needs the width macro; the rule is enforced by linkage for the +// session-aware wrappers and by convention for the raw primitive. +static NonceResult nonceCheck(const uint8_t* nonce, uint64_t* counter_out) { uint8_t nonce_session_id[8]; uint64_t nonce_counter = 0; memcpy(nonce_session_id, nonce, 8); for (int i = 0; i < 8; i++) { nonce_counter = (nonce_counter << 8) | nonce[8 + i]; } + if (counter_out != nullptr) *counter_out = nonce_counter; + if (!constantTimeCompare(nonce_session_id, encryptionSession.session_id, 8)) { - od_log_error("ERROR: Nonce session_id mismatch\n Nonce ID: %02X%02X%02X%02X%02X%02X%02X%02X\n Expected: %02X%02X%02X%02X%02X%02X%02X%02X", - nonce_session_id[0], nonce_session_id[1], nonce_session_id[2], nonce_session_id[3], - nonce_session_id[4], nonce_session_id[5], nonce_session_id[6], nonce_session_id[7], - encryptionSession.session_id[0], encryptionSession.session_id[1], encryptionSession.session_id[2], encryptionSession.session_id[3], - encryptionSession.session_id[4], encryptionSession.session_id[5], encryptionSession.session_id[6], encryptionSession.session_id[7]); - return false; - } - int64_t counter_diff = (int64_t)nonce_counter - (int64_t)encryptionSession.last_seen_counter; - if (counter_diff < -32 || counter_diff > 32) { - od_log_error("ERROR: Nonce counter outside replay window (counter=%llu, last_seen=%llu, diff=%lld)", - (unsigned long long)nonce_counter, (unsigned long long)encryptionSession.last_seen_counter, (long long)counter_diff); - return false; - } - if (nonce_counter <= encryptionSession.last_seen_counter && counter_diff != 0) { - bool already_seen = false; - for (int i = 0; i < 64; i++) { - if (encryptionSession.replay_window[i] == nonce_counter) { - already_seen = true; - break; - } + // [L7] Demoted from ERROR + rate-limited, and the full session-id dump + // is gone: a mismatched session id is usually a stale client talking to + // a device that re-authenticated, i.e. confusion rather than attack. + // The CCM tag remains the only tamper oracle. + if (nonceLogAllowed(&nonce_log_badsession_ms)) { + od_log_warn("Nonce session_id mismatch (%02X%02X.. vs %02X%02X..) - frame dropped, session kept", + nonce_session_id[0], nonce_session_id[1], + encryptionSession.session_id[0], encryptionSession.session_id[1]); } - if (already_seen) { - od_log_error("ERROR: Nonce counter already seen (replay detected)"); - return false; - } - } - if (nonce_counter > encryptionSession.last_seen_counter) { - encryptionSession.last_seen_counter = nonce_counter; + return NONCE_BAD_SESSION; } - static uint8_t replay_window_index = 0; - encryptionSession.replay_window[replay_window_index] = nonce_counter; - replay_window_index = (replay_window_index + 1) % 64; - return true; + return od_nonce_check(encryptionSession.replay_bitmap, + encryptionSession.last_seen_counter, nonce_counter); +} + +// Records a counter as consumed. Called from exactly one place: as the FIRST +// statement of decryptCommand's success arm, i.e. only after aes_ccm_decrypt +// has verified the tag (the D2 fix). +static void nonceCommit(uint64_t counter) { + od_nonce_commit(encryptionSession.replay_bitmap, + &encryptionSession.last_seen_counter, counter); } void getCurrentNonce(uint8_t* nonce) { @@ -207,14 +244,11 @@ void clearEncryptionSession() { memset(encryptionSession.server_nonce, 0, 16); memset(encryptionSession.pending_server_nonce, 0, 16); encryptionSession.authenticated = false; - encryptionSession.nonce_counter = 0; - encryptionSession.last_seen_counter = 0; - encryptionSession.integrity_failures = 0; + resetNonceState(); encryptionSession.session_start_time = 0; encryptionSession.last_activity = 0; encryptionSession.auth_attempts = 0; encryptionSession.server_nonce_time = 0; - memset(encryptionSession.replay_window, 0, sizeof(encryptionSession.replay_window)); od_log_info("Encryption session cleared"); } @@ -652,12 +686,12 @@ bool handleAuthenticate(uint8_t* data, uint16_t len) { return false; } encryptionSession.authenticated = true; - encryptionSession.nonce_counter = 0; - encryptionSession.last_seen_counter = 0; - encryptionSession.integrity_failures = 0; + // The client just proved it holds the key: whatever gated commands it got + // wrong before this point must not count toward the link drop. + resetAuthGateRejects(); + resetNonceState(); encryptionSession.session_start_time = currentTime; encryptionSession.last_activity = currentTime; - memset(encryptionSession.replay_window, 0, sizeof(encryptionSession.replay_window)); memset(encryptionSession.pending_server_nonce, 0, 16); encryptionSession.server_nonce_time = 0; uint8_t server_response[16]; @@ -686,13 +720,38 @@ bool handleAuthenticate(uint8_t* data, uint16_t len) { } bool decryptCommand(uint8_t* ciphertext, uint16_t ciphertext_len, uint8_t* plaintext, - uint16_t* plaintext_len, uint8_t* nonce_full, uint8_t* auth_tag, uint16_t command_header) { + uint16_t* plaintext_len, uint8_t* nonce_full, uint8_t* auth_tag, uint16_t command_header, + NonceResult* reason_out) { + // reason_out reports WHY the frame was rejected so the caller can tell a + // nonce rejection (ordinary packet loss) from a CCM tag failure (tamper + // evidence). NONCE_OK means "not rejected for a nonce reason" — every + // non-nonce failure path below leaves it at NONCE_OK. See Step 4b. + if (reason_out != nullptr) *reason_out = NONCE_OK; if (!isAuthenticated()) return false; - if (!verifyNonceReplay(nonce_full)) { - encryptionSession.integrity_failures++; - if (encryptionSession.integrity_failures >= 3) { - od_log_warn("Too many integrity failures, clearing session"); - clearEncryptionSession(); + + // Nonce failures are evidence of a LOSSY LINK, not of tampering, so they do + // NOT touch integrity_failures (the D1 fix). Only the CCM tag is a tamper + // oracle. [L7]: routing NONCE_BAD_SESSION here too is a deliberate policy + // change — a session-id mismatch is what a stale client sends after the + // device re-authenticated. See Step 4. + uint64_t nonce_counter = 0; + NonceResult nr = nonceCheck(nonce_full, &nonce_counter); + if (nr != NONCE_OK) { + if (reason_out != nullptr) *reason_out = nr; + if (nr != NONCE_BAD_SESSION && nonceLogAllowed(&nonce_log_window_ms)) { + // A replayed counter is normally BEHIND last_seen, where the wrapping + // fwd delta prints as a 20-digit number. Report the distance in the + // direction that is actually small, so the line is readable in the + // case it fires in. + const bool behind = (nr == NONCE_REPLAY); + od_log_warn("Nonce %s (counter=%llu last_seen=%llu %s=%llu) - frame dropped, session kept", + behind ? "replay" : "out-of-window", + (unsigned long long)nonce_counter, + (unsigned long long)encryptionSession.last_seen_counter, + behind ? "back" : "fwd", + (unsigned long long)(behind + ? (encryptionSession.last_seen_counter - nonce_counter) + : (nonce_counter - encryptionSession.last_seen_counter))); } return false; } @@ -715,6 +774,12 @@ bool decryptCommand(uint8_t* ciphertext, uint16_t ciphertext_len, uint8_t* plain ad, 2, ciphertext, encrypted_len, decrypted_with_length, auth_tag, ENCRYPTION_TAG_SIZE); if (success) { + // [L2] FIRST statement of the success arm — deliberately ahead of the + // early return for a malformed payload_length below. That frame is + // *authentic* (it passed the CCM tag) and today's unconditional commit + // does record it; committing after the early return would silently + // leave an authentic frame replayable. + nonceCommit(nonce_counter); uint8_t payload_length = decrypted_with_length[0]; if (payload_length > encrypted_len - 1) { od_log_error("ERROR: Invalid payload length in decrypted data"); diff --git a/src/encryption.h b/src/encryption.h index 374cd6e..f17c93c 100644 --- a/src/encryption.h +++ b/src/encryption.h @@ -3,6 +3,7 @@ #include #include +#include "nonce_window.h" bool deriveSessionKey(const uint8_t* master_key, const uint8_t* client_nonce, const uint8_t* server_nonce, uint8_t* session_key); @@ -14,12 +15,17 @@ bool isAuthenticated(); void clearEncryptionSession(); bool checkEncryptionSessionTimeout(); void updateEncryptionSessionActivity(); -bool verifyNonceReplay(uint8_t* nonce); void getCurrentNonce(uint8_t* nonce); void incrementNonceCounter(); bool handleAuthenticate(uint8_t* data, uint16_t len); +/// Decrypt one CCM-enveloped command. On failure, *reason_out (if non-null) +/// reports whether the frame was rejected for a NONCE reason — ordinary packet +/// loss, which the caller must NOT answer with a fatal NACK on the pipe path +/// (Step 4b) — or is NONCE_OK, meaning the failure was a tag failure or a +/// malformed frame, which keeps today's NACK. bool decryptCommand(uint8_t* ciphertext, uint16_t ciphertext_len, uint8_t* plaintext, - uint16_t* plaintext_len, uint8_t* nonce, uint8_t* auth_tag, uint16_t command_header); + uint16_t* plaintext_len, uint8_t* nonce, uint8_t* auth_tag, uint16_t command_header, + NonceResult* reason_out); bool encryptResponse(uint8_t* plaintext, uint16_t plaintext_len, uint8_t* ciphertext, uint16_t* ciphertext_len, uint8_t* nonce, uint8_t* auth_tag); /// Derive the 16-byte TLS-PSK for the LAN TLS channel from the configured master diff --git a/src/encryption_state.h b/src/encryption_state.h index 88e8f63..3c972ca 100644 --- a/src/encryption_state.h +++ b/src/encryption_state.h @@ -4,6 +4,7 @@ #include #include #include "structs.h" +#include "nonce_window.h" #ifdef TARGET_ESP32 #include "mbedtls/ccm.h" #endif @@ -18,7 +19,13 @@ struct EncryptionSession { #endif uint64_t nonce_counter; uint64_t last_seen_counter; - uint64_t replay_window[64]; + // Anti-replay: bit i == "counter (last_seen_counter - i) has been consumed". + // Bit 0 is last_seen_counter itself. The backward window is implicitly + // OD_NONCE_BACKWARD_BITS - 1; there is no separate window constant to keep + // in step, and no insertion index to reset. Replaces a uint64_t[64] value + // ring (512 B -> 32 B). See src/nonce_window.h and + // docs/PLAN_PHASE1_NONCE_REPLAY_2026-07-26.md Step 1 / Decision B. + uint64_t replay_bitmap[OD_NONCE_BITMAP_WORDS]; uint32_t last_activity; uint8_t integrity_failures; uint32_t session_start_time; diff --git a/src/main.cpp b/src/main.cpp index 01bb2ac..8221b1f 100644 --- a/src/main.cpp +++ b/src/main.cpp @@ -422,6 +422,10 @@ void loop() { } } flushResponseQueueToBle(); + // Deliberately AFTER the flush: the guard's final 00 xx FE is already in the + // response ring when the drop is requested, and dropping the link first would + // strand it — leaving the client with no idea why it was disconnected. + serviceBleAuthAbuseDisconnect(); // Service the flag-only BLE callbacks on this (single) task. Cleanup runs // before the advertising restart so a disconnected session is fully torn down // before the radio re-arms. @@ -523,6 +527,9 @@ void loop() { else{ idleDelay(500); } + // nRF notifies responses inline from the command callback, so the guard's + // 00 xx FE is already on the air by the time this runs. + serviceBleAuthAbuseDisconnect(); ble_nrf_advertising_tick(); processButtonEvents(); processTouchInput(); diff --git a/src/main.h b/src/main.h index 2e4acfa..3211d7d 100644 --- a/src/main.h +++ b/src/main.h @@ -271,9 +271,8 @@ void clearEncryptionSession(); bool checkEncryptionSessionTimeout(); void updateEncryptionSessionActivity(); bool handleAuthenticate(uint8_t* data, uint16_t len); -bool decryptCommand(uint8_t* ciphertext, uint16_t ciphertext_len, uint8_t* plaintext, uint16_t* plaintext_len, uint8_t* nonce, uint8_t* auth_tag, uint16_t command_header); +bool decryptCommand(uint8_t* ciphertext, uint16_t ciphertext_len, uint8_t* plaintext, uint16_t* plaintext_len, uint8_t* nonce, uint8_t* auth_tag, uint16_t command_header, NonceResult* reason_out); bool encryptResponse(uint8_t* plaintext, uint16_t plaintext_len, uint8_t* ciphertext, uint16_t* ciphertext_len, uint8_t* nonce, uint8_t* auth_tag); -bool verifyNonceReplay(uint8_t* nonce); void incrementNonceCounter(); void getCurrentNonce(uint8_t* nonce); diff --git a/src/nonce_window.h b/src/nonce_window.h new file mode 100644 index 0000000..4442edc --- /dev/null +++ b/src/nonce_window.h @@ -0,0 +1,147 @@ +#ifndef NONCE_WINDOW_H +#define NONCE_WINDOW_H + +// Anti-replay sliding window — pure state machine, ZERO dependencies. +// +// This header deliberately includes nothing from Arduino, mbedtls, or the +// firmware logging layer, and it touches no global state. Everything here +// operates on plain values passed in by the caller, so the whole state machine +// can be compiled and exercised on a host under UBSan/ASan by +// tools/test_nonce_window.cpp. See docs/PLAN_PHASE1_NONCE_REPLAY_2026-07-26.md +// Decision D. +// +// Representation (RFC 4303 / RFC 6347 "shifting" style, as opposed to the +// circular RFC 6479 / WireGuard style — see Decision B): +// +// bit i of the bitmap == "counter (last_seen - i) has been consumed". +// bit 0 is last_seen itself. +// +// The backward window is therefore implicitly OD_NONCE_BACKWARD_BITS - 1; there +// is no separate window constant to keep in step, and no insertion index to +// reset. "Not seen" is a clear bit rather than a reserved sentinel value, so a +// fresh session (last_seen = 0, all-zero bitmap) accepts counter 0 exactly once +// with no has_seen_counter flag. + +#include +#include +#include + +// Width of the backward (out-of-order tolerance) window, in bits. +// uint64_t[4] = 32 B. Kept strictly greater than OD_NONCE_FORWARD_CAP so that a +// legal forward slide can never exceed the bitmap width — that keeps the +// wholesale-clear branch in od_nonce_commit() off the normal path. +#define OD_NONCE_BACKWARD_BITS 256 + +// Largest forward jump that is accepted. This is a HEURISTIC, not an invariant +// firmware can prove: the real bound lives in the client's retransmit budget +// (max_retx = max(3*W, n/2)) and its blocks_per_ack setting, both of which live +// in another repo and one of which is a user-facing Home Assistant option. See +// Decision A in docs/PLAN_PHASE1_NONCE_REPLAY_2026-07-26.md. +#define OD_NONCE_FORWARD_CAP 128 + +#define OD_NONCE_BITMAP_WORDS (OD_NONCE_BACKWARD_BITS / 64) + +enum NonceResult { + NONCE_OK = 0, + NONCE_BAD_SESSION, + NONCE_OUT_OF_WINDOW, + NONCE_REPLAY +}; + +static inline bool od_nonce_bit_test(const uint64_t* bm, uint64_t bit) { + return ((bm[(size_t)(bit >> 6)] >> (unsigned)(bit & 63u)) & 1ull) != 0ull; +} + +static inline void od_nonce_bit_set(uint64_t* bm, uint64_t bit) { + bm[(size_t)(bit >> 6)] |= (1ull << (unsigned)(bit & 63u)); +} + +// Pure: decides whether `counter` may be accepted. Writes nothing. +// +// The four tests are ORDERED and the order is load-bearing. fwd and back sum to +// zero mod 2^64, so they cannot both be small: with the current constants +// fwd <= 128 && back < 256 would require fwd + back <= 384 == 0 (mod 2^64), +// true only when both are zero — the case the first test already consumed. A +// counter far from the window in either direction leaves both huge and falls +// through to NONCE_OUT_OF_WINDOW. Do not reorder. +// +// Unsigned wrapping arithmetic only: the counter is parsed off the wire BEFORE +// the CCM tag is verified, so an unauthenticated attacker controls both +// operands. Converting a uint64_t >= 2^63 to int64_t is implementation-defined +// before C++20, the signed subtraction can overflow, and negating INT64_MIN is +// UB — three ways to be undefined on attacker-chosen input. Unsigned overflow +// is defined as modular arithmetic, making this total over all 2^64 inputs. +static inline NonceResult od_nonce_check(const uint64_t* bm, uint64_t last_seen, uint64_t counter) { + const uint64_t fwd = counter - last_seen; /* wraps; 0 when equal */ + const uint64_t back = last_seen - counter; /* wraps; fwd + back == 0 mod 2^64 */ + + if (fwd == 0u) return od_nonce_bit_test(bm, 0) ? NONCE_REPLAY : NONCE_OK; + if (fwd <= OD_NONCE_FORWARD_CAP) return NONCE_OK; /* ahead: cannot have been seen */ + if (back < OD_NONCE_BACKWARD_BITS) return od_nonce_bit_test(bm, back) ? NONCE_REPLAY : NONCE_OK; + return NONCE_OUT_OF_WINDOW; +} + +// Shift the bitmap left by `shift` bits (bit i -> bit i + shift), clearing the +// vacated low bits. Handles shift == 0 and shift >= 64 explicitly: `x << 64` is +// undefined behaviour in C/C++ and is the classic bug in this pattern. +static inline void od_nonce_bitmap_shift_left(uint64_t* bm, uint64_t shift) { + if (shift == 0u) return; + if (shift >= OD_NONCE_BACKWARD_BITS) { + memset(bm, 0, sizeof(uint64_t) * OD_NONCE_BITMAP_WORDS); + return; + } + const size_t word_shift = (size_t)(shift >> 6); + const unsigned bit_shift = (unsigned)(shift & 63u); + for (size_t i = OD_NONCE_BITMAP_WORDS; i-- > 0;) { + uint64_t v = 0u; + if (i >= word_shift) { + v = bm[i - word_shift] << bit_shift; + if (bit_shift != 0u && i > word_shift) { + v |= bm[i - word_shift - 1] >> (64u - bit_shift); + } + } + bm[i] = v; + } +} + +// Records `counter` as consumed. MUST only be called after the frame carrying +// it has been authenticated (CCM tag verified) — that is the D2 fix. +// +// Defined (never UB) for every input, including inputs od_nonce_check() would +// have rejected: the host test calls it directly, and a future +// OD_NONCE_FORWARD_CAP increase must not be able to produce an over-wide shift. +// The wholesale-clear path is unreachable through od_nonce_check() today (cap +// 128 < 256 bits) but is still correct when it fires on a FORWARD jump: every +// counter it discards is then >= 256 behind and is rejected on width, never +// mis-reported as unseen. +// +// Sharp edge, stated rather than hidden: a counter more than +// OD_NONCE_BACKWARD_BITS *behind* last_seen also lands in the forward branch +// (fwd and back are complements, so its fwd is enormous), clearing the bitmap +// and RE-WINDING last_seen to that counter — un-seeing everything. That is +// unreachable through od_nonce_check(), which returns NONCE_OUT_OF_WINDOW for +// such a counter so it is never committed, and the contract above is that +// commit runs only for frames that both checked OK and authenticated. It is the +// edge to watch if a future caller ever commits without checking first. +static inline void od_nonce_commit(uint64_t* bm, uint64_t* last_seen, uint64_t counter) { + const uint64_t fwd = counter - *last_seen; + const uint64_t back = *last_seen - counter; + + if (fwd == 0u) { + od_nonce_bit_set(bm, 0); + return; + } + if (back < OD_NONCE_BACKWARD_BITS) { + /* backward, inside the window: last_seen does not move. + Unambiguous: fwd and back are complements mod 2^64, so back < 256 + forces fwd >= 2^64 - 255 — they can never both be small. */ + od_nonce_bit_set(bm, back); + return; + } + /* forward */ + od_nonce_bitmap_shift_left(bm, fwd); + *last_seen = counter; + od_nonce_bit_set(bm, 0); +} + +#endif // NONCE_WINDOW_H diff --git a/tools/test_nonce_window.cpp b/tools/test_nonce_window.cpp new file mode 100644 index 0000000..c73862c --- /dev/null +++ b/tools/test_nonce_window.cpp @@ -0,0 +1,702 @@ +// Host test for src/nonce_window.h — standalone, no test framework. +// +// Build and run from the repo root: +// +// 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 +// +// This file is as much a written-down statement of the intended semantics of the +// anti-replay window as it is a test. Each block names the coverage item from +// docs/PLAN_PHASE1_NONCE_REPLAY_2026-07-26.md, Decision D, that it discharges. +// +// The representation under test (see the header): +// bit i of the bitmap == "counter (last_seen - i) has been consumed" +// bit 0 == last_seen itself +// so the backward window is OD_NONCE_BACKWARD_BITS wide (indices 0..255) and the +// forward acceptance window is OD_NONCE_FORWARD_CAP wide. + +#include "../src/nonce_window.h" + +#include +#include +#include +#include +#include +#include +#include + +// --------------------------------------------------------------------------- +// Make UBSan reports fatal +// +// UBSan defaults to "print and keep going", so a regression that reintroduces +// signed counter arithmetic would emit a runtime-error line and still exit 0 — +// invisible to CI. __ubsan_on_report is the sanitizer's weak notification hook; +// overriding it turns any report into a nonzero exit without needing the caller +// to remember UBSAN_OPTIONS=halt_on_error=1. +// +// Defined unconditionally: libubsan declares it weak, so this overrides it when +// the sanitizer is linked in, and is simply unreferenced when it is not. (It +// cannot be guarded on a macro — GCC only started defining __SANITIZE_UNDEFINED__ +// in GCC 14, and this must work on older CI runners.) +// --------------------------------------------------------------------------- + +extern "C" void __ubsan_on_report(void); +extern "C" void __ubsan_on_report(void) { + std::fputs("FAIL: undefined behaviour reported by UBSan (see the runtime error above)\n", + stderr); + std::fflush(stderr); + std::_Exit(EXIT_FAILURE); +} + +// --------------------------------------------------------------------------- +// Minimal check harness +// --------------------------------------------------------------------------- + +static unsigned long g_checks = 0; +static unsigned long g_failures = 0; + +// Free-form context describing the enclosing loop iteration, printed on failure +// so that a failing case inside a data-driven loop is identifiable. +static char g_context[256] = ""; + +static void set_context(const char* fmt, ...) __attribute__((format(printf, 1, 2))); + +static void set_context(const char* fmt, ...) { + va_list ap; + va_start(ap, fmt); + vsnprintf(g_context, sizeof(g_context), fmt, ap); + va_end(ap); +} + +static void clear_context(void) { g_context[0] = '\0'; } + +static const char* res_name(NonceResult r) { + switch (r) { + case NONCE_OK: return "NONCE_OK"; + case NONCE_BAD_SESSION: return "NONCE_BAD_SESSION"; + case NONCE_OUT_OF_WINDOW: return "NONCE_OUT_OF_WINDOW"; + case NONCE_REPLAY: return "NONCE_REPLAY"; + } + return "NONCE_"; +} + +static void report_failure(const char* file, int line, const char* expr) { + ++g_failures; + if (g_context[0] != '\0') { + std::printf("FAIL %s:%d [%s]: %s\n", file, line, g_context, expr); + } else { + std::printf("FAIL %s:%d: %s\n", file, line, expr); + } +} + +#define CHECK(expr) \ + do { \ + ++g_checks; \ + if (!(expr)) report_failure(__FILE__, __LINE__, #expr); \ + } while (0) + +// Compares two NonceResult values and prints both symbolic names on mismatch. +#define CHECK_RES(actual, expected) \ + do { \ + ++g_checks; \ + const NonceResult check_a_ = (actual); \ + const NonceResult check_e_ = (expected); \ + if (check_a_ != check_e_) { \ + ++g_failures; \ + if (g_context[0] != '\0') { \ + std::printf("FAIL %s:%d [%s]: %s -> got %s, want %s\n", \ + __FILE__, __LINE__, g_context, #actual, \ + res_name(check_a_), res_name(check_e_)); \ + } else { \ + std::printf("FAIL %s:%d: %s -> got %s, want %s\n", __FILE__, \ + __LINE__, #actual, res_name(check_a_), \ + res_name(check_e_)); \ + } \ + } \ + } while (0) + +// --------------------------------------------------------------------------- +// State container +// --------------------------------------------------------------------------- + +struct NonceState { + uint64_t bm[OD_NONCE_BITMAP_WORDS]; + uint64_t last_seen; +}; + +// All members are uint64_t, so the struct has no padding and can be compared +// byte-for-byte by the purity test below. +static_assert(sizeof(NonceState) == sizeof(uint64_t) * (OD_NONCE_BITMAP_WORDS + 1), + "NonceState must be padding-free for the byte-identical purity check"); + +static void state_reset(NonceState* s, uint64_t last_seen) { + std::memset(s->bm, 0, sizeof(s->bm)); + s->last_seen = last_seen; +} + +static NonceResult check(const NonceState* s, uint64_t counter) { + return od_nonce_check(s->bm, s->last_seen, counter); +} + +static void commit(NonceState* s, uint64_t counter) { + od_nonce_commit(s->bm, &s->last_seen, counter); +} + +// Accept-and-record, the way firmware uses the pair: check, and only commit if +// the frame would be accepted (and, in firmware, only after the CCM tag verifies). +static NonceResult check_and_commit(NonceState* s, uint64_t counter) { + const NonceResult r = check(s, counter); + if (r == NONCE_OK) commit(s, counter); + return r; +} + +// --------------------------------------------------------------------------- +// Decision D coverage: fresh session (last_seen = 0, empty bitmap) +// +// "Not seen" is a clear bit, not a reserved sentinel, so counter 0 on a virgin +// state is accepted exactly once with no has_seen_counter flag anywhere. +// --------------------------------------------------------------------------- + +static void test_fresh_session(void) { + NonceState s; + state_reset(&s, 0); + + CHECK_RES(check(&s, 0), NONCE_OK); + commit(&s, 0); + CHECK_RES(check(&s, 0), NONCE_REPLAY); + + // last_seen has not moved: committing at fwd == 0 only sets bit 0. + CHECK(s.last_seen == 0); + CHECK(od_nonce_bit_test(s.bm, 0)); + + // Counter 1 is one ahead and cannot have been seen. + CHECK_RES(check(&s, 1), NONCE_OK); + + // Counter UINT64_MAX is one *behind* 0 under modular arithmetic (back == 1), + // inside the backward window, and its bit is clear. + CHECK_RES(check(&s, UINT64_MAX), NONCE_OK); + + // ...and 256 behind is off the end of the window. + CHECK_RES(check(&s, 0u - (uint64_t)OD_NONCE_BACKWARD_BITS), NONCE_OUT_OF_WINDOW); + CHECK_RES(check(&s, 0u - (uint64_t)(OD_NONCE_BACKWARD_BITS - 1)), NONCE_OK); +} + +// --------------------------------------------------------------------------- +// Decision D coverage: D3 — re-presenting a committed counter is REPLAY, +// including at fwd == 0, the case today's firmware exempts. +// --------------------------------------------------------------------------- + +static void test_d3_same_counter_replay(void) { + const uint64_t base = 1000000u; + + // fwd == 0: the exempted case. + { + NonceState s; + state_reset(&s, base); + CHECK_RES(check(&s, base), NONCE_OK); // virgin bit 0 + commit(&s, base); + CHECK_RES(check(&s, base), NONCE_REPLAY); + CHECK_RES(check(&s, base), NONCE_REPLAY); // and it stays REPLAY + } + + // Forward acceptance then immediate re-presentation: the accepted counter + // becomes the new last_seen, so the replay lands on fwd == 0 again. + { + NonceState s; + state_reset(&s, base); + commit(&s, base); + CHECK_RES(check_and_commit(&s, base + 5), NONCE_OK); + CHECK(s.last_seen == base + 5); + CHECK_RES(check(&s, base + 5), NONCE_REPLAY); + // The old last_seen is now 5 behind and still recorded. + CHECK_RES(check(&s, base), NONCE_REPLAY); + // A gap counter between them was never committed. + CHECK_RES(check(&s, base + 3), NONCE_OK); + } + + // Backward, in-window acceptance then re-presentation. + { + NonceState s; + state_reset(&s, base); + commit(&s, base); + CHECK_RES(check_and_commit(&s, base - 100), NONCE_OK); + CHECK(s.last_seen == base); // backward commits never move last_seen + CHECK_RES(check(&s, base - 100), NONCE_REPLAY); + CHECK_RES(check(&s, base - 99), NONCE_OK); + CHECK_RES(check(&s, base - 101), NONCE_OK); + } +} + +// --------------------------------------------------------------------------- +// Decision D coverage: purity of od_nonce_check (D2) +// +// THE most important assertion in this file. "The tag is the only thing that may +// advance replay state" rests entirely on od_nonce_check writing nothing, so a +// pre-authentication call on attacker-controlled input cannot poison the window. +// Every result class is exercised against one snapshot and the whole state is +// compared byte-for-byte afterwards. +// --------------------------------------------------------------------------- + +static void test_check_is_pure(void) { + const uint64_t base = 500000u; + + NonceState s; + state_reset(&s, base); + // Build a non-trivial bitmap: bits at word boundaries and in between. + commit(&s, base); + const uint64_t seen_offsets[] = {1, 2, 63, 64, 65, 127, 128, 191, 192, 254, 255}; + for (size_t i = 0; i < sizeof(seen_offsets) / sizeof(seen_offsets[0]); ++i) { + commit(&s, base - seen_offsets[i]); + } + + unsigned char snapshot[sizeof(NonceState)]; + std::memcpy(snapshot, &s, sizeof(snapshot)); + + // One input per result class produced by od_nonce_check. + struct Case { + uint64_t counter; + NonceResult expected; + const char* what; + }; + const Case cases[] = { + {base, NONCE_REPLAY, "fwd == 0, bit 0 set -> REPLAY"}, + {base + 1, NONCE_OK, "forward by 1 -> OK"}, + {base + OD_NONCE_FORWARD_CAP, NONCE_OK, "forward at the cap -> OK"}, + {base - 3, NONCE_OK, "backward, unseen -> OK"}, + {base - 64, NONCE_REPLAY, "backward, seen -> REPLAY"}, + {base + OD_NONCE_FORWARD_CAP + 1, NONCE_OUT_OF_WINDOW, "past the forward cap"}, + {base - OD_NONCE_BACKWARD_BITS, NONCE_OUT_OF_WINDOW, "past the backward window"}, + {base + (1ull << 62), NONCE_OUT_OF_WINDOW, "far ahead"}, + {base - (1ull << 62), NONCE_OUT_OF_WINDOW, "far behind"}, + }; + for (size_t i = 0; i < sizeof(cases) / sizeof(cases[0]); ++i) { + set_context("purity case: %s", cases[i].what); + CHECK_RES(check(&s, cases[i].counter), cases[i].expected); + } + clear_context(); + + CHECK(std::memcmp(snapshot, &s, sizeof(snapshot)) == 0); + + // The fwd == 0 OK class needs a state where bit 0 is clear; check purity there too. + NonceState fresh; + state_reset(&fresh, base); + unsigned char fresh_snapshot[sizeof(NonceState)]; + std::memcpy(fresh_snapshot, &fresh, sizeof(fresh_snapshot)); + CHECK_RES(check(&fresh, base), NONCE_OK); + CHECK(std::memcmp(fresh_snapshot, &fresh, sizeof(fresh_snapshot)) == 0); + + // Repeated calls on every input class, in bulk, still change nothing. + for (int rep = 0; rep < 4; ++rep) { + for (size_t i = 0; i < sizeof(cases) / sizeof(cases[0]); ++i) { + (void)check(&s, cases[i].counter); + } + } + CHECK(std::memcmp(snapshot, &s, sizeof(snapshot)) == 0); +} + +// --------------------------------------------------------------------------- +// Decision D coverage: shift edges +// +// For each forward delta d, seed a window with a spread of previously consumed +// counters, slide forward by d, and then require: +// - every seeded counter whose new distance (d + i) is < OD_NONCE_BACKWARD_BITS +// is still reported REPLAY (the bit moved with the window, it did not fall +// off early and it did not land on the wrong index), and +// - every seeded counter whose new distance is >= OD_NONCE_BACKWARD_BITS is +// reported OUT_OF_WINDOW, never REPLAY. +// +// d = 0, 1, 63, 64, 65, 127, 128 are reachable through od_nonce_check (capped by +// OD_NONCE_FORWARD_CAP). d = 129, 191, 192, 255, 256, 257 are driven directly +// against od_nonce_commit to cover the word boundaries and the wholesale-clear +// guard, which no reachable input can hit while the cap stays below the width. +// --------------------------------------------------------------------------- + +static const uint64_t kSeedOffsets[] = {0, 1, 2, 63, 64, 65, 127, 128, 129, 191, 192, 254, 255}; +static const size_t kSeedCount = sizeof(kSeedOffsets) / sizeof(kSeedOffsets[0]); + +static void seed_window(NonceState* s, uint64_t base) { + state_reset(s, base); + for (size_t i = 0; i < kSeedCount; ++i) { + commit(s, base - kSeedOffsets[i]); + } + // Sanity: everything seeded reads back as consumed, before any slide. + for (size_t i = 0; i < kSeedCount; ++i) { + CHECK_RES(check(s, base - kSeedOffsets[i]), NONCE_REPLAY); + } + CHECK(s->last_seen == base); +} + +static void run_shift_edge(uint64_t d, bool reachable_through_check) { + const uint64_t base = 1000000u; + NonceState s; + seed_window(&s, base); + + set_context("shift edge d=%" PRIu64, d); + + if (reachable_through_check) { + // d == 0 lands on the fwd == 0 branch, whose bit is already set by the seed. + CHECK_RES(check(&s, base + d), d == 0 ? NONCE_REPLAY : NONCE_OK); + commit(&s, base + d); + } else { + // Beyond the forward cap: od_nonce_check rejects it, so drive commit directly. + CHECK_RES(check(&s, base + d), NONCE_OUT_OF_WINDOW); + commit(&s, base + d); + } + + CHECK(s.last_seen == base + d); + // The newly committed counter is always recorded. + CHECK_RES(check(&s, base + d), NONCE_REPLAY); + + for (size_t i = 0; i < kSeedCount; ++i) { + const uint64_t counter = base - kSeedOffsets[i]; + const uint64_t new_back = d + kSeedOffsets[i]; + const bool wholesale_clear = (d >= OD_NONCE_BACKWARD_BITS); + set_context("shift edge d=%" PRIu64 ", seeded offset %" PRIu64 + " (new back %" PRIu64 ")", + d, kSeedOffsets[i], new_back); + if (new_back < OD_NONCE_BACKWARD_BITS && !wholesale_clear) { + CHECK_RES(check(&s, counter), NONCE_REPLAY); + } else { + CHECK_RES(check(&s, counter), NONCE_OUT_OF_WINDOW); + } + } + + // Counters that were never seeded and are still inside the backward window + // must read as unseen, not as bits smeared by the shift. + for (uint64_t back = 1; back < OD_NONCE_BACKWARD_BITS; ++back) { + const uint64_t counter = (base + d) - back; + // Is this counter one of the seeded ones (or the pre-slide last_seen)? + bool seeded = false; + for (size_t i = 0; i < kSeedCount; ++i) { + if (counter == base - kSeedOffsets[i]) seeded = true; + } + const bool expect_replay = seeded && (d < OD_NONCE_BACKWARD_BITS); + set_context("shift edge d=%" PRIu64 ", back=%" PRIu64, d, back); + CHECK_RES(check(&s, counter), expect_replay ? NONCE_REPLAY : NONCE_OK); + } + + // Exactly at the window edge and beyond it: rejected on width. + set_context("shift edge d=%" PRIu64 ", window edge", d); + CHECK_RES(check(&s, (base + d) - OD_NONCE_BACKWARD_BITS), NONCE_OUT_OF_WINDOW); + CHECK_RES(check(&s, (base + d) - (OD_NONCE_BACKWARD_BITS + 1)), NONCE_OUT_OF_WINDOW); + + clear_context(); +} + +static void test_shift_edges(void) { + const uint64_t reachable[] = {0, 1, 63, 64, 65, 127, 128}; + for (size_t i = 0; i < sizeof(reachable) / sizeof(reachable[0]); ++i) { + run_shift_edge(reachable[i], true); + } + const uint64_t direct[] = {129, 191, 192, 255, 256, 257}; + for (size_t i = 0; i < sizeof(direct) / sizeof(direct[0]); ++i) { + run_shift_edge(direct[i], false); + } +} + +// --------------------------------------------------------------------------- +// Decision D coverage: wholesale slide +// +// A forward jump of at least OD_NONCE_BACKWARD_BITS clears the bitmap outright +// (x << 64 is UB, and shifting a 256-bit map by >= 256 has no surviving bits). +// Everything previously consumed must then come back OUT_OF_WINDOW, NOT REPLAY. +// Both reject, so conflating them is invisible in behaviour — and would hide a +// genuine slide bug where the map was cleared when it should not have been. +// --------------------------------------------------------------------------- + +static void test_wholesale_slide(void) { + const uint64_t base = 1000000u; + const uint64_t jumps[] = {OD_NONCE_BACKWARD_BITS, OD_NONCE_BACKWARD_BITS + 1, 300, 100000, + (1ull << 40)}; + + for (size_t j = 0; j < sizeof(jumps) / sizeof(jumps[0]); ++j) { + const uint64_t d = jumps[j]; + NonceState s; + seed_window(&s, base); + set_context("wholesale slide d=%" PRIu64, d); + + commit(&s, base + d); + CHECK(s.last_seen == base + d); + + // Bitmap holds exactly one bit: the counter just committed. + CHECK(s.bm[0] == 1ull); + for (size_t w = 1; w < OD_NONCE_BITMAP_WORDS; ++w) { + CHECK(s.bm[w] == 0ull); + } + + for (size_t i = 0; i < kSeedCount; ++i) { + CHECK_RES(check(&s, base - kSeedOffsets[i]), NONCE_OUT_OF_WINDOW); + } + // The whole new backward window is unseen except bit 0. + CHECK_RES(check(&s, base + d), NONCE_REPLAY); + for (uint64_t back = 1; back < OD_NONCE_BACKWARD_BITS; ++back) { + CHECK_RES(check(&s, (base + d) - back), NONCE_OK); + } + } + clear_context(); +} + +// --------------------------------------------------------------------------- +// Decision D coverage: bit index correctness after a forward commit +// +// Direct statement of the shift invariant, below the od_nonce_check layer: the +// bit that denoted counter (L - i) under last_seen = L must denote exactly the +// same counter under L' = L + d, i.e. it must sit at index i + d. Bits pushed to +// index >= OD_NONCE_BACKWARD_BITS are gone. +// --------------------------------------------------------------------------- + +static void test_bit_indices_after_shift(void) { + const uint64_t base = 1000000u; + const uint64_t deltas[] = {1, 63, 64, 65, 127}; + + for (size_t k = 0; k < sizeof(deltas) / sizeof(deltas[0]); ++k) { + const uint64_t d = deltas[k]; + NonceState s; + seed_window(&s, base); + + bool before[OD_NONCE_BACKWARD_BITS]; + for (uint64_t i = 0; i < OD_NONCE_BACKWARD_BITS; ++i) { + before[i] = od_nonce_bit_test(s.bm, i); + } + + commit(&s, base + d); + + for (uint64_t i = 0; i < OD_NONCE_BACKWARD_BITS; ++i) { + set_context("bit index d=%" PRIu64 ", old index %" PRIu64, d, i); + if (i + d < OD_NONCE_BACKWARD_BITS) { + // Same counter, new index. + CHECK(od_nonce_bit_test(s.bm, i + d) == before[i]); + } + // The vacated low bits are all clear except bit 0, which the commit set. + if (i < d) { + CHECK(od_nonce_bit_test(s.bm, i) == (i == 0)); + } + } + clear_context(); + } +} + +// --------------------------------------------------------------------------- +// Decision D coverage: counter arithmetic [M1] +// +// Expectations here are derived from the modular definition in the header, not +// from intuition about "before" and "after": +// fwd = counter - last_seen (mod 2^64) +// back = last_seen - counter (mod 2^64) +// and the ordered tests fwd == 0, fwd <= CAP, back < BITS, else OUT_OF_WINDOW. +// UBSan makes the whole file self-checking against the signed formulation, in +// which these same inputs are undefined (int64_t conversion of values >= 2^63, +// signed overflow, negating INT64_MIN). +// --------------------------------------------------------------------------- + +static void test_counter_arithmetic_extremes(void) { + const uint64_t two63 = 1ull << 63; + + // counter = 2^63, last_seen = 1. The input that makes the signed form UB. + // fwd = 2^63 - 1 -> > CAP + // back = 1 - 2^63 = 2^63 + 1 -> >= BITS + // so it is out of the window in both directions, which is the only sane + // answer for a counter half the space away. + { + NonceState s; + state_reset(&s, 1); + CHECK(two63 - 1u > (uint64_t)OD_NONCE_FORWARD_CAP); + CHECK(1u - two63 == two63 + 1u); + CHECK_RES(check(&s, two63), NONCE_OUT_OF_WINDOW); + } + // The mirror image: last_seen = 2^63, counter = 1. + { + NonceState s; + state_reset(&s, two63); + CHECK_RES(check(&s, 1), NONCE_OUT_OF_WINDOW); + } + // And 2^63 apart in the other direction, from a high last_seen. + { + NonceState s; + state_reset(&s, UINT64_MAX); + CHECK_RES(check(&s, UINT64_MAX + two63), NONCE_OUT_OF_WINDOW); + CHECK_RES(check(&s, UINT64_MAX - two63), NONCE_OUT_OF_WINDOW); + } + + // Near UINT64_MAX, including the wrap past it. last_seen = UINT64_MAX - 2. + { + const uint64_t L = UINT64_MAX - 2u; + NonceState s; + state_reset(&s, L); + + CHECK_RES(check(&s, L), NONCE_OK); // fwd == 0, bit 0 clear + commit(&s, L); + CHECK_RES(check(&s, L), NONCE_REPLAY); + + // Forward, no wrap yet. + CHECK_RES(check(&s, UINT64_MAX - 1u), NONCE_OK); // fwd == 1 + CHECK_RES(check(&s, UINT64_MAX), NONCE_OK); // fwd == 2 + // Forward, wrapping past UINT64_MAX. fwd = 0 - (2^64 - 3) = 3. + CHECK(0u - L == 3u); + CHECK_RES(check(&s, 0), NONCE_OK); + CHECK(1u - L == 4u); + CHECK_RES(check(&s, 1), NONCE_OK); + // Still forward at the cap, wrapped. + CHECK_RES(check(&s, L + (uint64_t)OD_NONCE_FORWARD_CAP), NONCE_OK); + CHECK_RES(check(&s, L + (uint64_t)OD_NONCE_FORWARD_CAP + 1u), NONCE_OUT_OF_WINDOW); + // Backward across the low end of the space: L - 1 == UINT64_MAX - 3. + CHECK_RES(check(&s, L - 1u), NONCE_OK); + CHECK_RES(check(&s, L - (uint64_t)(OD_NONCE_BACKWARD_BITS - 1)), NONCE_OK); + CHECK_RES(check(&s, L - (uint64_t)OD_NONCE_BACKWARD_BITS), NONCE_OUT_OF_WINDOW); + + // Accept a wrapped counter and slide the window across the wrap point. + CHECK_RES(check_and_commit(&s, 1), NONCE_OK); + CHECK(s.last_seen == 1u); + + // The pre-wrap counter L is now back = 1 - L = 4 behind, and is recorded. + CHECK(1u - L == 4u); + CHECK_RES(check(&s, L), NONCE_REPLAY); + // Its never-committed neighbours across the wrap are unseen. + CHECK_RES(check(&s, UINT64_MAX - 1u), NONCE_OK); // back == 3 + CHECK_RES(check(&s, UINT64_MAX), NONCE_OK); // back == 2 + CHECK_RES(check(&s, 0), NONCE_OK); // back == 1 + CHECK_RES(check(&s, 1), NONCE_REPLAY); // fwd == 0, committed + + // The backward window reaches BITS-1 below zero and stops there. + CHECK_RES(check(&s, 1u - (uint64_t)(OD_NONCE_BACKWARD_BITS - 1)), NONCE_OK); + CHECK_RES(check(&s, 1u - (uint64_t)OD_NONCE_BACKWARD_BITS), NONCE_OUT_OF_WINDOW); + + // Commit a backward, wrapped counter and read it back. + CHECK_RES(check_and_commit(&s, UINT64_MAX), NONCE_OK); + CHECK(s.last_seen == 1u); // backward commits do not move last_seen + CHECK_RES(check(&s, UINT64_MAX), NONCE_REPLAY); + CHECK_RES(check(&s, UINT64_MAX - 1u), NONCE_OK); + } +} + +// --------------------------------------------------------------------------- +// Decision D coverage: differential / property test against a naive oracle +// +// The oracle is the obvious-but-unshippable implementation: the exact set of +// counters consumed, plus last_seen. It reproduces od_nonce_check's decision +// directly from the definition, and prunes counters that have fallen out of the +// backward window when last_seen advances — that pruning is what makes it agree +// on REPLAY versus OUT_OF_WINDOW rather than only on accept versus reject. +// +// Counters stay far from 0 and from 2^64 so the oracle needs no wrap handling; +// the wrap cases are covered exhaustively by test_counter_arithmetic_extremes(). +// --------------------------------------------------------------------------- + +struct Oracle { + std::set seen; + uint64_t last_seen; +}; + +static NonceResult oracle_check(const Oracle& o, uint64_t counter) { + const uint64_t fwd = counter - o.last_seen; + const uint64_t back = o.last_seen - counter; + if (fwd == 0u) return o.seen.count(counter) ? NONCE_REPLAY : NONCE_OK; + if (fwd <= (uint64_t)OD_NONCE_FORWARD_CAP) return NONCE_OK; + if (back < (uint64_t)OD_NONCE_BACKWARD_BITS) { + return o.seen.count(counter) ? NONCE_REPLAY : NONCE_OK; + } + return NONCE_OUT_OF_WINDOW; +} + +static void oracle_commit(Oracle* o, uint64_t counter) { + const uint64_t fwd = counter - o->last_seen; + const uint64_t back = o->last_seen - counter; + if (fwd == 0u) { + o->seen.insert(counter); + return; + } + if (back < (uint64_t)OD_NONCE_BACKWARD_BITS) { + o->seen.insert(counter); + return; + } + // Forward: the window slides, and everything now at or past the far edge is + // forgotten — exactly the bits the shift pushes off the end of the bitmap. + o->last_seen = counter; + o->seen.insert(counter); + while (!o->seen.empty() && + (o->last_seen - *o->seen.begin()) >= (uint64_t)OD_NONCE_BACKWARD_BITS) { + o->seen.erase(o->seen.begin()); + } +} + +static void test_differential_against_oracle(void) { + std::mt19937_64 rng(0xD15EA5EULL); // fixed seed: this test must be reproducible + + const int kSequences = 40; + const int kStepsPerSequence = 200; + + for (int seq = 0; seq < kSequences; ++seq) { + const uint64_t base = (1ull << 40) + (uint64_t)seq * 4096u; + + NonceState s; + state_reset(&s, base); + Oracle o; + o.last_seen = base; + + for (int step = 0; step < kStepsPerSequence; ++step) { + const unsigned bucket = (unsigned)(rng() % 100u); + uint64_t counter; + if (bucket < 40u) { + // Forward inside the cap, including fwd == 0 (a replay probe). + counter = s.last_seen + (rng() % (uint64_t)(OD_NONCE_FORWARD_CAP + 1)); + } else if (bucket < 60u) { + // Forward past the cap: a gap too large to accept. + counter = s.last_seen + (uint64_t)OD_NONCE_FORWARD_CAP + 1u + + (rng() % 1000u); + } else if (bucket < 90u) { + // Backward inside or just outside the window. + counter = s.last_seen - (1u + rng() % (uint64_t)(OD_NONCE_BACKWARD_BITS + 64)); + } else { + // Far behind. + counter = s.last_seen - (uint64_t)OD_NONCE_BACKWARD_BITS - + (rng() % 100000u); + } + + set_context("differential seq=%d step=%d counter=%" PRIu64 + " last_seen=%" PRIu64, + seq, step, counter, s.last_seen); + + const NonceResult got = check(&s, counter); + const NonceResult want = oracle_check(o, counter); + CHECK_RES(got, want); + CHECK(o.last_seen == s.last_seen); + + if (got == NONCE_OK) { + commit(&s, counter); + oracle_commit(&o, counter); + CHECK(o.last_seen == s.last_seen); + } + } + + // End of sequence: sweep the entire backward window and compare, so that + // any bit the two models disagree about is caught even if the random + // walk never probed it. + for (uint64_t back = 0; back < (uint64_t)OD_NONCE_BACKWARD_BITS + 8u; ++back) { + const uint64_t counter = s.last_seen - back; + set_context("differential sweep seq=%d back=%" PRIu64, seq, back); + CHECK_RES(check(&s, counter), oracle_check(o, counter)); + } + } + clear_context(); +} + +// --------------------------------------------------------------------------- + +int main(void) { + test_fresh_session(); + test_d3_same_counter_replay(); + test_check_is_pure(); + test_shift_edges(); + test_wholesale_slide(); + test_bit_indices_after_shift(); + test_counter_arithmetic_extremes(); + test_differential_against_oracle(); + + if (g_failures != 0u) { + std::printf("FAILED %lu of %lu checks\n", g_failures, g_checks); + return EXIT_FAILURE; + } + std::printf("PASSED %lu checks\n", g_checks); + return EXIT_SUCCESS; +}