Fix wolfCLU Fenrir

[aidangarske]

Description

F-570 Allocate +1 for null terminator in IV and key string copies
F-572 Move XMEMSET before wc_dilithium_init in sign path
F-573 Free data and hash buffers on all return paths in verify
F-574 Add ForceZero on password buffer in PKCS12
F-575 Add ForceZero on password and keyBuffer in PKCS8
F-645 Use wolfCLU_checkOutform for RSA -outform option
F-646 Add #else error handling for disabled hash algorithms in switch
F-647 Return WOLFCLU_SUCCESS from wolfCLU_setAttributes
F-648 Check wc_RsaPrivateKeyDecode return value directly
F-649 Check wc_EccPrivateKeyDecode return value directly
F-657 Map sha224 to WC_HASH_TYPE_SHA224, use else-if chain
F-658 Move XMEMSET before wc_dilithium_init in verify path
F-659 Heap-allocate large temp buffers in GenChimeraCertSign
F-660 Use XSTRCMP for exact match in config parsing
F-732 Map dgst -out to WOLFCLU_OUTFILE
F-733 Write actual signature length in ED25519 sign output
F-734 Write actual DER size in ECC private key output
F-735 Change || to && in XMSS/XMSSMT argv bounds check
F-736 Guard base64 allocation with success check in rand
F-740 Use wolfCLU_checkOutform for CRL -outform option
F-741 Remove free of borrowed EVP_PKEY from X509_get0_pubkey
F-742 Add ret+1 < argc guard before argv access (3 files)
F-1108 Return error code on file open failure in dilithium sign
F-1109 Break on BN_rand failure, guard serial number set
F-1484 Free data buffer on read failure in sign
F-1485 Add NULL check on X509_NAME_oneline return
F-1486 Refactor RSA certgen to goto cleanup for resource management
F-1487 Free keyBuf on read failure in RSA certgen
F-1488 Free keyBuf on read failure in ED25519 certgen
F-1489 Add ForceZero before freeing signer key in OCSP responder

Test coverage

Tests updated and what they cover

tests/pkey/rsa-test.sh

• F-645 - verify -outform error message references "outform"

tests/x509/CRL-verify-test.sh

• F-740 - verify -outform error message references "outform"

tests/encrypt/enc-test.sh

• F-570 - encrypt with explicit hex key/IV succeeds

tests/genkey_sign_ver/genkey-sign-ver-test.sh

• F-733 - ED25519 sig file is exactly 64 bytes
• F-734 - ECC DER key file size is reasonable, not buffer-sized
• F-742 - missing -inkey value fails gracefully, not segfault
• F-735 - missing -height value fails gracefully (if XMSS compiled in)
• F-648/F-649 — sign with empty key file returns error

tests/hash/hash-test.sh

• F-742 - missing -in value fails gracefully, not segfault

tests/bench/bench-test.sh

• F-742 - missing -time value fails gracefully, not segfault

tests/dgst/dgst-test.sh

• F-732 - dgst -out creates output file and round-trips with -signature

tests/x509/x509-process-test.sh

• F-741 - x509 -modulus -noout does not crash

tests/x509/x509-req-test.sh

• F-657 - SHA-224 cert signature algorithm check
• F-660 - abbreviated keyUsage "d" does not match digitalSignature
• F-647 - req with challengePassword attribute succeeds

Fixed fsan leak for x509 that was being leaked and suppressed by command line tests
SHA-224 test assertion fails if not found now
properly error on dillithium return
Add missing force zeros for keybuf
fix ed25519 certgen cleanup on error paths

Other tests not covered by test validated by internal testing suite + code review since test paths where not hit with simple command line code tests in make check

[aidangarske]

Skoll Code Review

Overall recommendation: REQUEST_CHANGES
Findings: 11 total — 11 posted, 0 skipped

Posted findings

• [High] C90 mixed declarations and code in ed25519 certgen — src/certgen/clu_certgen_ed25519.c:71
• [High] ECC sign/verify hardcodes SHA-256 hash — backward-incompatible behavioral change — src/sign-verify/clu_sign.c:392-404
• [High] X509_get0_pubkey result freed despite 'get0' borrowed-pointer convention — src/x509/clu_cert_setup.c:789
• [Medium] Modulus code continues past error when num is NULL — src/x509/clu_cert_setup.c:762-782
• [Medium] Inconsistent success return value between RSA and ED25519 certgen — src/certgen/clu_certgen_rsa.c:252
• [Medium] XMSS/XMSSMT argv bounds check uses NULL instead of argc comparison — src/genkey/clu_genkey_setup.c:454
• [Medium] ECC verify also hardcodes SHA-256 hash — must match sign change — src/sign-verify/clu_verify.c:582-593
• [Medium] No test coverage for ECC sign/verify behavioral change — tests/genkey_sign_ver/genkey-sign-ver-test.sh
• [Low] wolfCLU_StringToHashType md5 check not chained with else-if — src/x509/clu_x509_sign.c:982-1000
• [Low] Unused outBuf allocation in ECC verify path — src/sign-verify/clu_verify.c:565-580
• [Low] Stack-allocated hashBuf/hashDigest not zeroed after use — src/sign-verify/clu_sign.c:394

Review generated by Skoll via openclaw

[aidangarske]

Skoll Code Review

Overall recommendation: REQUEST_CHANGES
Findings: 13 total — 10 posted, 3 skipped
7 finding(s) posted as inline comments (see file-level comments below)

Posted findings

• [Critical] ForceZero on keyBuffer corrupts BIO data source before it is read — src/pkcs/clu_pkcs8.c:190
• [High] C89: Declaration after statements in ed25519 certgen — src/certgen/clu_certgen_ed25519.c:71
• [High] BN_to_ASN1_INTEGER NULL not checked before wolfSSL_X509_set_serialNumber — src/x509/clu_x509_sign.c:1340-1346
• [Medium] ECC sign/verify hardcodes SHA-256 hash, ignoring user-selected algorithm — src/sign-verify/clu_sign.c:395
• [Medium] wolfCLU_StringToHashType: md5 not chained into else-if — src/x509/clu_x509_sign.c:982-1000
• [Medium] Missing bounds check on argv[ret+1] for Dilithium/ML-DSA -level argument — src/genkey/clu_genkey_setup.c:292-293
• [Medium] Missing bounds check on argv[ret+1] for -outform in genkey_setup — src/genkey/clu_genkey_setup.c:78-79
• [Medium] Missing ForceZero on private key buffer in sign functions — src/sign-verify/clu_sign.c:285-286
• [Medium] ECC sign/verify behavior change is breaking but not documented — src/sign-verify/clu_sign.c:392-404
• [Medium] No test coverage for ECC sign/verify SHA-256 hashing change — tests/genkey_sign_ver/genkey-sign-ver-test.sh

Skipped findings

• [Low] RSA certgen uses literal 1 instead of WOLFCLU_SUCCESS
• [Low] WOLFCLU_OUTFILE and WOLFCLU_INFILE both set sigFile without comment
• [Low] RSA certgen error paths use -1 instead of named error constants

Review generated by Skoll

[aidangarske]

🚨 [Critical] ForceZero on keyBuffer corrupts BIO data source before it is read

wolfCLU_ForceZero(keyBuffer, sizeof(keyBuffer)) is called at line 190, immediately after creating a BIO from keyBuffer via wolfSSL_BIO_new_mem_buf(keyBuffer, keyLen) at line 171. BIO_new_mem_buf creates a read-only BIO that references (does not copy) the provided buffer. The BIO is then read at lines 193-216 via wolfSSL_PEM_read_bio_PrivateKey(bioIn, ...) and wolfSSL_d2i_PrivateKey_bio(bioIn, ...), but by that point the underlying data has been zeroed. This will cause all PKCS8 stdin key parsing to fail, as the BIO reads all zeros instead of key data. Additionally, keyBuffer is a stack variable scoped to the if block (lines 157-191), so the BIO references memory that is out of scope when it is later read.

171:            bioIn = wolfSSL_BIO_new_mem_buf(keyBuffer, keyLen);
...
190:        wolfCLU_ForceZero(keyBuffer, sizeof(keyBuffer));
191:    }
192:
193:    if (ret == WOLFCLU_SUCCESS && pass == NULL) {
194:        if (inForm == PEM_FORM) {
195:            pkey = wolfSSL_PEM_read_bio_PrivateKey(bioIn, NULL, NULL, NULL);

Recommendation: Move the ForceZero to after the last use of bioIn, or restructure to copy the key data to heap-allocated memory that outlives the scope.

[aidangarske]

⚠️ [High] C89: Declaration after statements in ed25519 certgen

The variables int keyInit = 0, rngInit = 0; are declared at line 71 after executable statements (XFOPEN, XFSEEK, XFREAD, XFCLOSE on lines 50-69). wolfSSL projects target C89/C90 for portability to embedded systems. C89 requires all variable declarations to appear at the beginning of a block before any statements. This will fail to compile with -std=c89, -pedantic, or on older compilers used in embedded environments.

69:    XFCLOSE(keyFile);
70:
71:    int keyInit = 0, rngInit = 0;
72:
73:    ret = wc_ed25519_init(&key);

Recommendation: Move int keyInit = 0, rngInit = 0; to the top of the function body, before any executable statements.

[aidangarske]

⚠️ [High] BN_to_ASN1_INTEGER NULL not checked before wolfSSL_X509_set_serialNumber

In the random serial number generation do-while loop (lines 1326-1343), when (numBits % 8) != 7, wolfSSL_BN_to_ASN1_INTEGER(bn, NULL) is called to set s. If this function returns NULL (e.g., due to OOM), s remains NULL (initialized at line 1310). The loop exits because (numBits % 8) != 7, and ret is still WOLFCLU_SUCCESS (since BN_rand succeeded). At line 1345-1346, wolfSSL_X509_set_serialNumber(x509, NULL) is called, which could dereference the NULL pointer and crash.

1339:                if ((numBits % 8) != 7) {
1340:                    s = wolfSSL_BN_to_ASN1_INTEGER(bn, NULL);
1341:                }
1342:                wolfSSL_BN_free(bn);
1343:            } while ((numBits % 8) == 7);
1344:        }
1345:        if (ret == WOLFCLU_SUCCESS) {
1346:            wolfSSL_X509_set_serialNumber(x509, s);
1347:        }

Recommendation: Add a NULL check on s after the do-while loop exits successfully, before calling wolfSSL_X509_set_serialNumber.

[aidangarske]

🔶 [Medium] ECC sign/verify hardcodes SHA-256 hash, ignoring user-selected algorithm

The new ECC sign code unconditionally uses wc_Sha256Hash() (line 395) regardless of what hash algorithm the user selected via -sha384, -sha512, etc. The same SHA-256 hardcoding appears in wolfCLU_verify_signature_ecc at clu_verify.c:585. While the sign and verify paths are now internally consistent (both hash with SHA-256), this means: (1) the -sha384 and -sha512 flags are silently ignored for ECC operations, and (2) this is a breaking behavioral change — existing signatures created with the old code (which signed raw data) will no longer verify.

392:        /* wc_ecc_sign_hash expects a hash digest, not raw data */
393:        {
394:            byte hashBuf[WC_SHA256_DIGEST_SIZE];
395:            ret = wc_Sha256Hash(data, fSz, hashBuf);

Recommendation: Pass the hash type through the sign/verify call chain so the user's -sha384/-sha512 selection is honored. At minimum, document this as a breaking change.

[aidangarske]

🔶 [Medium] wolfCLU_StringToHashType: md5 not chained into else-if

The md5 check at line 982 is a standalone if statement, while lines 986-1000 use an else if chain for SHA variants. This means if the input were something like md5sha256, the md5 check would match (via XSTRNCMP(in, "md5", 3)), set ret = WC_HASH_TYPE_MD5, but then the else-if chain would also be evaluated. In this case the else-if chain wouldn't override it (since md5sha256 doesn't start with sha512, etc.), so it works by coincidence. The md5 check should be part of the else-if chain for consistency and safety.

982:    if (XSTRNCMP(in, "md5", 3) == 0) {
983:        ret = WC_HASH_TYPE_MD5;
984:    }
985:
986:    if (XSTRNCMP(in, "sha512", 6) == 0) {
987:        ret = WC_HASH_TYPE_SHA512;
988:    }
989:    else if (XSTRNCMP(in, "sha384", 6) == 0) {

Recommendation: Chain the md5 check into the else-if, or use XSTRCMP for exact match as done in the config parsing changes.

[aidangarske]

🔶 [Medium] ECC sign/verify behavior change is breaking but not documented

The old code signed raw data directly with wc_ecc_sign_hash. The new code hashes data with SHA-256 first. While the new behavior is semantically correct (the API name implies a hash input), this is a breaking change: any ECC signatures produced by the old code will fail to verify with the new code, and vice versa. The PR description lists this as a fix but doesn't call out the backward-incompatibility.

Previously: wc_ecc_sign_hash(data, fSz, outBuf, &outLen, &rng, &key)
Now: wc_Sha256Hash(data, fSz, hashBuf); wc_ecc_sign_hash(hashBuf, WC_SHA256_DIGEST_SIZE, outBuf, &outLen, &rng, &key)

Recommendation: Document this as a breaking change in the PR description and release notes.

[aidangarske]

🔶 [Medium] No test coverage for ECC sign/verify SHA-256 hashing change

The ECC sign and verify functions were changed to hash input data with SHA-256 before signing/verifying. This is a significant behavioral change, but there is no test that generates an ECC signature and then verifies it to confirm the sign-verify round-trip works correctly with the new hashing. The existing test only signs with an empty key file to test error handling.

# Test signing with invalid key file expects error
./wolfssl -ecc -sign -inkey /dev/null ...

Recommendation: Add a test that generates an ECC key, signs data, and verifies the signature to confirm the round-trip works with the new SHA-256 hashing.