Skip to content

Fix AES SM MAC compare in unprotect() - #273

Open
bahadirtmzr wants to merge 1 commit into
AndyQ:mainfrom
bahadirtmzr:security/verified-findings-only
Open

Fix AES SM MAC compare in unprotect()#273
bahadirtmzr wants to merge 1 commit into
AndyQ:mainfrom
bahadirtmzr:security/verified-findings-only

Conversation

@bahadirtmzr

@bahadirtmzr bahadirtmzr commented Aug 18, 2026

Copy link
Copy Markdown

unprotect() is supposed to compare the first 8 bytes of the computed AES-CMAC with DO'8E.

It currently does this:

var CCb = mac(algoName: algoName, key: self.ksmac, msg: K)
if CCb.count > 8 {
    CCb = [UInt8](CC[0..<8])
}

CC is the MAC from the chip, so that assignment throws away the computed CMAC. AES-CMAC is 16 bytes, so we always take the branch and the compare becomes CC == CC. protect() already truncates CCb. The existing 3DES tests never hit this because DES MACs are already 8 bytes.

After PACE-AES or Chip Authentication AES, a response with one ciphertext byte flipped and the original MAC still unprotects.

testAESUnprotectRejectsModifiedCiphertext covers that case. testAESUnprotectAcceptsValidMAC checks a correctly MACed AES response still works.

The fix is the one assignment: truncate CCb, not CC.

@bahadirtmzr
bahadirtmzr marked this pull request as ready for review August 18, 2026 15:27
@bahadirtmzr
bahadirtmzr force-pushed the security/verified-findings-only branch from ec138fa to 5b8d987 Compare August 18, 2026 16:38
@bahadirtmzr bahadirtmzr changed the title Fix AES Secure Messaging MAC check using the received checksum Fix AES SM MAC compare in unprotect() Aug 18, 2026
unprotect() copies CC (the MAC from the chip) instead of CCb when the computed AES-CMAC is longer than 8 bytes, so the integrity check compares the chip MAC with itself. protect() already truncates CCb correctly. Existing 3DES tests never hit this branch because DES MACs are already 8 bytes.
@bahadirtmzr
bahadirtmzr force-pushed the security/verified-findings-only branch from 5b8d987 to 7d9cb8a Compare August 18, 2026 16:41
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.

1 participant