F-1899 : fix potential heap buffer over-read#220
F-1899 : fix potential heap buffer over-read#220miyazakh wants to merge 4 commits intowolfSSL:mainfrom
Conversation
There was a problem hiding this comment.
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_dilithiumto 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.
| 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; |
There was a problem hiding this comment.
In the wc_dilithium_init() failure path, ret is set to WOLFCLU_FAILURE (which is 0). This function uses ret==0 as the “no error” sentinel, so the code will continue as if initialization succeeded and can end up returning overall success. Capture the init failure into ret (e.g., ret = wc_dilithium_init(...)) and ensure the error value stays non-zero/negative (or return immediately) so subsequent steps and the final return don’t report success.
| XFSEEK(privKeyFile, 0, SEEK_END); | ||
| privFileSz = (int)XFTELL(privKeyFile); | ||
| privBuf = (byte*)XMALLOC(privFileSz+1, HEAP_HINT, | ||
| DYNAMIC_TYPE_TMP_BUFFER); |
There was a problem hiding this comment.
privFileSz is taken directly from XFTELL() and then used in privFileSz+1 allocation without validating the seek/tell results. If XFTELL() fails (e.g., returns -1) this can underflow/overflow the allocation size and lead to invalid reads. Check XFSEEK/XFTELL return values and validate privFileSz > 0 (and within a reasonable upper bound) before allocating/reading.
| /* verify the hash with Ecc public key */ | ||
| if (ret == 0) { | ||
| ret = wc_ecc_verify_hash(sig, sigSz, hashBuf, digestSz, | ||
| &stat, &key); |
There was a problem hiding this comment.
wc_ecc_verify_hash() returns 0 even when the signature is invalid (it reports validity via the stat out-param). Right now this call’s result is used as the only success indicator, so an invalid signature can still propagate as overall success. After this call, treat stat != 1 as a verification failure and set a non-success return code so the CLI exits non-zero on invalid signatures.
| &stat, &key); | |
| &stat, &key); | |
| if (ret == 0 && stat != 1) { | |
| wolfCLU_LogError("Invalid Signature."); | |
| ret = -1; | |
| } |
| /* hash the input data before signing -- ECDSA signs a digest, not raw | ||
| * data. Select a curve-appropriate hash paired with the curve | ||
| * strength; ECDSA will truncate the digest as needed. */ | ||
| keySz = wc_ecc_size(&key); | ||
| if (keySz <= 32) { | ||
| hashType = WC_HASH_TYPE_SHA256; | ||
| } |
There was a problem hiding this comment.
This PR description focuses on Dilithium heap over-read/refactor, but this hunk also changes ECC signing semantics by hashing the input before ECDSA signing (previously the input bytes were passed directly to wc_ecc_sign_hash). Please call out this behavioral change in the PR description/changelog, since it can affect interoperability with signatures generated/verified by previous versions.
Fix potential heap buffer over-read
Refactor
wolfCLU_sign_data_dilithiumAdd test coverage
Depend on : #219