Fixes for buffer bounds, error handling, and key material cleanup - #257
Fixes for buffer bounds, error handling, and key material cleanup#257cconlon wants to merge 10 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR hardens the wolfCrypt JNI/JCE bindings by adding bounds checks for direct ByteBuffer digest finalization, normalizing/aligning error handling with Java/JCE contracts, and ensuring key/secret material is properly zeroized before native structures are freed.
Changes:
- Add direct
ByteBuffercapacity/position validation in MD5 and SHA/SHA-2/SHA-3native_finalJNI wrappers, plus regression tests that assert invalid positions throw. - Introduce shared JNI helpers to zeroize copied secret byte-array inputs before releasing them, and apply them to ML-KEM and SLH-DSA private/seed inputs.
- Ensure AES/DES3 native structs are initialized at allocation and have their internal key schedules zeroized via
wc_*Free()prior to freeing the native memory; align LMS verify behavior to returnfalsefor wrong-length signatures.
Reviewed changes
Copilot reviewed 29 out of 34 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| src/test/java/com/wolfssl/wolfcrypt/test/ShaTest.java | Adds regression test for invalid native_final(ByteBuffer, position) bounds handling. |
| src/test/java/com/wolfssl/wolfcrypt/test/Sha224Test.java | Adds regression test for invalid native_final position handling. |
| src/test/java/com/wolfssl/wolfcrypt/test/Sha256Test.java | Adds regression test for invalid native_final position handling. |
| src/test/java/com/wolfssl/wolfcrypt/test/Sha384Test.java | Adds regression test for invalid native_final position handling. |
| src/test/java/com/wolfssl/wolfcrypt/test/Sha512Test.java | Adds regression test for invalid native_final position handling. |
| src/test/java/com/wolfssl/wolfcrypt/test/Sha3Test.java | Adds regression test for invalid native_final position handling for SHA3-256. |
| src/test/java/com/wolfssl/wolfcrypt/test/Md5Test.java | Adds regression test for invalid native_final position handling. |
| src/test/java/com/wolfssl/wolfcrypt/test/LmsTest.java | Adds test ensuring truncated LMS signatures verify as false (not exceptions). |
| src/test/java/com/wolfssl/provider/jce/test/WolfCryptLmsSignatureTest.java | Adds JCE-level test ensuring truncated LMS signatures return false. |
| src/main/java/com/wolfssl/wolfcrypt/BlockCipher.java | Adds nativeFree() hook and calls it under pointerLock before freeing native structs. |
| src/main/java/com/wolfssl/wolfcrypt/Aes.java | Overrides nativeFree() to call wc_AesFree() for key schedule zeroization. |
| src/main/java/com/wolfssl/wolfcrypt/AesEcb.java | Overrides nativeFree() to call wc_AesFree() for key schedule zeroization. |
| src/main/java/com/wolfssl/wolfcrypt/Des3.java | Overrides nativeFree() to call wc_Des3Free() for key schedule zeroization. |
| src/main/java/com/wolfssl/wolfcrypt/AesCtr.java | Calls wc_AesFree() under pointerLock in releaseNativeStruct() for key schedule zeroization. |
| src/main/java/com/wolfssl/wolfcrypt/AesOfb.java | Calls wc_AesFree() under pointerLock in releaseNativeStruct() for key schedule zeroization. |
| jni/jni_sha.c | Adds direct-buffer capacity checks to prevent out-of-bounds writes during finalization (SHA/SHA-2/SHA-3). |
| jni/jni_md5.c | Adds direct-buffer capacity checks to prevent out-of-bounds writes during finalization (MD5). |
| jni/jni_native_struct.c | Adds secret-array pin/release helpers that zeroize copied secret inputs before releasing. |
| jni/include/wolfcrypt_jni_NativeStruct.h | Exposes new getSecretByteArray / releaseSecretByteArray helper declarations. |
| jni/jni_mlkem.c | Uses secret-array helpers to zeroize copied ML-KEM secret inputs before releasing arrays. |
| jni/jni_slhdsa.c | Uses secret-array helpers to zeroize copied SLH-DSA secret inputs before releasing arrays. |
| jni/jni_lms.c | Treats wrong-length signatures as verification failure (false) rather than throwing. |
| jni/jni_wolfssl_x509_store_ctx.c | Normalizes non-negative failure codes from X509_STORE_add_cert to negative error codes. |
| jni/jni_ecc.c | Adds null checks for GetStringUTFChars, fixes curve size sign handling, and guards PKCS#8 allocation on successful size query. |
| jni/jni_aes.c | Initializes AES at allocation (wc_AesInit) and adds JNI wrapper for wc_AesFree. |
| jni/jni_aesecb.c | Initializes AES-ECB at allocation and adds JNI wrapper for wc_AesFree. |
| jni/jni_aesctr.c | Initializes AES-CTR at allocation and adds JNI wrapper for wc_AesFree. |
| jni/jni_aesofb.c | Initializes AES-OFB at allocation and adds JNI wrapper for wc_AesFree. |
| jni/jni_des3.c | Initializes DES3 at allocation (wc_Des3Init) and adds JNI wrapper for wc_Des3Free. |
| jni/include/com_wolfssl_wolfcrypt_Aes.h | Declares JNI wc_AesFree() method. |
| jni/include/com_wolfssl_wolfcrypt_AesEcb.h | Declares JNI wc_AesFree() method. |
| jni/include/com_wolfssl_wolfcrypt_AesCtr.h | Declares JNI wc_AesFree() method. |
| jni/include/com_wolfssl_wolfcrypt_AesOfb.h | Declares JNI wc_AesFree() method. |
| jni/include/com_wolfssl_wolfcrypt_Des3.h | Declares JNI wc_Des3Free() method. |
Files not reviewed (5)
- jni/include/com_wolfssl_wolfcrypt_Aes.h: Generated file
- jni/include/com_wolfssl_wolfcrypt_AesCtr.h: Generated file
- jni/include/com_wolfssl_wolfcrypt_AesEcb.h: Generated file
- jni/include/com_wolfssl_wolfcrypt_AesOfb.h: Generated file
- jni/include/com_wolfssl_wolfcrypt_Des3.h: Generated file
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| result = JNI_TRUE; | ||
| } | ||
| else if (ret != SIG_VERIFY_E) { | ||
| else if (ret != SIG_VERIFY_E && ret != BUFFER_E) { |
There was a problem hiding this comment.
Nice fix on F-6904 for the wrong-length case. One more malformed-signature path lands in this same else branch though, so verify() still throws rather than returning false for it.
A correct-length signature whose embedded LMS type field has been altered returns SIG_TYPE_E (-231), not SIG_VERIFY_E or BUFFER_E. wc_LmsKey_Verify passes the length check (sigSz == params->sig_len) and calls wc_hss_verify, then wc_lms_verify, which compares the signature's LMS type word against the key's type and returns SIG_TYPE_E on mismatch (wc_lms_impl.c, the if (sigType != (params->lmsType & LMS_H_W_MASK)) check). That code propagates unchanged back to here, and since SIG_TYPE_E is neither SIG_VERIFY_E nor BUFFER_E, the else branch calls throwWolfCryptExceptionFromError. So a malformed-but-correct-length signature still throws, where the wrong-length one now returns false after F-6904. Both are malformed signatures, so the two paths behave inconsistently, and extending the same handling to SIG_TYPE_E would make verify() return false here too.
A single byte flip in the signature's LMS type field reproduces it. For a single-level key that word sits at offset LMS_L_LEN + LMS_Q_LEN + LMS_TYPE_LEN + hash_len + p*hash_len from the start of the signature, i.e. 1132 for SHA256_M32 with W8, where the leading LMS_L_LEN is the HSS levels field that wc_hss_verify consumes before wc_lms_verify. The length stays valid, the type word no longer matches the key, and verify() throws instead of returning false. The new truncated-signature test covers the length case well; a companion test that corrupts the type field and asserts verify() returns false would cover this one.
For consistency with the length case, it may be cleanest to treat the whole malformed-signature family uniformly: SIG_TYPE_E for certain, and it is worth checking whether an unsupported or otherwise invalid embedded type can surface yet another code, so that verify() returns false for any malformed signature and only throws on genuine operational errors (MEMORY_E, BAD_STATE_E, and so on). I traced this while looking at how LMS verify handles malformed input, and am glad to help with a repro or the extra test if useful.
There was a problem hiding this comment.
Thanks @Arpan0995! I made this fix and squashed into the commit for F-6904 (98cefc5).
This PR includes 10 Fenrir fixes:
Signature.verify()contractX509_STORE_add_certfailure codes to negative so Java does not read compat layer failure values as successimport_public_rawcoordinate length validationjni_native_struct.cGetStringUTFCharsresults in ECC curve name paths before usewc_AesInit/wc_Des3Initat allocation andwc_AesFree/wc_Des3FreeinreleaseNativeStruct()for Aes, AesCtr, AesEcb, AesOfb, and Des3 so key schedules are zeroized before thenative struct is freed