Fixes across debug logging, key zeroization, and native input handling - #267
Conversation
There was a problem hiding this comment.
Pull request overview
This PR applies a set of “Fenrir” fixes across the wolfCrypt JNI/JCE layers, primarily targeting safer native-struct lifecycle handling (zeroization on release), stricter native input validation, and more accurate debug logging for ByteBuffer/offset-based operations.
Changes:
- Add native
native_free()paths for HMAC and ChaCha to free/zeroize sensitive native structs before releasing JNI-managed memory, plus tests for the HMAC release lifecycle. - Improve debug logging to report the correct processed/written regions for ByteBuffer and offset-based JNI calls (AES-CTR, MD5, SHA*, RNG).
- Tighten native AES-CTS IV validation (require exactly one AES block) and avoid
XMALLOC(0)on AES-GCM/CCM AAD-only paths; update related tests/docs.
Reviewed changes
Copilot reviewed 16 out of 18 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/test/java/com/wolfssl/wolfcrypt/test/HmacTest.java | Adds coverage for Hmac native struct release/re-key/double-release lifecycle. |
| src/test/java/com/wolfssl/wolfcrypt/test/AesCtsTest.java | Extends setKey validation tests for short/long IV rejection. |
| src/test/java/com/wolfssl/provider/jce/test/WolfCryptUtilTest.java | Updates stale test comment to reflect actual WKS conversion behavior. |
| src/main/java/com/wolfssl/wolfcrypt/Hmac.java | Calls new native free/zeroize routine before releasing the NativeStruct pointer. |
| src/main/java/com/wolfssl/wolfcrypt/Chacha.java | Adds native free/zeroize hook on release to clear ChaCha key/state before free. |
| src/main/java/com/wolfssl/provider/jce/WolfCryptMessageDigestSha3.java | Fixes incorrect Javadoc label for wcSHA3_224. |
| jni/jni_sha.c | Adjusts debug output to log correct ByteBuffer/offset regions for SHA* update/final paths. |
| jni/jni_md5.c | Adjusts debug output to log correct ByteBuffer regions for MD5 update/final. |
| jni/jni_rng.c | Adjusts debug output to log correct generated output regions for RNG ByteBuffer/array APIs. |
| jni/jni_aesctr.c | Adjusts debug output to log correct written output region for AES-CTR ByteBuffer update. |
| jni/jni_aescts.c | Enforces IV length == AES block size in native AES-CTS key setup. |
| jni/jni_aesgcm.c | Avoids XMALLOC(0) by allocating at least 1 byte on AAD-only encrypt/decrypt paths. |
| jni/jni_aesccm.c | Avoids XMALLOC(0) by allocating at least 1 byte on AAD-only encrypt/decrypt paths. |
| jni/jni_hmac.c | Adds JNI native_free that frees internal resources and zeroizes Hmac struct contents. |
| jni/jni_chacha.c | Adds JNI native_free that zeroizes ChaCha struct contents. |
| jni/jni_rsa.c | Adds explicit non-blinding compilation arm for wc_RsaSetRNG JNI wrapper. |
| jni/include/com_wolfssl_wolfcrypt_Hmac.h | Declares new JNI native_free entrypoint for Hmac. |
| jni/include/com_wolfssl_wolfcrypt_Chacha.h | Declares new JNI native_free entrypoint for Chacha. |
Files not reviewed (2)
- jni/include/com_wolfssl_wolfcrypt_Chacha.h: Generated file
- jni/include/com_wolfssl_wolfcrypt_Hmac.h: Generated file
Suppressed comments (5)
jni/jni_sha.c:594
- If ret != 0 here (including BAD_FUNC_ARG when
hash_bufferis non-direct/NULL), an exception is thrown but the function continues into debug logging that useshash + position/LogHex(hash, ...). That can dereference NULL and crash the JVM. Return immediately after throwing (or only log on success).
if (ret != 0) {
throwWolfCryptExceptionFromError(env, ret);
}
LogStr("wc_Sha224Final(sha=%p, hash) = %d\n", sha, ret);
jni/jni_sha.c:814
- This function throws on error but then always executes debug logging that assumes
hashis non-NULL (hash + position,LogHex(hash, ...)). Passing a non-direct ByteBuffer can therefore lead to a JVM crash. Return immediately after throwing (or guard logging underret == 0).
if (ret != 0)
throwWolfCryptExceptionFromError(env, ret);
LogStr("wc_Sha256Final(sha=%p, hash) = %d\n", sha, ret);
LogStr("hash[%u]: [%p]\n", (word32)SHA256_DIGEST_SIZE, hash + position);
jni/jni_sha.c:1020
- On error, this code throws but continues into debug logging that uses
hash + positionandLogHex(hash, ...)without ensuringhashis non-NULL. With a non-direct ByteBuffer,hashcan be NULL, leading to a JVM crash. Return immediately after throwing (or only log on success).
if (ret != 0)
throwWolfCryptExceptionFromError(env, ret);
LogStr("wc_Sha384Final(sha=%p, hash) = %d\n", sha, ret);
LogStr("hash[%u]: [%p]\n", (word32)SHA384_DIGEST_SIZE, hash + position);
jni/jni_sha.c:1227
- If
hash_bufferis non-direct/NULL,hashbecomes NULL and ret becomes BAD_FUNC_ARG. The function throws but then continues into debug logging (hash + position,LogHex(hash, ...)), which can dereference NULL and crash the JVM. Return immediately after throwing (or guard logging underret == 0).
if (ret != 0)
throwWolfCryptExceptionFromError(env, ret);
LogStr("wc_Sha512Final(sha=%p, hash) = %d\n", sha, ret);
LogStr("hash[%u]: [%p]\n", (word32)SHA512_DIGEST_SIZE, hash + position);
jni/jni_rng.c:193
- When ret != 0 (including BAD_FUNC_ARG for NULL buffer / invalid bounds), an exception is thrown but debug logging still runs and uses
buffer + offset/LogHex(buffer, ...)without ensuringbufferis non-NULL. That can crash the JVM. Guard the logging so it only executes on success (but still run releaseByteArray on all paths).
}
LogStr("wc_RNG_GenerateBlock(rng=%p, buffer, length) = %d\n", rng, ret);
LogStr("output[%u]: [%p]\n", (word32)length, buffer + offset);
LogHex(buffer, offset, length);
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
Retest this please (FIPSv6 fixed by PR to master) |
2df81ec to
1e0c921
Compare
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #267
Scan targets checked: wolfcrypt-jni-src, wolfcrypt-jni-bugs
Fenrir result: Approved ✅
No new issues found in the changed files.
Advisory only — this automated result does not count as a GitHub approval.
This PR includes ten Fenrir fixes: