Skip to content

F-1899 : fix potential heap buffer over-read #220

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
miyazakh wants to merge 2 commits into
base: main
Choose a base branch
from
+121 −109
Open

F-1899 : fix potential heap buffer over-read #220

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4328349
fix f-1899 potential heap buffer over-read
miyazakh Apr 8, 2026
620f391
addressed Copilot review comments
miyazakh Apr 8, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
207 changes: 98 additions & 109 deletions src/sign-verify/clu_sign.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -639,153 +641,140 @@ 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) {
Copy link
Copy Markdown
Contributor

@dgarske dgarske Apr 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟠 [High] wc_dilithium_init failure not captured in ret — execution continues on error
🚫 BLOCK bug

The return value of wc_dilithium_init(key) is checked for the log message but never stored in ret. Since ret remains 0, every subsequent if (ret == 0) guard evaluates to true and the function continues operating on an uninitialized key. This is a regression: the old code returned WOLFCLU_FAILURE immediately on init failure. Compare with the equivalent wolfCLU_sign_data_ed25519 at line 486 which correctly does ret = wc_ed25519_init(&key);. Additionally, the log message prints ret (which is 0) instead of the actual error code from wc_dilithium_init.

Suggestion:

Suggested change
if (wc_dilithium_init(key) != 0) {
/* init the dilithium key */
ret = wc_dilithium_init(key);
if (ret != 0) {
wolfCLU_LogError("Failed to initialize Dilithium Key.\nRET: %d", ret);
}

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;
}
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);
Comment on lines 647 to +654
Copy link

Copilot AI Apr 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wc_dilithium_init(key) errors aren’t propagated correctly: the return value is not stored in ret (so the log prints RET: 0), and the failure path sets ret = WOLFCLU_FAILURE (which is 0), so the function continues as “success”. Additionally, in WOLFSSL_SMALL_STACK builds this block frees key, but later cleanup still calls wc_dilithium_free(key) and XFREE(key) again, which is a use-after-free/double-free. Capture the init return into ret (keep it non-zero/negative), and use a single cleanup path that frees key exactly once.

Copilot uses AI. Check for mistakes.
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;
}

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) {
privKeyFile = XFOPEN(privKey, "rb");
if (privKeyFile == NULL) {
wolfCLU_LogError("Faild to open Private key FILE.");
ret = BAD_FUNC_ARG;
}
}

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) {
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;
}
} else {
wolfCLU_LogError("Incorrect private key file size: %d",
privFileSz);
ret = WOLFCLU_FATAL_ERROR;
}
}
Comment on lines 667 to +687
Copy link

Copilot AI Apr 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Spelling/typos in user-facing log messages: "Faild to open Private key FILE." and "incorecct size". These make debugging harder and look unpolished; please correct the strings (e.g., "Failed" / "incorrect").

Copilot uses AI. Check for mistakes.
}
if (ret == 0) {
XMEMSET(privBuf, 0, privFileSz+1);
privBufSz = privFileSz;
if (XFSEEK(privKeyFile, 0, SEEK_SET) != 0 ||
(int)XFREAD(privBuf, 1, privFileSz, privKeyFile) != privFileSz) {
Comment on lines +669 to +693
Copy link

Copilot AI Apr 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

File size handling needs validation: privFileSz is taken from XFTELL() without checking XFSEEK/XFTELL results or ensuring privFileSz > 0. If XFTELL() returns -1 (or an unexpectedly large value), privFileSz+1 can underflow/overflow the allocation and XMEMSET/XFREAD will operate on an invalid size, reintroducing the potential heap buffer over-read. Check XFSEEK return, validate XFTELL >= 0 and within a sane upper bound, and only then allocate/read (using an unsigned type like size_t for sizes).

Suggested change
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;
}
} else {
wolfCLU_LogError("Incorrect private key file size: %d",
privFileSz);
ret = WOLFCLU_FATAL_ERROR;
}
}
}
if (ret == 0) {
XMEMSET(privBuf, 0, privFileSz+1);
privBufSz = privFileSz;
if (XFSEEK(privKeyFile, 0, SEEK_SET) != 0 ||
(int)XFREAD(privBuf, 1, privFileSz, privKeyFile) != privFileSz) {
long privFilePos;
size_t privAllocSz;
if (XFSEEK(privKeyFile, 0, SEEK_END) != 0) {
wolfCLU_LogError("Failed to seek to end of file.");
ret = WOLFCLU_FATAL_ERROR;
}
if (ret == 0) {
privFilePos = XFTELL(privKeyFile);
if (privFilePos <= 0 ||
privFilePos > DILITHIUM_MAX_BOTH_KEY_PEM_SIZE) {
wolfCLU_LogError("Incorrect private key file size: %ld",
privFilePos);
ret = WOLFCLU_FATAL_ERROR;
}
else {
privFileSz = (int)privFilePos;
privAllocSz = (size_t)privFileSz + 1U;
privBuf = (byte*)XMALLOC(privAllocSz, HEAP_HINT,
DYNAMIC_TYPE_TMP_BUFFER);
if (privBuf == NULL) {
ret = MEMORY_E;
}
}
}
}
if (ret == 0) {
size_t privReadSz = (size_t)privFileSz;
XMEMSET(privBuf, 0, privReadSz + 1U);
privBufSz = privFileSz;
if (XFSEEK(privKeyFile, 0, SEEK_SET) != 0 ||
XFREAD(privBuf, 1, privReadSz, privKeyFile) != privReadSz) {

Copilot uses AI. Check for mistakes.
wolfCLU_LogError("Incorrect private key file 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"
Copy link
Copy Markdown
Contributor

@dgarske dgarske Apr 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 [Medium] Error log for allocation failure prints stale ret value (always 0)
💡 SUGGEST bug

When XMALLOC for outBuf returns NULL, the log message "Failed to allocate signature buffer.\nRET: %d" formats ret, which is still 0 at that point (the ret = MEMORY_E assignment happens on the next line). The printed error code is misleading.

Suggestion:

Suggested change
wolfCLU_LogError("Failed to allocate signature"
if (outBuf == NULL) {
ret = MEMORY_E;
wolfCLU_LogError("Failed to allocate signature"
" buffer.\nRET: %d", ret);
}

" 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);
}

Comment on lines +763 to 764
Copy link

Copilot AI Apr 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The final return return (ret >= 0) ? WOLFCLU_SUCCESS : ret; can mask failures as success when ret is 0 (e.g., WOLFCLU_FAILURE is defined as 0). This can cause the CLI to exit successfully even when signing fails. Consider returning ret directly (mapping only ret == 0 to WOLFCLU_SUCCESS if you strictly use 0-as-success), or ensure this function never uses WOLFCLU_FAILURE (0) for error cases.

Copilot uses AI. Check for mistakes.
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;
Expand Down
23 changes: 23 additions & 0 deletions tests/genkey_sign_ver/genkey-sign-ver-test.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down
Loading