From c5d3654a1c005a438747adfeaefc4e8d2d6b8055 Mon Sep 17 00:00:00 2001 From: jlengelbrecht Date: Fri, 3 Jul 2026 01:48:42 -0400 Subject: [PATCH] fix: don't desync rxSeq on a SeqCrypt MAC failure decrypt() advanced rxSeq by 1 on a MAC failure. Each SAKE direction uses a fixed sequence parity (Session seeds clientCrypt=0 even, serverCrypt=1 odd) and the parity bit is not carried on the wire -- the receiver rebuilds the full sequence from its own rxSeq (seq = rxSeq + 2*delta). A +1 nudge on failure therefore flips rxSeq's parity, so every subsequent packet reconstructs to the wrong sequence: the next genuine packet fails to authenticate, and rxSeq keeps drifting on each failure until delta exceeds MAX_RX_DELTA and valid packets are rejected as forgeries -- wedging the session into a re-handshake. Leave rxSeq unchanged on failure instead. The existing reorder-window delta already re-syncs on the next packet, and rxSeq can never run ahead of the sender, so it cannot desync. The failing-message hex in MacFailureException is unchanged. Adds macFailureDoesNotDesyncSubsequentDecrypt: a corrupted packet is rejected with rxSeq left untouched, and the next genuine packet still decrypts. This test fails against the previous rxSeq + 1 behaviour. --- .../java/org/openminimed/sake/SeqCrypt.java | 15 ++++++++--- .../org/openminimed/sake/SeqCryptTest.java | 27 +++++++++++++++++++ 2 files changed, 38 insertions(+), 4 deletions(-) diff --git a/lib/src/main/java/org/openminimed/sake/SeqCrypt.java b/lib/src/main/java/org/openminimed/sake/SeqCrypt.java index d386f97..f6dd932 100644 --- a/lib/src/main/java/org/openminimed/sake/SeqCrypt.java +++ b/lib/src/main/java/org/openminimed/sake/SeqCrypt.java @@ -117,12 +117,19 @@ public byte[] decrypt(byte[] message) throws MacFailureException { byte[] iv = buildIv(seq); byte[] plaintext = AesCtr.crypt(key, iv, ciphertext); - boolean macOk = (tagPrefix[0] == message[ciphertextLen + 1]) + boolean macOk = + (tagPrefix[0] == message[ciphertextLen + 1]) && (tagPrefix[1] == message[ciphertextLen + 2]); - rxSeq = macOk ? seq + 2 : rxSeq + 1; - - if (!macOk) { + // Advance rxSeq only on success. Each direction uses a fixed sequence parity and the + // parity bit is not carried on the wire (the receiver rebuilds seq from its own rxSeq), + // so leaving rxSeq untouched on a MAC failure preserves that parity and lets the reorder + // window re-sync on the next packet. Nudging it on failure (e.g. rxSeq + 1) flips the + // parity, desyncs every subsequent packet, and eventually exceeds MAX_RX_DELTA -- wedging + // the session. + if (macOk) { + rxSeq = seq + 2; + } else { throw new MacFailureException( "MAC verification failed at seq=" + seq + " message=" + bytesToHex(message)); } diff --git a/lib/src/test/java/org/openminimed/sake/SeqCryptTest.java b/lib/src/test/java/org/openminimed/sake/SeqCryptTest.java index d89bdbc..3b41b2d 100644 --- a/lib/src/test/java/org/openminimed/sake/SeqCryptTest.java +++ b/lib/src/test/java/org/openminimed/sake/SeqCryptTest.java @@ -129,4 +129,31 @@ void encryptRejectsSequenceAtFortyBitBoundary() { SeqCrypt sc = new SeqCrypt(KEY, NONCE, SeqCrypt.MAX_SEQ); assertThrows(IllegalStateException.class, () -> sc.encrypt(PLAIN_2)); } + + /** + * A single MAC failure must not desync the receiver. Because each direction uses a fixed + * sequence parity (the parity bit is not carried on the wire; the receiver rebuilds the full + * sequence from its own {@code rxSeq}), {@code rxSeq} must be left untouched on failure so the + * next genuine packet still authenticates. Advancing it by 1 on failure would flip the parity + * and break every subsequent packet. + */ + @Test + void macFailureDoesNotDesyncSubsequentDecrypt() throws Exception { + SeqCrypt tx = new SeqCrypt(KEY, NONCE, 0L); + SeqCrypt rx = new SeqCrypt(KEY, NONCE, 0L); + + // seq 0: clean round trip advances rxSeq to 2. + assertArrayEquals(PLAIN_0, rx.decrypt(tx.encrypt(PLAIN_0))); + assertEquals(2L, rx.getRxSeq()); + + // seq 2: arrives corrupted -> rejected, and rxSeq must be left unchanged. + byte[] corrupt = tx.encrypt(PLAIN_1); // tx: 2 -> 4 + corrupt[0] ^= (byte) 0x01; + assertThrows(MacFailureException.class, () -> rx.decrypt(corrupt)); + assertEquals(2L, rx.getRxSeq(), "rxSeq must be unchanged after a MAC failure"); + + // seq 4: the next genuine packet must still authenticate and decrypt. + assertArrayEquals(PLAIN_2, rx.decrypt(tx.encrypt(PLAIN_2))); // tx: 4 -> 6 + assertEquals(6L, rx.getRxSeq()); + } }