Repository navigation
Ensure ECCSI SSK is in [1, q-1]. Fix multiple issues in TI/Xilinx AES. Update base16/64/blake2b documentation. - #11902
kareem-wolfssl wants to merge 4 commits into
Conversation
|
@claude review |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Additional findings (outside the current diff — GitHub can't attach inline comments there):
-
🟣
wolfcrypt/src/port/ti/ti-aes.c— Pre-existing: on TI, a hardware failure in the main AES-GCM/CCM encrypt path is still reported as AES_GCM_AUTH_E, even in CCM mode. This change makes the other paths return WC_HW_E or a mode-correct code, so encrypt is now the inconsistent one. Fix: return WC_HW_E when ROM_AESDataProcessAuth returns false in AesAuthEncrypt. That matches the new early-path handling at ti-aes.c:617-625 and keeps the existing ForceZero of out and authTag.Why this was flagged
AesAuthEncrypt (ti-aes.c:577) is reached via wc_AesGcmEncrypt, wc_AesCcmEncrypt and GMAC. When ROM_AESDataProcessAuth returns false (ti-aes.c:667-674), the code sets
ret = AES_GCM_AUTH_Eat ti-aes.c:679 regardless of mode. Encryption has no authentication to fail, so this is an engine failure. In the same function this PR now returns WC_HW_E for an engine failure in the empty-input path (ti-aes.c:617-625). In AesAuthDecrypt it now picks AES_CCM_AUTH_E for CCM at ti-aes.c:808. So a CCM caller on TI hardware gets an AES-GCM authentication error for a hardware fault, and the error depends on which branch ran. The base behaved the same way (pre-existing). This PR's partial fix leaves this sibling path inconsistent.Verification: Pre-existing. In AesAuthEncrypt (wolfcrypt/src/port/ti/ti-aes.c:668-680), when ROM_AESDataProcessAuth returns false, the code still returns AES_GCM_AUTH_E whatever the mode. That includes CCM. This PR changes the empty-input path (lines 617-625) to return WC_HW_E, and AesAuthDecrypt now picks the code by mode at line 808. The harm is limited because out and authTag are zeroed (lines 676-678), and the base branch already returns this code.
-
🟣
wolfcrypt/src/port/ti/ti-aes.c— pre-existing: TI AES-GCM callers with a non-12-byte nonce can still get an IV derived from an uninitialized subkey if the hardware call fails. The PR checks ROM_AESDataProcess elsewhere but not at ti-aes.c:476 in AesAuthSetIv. If that call fails,subkeyis never written and is still loaded via ROM_AESKey2Set to GHASH the nonce. Fix: check the result at ti-aes.c:476 and return WC_HW_E. This means AesAuthSetIv must return int, and AesAuthEncrypt/AesAuthDecrypt must pass that error up, the same way AesAlign16 now does.Why this was flagged
The trigger is wc_AesGcmEncrypt/Decrypt on TI hardware with a nonce length other than GCM_NONCE_MID_SZ, which reaches the else branch of AesAuthSetIv (ti-aes.c:462). The trigger also needs ROM_AESDataProcess to return false at ti-aes.c:476. The stack array
subkey(ti-aes.c:464) is then still uninitialized. It is passed to ROM_AESKey2Set at ti-aes.c:481, so the J0 counter written to aes->reg comes from whatever was on the stack. Encryption then goes ahead and reports success with a wrong tag and keystream, with no error to the caller. Different nonces could even map to the same counter. This PR adds checks with the same purpose at AesAlign16 (ti-aes.c:165) and at both empty-input tag paths (ti-aes.c:617, 745), so this sibling is the one ROM_AESDataProcess call left unchecked. The base has the same behaviour here.Verification: pre-existing. Trigger: TI GCM with a nonce length other than GCM_NONCE_MID_SZ, and ROM_AESDataProcess returning false.
ROM_AESDataProcess(AES_BASE, zeros, subkey, sizeof zeros);(wolfcrypt/src/port/ti/ti-aes.c:476) ignores its bool result, andROM_AESKey2Set(AES_BASE, subkey, ...)(line 481) then loads whatever subkey holds. The base already behaves the same way by the same route.
| WC_DECLARE_VAR(order, mp_int, 1, key->heap); | ||
|
|
||
| if (mp_iszero(ssk)) { | ||
| return ECC_PRIV_KEY_E; | ||
| } | ||
|
|
There was a problem hiding this comment.
🟡 (optional) Callers of wc_SetEccsiPair can still install a negative SSK, which is outside the [1, q-1] range this change is meant to enforce. eccsi_check_ssk only rejects zero and values where mp_cmp(ssk, order) != MP_LT. A negative mp_int compares less than q, so it passes. Fix: in eccsi_check_ssk, reject mp_iszero(ssk) || mp_isneg(ssk) before the order comparison. mp_isneg is defined for sp_int, tfm and integer, so the check builds under every math backend.
Why this was flagged
wc_SetEccsiPair (eccsi.c:1785) takes a caller-supplied const mp_int* ssk. When the math library supports sign (WOLFSSL_SP_INT_NEGATIVE, e.g. with OPENSSL_EXTRA, or the tfm/integer backends), the mp_int can be negative. eccsi_check_ssk (eccsi.c:205) checks mp_iszero, then mp_cmp((mp_int*)ssk, order) != MP_LT. A negative value is MP_LT the positive order, so the check returns 0. mp_copy then stores it in key->ssk and signing uses it. The decode paths read unsigned bytes and cannot produce a negative value, so wc_SetEccsiPair is the only entry that gets past the new range check. The PR states the goal is to ensure SSK is in [1, q-1], and this input is outside that range yet accepted.
Verification: The new eccsi_check_ssk (wolfcrypt/src/eccsi.c:205-229) has only two tests. With WOLFSSL_SP_INT_NEGATIVE, _sp_cmp (wolfcrypt/src/sp_int.c:5821-5826) compares signs first. So any negative ssk compares MP_LT against the positive order and passes. For example, ssk = -q passes the check but acts as a zero signing key. The severity is low because the base branch did no range check at all.
Tighten auth code size checks in Xilinx AES support.
6542711 to
2777b74
Compare
Description
Testing
Built-in + added tests
Checklist