Skip to content

Commit d7308b8

Browse files
committed
docs(nonce): record the Decision A reversal; restore the surviving citations
d65696f brought the Phase 1 planning doc onto this branch. It had been absent from every branch, so aef3a6b stripped the citations pointing at it as dangling. Now that it exists they belong back -- but not all of them, because aef3a6b reversed one of the decisions it records. Doc: Decision A ("forward cap 128") is struck through and marked REVERSED, with a pointer to a new section at the end that carries the argument -- what the cap did (nothing commits, so last_seen never advances, and each retransmission's fresh higher counter is rejected further out than the last), why no number could have been safe (the ceiling is the client's retransmit budget scaled by a user-facing HA option, and it accumulates across aborted attempts), why removing it costs nothing (the counter is inside the CCM nonce; commit is post-tag; the session id is cleartext so the DoS argument never held), and what replaced it. A table records the effect on every other item: B, C, D and E stand; Step 6's "write the mechanism, not a number" is now literal; Step 5's hardware tests 1/2/2c are obsolete as written, since they existed to validate the 128. Worth noting the doc had already flagged OD_NONCE_FORWARD_CAP as "unvalidated against a real link" in its own unverified-on-hardware list. The flaw was found by analysis rather than by the bench it was waiting on, so that list is unchanged by this reversal. Code: restored the citations that still hold -- Decision D at nonce_window.h and the CI job and the host test, Decision B for the shifting-bitmap representation (unchanged; only the arithmetic over it moved), Step 1 at encryption.cpp and encryption_state.h. The two sites that used to cite Decision A now cite the reversal instead, so a reader lands on the argument that supersedes it rather than on the superseded one. Comments only; no behaviour change. 47445 host checks pass; nrf52840custom builds.
1 parent eeaef4b commit d7308b8

8 files changed

Lines changed: 812 additions & 7 deletions

‎.github/workflows/main.yaml‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,8 @@ jobs:
2121
- name: Build and run the nonce-window host test
2222
# -fsanitize=undefined is not decoration: it is what catches the `x << 64`
2323
# UB in the bitmap shift automatically rather than relying on the test
24-
# author to predict it.
24+
# author to predict it. See Decision D in
25+
# docs/PLAN_PHASE1_NONCE_REPLAY_2026-07-26.md.
2526
run: |
2627
g++ -std=c++17 -Wall -Wextra -Werror -O1 -fsanitize=undefined,address \
2728
tools/test_nonce_window.cpp -o /tmp/test_nonce_window

‎docs/PLAN_BLE_IDLE_SESSION_DISCONNECT_2026-07-29.md‎

Lines changed: 686 additions & 0 deletions
Large diffs are not rendered by default.

‎docs/PLAN_PHASE1_NONCE_REPLAY_2026-07-26.md‎

Lines changed: 107 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -379,7 +379,14 @@ loss, not as an unrecoverable condition.
379379

380380
**All five are settled.** Nothing blocks implementation.
381381

382-
### Decision A — RESOLVED: forward cap **128**
382+
### Decision A — ~~RESOLVED: forward cap **128**~~ — **REVERSED 2026-07-31**
383+
384+
> ⛔ **This decision no longer holds. There is no forward cap.** The constant was removed in
385+
> `aef3a6b` because the cap made a session permanently unrecoverable once a gap crossed it. See
386+
> [Reversal of Decision A](#reversal-of-decision-a--the-forward-cap-was-removed-2026-07-31) at the
387+
> end of this file. The analysis below is retained because its framing of *what the gap is* is
388+
> still correct and is what ultimately showed the cap could not be sized safely — but do not
389+
> implement from it.
383390
384391
> **Revised after adversarial review** (`C1`, `M2` in
385392
> [`FINDINGS_PHASE1_PLAN_REVIEW_2026-07-26.md`](FINDINGS_PHASE1_PLAN_REVIEW_2026-07-26.md)).
@@ -975,3 +982,102 @@ Everything below is still open:
975982
does HA recover cleanly from that disconnect (as opposed to from the `0xFE` it would otherwise
976983
have seen)? Per the pipe-window arithmetic above this **will** happen with default settings.
977984
- Does `55a2478`'s `AUTH_REQUIRED` actually drive HA's reauth flow end to end?
985+
986+
---
987+
988+
# Reversal of Decision A — the forward cap was removed (2026-07-31)
989+
990+
**Commit `aef3a6b`, on `fix/nonce-replay-window` (rebased onto the squashed `#132`/`#133`/`#134`
991+
`main`).** Decision A is reversed. `OD_NONCE_FORWARD_CAP` no longer exists, and comparison moved
992+
from modular to numeric ordering. Decisions B, C and D stand.
993+
994+
## What the cap did
995+
996+
The cap did not merely fail to help — it converted a transient link fault into a permanent session
997+
fault. Once a gap exceeded 128:
998+
999+
1. `od_nonce_check()` returns `NONCE_OUT_OF_WINDOW`, so nothing commits.
1000+
2. `last_seen_counter` therefore never advances — that is the D2 fix working as designed.
1001+
3. The client re-encrypts every retransmission with a **fresh, higher** counter and never resends
1002+
the original ciphertext (`_write_pipe_frame`, `device.py:2683`), so the next frame is rejected at
1003+
a *greater* distance than the last.
1004+
4. Every subsequent frame is rejected, forever. Only re-authentication recovers, and the client
1005+
deliberately does not re-authenticate mid-transfer (`device.py:778`). The transfer stalls until
1006+
the 15-minute stuck-transfer watchdog releases the panel.
1007+
1008+
Step 4b's silent drop does not rescue this. Dropping silently avoids the immediate fatal teardown a
1009+
`0x81` NACK would cause, but the transfer is dead either way.
1010+
1011+
## Why the cap could not be sized
1012+
1013+
Decision A's own framing was right and is what condemns it: the ceiling is the client's retransmit
1014+
budget `max_retx = max(3*W, n/2)`, scaled by `blocks_per_ack`, a user-facing Home Assistant option
1015+
in another repo. That is order thousands for a full-panel upload — and it **accumulates across
1016+
aborted attempts**, because the client's counter keeps climbing while `last_seen` is frozen. With
1017+
`W = 32` and `blocks_per_ack = 1`, 16 queued gap-ACKs at `PIPE_RETX_ACK_SPACING = 2` burn 128
1018+
counters on repairs alone. No firmware-side number is defensible.
1019+
1020+
## Why removing it costs nothing
1021+
1022+
The cap was vestigial once D2 landed. All 8 counter bytes sit inside the CCM nonce, so a tampered
1023+
counter changes the keystream and fails the tag; `nonceCommit()` runs only on the success arm; and
1024+
passing the check mutates nothing. An attacker who cannot forge a tag could not advance `last_seen`
1025+
at any distance, cap or no cap. Nor is it DoS protection: the session id is cleartext in every
1026+
frame, so anyone able to flood CCM with a capped window could flood it without one.
1027+
1028+
## What replaced it
1029+
1030+
Numeric ordering, matching RFC 4303 Appendix A2, which likewise has no forward bound:
1031+
1032+
```
1033+
counter == last_seen -> bit 0 set ? REPLAY : OK
1034+
counter > last_seen -> OK (any distance; the tag is the gate)
1035+
counter < last_seen, back<256 -> bit[back] set ? REPLAY : OK
1036+
counter < last_seen, back>=256 -> OUT_OF_WINDOW
1037+
```
1038+
1039+
Consequences worth recording:
1040+
1041+
- **Modular arithmetic is gone.** It made a counter far behind indistinguishable from one far
1042+
ahead, which is what allowed an ancient counter to present as an enormous forward jump — the
1043+
"sharp edge" the old header admitted, where committing one would rewind `last_seen` and clear the
1044+
bitmap. That is now impossible by construction rather than by the caller's contract. **Do not**
1045+
reintroduce a bound as `cap = UINT64_MAX` or as "not-backward implies forward": either restores
1046+
the overlap. `test_far_behind_is_never_forward()` pins both.
1047+
- **Counters no longer wrap**, per RFC 4303 §3.3.3. Reaching `UINT64_MAX` requires
1048+
re-authentication; wrapping would reuse a `(key, nonce)` pair. Unreachable in practice.
1049+
- **`NONCE_OUT_OF_WINDOW` now means only "too far behind."** The rejection log computes direction
1050+
from the counters instead of inferring it from the reason, which would print an underflowed
1051+
20-digit distance.
1052+
- **`OD_NONCE_BACKWARD_BITS` keeps its value but loses its old justification**, which was stated in
1053+
terms of the cap ("kept strictly greater than `OD_NONCE_FORWARD_CAP`"). It is now purely
1054+
out-of-order tolerance, and its exact value is not load-bearing: a backward rejection is
1055+
self-healing, because the retransmit carries a higher counter that is accepted unconditionally.
1056+
1057+
## Effect on the rest of this document
1058+
1059+
| Item | Status |
1060+
|---|---|
1061+
| **Decision A** | **Reversed.** No forward cap. |
1062+
| **Decision B** — shifting bitmap, `uint64_t[4]`, IPsec/DTLS style | **Stands.** The representation is unchanged; only the arithmetic over it moved from modular to numeric. |
1063+
| **Decision C** — file-static wrappers | **Stands**, including its as-built correction. |
1064+
| **Decision D** — standalone host test + separate CI job | **Stands.** |
1065+
| **Decision E** — no wire change | **Stands.** This reversal changes no byte on the wire: the accept set only grows, so no peer needs updating in lockstep. |
1066+
| **Step 6** — "write the mechanism, not a number" | **Stands, and is now literal**: there is no number to write. The `communication.cpp` comment records why there cannot be one. |
1067+
| **Step 5 hardware tests 1, 2, 2c** | **Obsolete as written.** They existed to validate the 128 figure. What replaces them is confirming a transfer survives a gap that *would* have crossed it. |
1068+
| **"Unverified on hardware"** | Still accurate, and this reversal did not change it: the cap was condemned by analysis, not by the bench. |
1069+
1070+
## Verification
1071+
1072+
Host suite: **47445 checks** pass under `-Werror` with ASan+UBSan. The same suite run against the
1073+
pre-change implementation fails **1635** checks, which is the evidence that the new tests
1074+
discriminate rather than merely pass. Added `test_forward_gap_is_not_a_cliff()` (the defect as a
1075+
*sequence*, since a point test at `last_seen+129` would also pass under `cap = UINT64_MAX`),
1076+
`test_far_behind_is_never_forward()`, and `od_nonce_never_accepts_consumed()` — a sweep over every
1077+
counter ever committed, which is the one assertion in the file that does not restate the code. The
1078+
oracle no longer prunes its seen set; that pruning encoded the implementation's forgetting and so
1079+
could only ever agree with it.
1080+
1081+
Builds: `nrf52840custom`, `esp32-c3-N16`, `esp32-N4`.
1082+
1083+
Still unverified on hardware, unchanged from the list above.

‎src/communication.cpp‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -900,6 +900,10 @@ void imageDataWritten(BLEConnHandle conn_hdl, BLECharPtr chr, uint8_t* data, uin
900900
// advance, and each retransmission's still-higher counter would be
901901
// rejected further out than the last, until re-authentication. That is
902902
// a transient link fault promoted to a permanent session fault.
903+
//
904+
// This reverses Decision A of
905+
// docs/PLAN_PHASE1_NONCE_REPLAY_2026-07-26.md, which specified a cap of
906+
// 128; see "Reversal of Decision A" at the end of that file.
903907
handlePipeWriteData(data + 2, len - 2);
904908
break;
905909
case CMD_PIPE_WRITE_END: // 0x0082

‎src/encryption.cpp‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -119,7 +119,7 @@ void deriveSessionId(const uint8_t* session_key, const uint8_t* client_nonce,
119119
// populated). In particular nonce_counter — the device's OWN outbound counter —
120120
// must keep being zeroed here: a device that carried it across a re-auth while
121121
// the client restarts at 0 would reproduce the [H2] keystream reuse against
122-
// itself.
122+
// itself. See docs/PLAN_PHASE1_NONCE_REPLAY_2026-07-26.md Step 1.
123123
static void resetNonceState(void) {
124124
encryptionSession.nonce_counter = 0; /* device's OWN outbound counter */
125125
encryptionSession.last_seen_counter = 0;

‎src/encryption_state.h‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,8 @@ struct EncryptionSession {
2525
// in step, and no insertion index to reset. Replaces a uint64_t[64] value
2626
// ring (512 B -> 32 B). last_seen_counter only ever moves UP; see
2727
// src/nonce_window.h for the decision rule and why that is the whole
28-
// anti-replay argument.
28+
// anti-replay argument, and docs/PLAN_PHASE1_NONCE_REPLAY_2026-07-26.md
29+
// Step 1 / Decision B.
2930
uint64_t replay_bitmap[OD_NONCE_BITMAP_WORDS];
3031
uint32_t last_activity;
3132
uint8_t integrity_failures;

‎src/nonce_window.h‎

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -7,10 +7,12 @@
77
// firmware logging layer, and it touches no global state. Everything here
88
// operates on plain values passed in by the caller, so the whole state machine
99
// can be compiled and exercised on a host under UBSan/ASan by
10-
// tools/test_nonce_window.cpp.
10+
// tools/test_nonce_window.cpp. See Decision D in
11+
// docs/PLAN_PHASE1_NONCE_REPLAY_2026-07-26.md.
1112
//
1213
// Representation ("shifting" style, as opposed to the circular RFC 6479 /
13-
// WireGuard style):
14+
// WireGuard style -- Decision B in docs/PLAN_PHASE1_NONCE_REPLAY_2026-07-26.md,
15+
// which still stands; only the arithmetic over it moved from modular to numeric):
1416
//
1517
// bit i of the bitmap == "counter (last_seen - i) has been consumed".
1618
// bit 0 is last_seen itself.
@@ -101,6 +103,10 @@ static inline void od_nonce_bit_set(uint64_t* bm, uint64_t bit) {
101103
// every subsequent frame — each carrying a still higher counter — would be
102104
// rejected further out than the last, until re-authentication.
103105
//
106+
// This reverses Decision A of docs/PLAN_PHASE1_NONCE_REPLAY_2026-07-26.md, which
107+
// specified a cap of 128; that file's "Reversal of Decision A" section carries the
108+
// full argument and the arithmetic showing no cap could have been sized safely.
109+
//
104110
// The security invariant is "a consumed counter is never returned NONCE_OK
105111
// again", and it rests on last_seen being monotonically non-decreasing: only the
106112
// forward branch of od_nonce_commit() assigns it, and only upward. Every consumed

‎tools/test_nonce_window.cpp‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,8 @@
77
// /tmp/test_nonce_window
88
//
99
// This file is as much a written-down statement of the intended semantics of the
10-
// anti-replay window as it is a test.
10+
// anti-replay window as it is a test. It discharges Decision D of
11+
// docs/PLAN_PHASE1_NONCE_REPLAY_2026-07-26.md.
1112
//
1213
// The representation under test (see the header):
1314
// bit i of the bitmap == "counter (last_seen - i) has been consumed"

0 commit comments

Comments
 (0)