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 3 commits into
Conversation
Tighten auth code size checks in Xilinx AES support.
|
@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.
Description
Testing
Built-in + added tests
Checklist