From 432834940056aadbe36242f60a340481ca6141f9 Mon Sep 17 00:00:00 2001 From: Hideki Miyazaki Date: Wed, 8 Apr 2026 16:14:21 +0900 Subject: [PATCH 1/2] fix f-1899 potential heap buffer over-read add test coverage --- src/sign-verify/clu_sign.c | 191 ++++++++---------- tests/genkey_sign_ver/genkey-sign-ver-test.sh | 23 +++ 2 files changed, 109 insertions(+), 105 deletions(-) diff --git a/src/sign-verify/clu_sign.c b/src/sign-verify/clu_sign.c index cc15526a..03371a7a 100644 --- a/src/sign-verify/clu_sign.c +++ b/src/sign-verify/clu_sign.c @@ -617,16 +617,18 @@ int wolfCLU_sign_data_dilithium (byte* data, char* out, word32 dataSz, char* pri { #ifdef HAVE_DILITHIUM int ret = 0; + int privFileSz = 0; + word32 index = 0; + XFILE privKeyFile = NULL; byte* privBuf = NULL; - int privFileSz = 0; + word32 privBufSz = 0; - word32 index = 0; byte* outBuf = NULL; word32 outBufSz = 0; WC_RNG rng; - XMEMSET(&rng, 0, sizeof(rng)); + #ifdef WOLFSSL_SMALL_STACK dilithium_key* key; @@ -639,153 +641,132 @@ int wolfCLU_sign_data_dilithium (byte* data, char* out, word32 dataSz, char* pri dilithium_key key[1]; #endif + XMEMSET(&rng, 0, sizeof(rng)); + XMEMSET(key, 0, sizeof(dilithium_key)); + /* init the dilithium key */ if (wc_dilithium_init(key) != 0) { wolfCLU_LogError("Failed to initialize Dilithium Key.\nRET: %d", ret); #ifdef WOLFSSL_SMALL_STACK XFREE(key, HEAP_HINT, DYNAMIC_TYPE_TMP_BUFFER); #endif - return WOLFCLU_FAILURE; + ret = WOLFCLU_FAILURE; } - XMEMSET(key, 0, sizeof(dilithium_key)); - if (wc_InitRng(&rng) != 0) { - wolfCLU_LogError("Failed to initialize rng.\nRET: %d", ret); - wc_dilithium_free(key); - #ifdef WOLFSSL_SMALL_STACK - XFREE(key, HEAP_HINT, DYNAMIC_TYPE_TMP_BUFFER); - #endif - return WOLFCLU_FAILURE; + /* initialize RNG */ + if (ret == 0) { + ret = wc_InitRng(&rng); + if (ret != 0) { + wolfCLU_LogError("Failed to initialize rng.\nRET: %d", ret); + } } /* open and read private key */ - privKeyFile = XFOPEN(privKey, "rb"); - if (privKeyFile == NULL) { - wolfCLU_LogError("Faild to open Private key FILE."); - wc_FreeRng(&rng); - wc_dilithium_free(key); - #ifdef WOLFSSL_SMALL_STACK - XFREE(key, HEAP_HINT, DYNAMIC_TYPE_TMP_BUFFER); - #endif - return ret; + if (ret == 0) { + privKeyFile = XFOPEN(privKey, "rb"); + if (privKeyFile == NULL) { + wolfCLU_LogError("Faild to open Private key FILE."); + ret = BAD_FUNC_ARG; + } } - - XFSEEK(privKeyFile, 0, SEEK_END); - privFileSz = (int)XFTELL(privKeyFile); - privBuf = (byte*)XMALLOC(privFileSz+1, HEAP_HINT, DYNAMIC_TYPE_TMP_BUFFER); - if (privBuf == NULL) { - XFCLOSE(privKeyFile); - wc_FreeRng(&rng); - wc_dilithium_free(key); - #ifdef WOLFSSL_SMALL_STACK - XFREE(key, HEAP_HINT, DYNAMIC_TYPE_TMP_BUFFER); - #endif - return MEMORY_E; + if (ret == 0) { + XFSEEK(privKeyFile, 0, SEEK_END); + privFileSz = (int)XFTELL(privKeyFile); + privBuf = (byte*)XMALLOC(privFileSz+1, HEAP_HINT, + DYNAMIC_TYPE_TMP_BUFFER); + if (privBuf == NULL) { + ret = MEMORY_E; + } } - - XMEMSET(privBuf, 0, privFileSz+1); - privBufSz = privFileSz; - XFSEEK(privKeyFile, 0, SEEK_SET); - if (XFREAD(privBuf, 1, privBufSz, privKeyFile) != privBufSz) { - wolfCLU_Log(WOLFCLU_L0, "incorecct size: %d", privFileSz); - XFREE(privBuf, HEAP_HINT, DYNAMIC_TYPE_TMP_BUFFER); - wc_FreeRng(&rng); - wc_dilithium_free(key); - #ifdef WOLFSSL_SMALL_STACK - XFREE(key, HEAP_HINT, DYNAMIC_TYPE_TMP_BUFFER); - #endif - return ret; + if (ret == 0) { + XMEMSET(privBuf, 0, privFileSz+1); + privBufSz = privFileSz; + if (XFSEEK(privKeyFile, 0, SEEK_SET) != 0 || + (int)XFREAD(privBuf, 1, privFileSz, privKeyFile) != privFileSz) { + wolfCLU_Log(WOLFCLU_L0, "incorecct size: %d", privFileSz); + ret = WOLFCLU_FATAL_ERROR; + } } - XFCLOSE(privKeyFile); /* convert PEM to DER if necessary */ - if (inForm == PEM_FORM) { + if (inForm == PEM_FORM && ret == 0) { ret = wolfCLU_KeyPemToDer(&privBuf, privFileSz, 0); if (ret < 0) { wolfCLU_LogError("Failed to convert PEM to DER.\nRET: %d", ret); - XFREE(privBuf, HEAP_HINT, DYNAMIC_TYPE_TMP_BUFFER); - wc_FreeRng(&rng); - wc_dilithium_free(key); - #ifdef WOLFSSL_SMALL_STACK - XFREE(key, HEAP_HINT, DYNAMIC_TYPE_TMP_BUFFER); - #endif - return ret; } else { - privFileSz = ret; + /* update privBuf and privFileSz with the converted DER data */ + privBufSz = privFileSz = ret; + ret = 0; } } - /* retrieving private key and staoring in the Dilithium key */ - ret = wc_Dilithium_PrivateKeyDecode(privBuf, &index, key, privBufSz); - XFREE(privBuf, HEAP_HINT, DYNAMIC_TYPE_TMP_BUFFER); - if (ret != 0) { - wolfCLU_LogError("Failed to decode private key.\nRET: %d", ret); - wc_FreeRng(&rng); - wc_dilithium_free(key); - #ifdef WOLFSSL_SMALL_STACK - XFREE(key, HEAP_HINT, DYNAMIC_TYPE_TMP_BUFFER); - #endif - return ret; + /* retrieving private key and storing in the Dilithium key */ + if (ret == 0) { + ret = wc_Dilithium_PrivateKeyDecode(privBuf, &index, key, privBufSz); + if (ret != 0) { + wolfCLU_LogError("Failed to decode private key.\nRET: %d", ret); + } } - /* malloc signature buffer */ - outBufSz = wc_dilithium_sig_size(key); - outBuf = (byte*)XMALLOC(outBufSz, HEAP_HINT, DYNAMIC_TYPE_TMP_BUFFER); - if (outBuf == NULL) { - wc_FreeRng(&rng); - wc_dilithium_free(key); - #ifdef WOLFSSL_SMALL_STACK - XFREE(key, HEAP_HINT, DYNAMIC_TYPE_TMP_BUFFER); - #endif - return MEMORY_E; + if (ret == 0) { + outBufSz = wc_dilithium_sig_size(key); + outBuf = (byte*)XMALLOC(outBufSz, HEAP_HINT, DYNAMIC_TYPE_TMP_BUFFER); + if (outBuf == NULL) { + wolfCLU_LogError("Failed to allocate signature" + " buffer.\nRET: %d", ret); + ret = MEMORY_E; + } } - /* sign the message usign Dilithium private key. Note that the context is * empty. This is for interoperability. */ - ret = wc_dilithium_sign_ctx_msg(NULL, 0, data, dataSz, outBuf, &outBufSz, - key, &rng); - if (ret != 0) { - wolfCLU_LogError("Failed to sign data with Dilithium private key.\nRET: %d", ret); - XFREE(outBuf, HEAP_HINT, DYNAMIC_TYPE_TMP_BUFFER); - wc_FreeRng(&rng); - wc_dilithium_free(key); - #ifdef WOLFSSL_SMALL_STACK - XFREE(key, HEAP_HINT, DYNAMIC_TYPE_TMP_BUFFER); - #endif - return ret; + if (ret == 0) { + ret = wc_dilithium_sign_ctx_msg(NULL, 0, data, dataSz, outBuf, + &outBufSz, key, &rng); + if (ret != 0) { + wolfCLU_LogError("Failed to sign data with" + " Dilithium private key.\nRET: %d", ret); + } } - else { + + if (ret == 0) { XFILE outFile; outFile = XFOPEN(out, "wb"); + if (outFile == NULL) { wolfCLU_LogError("Failed to open output file %s", out); - XFREE(outBuf, HEAP_HINT, DYNAMIC_TYPE_TMP_BUFFER); - wc_FreeRng(&rng); - wc_dilithium_free(key); - #ifdef WOLFSSL_SMALL_STACK - XFREE(key, HEAP_HINT, DYNAMIC_TYPE_TMP_BUFFER); - #endif - return BAD_FUNC_ARG; - } - XFWRITE(outBuf, 1, outBufSz, outFile); - XFCLOSE(outFile); + ret = BAD_FUNC_ARG; + } else { + XFWRITE(outBuf, 1, outBufSz, outFile); + XFCLOSE(outFile); + } } - XFREE(outBuf, HEAP_HINT, DYNAMIC_TYPE_TMP_BUFFER); - wc_FreeRng(&rng); - wc_dilithium_free(key); + /* cleanup allocated resources */ + if (privKeyFile != NULL) + XFCLOSE(privKeyFile); + if (privBuf != NULL) { + XFREE(privBuf, HEAP_HINT, DYNAMIC_TYPE_TMP_BUFFER); + } + + if (outBuf != NULL) { + XFREE(outBuf, HEAP_HINT, DYNAMIC_TYPE_TMP_BUFFER); + } + + wc_dilithium_free(key); + wc_FreeRng(&rng); #ifdef WOLFSSL_SMALL_STACK XFREE(key, HEAP_HINT, DYNAMIC_TYPE_TMP_BUFFER); #endif - return WOLFCLU_SUCCESS; + /* expected ret == WOLFCLU_SUCCESS */ + return (ret >= 0) ? WOLFCLU_SUCCESS : ret; #else (void)data; (void)out; (void)dataSz; - (void) privKey; + (void)privKey; (void)inForm; return NOT_COMPILED_IN; diff --git a/tests/genkey_sign_ver/genkey-sign-ver-test.sh b/tests/genkey_sign_ver/genkey-sign-ver-test.sh index 53239bd1..46c3efb6 100755 --- a/tests/genkey_sign_ver/genkey-sign-ver-test.sh +++ b/tests/genkey_sign_ver/genkey-sign-ver-test.sh @@ -50,6 +50,9 @@ cleanup_genkey_sign_ver(){ rm rsa-sigout.private_result rm rsa-sigout.public_result rm mldsa-signed.sig + rm -f mldsa_bad_pubkey.sig + rm -f mldsa_bad_corrupt.sig + rm -f mldsakey_corrupt.priv rm xmss-signed.sig # rm xmssmt-signed.sig rm sign-this.txt @@ -263,6 +266,26 @@ RESULT=$? RESULT=$? [ $RESULT -eq 0 ] && \ printf '%s\n' "dilithium sign to invalid path should have failed" && exit 99 + +# Dilithium sign with public key as inkey must fail gracefully +./wolfssl -dilithium -sign -inkey mldsakey.pub -inform der \ + -in sign-this.txt -out mldsa_bad_pubkey.sig +RESULT=$? +[ $RESULT -eq 0 ] && \ + printf '%s\n' "dilithium sign with public key should have failed" && exit 99 +[ -f mldsa_bad_pubkey.sig ] && \ + printf '%s\n' "dilithium sign with public key: output file must not be created" && exit 99 + +# Dilithium sign with corrupted private key must fail gracefully +printf '%s' "INVALID KEY DATA" > mldsakey_corrupt.priv +./wolfssl -dilithium -sign -inkey mldsakey_corrupt.priv -inform der \ + -in sign-this.txt -out mldsa_bad_corrupt.sig +RESULT=$? +[ $RESULT -eq 0 ] && \ + printf '%s\n' "dilithium sign with corrupted key should have failed" && exit 99 +[ -f mldsa_bad_corrupt.sig ] && \ + printf '%s\n' "dilithium sign with corrupted key: output file must not be created" && exit 99 +rm -f mldsakey_corrupt.priv fi # Check if xmss is availabe From 620f391928355ed08fba59375796ef0da04d56c1 Mon Sep 17 00:00:00 2001 From: Hideki Miyazaki Date: Thu, 9 Apr 2026 08:41:40 +0900 Subject: [PATCH 2/2] addressed Copilot review comments --- src/sign-verify/clu_sign.c | 28 ++++++++++++++++++---------- 1 file changed, 18 insertions(+), 10 deletions(-) diff --git a/src/sign-verify/clu_sign.c b/src/sign-verify/clu_sign.c index 03371a7a..8fecda06 100644 --- a/src/sign-verify/clu_sign.c +++ b/src/sign-verify/clu_sign.c @@ -647,10 +647,6 @@ int wolfCLU_sign_data_dilithium (byte* data, char* out, word32 dataSz, char* pri /* init the dilithium key */ if (wc_dilithium_init(key) != 0) { wolfCLU_LogError("Failed to initialize Dilithium Key.\nRET: %d", ret); - #ifdef WOLFSSL_SMALL_STACK - XFREE(key, HEAP_HINT, DYNAMIC_TYPE_TMP_BUFFER); - #endif - ret = WOLFCLU_FAILURE; } /* initialize RNG */ @@ -670,12 +666,24 @@ int wolfCLU_sign_data_dilithium (byte* data, char* out, word32 dataSz, char* pri } } if (ret == 0) { - XFSEEK(privKeyFile, 0, SEEK_END); - privFileSz = (int)XFTELL(privKeyFile); - privBuf = (byte*)XMALLOC(privFileSz+1, HEAP_HINT, + if (XFSEEK(privKeyFile, 0, SEEK_END) != 0) { + wolfCLU_LogError("Failed to seek to end of file."); + ret = WOLFCLU_FATAL_ERROR; + } + if (ret == 0) { + privFileSz = (int)XFTELL(privKeyFile); + if (privFileSz > 0 && + privFileSz <= DILITHIUM_MAX_BOTH_KEY_PEM_SIZE) { + privBuf = (byte*)XMALLOC(privFileSz+1, HEAP_HINT, DYNAMIC_TYPE_TMP_BUFFER); - if (privBuf == NULL) { - ret = MEMORY_E; + if (privBuf == NULL) { + ret = MEMORY_E; + } + } else { + wolfCLU_LogError("Incorrect private key file size: %d", + privFileSz); + ret = WOLFCLU_FATAL_ERROR; + } } } if (ret == 0) { @@ -683,7 +691,7 @@ int wolfCLU_sign_data_dilithium (byte* data, char* out, word32 dataSz, char* pri privBufSz = privFileSz; if (XFSEEK(privKeyFile, 0, SEEK_SET) != 0 || (int)XFREAD(privBuf, 1, privFileSz, privKeyFile) != privFileSz) { - wolfCLU_Log(WOLFCLU_L0, "incorecct size: %d", privFileSz); + wolfCLU_LogError("Incorrect private key file size: %d", privFileSz); ret = WOLFCLU_FATAL_ERROR; } }