Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 11 additions & 4 deletions lib/src/main/java/org/openminimed/sake/SeqCrypt.java
Original file line number Diff line number Diff line change
Expand Up @@ -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));
}
Expand Down
27 changes: 27 additions & 0 deletions lib/src/test/java/org/openminimed/sake/SeqCryptTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -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());
}
}
Loading