Boundary checks + zeroization improvements. Fix ed25519 and ECC dispatchers. Make PKCS7 padding checks CT.#7
Merged
Conversation
There was a problem hiding this comment.
Pull request overview
This PR hardens the PSA engine against boundary/length issues and improves secret-handling by adding zeroization on abort/error paths, while also fixing Twisted Edwards (Ed25519/Ed448) dispatching and making CBC-PKCS7 padding validation constant-time.
Changes:
- Add constant-time CBC-PKCS7 padding validation and fix CBC multipart decrypt length underflow handling.
- Improve zeroization of sensitive buffers/contexts in cipher, MAC, AEAD, and key import/generation flows.
- Fix Twisted Edwards (Ed25519/Ed448) dispatch ordering for sign/verify, key generation, and public key export; add regression tests.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| test/psa_server/psa_api_test.c | Adds regression tests for PKCS7 multipart CBC decrypt and Twisted Edwards message-signing / public-key export. |
| src/psa_tls_prf.c | Adds size_t→word32 narrowing guards for TLS 1.3 PRF/HKDF inputs/outputs. |
| src/psa_mac.c | Ensures stack buffers are zeroized on MAC finish/verify paths; zeroizes MAC context on abort. |
| src/psa_key_storage.c | Zeroizes import buffer before free; routes Twisted Edwards keygen and public-key export via ED25519/ED448-specific implementations. |
| src/psa_cipher.c | Prevents multipart underflow; implements constant-time CBC-PKCS7 padding checks; zeroizes cipher context on abort. |
| src/psa_asymmetric_api.c | Fixes dispatch order so Twisted Edwards keys are handled before generic ECC sign/verify paths. |
| src/psa_aead.c | Zeroizes AEAD AAD/input buffers before freeing on abort. |
Comments suppressed due to low confidence (3)
src/psa_key_storage.c:1074
- In
psa_generate_key(), Twisted Edwards key generation is placed under#ifdef HAVE_ECC. This means ED25519/ED448 keypair generation will be unavailable (PSA_ERROR_NOT_SUPPORTED) in builds that enableHAVE_ED25519/HAVE_ED448but notHAVE_ECC, even though EdDSA sign/verify paths are guarded only byHAVE_ED25519/HAVE_ED448elsewhere. Consider loosening the compile-time guard to allow Twisted Edwards generation when the corresponding ED* feature macro is enabled (and returning NOT_SUPPORTED only when neither ECC nor ED* support is compiled in).
if (PSA_KEY_TYPE_IS_ECC_KEY_PAIR(key_type)) {
#ifdef HAVE_ECC
psa_ecc_family_t family = PSA_KEY_TYPE_ECC_GET_FAMILY(key_type);
size_t priv_buf_size = PSA_KEY_EXPORT_ECC_KEY_PAIR_MAX_SIZE(key_bits);
size_t pub_buf_size = PSA_KEY_EXPORT_ECC_PUBLIC_KEY_MAX_SIZE(key_bits);
size_t priv_len = 0;
size_t pub_len = 0;
uint8_t *pub_buf = NULL;
key_data = (uint8_t *)XMALLOC(priv_buf_size, NULL,
DYNAMIC_TYPE_TMP_BUFFER);
if (key_data == NULL) {
return PSA_ERROR_INSUFFICIENT_MEMORY;
}
pub_buf = (uint8_t *)XMALLOC(pub_buf_size, NULL,
DYNAMIC_TYPE_TMP_BUFFER);
if (pub_buf == NULL) {
wc_ForceZero(key_data, priv_buf_size);
XFREE(key_data, NULL, DYNAMIC_TYPE_TMP_BUFFER);
return PSA_ERROR_INSUFFICIENT_MEMORY;
}
if (family == PSA_ECC_FAMILY_TWISTED_EDWARDS) {
#ifdef HAVE_ED25519
if (key_bits == 255) {
status = psa_asymmetric_generate_key_ed25519(key_type, key_bits,
key_data, priv_buf_size,
&priv_len,
pub_buf, pub_buf_size,
&pub_len);
}
else
#endif
#ifdef HAVE_ED448
if (key_bits == 448) {
status = psa_asymmetric_generate_key_ed448(key_type, key_bits,
key_data, priv_buf_size,
&priv_len,
pub_buf, pub_buf_size,
&pub_len);
}
else
#endif
{
status = PSA_ERROR_INVALID_ARGUMENT;
}
}
else {
status = psa_asymmetric_generate_key_ecc(key_type, key_bits,
key_data, priv_buf_size,
&priv_len,
pub_buf, pub_buf_size,
&pub_len);
}
XFREE(pub_buf, NULL, DYNAMIC_TYPE_TMP_BUFFER);
if (status != PSA_SUCCESS) {
wc_ForceZero(key_data, priv_buf_size);
XFREE(key_data, NULL, DYNAMIC_TYPE_TMP_BUFFER);
return status;
}
status = psa_import_key(attributes, key_data, priv_len, key_id);
wc_ForceZero(key_data, priv_buf_size);
XFREE(key_data, NULL, DYNAMIC_TYPE_TMP_BUFFER);
return status;
#else
return PSA_ERROR_NOT_SUPPORTED;
#endif
src/psa_key_storage.c:1478
psa_export_public_key()'s ECC handling is currently wrapped in#if defined(HAVE_ECC) && defined(HAVE_ECC_KEY_EXPORT) && defined(HAVE_ECC_KEY_IMPORT), but the Twisted Edwards export path inside that block depends onHAVE_ED25519/HAVE_ED448, not onHAVE_ECC(_KEY_*). As written, ED25519/ED448 public key export will be compiled out (and return NOT_SUPPORTED) in configurations where EdDSA is enabled without ECC key import/export. Consider splitting the guard so Twisted Edwards public key export is enabled whenHAVE_ED25519/HAVE_ED448is set, and reserve theHAVE_ECC_KEY_*guard only for the Weierstrass ECC (x9.63) branch.
#if defined(HAVE_ECC) && defined(HAVE_ECC_KEY_EXPORT) && defined(HAVE_ECC_KEY_IMPORT)
if (PSA_KEY_TYPE_IS_ECC_PUBLIC_KEY(attributes.type)) {
if (data_size < key_data_length) {
status = PSA_ERROR_BUFFER_TOO_SMALL;
}
else {
XMEMCPY(data, key_data, key_data_length);
*data_length = key_data_length;
status = PSA_SUCCESS;
}
}
else {
psa_ecc_family_t family = PSA_KEY_TYPE_ECC_GET_FAMILY(attributes.type);
if (family == PSA_ECC_FAMILY_TWISTED_EDWARDS) {
#ifdef HAVE_ED25519
if (attributes.bits == 255) {
status = psa_asymmetric_export_public_key_ed25519(
attributes.type, attributes.bits, key_data,
key_data_length, data, data_size, data_length);
}
else
#endif
#ifdef HAVE_ED448
if (attributes.bits == 448) {
status = psa_asymmetric_export_public_key_ed448(
attributes.type, attributes.bits, key_data,
key_data_length, data, data_size, data_length);
}
else
#endif
{
status = PSA_ERROR_NOT_SUPPORTED;
}
}
else {
ecc_key ecc;
word32 out_len = (word32)data_size;
int curve_id = wc_psa_get_ecc_curve_id(attributes.type,
attributes.bits);
if (curve_id == ECC_CURVE_INVALID) {
status = PSA_ERROR_NOT_SUPPORTED;
}
else if (wc_ecc_init(&ecc) != 0) {
status = PSA_ERROR_INSUFFICIENT_MEMORY;
}
else {
ret = wc_ecc_import_private_key_ex(key_data,
(word32)key_data_length,
NULL, 0, &ecc, curve_id);
if (ret != 0) {
wc_ecc_free(&ecc);
status = psa_wc_error_to_psa_status(ret);
}
else {
ret = wc_ecc_make_pub_ex(&ecc, NULL, NULL);
if (ret != 0) {
wc_ecc_free(&ecc);
status = psa_wc_error_to_psa_status(ret);
}
else {
ret = wc_ecc_export_x963(&ecc, data, &out_len);
wc_ecc_free(&ecc);
if (ret != 0) {
status = psa_wc_error_to_psa_status(ret);
}
else {
*data_length = (size_t)out_len;
status = PSA_SUCCESS;
}
}
}
}
}
}
#else
status = PSA_ERROR_NOT_SUPPORTED;
#endif
src/psa_aead.c:856
psa_aead_abort()now zeroizesaadandinputbuffers before freeing, but thewolfpsa_aead_ctx_titself (which contains the nonce and other operation state) is still freed without being scrubbed. For consistency withpsa_cipher_abort()/psa_mac_abort()and to avoid leaving nonce/state in freed heap memory, wipe*ctx(and ideally clearoperation->opaque) beforeXFREE(ctx, ...).
if (ctx != NULL) {
if (ctx->aad != NULL) {
wc_ForceZero(ctx->aad, ctx->aad_length);
XFREE(ctx->aad, NULL, DYNAMIC_TYPE_TMP_BUFFER);
}
if (ctx->input != NULL) {
wc_ForceZero(ctx->input, ctx->input_length);
XFREE(ctx->input, NULL, DYNAMIC_TYPE_TMP_BUFFER);
}
if (ctx->key != NULL) {
wc_ForceZero(ctx->key, ctx->key_length);
XFREE(ctx->key, NULL, DYNAMIC_TYPE_TMP_BUFFER);
}
XFREE(ctx, NULL, DYNAMIC_TYPE_TMP_BUFFER);
operation->opaque = (uintptr_t)NULL;
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
dgarske
approved these changes
Mar 23, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
F/1188 - Guard PKCS7 CBC decrypt length underflow (
8affc91)F/1189 - Fix EdDSA PSA dispatch order (
6dd7db7)F/1196 - zero cipher context before free (
3ba730c)F/1197 - zero import buffer before free (
66a24e6)F/1198 - zeroize MAC context on abort (
996cc15)F/1190 - Dispatch Edwards key generation correctly (
24bbd81)F/1191 - Route Twisted Edwards public key export (
168dcec)F/1199 - Make CBC PKCS7 padding check constant time (
cf32ac0)F/1200 - Zero AEAD buffers on abort (
f0965c7)F/1201 - check TLS13 PRF length narrowing (
52b036a)F/1202 - zero MAC stack buffer on exit (
9a01ef3)F/1203 - Zero MAC verify stack buffer (
dc92685)