Skip to content

fix: don't desync rxSeq on a SeqCrypt MAC failure#2

Merged
palmarci merged 1 commit into
OpenMinimed:mainfrom
jlengelbrecht:fix/seqcrypt-macfail-rxseq-desync
Jul 5, 2026
Merged

fix: don't desync rxSeq on a SeqCrypt MAC failure#2
palmarci merged 1 commit into
OpenMinimed:mainfrom
jlengelbrecht:fix/seqcrypt-macfail-rxseq-desync

Conversation

@jlengelbrecht

Copy link
Copy Markdown
Member

Summary

SeqCrypt.decrypt() advances rxSeq by 1 on a MAC failure (rxSeq = macOk ? seq + 2 : rxSeq + 1, added in 07346cc). That desyncs the receiver for this cipher's sequencing scheme.

Each SAKE direction uses a fixed sequence paritySession seeds clientCrypt at 0 (even) and serverCrypt at 1 (odd), and every message advances by 2. The parity bit is not carried on the wire: the trailer holds (seq >>> 1) & 0xFF, and the receiver rebuilds the full sequence from its own counter as seq = rxSeq + 2*delta. So the reconstructed sequence's parity comes entirely from rxSeq.

A +1 on failure flips that parity. The consequences:

  • The next genuine packet reconstructs to an odd (wrong) sequence → wrong IV and wrong CMAC → it fails to authenticate even though it's valid.
  • rxSeq keeps drifting by 1 on each failure until delta exceeds MAX_RX_DELTA (128), at which point valid packets are rejected as forgeries and the session can only recover with a full re-handshake.

This is a regression versus the prior behaviour (which left rxSeq untouched on failure and self-recovered through the existing reorder window), and it diverges from PythonSake, which recovers by brute-forcing the real sequence rather than a fixed nudge.

Fix

Leave rxSeq unchanged on a MAC failure. The reorder-window delta already re-syncs on the next packet, and because rxSeq never advances past a confirmed packet it can't run ahead of the sender, so it can't desync. The failing-message hex in MacFailureException (d78ff25) is kept.

Test

Adds macFailureDoesNotDesyncSubsequentDecrypt: a corrupted packet is rejected with rxSeq left untouched, then the next genuine packet still authenticates and decrypts. It fails against the previous rxSeq + 1 behaviour and passes with this change. :lib:test and spotlessCheck are green.

Verification

Beyond the unit test: driving a live encrypt/decrypt pair through one corrupted packet, the +1 code flipped rxSeq 2 → 3 and the next valid packet failed to decrypt; with this change rxSeq stays 2 and the next valid packet decrypts cleanly.

Context

Found while pulling these commits into a downstream consumer (GlycemicGPT's vendored SAKE driver). We're carrying this exact fix as a temporary local patch and will re-vendor once it lands here so the two stay in sync. Happy to adjust the approach — e.g. porting PythonSake's brute-force recovery instead — if you'd prefer that direction.

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.
@jlengelbrecht

Copy link
Copy Markdown
Member Author

@palmarci flagging this for your review since it touches your 07346cc MAC-failure handling — you're the protocol author, so I'd rather you make the call than merge over you.

Short version: the rxSeq + 1 on failure flips the receiver's sequence parity (each direction is fixed even/odd, and the parity bit isn't on the wire — the receiver rebuilds seq = rxSeq + 2*delta), so the next genuine packet fails to authenticate and rxSeq drifts until it trips MAX_RX_DELTA. I verified it live: one corrupted packet flips rxSeq 2→3 and the next valid packet fails to decrypt; leaving rxSeq untouched keeps it at 2 and it decrypts. There's a regression test in the PR.

I kept your d78ff25 failing-message hex logging as-is. This does the minimal "leave rxSeq alone on failure" fix — but if you'd rather port PythonSake's brute-force sequence recovery instead, I'm happy to take it that direction. No rush on your end: our downstream consumer already carries this as a temporary patch, so nothing's blocked while you look.

Full disclosure: I do have merge access on this repo, so I could merge this myself — but I don't think I should land a change to your protocol code without your review, so I'm leaving it to you. On that note, now that JavaSake is picking up real downstream consumers, it might be worth adding branch protection on main (require a PR review before merge). Totally your call on your repo — I mention it partly so that this kind of "please review before I merge" is enforced by the repo rather than depending on me choosing to hold off. Happy to help set that up if useful.

@jlengelbrecht jlengelbrecht requested a review from palmarci July 3, 2026 05:55
@palmarci palmarci merged commit cd4b05e into OpenMinimed:main Jul 5, 2026
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants