Conversation
There was a problem hiding this comment.
Pull request overview
This PR is a broad "security hardening" pass across the OpenSSL FIPS Java provider's native and Java layers. It centralizes the OpenSSL library context behind an atomic accessor (jssl_libctx()), enforces the FIPS provider on EVP fetches, tightens JNI error handling and resource cleanup, adds input validation/zeroization across cipher/MAC/PBKDF2/signature/KeyAgreement code paths, removes noisy ERR_print_errors_fp calls, and introduces a brand-new KeyPairGenerator SPI for DH/EC. It also bumps the artifact version 0.6.0 → 0.7.0 and adds compiler/linker hardening flags to the Makefile.
Changes:
- Replace
extern OSSL_LIB_CTX *global_libctxdirect access with an atomic-loadedjssl_libctx()and enforceprovider=fipson EVP_RAND/EVP_MAC fetches. - Add
OpenSSLKeyPairGenerator(DH/EC) with native DER encoding helpers, plus pervasive null-checks, zeroization, and clearer error propagation across DRBG/Cipher/MAC/Signature/PBKDF2/KeyAgreement classes. - Behavior changes: GMAC now prepends a random 12-byte nonce to its tag; PBKDF2 now requires explicit key length and ≥1000 iterations; Ed25519/Ed448
sv_updateaccumulates input across calls;engineSetSeednow feeds the user buffer as DRBG additional input rather than seed material.
Reviewed changes
Copilot reviewed 53 out of 54 changed files in this pull request and generated 25 comments.
Show a summary per file
| File | Description |
|---|---|
| Makefile | Adds hardening CFLAGS/LDFLAGS and keypairgenerator source dir. |
| pom.xml | Version bump 0.6.0 → 0.7.0. |
| src/main/native/c/init.c | Atomic-managed global_libctx, secure_getenv, EVP_set_default_properties("fips=yes"), removes stderr logging. |
| src/main/native/c/drbg.c | FIPS-only RAND fetch; reseed/seed APIs return jssl_status; reseed_with_params no longer pulls fresh getrandom entropy. |
| src/main/native/c/OpenSSLDrbg.c | Hardened error/return paths and JNI cleanup; engineSetSeed arg order changed. |
| src/main/native/c/OpenSSLCipher.c, cipher.c | Cleansing key/IV on re-init; bounds checks; padding rejection; goto-cleanup refactor. |
| src/main/native/c/OpenSSLMAC.c, mac.c | FIPS-only MAC fetch with libctx; goto-cleanup; silent set_params failure. |
| src/main/native/c/OpenSSLSignature.c, signature.c | Sign/verify input validation, accumulating Ed update buffer, sv_context length type widened. |
| src/main/native/c/OpenSSLPBKDF2.c | New key_length JNI argument, validated against MAX_KEY_SIZE=256. |
| src/main/native/c/OpenSSLKeyAgreement.c, keyagreement.c | Use d2i_PrivateKey_ex/d2i_PUBKEY_ex; cleanse shared secret. |
| src/main/native/c/OpenSSLKeyPairGenerator.{c,h}, jni headers | New native key-pair generation via OSSL encoder API. |
| src/main/native/c/OpenSSLMD.c, md.c | JNI null checks, fix EVP_DigestFinal_ex length type. |
| src/main/native/c/RSAKEM*.c, KeyConverter.c, evp_utils.c | Migrate from global_libctx to jssl_libctx(). |
| src/main/native/c/jni_utils.c, kdf.c | Add throwIllegalArgument, propagate kdf populate errors. |
| src/main/native/include/* | Header updates for new APIs and signatures. |
| src/main/java/.../cipher/OpenSSLCipher.java | AlgorithmParameters/AAD support, IV auto-generation, key-material zeroization, key-unwrap for public/private. |
| src/main/java/.../drbg/OpenSSLDrbg.java | ProviderException on init failure; engineSetSeed now passes seed as additional input. |
| src/main/java/.../mac/OpenSSLMAC.java, GMACWithAes128GCM.java | State checks, ByteBuffer fix; GMAC now prepends random nonce to tag. |
| src/main/java/.../md/OpenSSLMD.java | ByteBuffer engineUpdate fix. |
| src/main/java/.../signature/OpenSSLSignature.java | PSS parameter round-trip, init-state checks, input validation. |
| src/main/java/.../kdf/OpenSSLPBKDF2.java | FIPS min iteration count, key-length plumbing, defensive cloning. |
| src/main/java/.../keyagreement/{OpenSSL,DH,ECDH}KeyAgreement.java | Null-checks, key-bytes zeroization. |
| src/main/java/.../keyencapsulation/OpenSSLKEMRSA.java | Wipe encoded key after decapsulator init. |
| src/main/java/.../keypairgenerator/* | New DH/EC KeyPairGeneratorSpi, Encoded{Public,Private}Key holders. |
| src/main/java/.../provider/OpenSSLFIPSProvider.java | Register new KeyPairGenerator services. |
| src/main/java/.../util/NativeMemoryCleaner.java | cleaner field made final. |
| src/main/java/.../key/KeyConverter.java | Wipe encoded key after native call. |
| src/test/java/{MacTest,KeyAgreementTest}.java, src/test/native/* | Test adaptations for new GMAC output format, libctx parameter, new include path. |
| src/test/consumer-snap/snapcraft.yaml | Update jar version to 0.7.0. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
| synchronized (LOCK) { | ||
| reseed0(seed, false, null); | ||
| reseed0(null, false, seed); |
Comment on lines
+41
to
+62
| byte[] nonce = new byte[NONCE_LEN]; | ||
| RANDOM.nextBytes(nonce); | ||
| this.currentNonce = nonce; | ||
| return nonce; | ||
| } | ||
|
|
||
| @Override | ||
| protected byte[] engineDoFinal() { | ||
| byte[] tag = super.engineDoFinal(); | ||
| byte[] nonce = currentNonce; | ||
| if (nonce == null) { | ||
| return tag; | ||
| } | ||
| byte[] out = new byte[nonce.length + tag.length]; | ||
| System.arraycopy(nonce, 0, out, 0, nonce.length); | ||
| System.arraycopy(tag, 0, out, nonce.length, tag.length); | ||
| return out; | ||
| } | ||
|
|
||
| @Override | ||
| protected int engineGetMacLength() { | ||
| return NONCE_LEN + super.engineGetMacLength(); |
Comment on lines
+114
to
116
| protected void engineInit(Key key, AlgorithmParameterSpec params, SecureRandom random) throws InvalidKeyException { | ||
| engineInit(key, random); | ||
| } |
| if (0 == EVP_MAC_CTX_set_params(ctx, _params)) { | ||
| ERR_print_errors_fp(stderr); | ||
| } | ||
| EVP_MAC_CTX_set_params(ctx, _params); |
Comment on lines
+190
to
202
| jssl_status reseed_with_seed(DRBG* generator, byte seed[], int seed_length) { | ||
| int rc = EVP_RAND_reseed(generator->context, 0, seed, seed_length, NULL, 0); | ||
| OPENSSL_cleanse(seed, seed_length); | ||
| return rc > 0 ? SUCCESS : FAIL_EVP; | ||
| } | ||
|
|
||
| void reseed_with_seed_and_params(DRBG* generator, byte seed[], int seed_length, DRBGParams *params) { | ||
| EVP_RAND_reseed(generator->context, params->prediction_resistance, seed, seed_length, params->additional_data, params->additional_data_length); | ||
| jssl_status reseed_with_seed_and_params(DRBG* generator, byte seed[], int seed_length, DRBGParams *params) { | ||
| int rc = EVP_RAND_reseed(generator->context, params->prediction_resistance, | ||
| seed, seed_length, | ||
| params->additional_data, params->additional_data_length); | ||
| OPENSSL_cleanse(seed, seed_length); | ||
| return rc > 0 ? SUCCESS : FAIL_EVP; | ||
| } |
Comment on lines
+309
to
+323
| AlgorithmParameters ap; | ||
| if (isModeGCM()) { | ||
| ap = AlgorithmParameters.getInstance("GCM"); | ||
| ap.init(new GCMParameterSpec(GCM_TAG_LEN * 8, iv)); | ||
| } else if (isModeCCM()) { | ||
| ap = AlgorithmParameters.getInstance("CCM"); | ||
| ap.init(new GCMParameterSpec(GCM_TAG_LEN * 8, iv)); | ||
| } else { | ||
| ap = AlgorithmParameters.getInstance("AES"); | ||
| ap.init(new IvParameterSpec(iv)); | ||
| } | ||
| return ap; | ||
| } catch (NoSuchAlgorithmException | InvalidParameterSpecException e) { | ||
| throw new ProviderException("Could not encode AlgorithmParameters", e); | ||
| } |
Comment on lines
+440
to
+452
| private Key createKey(byte[] keyMaterial, String algo, int keyType) throws NoSuchAlgorithmException, InvalidKeyException { | ||
| switch (keyType) { | ||
| case Cipher.SECRET_KEY: | ||
| return new SecretKeySpec(keyMaterial, 0, keyMaterial.length, algo); | ||
| case Cipher.PUBLIC_KEY: | ||
| try { | ||
| return KeyFactory.getInstance(algo).generatePublic(new X509EncodedKeySpec(keyMaterial)); | ||
| } catch (InvalidKeySpecException e) { | ||
| throw new InvalidKeyException("Failed to decode public key for algorithm " + algo, e); | ||
| } | ||
| case Cipher.PRIVATE_KEY: | ||
| try { | ||
| return KeyFactory.getInstance(algo).generatePrivate(new PKCS8EncodedKeySpec(keyMaterial)); |
Comment on lines
+170
to
+183
| if (params == null || params.padding != Params.PSS_PADDING) { | ||
| return null; | ||
| } | ||
| String mgf1Digest = (params.mgf1Digest != null) ? params.mgf1Digest : params.digest; | ||
| if (mgf1Digest == null) { | ||
| return null; | ||
| } | ||
| try { | ||
| AlgorithmParameters ap = AlgorithmParameters.getInstance("RSASSA-PSS"); | ||
| ap.init(new PSSParameterSpec(params.digest, "MGF1", new MGF1ParameterSpec(mgf1Digest), params.saltLength, 1)); | ||
| return ap; | ||
| } catch (NoSuchAlgorithmException | InvalidParameterSpecException e) { | ||
| throw new ProviderException("Could not encode PSS AlgorithmParameters", e); | ||
| } |
Comment on lines
112
to
120
| protected void engineUpdate(ByteBuffer data) { | ||
| engineUpdate(data.array()); | ||
| int remaining = data.remaining(); | ||
| if (remaining <= 0) { | ||
| return; | ||
| } | ||
| byte[] chunk = new byte[remaining]; | ||
| data.get(chunk); | ||
| engineUpdate(chunk); | ||
| } |
Comment on lines
+206
to
+208
| private static boolean isGMAC(String macName) { | ||
| return macName.startsWith("GMAC"); | ||
| } |
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.
No description provided.