F-1899 : fix potential heap buffer over-read by miyazakh · Pull Request #220 · wolfSSL/wolfCLU

miyazakh · 2026-04-08T07:24:55Z

Fix potential heap buffer over-read
Refactor wolfCLU_sign_data_dilithium
Add test coverage

Depend on : #219

Copilot

Pull request overview

This PR addresses robustness and safety in signing/verification workflows, primarily targeting Dilithium signing error handling to prevent potential heap misuse, and extends regression coverage via shell tests.

Changes:

Refactors wolfCLU_sign_data_dilithium to improve cleanup/error-path handling and avoid unsafe buffer usage patterns.
Updates ECC sign/verify to hash input data before calling ECDSA primitives.
Adds Dilithium negative-path regression tests to ensure failures don’t create output files.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.

File	Description
tests/genkey_sign_ver/genkey-sign-ver-test.sh	Adds Dilithium failure-mode tests (invalid output path, wrong key type, corrupted key) and cleans up new artifacts.
src/x509/clu_x509_sign.c	Adjusts `wolfSSL_BIO_get_fp` calls (casts) while loading key material for Chimera cert signing.
src/sign-verify/clu_verify.c	Changes ECC verification to hash input data prior to `wc_ecc_verify_hash`.
src/sign-verify/clu_sign.c	Changes ECC signing to hash input data prior to `wc_ecc_sign_hash`; refactors Dilithium signing for safer handling.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

src/sign-verify/clu_sign.c

src/sign-verify/clu_verify.c

src/sign-verify/clu_sign.c

add test coverage

Copilot

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-04-08T23:46:08Z

src/sign-verify/clu_sign.c

    /* 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;
    }
-    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);


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 · 2026-04-08T23:46:08Z

src/sign-verify/clu_sign.c

+        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) {


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 · 2026-04-08T23:46:09Z

src/sign-verify/clu_sign.c

+    }



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 · 2026-04-08T23:46:09Z

src/sign-verify/clu_sign.c

    }
-
-    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;
+            }
+        }


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").

dgarske

🐺 Skoll Code Review

Overall recommendation: REQUEST_CHANGES
Findings: 3 total — 2 posted, 1 skipped

Posted findings

[High] wc_dilithium_init failure not captured in ret — execution continues on error — src/sign-verify/clu_sign.c:648-650
[Medium] Error log for allocation failure prints stale ret value (always 0) — src/sign-verify/clu_sign.c:724-725

Skipped findings

[Info] Extra blank line where XMEMSET was removed

Review generated by Skoll via openclaw

dgarske · 2026-04-10T17:10:23Z

src/sign-verify/clu_sign.c

+    XMEMSET(key, 0, sizeof(dilithium_key));
+
    /* init the dilithium key */
    if (wc_dilithium_init(key) != 0) {


🟠 [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);

}

dgarske · 2026-04-10T17:10:23Z

src/sign-verify/clu_sign.c

+        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"


🟡 [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);

}

Copilot AI review requested due to automatic review settings April 8, 2026 07:24

miyazakh self-assigned this Apr 8, 2026

Copilot started reviewing on behalf of miyazakh April 8, 2026 07:25 View session

Copilot AI reviewed Apr 8, 2026

View reviewed changes

src/sign-verify/clu_sign.c Outdated Show resolved Hide resolved

src/sign-verify/clu_sign.c Outdated Show resolved Hide resolved

src/sign-verify/clu_verify.c Show resolved Hide resolved

src/sign-verify/clu_sign.c Show resolved Hide resolved

fix f-1899 potential heap buffer over-read

4328349

add test coverage

miyazakh force-pushed the f-1899_heapbuffer_over-read branch from dc2f472 to 4328349 Compare April 8, 2026 23:25

miyazakh marked this pull request as ready for review April 8, 2026 23:41

Copilot AI review requested due to automatic review settings April 8, 2026 23:41

addressed Copilot review comments

620f391

Copilot started reviewing on behalf of miyazakh April 8, 2026 23:41 View session

Copilot AI reviewed Apr 8, 2026

View reviewed changes

miyazakh assigned wolfSSL-Bot and unassigned miyazakh Apr 9, 2026

dgarske requested changes Apr 10, 2026

View reviewed changes

dgarske assigned miyazakh and unassigned wolfSSL-Bot Apr 10, 2026

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

F-1899 : fix potential heap buffer over-read#220
miyazakh wants to merge 2 commits intowolfSSL:mainfrom
miyazakh:f-1899_heapbuffer_over-read

miyazakh commented Apr 8, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Apr 8, 2026

Uh oh!

Copilot AI Apr 8, 2026

Uh oh!

Copilot AI Apr 8, 2026

Uh oh!

Copilot AI Apr 8, 2026

Uh oh!

dgarske left a comment

Uh oh!

dgarske Apr 10, 2026

Uh oh!

dgarske Apr 10, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

miyazakh commented Apr 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Apr 8, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 8, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 8, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 8, 2026

Choose a reason for hiding this comment

Uh oh!

dgarske left a comment

Choose a reason for hiding this comment

🐺 Skoll Code Review

Posted findings

Uh oh!

dgarske Apr 10, 2026

Choose a reason for hiding this comment

Uh oh!

dgarske Apr 10, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

miyazakh commented Apr 8, 2026 •

edited

Loading