Conversation
There was a problem hiding this comment.
🟡 Changes recommended
releaseByteArrayZeroize() can call ReleaseByteArrayElements() twice even when isCopy == JNI_FALSE, which risks undefined behavior on pinned arrays and should be corrected before merging.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR hardens wolfcrypt-jni’s JNI and keystore handling by improving sensitive-data zeroization behavior, making WKS keystore loading robust to partial reads, and tightening CI validation for the Android Gradle wrapper.
Changes:
- Harden WKS loading by using
DataInputStream.readFully()for fixed-size regions and add a regression test for partial-read streams. - Add/extend JNI-side zeroization of copied byte arrays (keys, secrets, RNG output, RSA CRT exports) and detach threads created by the FIPS error callback.
- Add PBKDF password/salt bounds validation and enable Gradle wrapper validation in Android CI workflows.
File summaries
| File | Description |
|---|---|
| src/test/java/com/wolfssl/provider/jce/test/WolfSSLKeyStoreTest.java | Adds a regression test ensuring WKS loads correctly from partial-read streams. |
| src/main/java/com/wolfssl/provider/jce/WolfSSLKeyStore.java | Uses readFully() for WKS fixed-size reads; normalizes NULL AlgId params for SPKI matching. |
| jni/jni_rsa.c | Zeroizes JNI-copied RSA CRT export outputs on all paths. |
| jni/jni_rng.c | Zeroizes JNI-copied RNG output buffer after commit/abort. |
| jni/jni_pwdbased.c | Validates password/salt length arguments against array lengths before native use; adjusts zeroization length. |
| jni/jni_native_struct.c | Introduces releaseByteArrayZeroize() helper to commit/abort and wipe JNI copies. |
| jni/jni_md5.c | Rejects undersized hash arrays and improves logging/exception flow around wc_Md5Final. |
| jni/jni_fips.c | Detaches threads attached by the FIPS error callback; zeroizes copied key/secret buffers in multiple FIPS JNI wrappers. |
| jni/jni_aesofb.c | Zeroizes JNI-copied AES key material on release. |
| jni/jni_aesecb.c | Zeroizes JNI-copied AES key material on release. |
| jni/jni_aescts.c | Zeroizes JNI-copied AES key material on release. |
| jni/jni_aesctr.c | Zeroizes JNI-copied AES key material on release. |
| jni/jni_aes.c | Zeroizes JNI-copied AES key material on release. |
| jni/include/wolfcrypt_jni_NativeStruct.h | Declares releaseByteArrayZeroize() in the native struct helper header. |
| IDE/Android/gradle/wrapper/gradle-wrapper.properties | Updates wrapper properties (timeout/URL validation) and keeps SHA256 sum for Gradle distribution. |
| .github/workflows/android_gradle.yml | Adds Gradle wrapper validation step to CI. |
| .github/workflows/android_gradle_fipsready.yml | Adds Gradle wrapper validation step to CI (FIPS-ready workflow). |
Review details
- Files reviewed: 17/18 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
187cef1 to
535ff30
Compare
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #276
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.
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Pull request overview
Copilot reviewed 17 out of 18 changed files in this pull request and generated 2 comments.
535ff30 to
637c08b
Compare
637c08b to
5f58a2b
Compare
This PR fixes 10 Fenrir issues:
releaseByteArrayZeroize()and use it in the FIPS byte-array wrappers so copied keys, private material, shared secrets, and RNG output are wiped after commit or abortreadFully()so partial-read streams load