Add dynamic key allocation support for ML-KEM#10179
Conversation
|
Jenkins retest this please |
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #10179
Scan targets checked: wolfcrypt-api_misuse, wolfcrypt-bugs, wolfcrypt-compliance, wolfcrypt-concurrency, wolfcrypt-consttime, wolfcrypt-defaults, wolfcrypt-mutation, wolfcrypt-portability, wolfcrypt-proptest, wolfcrypt-src, wolfcrypt-zeroize
No new issues found in the changed files. ✅
Introduce the WOLFSSL_MLKEM_DYNAMIC_KEYS option to allow dynamic allocation of private and public key buffers in the MlKemKey struct. This change enables right-sizing of buffers based on the actual ML-KEM level and eliminates unnecessary memory usage for encapsulate-only operations.
0bcc578 to
b7cc7ad
Compare
|
Jenkins retest this please |
| static int mlkemkey_alloc_priv(MlKemKey* key, unsigned int k) | ||
| { | ||
| if (key->priv != NULL) { | ||
| ForceZero(key->priv, k * MLKEM_N * sizeof(sword16)); |
There was a problem hiding this comment.
When mlkemkey_alloc_priv is called with a non-NULL key->priv (the "reallocate" path), it calls ForceZero(key->priv, k * MLKEM_N * sizeof(sword16)) using the new k parameter to determine the size to zero. However, the old buffer was allocated with whatever k was passed to the previous call. If a future caller passes a different k (e.g., larger than the old allocation), this would zero past the end of the heap buffer (heap overflow). If the new k is smaller, sensitive private key data would remain unzeroed in freed memory.
Today's call sites all derive k from key->type which doesn't change during the key's lifetime, so this is not currently exploitable. But the function is documented as handling reallocation, and the missing size tracking makes it unsafe for that purpose. A simple fix is to track the allocated k in the struct (e.g., unsigned int priv_k) or to always use WC_ML_KEM_MAX_K for the ForceZero size.
Code:
static int mlkemkey_alloc_priv(MlKemKey* key, unsigned int k)
{
if (key->priv != NULL) {
ForceZero(key->priv, k * MLKEM_N * sizeof(sword16)); /* uses NEW k, not old */
XFREE(key->priv, key->heap, DYNAMIC_TYPE_TMP_BUFFER);
key->priv = NULL;
}
Suggestion:
Track the allocated size, or always zero using the maximum possible size:
if (key->priv != NULL) {
ForceZero(key->priv, WC_ML_KEM_MAX_K * MLKEM_N * sizeof(sword16));
XFREE(key->priv, key->heap, DYNAMIC_TYPE_TMP_BUFFER);
key->priv = NULL;
}
Alternatively, add a `priv_k` field to the struct to remember the allocation size.
There was a problem hiding this comment.
Fixed by using the new privAllocSz variable within the key object which stores the size of the allocated buffer. With that, we use the size of the current buffer for ForceZero() and then use the passed k for allocating the new buffer.
| ForceZero(&key->prf, sizeof(key->prf)); | ||
| #ifdef WOLFSSL_MLKEM_DYNAMIC_KEYS | ||
| if (key->priv != NULL) { | ||
| int k = mlkemkey_get_k(key); |
There was a problem hiding this comment.
Description:
In wc_MlKemKey_Free, the private key buffer is zeroed using mlkemkey_get_k(key) to determine the size. If key->type is ever invalid or 0 (e.g., due to corruption, double-free, or partial initialization failure), mlkemkey_get_k returns 0, and the ForceZero call becomes a no-op — the private key data remains in freed memory. This is the same root cause as the alloc_priv issue: the allocation size is not tracked alongside the buffer.
Code:
if (key->priv != NULL) {
int k = mlkemkey_get_k(key);
ForceZero(key->priv,
(word32)(k * MLKEM_N) * (word32)sizeof(sword16));
XFREE(key->priv, key->heap, DYNAMIC_TYPE_TMP_BUFFER);
key->priv = NULL;
}
Suggestion:
Use WC_ML_KEM_MAX_K for the ForceZero in Free, guaranteeing all possible data is zeroed:
if (key->priv != NULL) {
ForceZero(key->priv,
(word32)(WC_ML_KEM_MAX_K * MLKEM_N) * (word32)sizeof(sword16));
XFREE(key->priv, key->heap, DYNAMIC_TYPE_TMP_BUFFER);
key->priv = NULL;
}
There was a problem hiding this comment.
Fixed by storing the size of the private key buffer in the key object.
|
PR for the fixes is here: #10206 |
Introduce the
WOLFSSL_MLKEM_DYNAMIC_KEYSoption to allow dynamic allocation of private and public key buffers in theMlKemKeystruct. This change enables right-sizing of buffers based on the actual ML-KEM level and eliminates unnecessary memory usage for encapsulate-only operations.This greatly reduces the dynamic memory usage during TLS handshakes, which is especially important for resource-constrained systems.