Fail non-zero on verify failures; fix stdin keySize clobber (F-5362/F-5363/F-5970)#254
Open
julek-wolfssl wants to merge 6 commits into
Open
Fail non-zero on verify failures; fix stdin keySize clobber (F-5362/F-5363/F-5970)#254julek-wolfssl wants to merge 6 commits into
julek-wolfssl wants to merge 6 commits into
Conversation
wolfCLU_verify_signature_ecc, wolfCLU_verify_signature_ed25519 and wolfCLU_verify_signature_dilithium logged "Invalid Signature." when the underlying verify API returned successfully with stat/res != 1, but left ret unchanged so the command still exited 0. Set a failure code in the mismatch branch so an invalid signature exits non-zero. Add negative tests that verify a genuine signature against different data for ECC, Ed25519 and Dilithium and assert a non-zero (non-crash) exit.
wolfCLU_requestSetup logged "verify failed" when wolfSSL_X509_REQ_verify returned non-1 but left ret as WOLFCLU_SUCCESS, so `req -verify` exited 0 on a tampered CSR and still printed the request. Set WOLFCLU_FATAL_ERROR on verify failure; the later text/DER/PEM output blocks are already gated on ret == WOLFCLU_SUCCESS so they are skipped. Add TestReqVerify covering a good CSR (verify OK, exit 0) and a CSR with a corrupted signature (non-zero exit, no request emitted).
wolfCLU_setup passed &keySize to wolfCLU_GetStdinPassword, which writes the entered password length back through that pointer. keySize (the algorithm key size in bits) was therefore overwritten by the password length. With -pbkdf2 the EVP derivation length (keySize+7)/8 then collapsed to ~1-2 bytes, producing a brute-forceable AES key, and the cleanup memsets sized on keySize left most of the key buffer un-zeroed before free. Pass a dedicated buffer-capacity variable so keySize is preserved for the derivation length and cleanup. Use wolfCLU_ForceZero for the key/pwdKey/iv wipe so it is not optimized away before free. Add pty-driven tests for the interactive password prompt: the -pbkdf2 path must derive a full-length key and interoperate with -pass decryption.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes several cases where wolfCLU commands reported verification failures but still exited successfully, and it prevents encrypt/decrypt interactive password entry from corrupting the cipher key-size (which could lead to dangerously short derived keys). It also adds regression tests to ensure these failure modes remain fixed.
Changes:
- Make ECC/Ed25519/Dilithium signature verification return a non-zero exit code when the signature does not match (even if the underlying verify API returns success with
stat/res != 1). - Make
req -verifyfail (non-zero) when CSR signature verification fails, and ensure output is suppressed on failure. - Fix
encryptinteractive stdin password handling so it no longer overwriteskeySize, and usewolfCLU_ForceZerofor key material wiping; add pty-driven tests for the interactive prompt path.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/x509/x509-req-test.py | Adds CSR tampering helper and tests asserting req -verify fails and emits no CSR output on verify failure. |
| tests/genkey_sign_ver/genkey-sign-ver-test.py | Adds negative tests ensuring ECC/Ed25519/Dilithium verify fails (non-zero) when verifying a valid signature against different data. |
| tests/encrypt/enc-test.py | Adds POSIX pty-driven tests to exercise interactive password prompting and PBKDF2 key derivation/interoperability. |
| src/x509/clu_request_setup.c | Ensures CSR verify failure sets a fatal error code so req -verify exits non-zero and skips output. |
| src/sign-verify/clu_verify.c | Ensures invalid signature cases set a failure code and Dilithium returns it (instead of always success). |
| src/crypto/clu_crypto_setup.c | Stops stdin password reading from clobbering keySize and uses wolfCLU_ForceZero for wiping key material. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Pass a word32 directly to wolfCLU_GetStdinPassword instead of casting the address of an int. The previous (word32*)&int cast assumed int is 32-bit and relied on type punning; a word32 matches the parameter type exactly.
Read the source DER with `with open(...)` in _flip_last_der_byte so the file descriptor is released promptly, matching the rest of the test file.
Member
Author
|
retest this please |
The FIPS build rejects PBKDF2 passwords shorter than HMAC_FIPS_MIN_KEY (14 chars), so the 12-char test password made the encrypt step exit non-zero and the new pty tests failed the FIPS check. Use a 25-char password, which still truncates keySize when the bug is present but satisfies the FIPS minimum (matching this file's existing longer -pbkdf2 passwords).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Three fixes from the fenrir batch, each with tests.
F-5362 — fail ECC/Ed25519/Dilithium verify on invalid signature
wolfCLU_verify_signature_ecc,_ed25519and_dilithiumlogged "Invalid Signature." when the underlying verify API returned successfully withstat/res != 1, but leftretunchanged, so the command still exited 0. The mismatch branch now sets a failure code so an invalid signature exits non-zero. Adds negative tests (verify a genuine signature against different data) for ECC, Ed25519 and Dilithium asserting a non-zero, non-crash exit.F-5363 — fail
reqcommand when CSR verification failswolfCLU_requestSetuplogged "verify failed" whenwolfSSL_X509_REQ_verifyreturned non-1 but leftretasWOLFCLU_SUCCESS, soreq -verifyexited 0 on a tampered CSR and still printed the request. Now setsWOLFCLU_FATAL_ERRORon verify failure; the later text/DER/PEM output blocks are already gated onret == WOLFCLU_SUCCESSso they are skipped. AddsTestReqVerifycovering a good CSR (verify OK, exit 0) and a corrupted-signature CSR (non-zero exit, no request emitted).F-5970 — stop clobbering keySize with stdin password length
wolfCLU_setuppassed&keySizetowolfCLU_GetStdinPassword, which writes the entered password length back through that pointer, overwritingkeySize(the algorithm key size in bits). With-pbkdf2the EVP derivation length(keySize+7)/8then collapsed to ~1-2 bytes, producing a brute-forceable AES key, and the cleanup memsets sized onkeySizeleft most of the key buffer un-zeroed before free. Now passes a dedicated buffer-capacity variable sokeySizeis preserved, and useswolfCLU_ForceZerofor the key/pwdKey/iv wipe so it is not optimized away. Adds pty-driven tests for the interactive password prompt: the-pbkdf2path must derive a full-length key and interoperate with-passdecryption.