From a5ee66633263b2113e781cea7f9d1ef556dec82c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tobias=20Frauenschl=C3=A4ger?= Date: Wed, 21 Oct 2026 09:31:09 +0200 Subject: [PATCH] Wipe the old GCM H when a device claims an AES key When a SETKEY device claimed a new key through wc_AesSetKey(), wc_AesCcmSetKey() or wc_AesSetKeyDirect(), the set-key wiped the old key schedule and devKey but left gcm.H and the GHASH table M0 of an earlier software GCM key in the context. Only wc_AesGcmSetKey() wiped them. gcmKeySet is already cleared on that path, so the stale H could not be used, but it is key material derived from the old key and goes with the rest of it. The pre-claim wipe in the generic, ARM32 and PPC set-key now clears H and M0 too, through a WC_AES_GCM_WIPE_H() macro that wc_AesGcmSetKey() uses as well. The CryptoCb AES set-key tests in test_aes.c took buffer sizes with sizeof, which is the size of a pointer under WOLFSSL_SMALL_STACK. In such builds test_wc_CryptoCb_AesSetKey and test_wc_CryptoCb_AesGcm_EncryptDecrypt failed at their first set-key with an 8-byte key, and the declined re-key check passed an 8-byte tag that wc_AesGcmEncrypt() rejects before it asks the device. Both tests now use the allocation sizes. --- tests/api.c | 12 +++++++++-- tests/api/test_aes.c | 48 ++++++++++++++++++++++---------------------- wolfcrypt/src/aes.c | 20 ++++++++++++++---- 3 files changed, 50 insertions(+), 30 deletions(-) diff --git a/tests/api.c b/tests/api.c index f6ac356d09..4c71568c07 100644 --- a/tests/api.c +++ b/tests/api.c @@ -38108,12 +38108,10 @@ static int test_wc_CryptoCb_AesGcm_SetKey(void) 0x00, 0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07, 0x08, 0x09, 0x0a, 0x0b, 0x0c, 0x0d, 0x0e, 0x0f }; -#ifdef WOLFSSL_AESGCM_STREAM const byte key2[AES_128_KEY_SIZE] = { 0xf0, 0xf1, 0xf2, 0xf3, 0xf4, 0xf5, 0xf6, 0xf7, 0xf8, 0xf9, 0xfa, 0xfb, 0xfc, 0xfd, 0xfe, 0xff }; -#endif const byte iv[GCM_NONCE_MID_SZ] = { 0xca, 0xfe, 0xba, 0xbe, 0xfa, 0xce, 0xdb, 0xad, 0xde, 0xca, 0xf8, 0x88 @@ -38125,8 +38123,10 @@ static int test_wc_CryptoCb_AesGcm_SetKey(void) byte ct[sizeof(msg)]; byte tag[WC_AES_BLOCK_SIZE]; byte pt[sizeof(msg)]; + byte zeroH[WC_AES_BLOCK_SIZE]; XMEMSET(&dev, 0, sizeof(dev)); + XMEMSET(zeroH, 0, sizeof(zeroH)); ExpectIntEQ(wc_AesInit(&aes, NULL, TEST_CRYPTOCB_UNREG_DEVID), 0); ExpectIntEQ(wc_AesGcmSetKey(&aes, key, sizeof(key)), 0); @@ -38193,6 +38193,14 @@ static int test_wc_CryptoCb_AesGcm_SetKey(void) sizeof(aad)), WC_NO_ERR_TRACE(MISSING_KEY)); #endif + /* A claim through wc_AesSetKey() wipes the old software key's H. */ + dev.declineSetKey = 1; + ExpectIntEQ(wc_AesGcmSetKey(&aes, key2, sizeof(key2)), 0); + dev.declineSetKey = 0; + ExpectBufNE(aes.gcm.H, zeroH, sizeof(zeroH)); + ExpectIntEQ(wc_AesSetKey(&aes, key, sizeof(key), NULL, AES_ENCRYPTION), 0); + ExpectBufEQ(aes.gcm.H, zeroH, sizeof(zeroH)); + if (aesInit) { wc_AesFree(&aes); } diff --git a/tests/api/test_aes.c b/tests/api/test_aes.c index e03c861a4e..0d4b72a3d6 100644 --- a/tests/api/test_aes.c +++ b/tests/api/test_aes.c @@ -12802,15 +12802,15 @@ int test_wc_CryptoCb_AesSetKey(void) ExpectIntEQ(aes->devId, TEST_CRYPTOCB_AES_DEVID); /* Direct callback entry guardrails. */ - ExpectIntEQ(wc_CryptoCb_AesSetKey(NULL, key, sizeof(key)), + ExpectIntEQ(wc_CryptoCb_AesSetKey(NULL, key, AES_128_KEY_SIZE), WC_NO_ERR_TRACE(BAD_FUNC_ARG)); - ExpectIntEQ(wc_CryptoCb_AesSetKey(aes, NULL, sizeof(key)), + ExpectIntEQ(wc_CryptoCb_AesSetKey(aes, NULL, AES_128_KEY_SIZE), WC_NO_ERR_TRACE(BAD_FUNC_ARG)); ExpectIntEQ(wc_CryptoCb_AesSetKey(aes, key, 0), WC_NO_ERR_TRACE(BAD_FUNC_ARG)); /* Set key - should trigger CryptoCB and "import" to mock SE */ - ret = wc_AesGcmSetKey(aes, key, sizeof(key)); + ret = wc_AesGcmSetKey(aes, key, AES_128_KEY_SIZE); ExpectIntEQ(ret, 0); /* Verify callback was invoked */ @@ -12821,16 +12821,16 @@ int test_wc_CryptoCb_AesSetKey(void) /* Verify key was "imported" to mock SE */ ExpectIntEQ(mockSeKey.valid, 1); - ExpectIntEQ(mockSeKey.keySz, (int)sizeof(key)); + ExpectIntEQ(mockSeKey.keySz, AES_128_KEY_SIZE); /* Verify keylen metadata stored in Aes struct */ - ExpectIntEQ(aes->keylen, (int)sizeof(key)); + ExpectIntEQ(aes->keylen, AES_128_KEY_SIZE); /* After SetKey succeeds via CryptoCB, verify key NOT in devKey */ { byte zeroKey[AES_128_KEY_SIZE] = {0}; /* Key should NOT be copied to devKey - SE owns it */ - ExpectIntEQ(XMEMCMP(aes->devKey, zeroKey, sizeof(key)), 0); + ExpectIntEQ(XMEMCMP(aes->devKey, zeroKey, AES_128_KEY_SIZE), 0); } #if defined(WOLFSSL_AESGCM_STREAM) && !defined(HAVE_FIPS) && \ @@ -12850,29 +12850,29 @@ int test_wc_CryptoCb_AesSetKey(void) /* Missing device context should fail in the callback instead of falling * through as a successful offload path. */ aes->devCtx = NULL; - ExpectIntEQ(wc_AesGcmEncrypt(aes, cipher, plain, sizeof(plain), - iv, sizeof(iv), authTag, sizeof(authTag), NULL, 0), + ExpectIntEQ(wc_AesGcmEncrypt(aes, cipher, plain, 16, + iv, GCM_NONCE_MID_SZ, authTag, AES_BLOCK_SIZE, NULL, 0), WC_NO_ERR_TRACE(BAD_FUNC_ARG)); /* Restore callback-owned state for the rest of the test. */ - ret = wc_AesGcmSetKey(aes, key, sizeof(key)); + ret = wc_AesGcmSetKey(aes, key, AES_128_KEY_SIZE); ExpectIntEQ(ret, 0); ExpectPtrEq(aes->devCtx, cryptoCbAesMockHandle); /* Test encrypt - callback performs crypto using stored key */ - ret = wc_AesGcmEncrypt(aes, cipher, plain, sizeof(plain), - iv, sizeof(iv), authTag, sizeof(authTag), + ret = wc_AesGcmEncrypt(aes, cipher, plain, 16, + iv, GCM_NONCE_MID_SZ, authTag, AES_BLOCK_SIZE, NULL, 0); ExpectIntEQ(ret, 0); /* Test decrypt - callback performs crypto using stored key */ - ret = wc_AesGcmDecrypt(aes, decrypted, cipher, sizeof(cipher), - iv, sizeof(iv), authTag, sizeof(authTag), + ret = wc_AesGcmDecrypt(aes, decrypted, cipher, 16, + iv, GCM_NONCE_MID_SZ, authTag, AES_BLOCK_SIZE, NULL, 0); ExpectIntEQ(ret, 0); /* Verify round-trip */ - ExpectIntEQ(XMEMCMP(plain, decrypted, sizeof(plain)), 0); + ExpectIntEQ(XMEMCMP(plain, decrypted, 16), 0); #if defined(WOLFSSL_AESGCM_STREAM) && !defined(HAVE_FIPS) && \ !defined(HAVE_SELFTEST) @@ -12882,8 +12882,8 @@ int test_wc_CryptoCb_AesSetKey(void) ExpectIntEQ(wc_AesGcmSetKey(aes, key, AES_128_KEY_SIZE), 0); cryptoCbAesDeclineSetKey = 0; ExpectNull(aes->devCtx); - ExpectIntEQ(wc_AesGcmEncrypt(aes, cipher, plain, sizeof(plain), - iv, sizeof(iv), authTag, sizeof(authTag), NULL, 0), + ExpectIntEQ(wc_AesGcmEncrypt(aes, cipher, plain, 16, + iv, GCM_NONCE_MID_SZ, authTag, AES_BLOCK_SIZE, NULL, 0), WC_NO_ERR_TRACE(BAD_FUNC_ARG)); ExpectIntEQ(wc_AesGcmInit(aes, NULL, 0, iv, GCM_NONCE_MID_SZ), 0); ExpectIntEQ(wc_AesGcmEncryptUpdate(aes, cipher, plain, 16, NULL, 0), 0); @@ -12911,7 +12911,7 @@ int test_wc_CryptoCb_AesSetKey(void) /* Cleanup */ wc_CryptoCb_UnRegisterDevice(TEST_CRYPTOCB_AES_DEVID); - ExpectIntEQ(wc_CryptoCb_AesSetKey(aes, key, sizeof(key)), + ExpectIntEQ(wc_CryptoCb_AesSetKey(aes, key, AES_128_KEY_SIZE), WC_NO_ERR_TRACE(CRYPTOCB_UNAVAILABLE)); /* Test software path (no devId) still works */ @@ -12921,7 +12921,7 @@ int test_wc_CryptoCb_AesSetKey(void) ret = wc_AesInit(aes, NULL, INVALID_DEVID); ExpectIntEQ(ret, 0); - ret = wc_AesGcmSetKey(aes, key, sizeof(key)); + ret = wc_AesGcmSetKey(aes, key, AES_128_KEY_SIZE); ExpectIntEQ(ret, 0); /* Callback should NOT have been invoked */ @@ -13250,7 +13250,7 @@ int test_wc_CryptoCb_AesGcm_EncryptDecrypt(void) ExpectIntEQ(aes->devId, TEST_CRYPTOCB_AESGCM_OFFLOAD_DEVID); /* Set key - should trigger CryptoCB and "import" to mock SE */ - ret = wc_AesGcmSetKey(aes, key, sizeof(key)); + ret = wc_AesGcmSetKey(aes, key, AES_128_KEY_SIZE); ExpectIntEQ(ret, 0); /* Verify SetKey callback was invoked */ @@ -13261,14 +13261,14 @@ int test_wc_CryptoCb_AesGcm_EncryptDecrypt(void) /* Verify key was "imported" to mock SE */ ExpectIntEQ(mockSeKeyOffload.valid, 1); - ExpectIntEQ(mockSeKeyOffload.keySz, (int)sizeof(key)); + ExpectIntEQ(mockSeKeyOffload.keySz, AES_128_KEY_SIZE); /* Verify keylen metadata stored in Aes struct */ - ExpectIntEQ(aes->keylen, (int)sizeof(key)); + ExpectIntEQ(aes->keylen, AES_128_KEY_SIZE); /* Encrypt via wolfCrypt API - should route through CryptoCb */ ret = wc_AesGcmEncrypt(aes, ciphertext, plaintext, 32, - iv, sizeof(iv), authTag, sizeof(authTag), + iv, GCM_NONCE_MID_SZ, authTag, AES_BLOCK_SIZE, aad, 16); ExpectIntEQ(ret, 0); @@ -13280,7 +13280,7 @@ int test_wc_CryptoCb_AesGcm_EncryptDecrypt(void) /* Assert: Auth tag is non-zero */ hasNonZero = 0; - for (i = 0; i < (int)sizeof(authTag); i++) { + for (i = 0; i < AES_BLOCK_SIZE; i++) { if (authTag[i] != 0) { hasNonZero = 1; break; @@ -13290,7 +13290,7 @@ int test_wc_CryptoCb_AesGcm_EncryptDecrypt(void) /* Decrypt via wolfCrypt API - should route through CryptoCb */ ret = wc_AesGcmDecrypt(aes, decrypted, ciphertext, 32, - iv, sizeof(iv), authTag, sizeof(authTag), + iv, GCM_NONCE_MID_SZ, authTag, AES_BLOCK_SIZE, aad, 16); ExpectIntEQ(ret, 0); diff --git a/wolfcrypt/src/aes.c b/wolfcrypt/src/aes.c index ae26b17424..b6322eae4b 100644 --- a/wolfcrypt/src/aes.c +++ b/wolfcrypt/src/aes.c @@ -160,6 +160,18 @@ block cipher mechanism that uses n-bit binary string parameter key with 128-bits #define WC_AES_GCM_UNKEY(aes) WC_DO_NOTHING #endif +#if defined(HAVE_AESGCM) && (defined(GCM_TABLE) || defined(GCM_TABLE_4BIT)) + #define WC_AES_GCM_WIPE_H(aes) do { \ + ForceZero((aes)->gcm.H, sizeof((aes)->gcm.H)); \ + ForceZero((aes)->gcm.M0, sizeof((aes)->gcm.M0)); \ + } while (0) +#elif defined(HAVE_AESGCM) + #define WC_AES_GCM_WIPE_H(aes) \ + ForceZero((aes)->gcm.H, sizeof((aes)->gcm.H)) +#else + #define WC_AES_GCM_WIPE_H(aes) WC_DO_NOTHING +#endif + #if defined(__aarch64__) && defined(WOLFSSL_ARMASM) #define WC_AES_ARM64_SVR_BEGIN() SAVE_VECTOR_REGISTERS(return _svr_ret;) #define WC_AES_ARM64_SVR_END() RESTORE_VECTOR_REGISTERS() @@ -5454,6 +5466,7 @@ static WARN_UNUSED_RESULT int wc_AesDecrypt(Aes* aes, const byte* inBlock, aes->rounds = 0; aes->keylen = 0; WC_AES_GCM_UNKEY(aes); + WC_AES_GCM_WIPE_H(aes); ForceZero(aes->key, sizeof(aes->key)); ForceZero(aes->devKey, sizeof(aes->devKey)); #endif @@ -5588,6 +5601,7 @@ static WARN_UNUSED_RESULT int wc_AesDecrypt(Aes* aes, const byte* inBlock, aes->rounds = 0; aes->keylen = 0; WC_AES_GCM_UNKEY(aes); + WC_AES_GCM_WIPE_H(aes); ForceZero(aes->key, sizeof(aes->key)); ForceZero(aes->devKey, sizeof(aes->devKey)); #endif @@ -6122,6 +6136,7 @@ static void AesSetKey_C(Aes* aes, const byte* key, word32 keySz, int dir) aes->rounds = 0; aes->keylen = 0; WC_AES_GCM_UNKEY(aes); + WC_AES_GCM_WIPE_H(aes); ForceZero(aes->key, sizeof(aes->key)); ForceZero(aes->devKey, sizeof(aes->devKey)); #ifdef WC_C_DYNAMIC_FALLBACK @@ -9336,10 +9351,7 @@ int wc_AesGcmSetKey(Aes* aes, const byte* key, word32 len) { /* No schedule: a device claimed the key or the set-key failed. */ if ((ret != 0) || (aes->rounds == 0)) { - ForceZero(aes->gcm.H, sizeof(aes->gcm.H)); - #if defined(GCM_TABLE) || defined(GCM_TABLE_4BIT) - ForceZero(aes->gcm.M0, sizeof(aes->gcm.M0)); - #endif + WC_AES_GCM_WIPE_H(aes); #ifdef WOLFSSL_IMX6_CAAM_BLOB ForceZero(local, sizeof(local)); #endif