fix(nonce): sliding-window replay protection; stop NACKing ordinary packet loss

src/communication.cpp

@@ -609,7 +609,36 @@ void imageDataWritten(BLEConnHandle conn_hdl, BLECharPtr chr, uint8_t* data, uin
         // screen. It moves to the failure path below, where it is the only thing that
         // separates a replay-window jump from nonce reuse from a wrong session key,
         // and where nRF (which has no RX hex line at all) would otherwise be blind.
-        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 under a
+            // FRESH, higher counter, and that counter is accepted unconditionally
+            // (nonce_window.h has no forward bound), so 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.
+            //
+            // Rare by construction now: the only surviving nonce rejections are a
+            // duplicate delivery and a counter more than OD_NONCE_BACKWARD_BITS
+            // behind, neither of which this transport produces. It stays because
+            // being wrong here costs the whole upload.
+            //
+            // 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;
+            }
             // 16 bytes render as 47 chars + NUL, an exact fit in 48; sized past that
             // so a future ENCRYPTION_NONCE_SIZE bump truncates nothing.
             char nonceHex[64];
@@ -688,10 +717,31 @@ 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.
+            //
+            // The forward gap is deliberately UNBOUNDED, and it has to be. 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 ceiling is the client's retransmit budget
+            // max_retx = max(3*W, n/2), scaled by blocks_per_ack, a user-facing
+            // Home Assistant option (1..32) in another repo: order thousands for a
+            // full-panel upload, and it accumulates ACROSS aborted attempts
+            // because the client never re-authenticates mid-transfer. Any firmware
+            // constant placed here would be a number this repo cannot prove and a
+            // client-side setting could silently falsify.
+            //
+            // So there is no such constant. A counter ahead of last_seen is
+            // accepted at any distance and gated by the CCM tag instead
+            // (src/nonce_window.h). A forward cap would not bound an attacker —
+            // checking commits nothing — but it would strand the session, because
+            // once a gap exceeded it nothing would commit, last_seen would never
+            // advance, and each retransmission's still-higher counter would be
+            // rejected further out than the last, until re-authentication. That is
+            // a transient link fault promoted to a permanent session fault.
             handlePipeWriteData(data + 2, len - 2);
             break;
         case CMD_PIPE_WRITE_END:      // 0x0082

src/display_service.cpp

@@ -2620,6 +2620,24 @@ static void sendPipeAck(void) {
 static void sendPipeNack(uint8_t err) {
     uint8_t r[8] = {RESP_NACK, 0x81, err, 0, 0, 0, 0, 0};
     pipeBuildAckPayload(r + 3);
+    // The only record that this session died and why. Everything below is
+    // observable from the client or a sniffer but nothing was observable from the
+    // device, so a failed upload could not be told apart from a stall or a link
+    // drop without one. Logged BEFORE the send so the line survives whatever the
+    // response path does, and at ERROR because this is always terminal.
+    //
+    // Cannot flood: pipeState.error is set immediately below and makes every
+    // later 0x0081 frame discard, so at most one of these per session.
+    //
+    // err 0x04 (out of window on both sides) deserves particular suspicion. A
+    // conforming client cannot produce it -- it only transmits seq within W of
+    // its own base, and the device's expected_seq is provably within W of that
+    // base too -- so seeing 0x04 means either a non-conforming peer or that the
+    // client's window rule has drifted from what this firmware assumes.
+    od_log_error("ERROR: PIPE NACK err=0x%02X (expected=%u highest_seen=%u queued=%u W=%u%s) - session fatal",
+                 (unsigned)err, (unsigned)pipeState.expected_seq, (unsigned)r[3],
+                 (unsigned)pipeState.queued_count, (unsigned)pipeState.window,
+                 pipeState.partial ? " partial" : "");
     sendResponse(r, sizeof(r));
     pipeState.error = true;
     // Partial transfers own partialCtx, not the full-frame direct-write session:

src/encryption.cpp

@@ -111,48 +111,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.
+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 +243,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 +685,9 @@ 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;
+        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 +716,42 @@ 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)) {
+            // Direction comes from the COUNTERS, not from `nr`. Under numeric
+            // ordering every rejection is at or behind last_seen: anything ahead
+            // is accepted at any distance, so NONCE_OUT_OF_WINDOW now means only
+            // "more than OD_NONCE_BACKWARD_BITS behind". Inferring the direction
+            // from the reason would pick the wrong subtraction, underflow, and
+            // print a 20-digit number -- the exact unreadability this guards
+            // against. Computed rather than assumed so the line survives a future
+            // rule change.
+            const bool behind = (nonce_counter <= encryptionSession.last_seen_counter);
+            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");