Wycheproof: streaming AES-GCM, Cipher security fixes, ECDH/DH validation#239
Open
MarkAtwood wants to merge 11 commits into
Open
Wycheproof: streaming AES-GCM, Cipher security fixes, ECDH/DH validation#239MarkAtwood wants to merge 11 commits into
MarkAtwood wants to merge 11 commits into
Conversation
Add JNI wrappers for wc_AesGcmEncryptInit/Update/Final and expose them through AesGcm.java. Includes: - Three new JNI functions in jni_aesgcm.c with null-pointer guard on GetByteArrayElements to handle OOM gracefully - Corresponding JNI declarations in com_wolfssl_wolfcrypt_AesGcm.h - Java wrappers with IDLE/STREAMING state machine enforcing that encryptUpdateStreaming and encryptFinalStreaming cannot be called before encryptInitStreaming, and that state resets to IDLE after encryptFinalStreaming even if the native call throws
WolfCryptCipher.java — four independent improvements: 1. Streaming AES-GCM encrypt path: when WOLFSSL_AESGCM_STREAM is compiled in (FeatureDetect.AesGcmStreamEnabled()), encrypting with AES/GCM/NoPadding uses the streaming API so plaintext is never fully buffered. Enforces the NIST SP 800-38D plaintext limit (2^32-2 blocks) via gcmBytesEncrypted counter. AAD is passed on the first update call, or in doFinal if no update was called, always before any plaintext. 2. IV-reuse prevention: adds 'finalized' flag set in the finally block of engineDoFinal. engineUpdate (both overloads), engineWrap, and engineUnwrap all throw IllegalStateException if called after doFinal without re-initializing, closing the window where an attacker could reuse a GCM IV by calling wrap after doFinal on the same cipher instance. 3. BadPaddingException on PKCS5 unpadding failure: AES-CBC and 3DES decrypt no longer propagates WolfCryptException from unPadPKCS7; it is caught and rethrown as BadPaddingException so callers receive the standard JCE exception type. 4. OAEP AlgorithmParameters: stores the active OAEPParameterSpec and returns it from engineGetParameters(), allowing callers to retrieve the hash and MGF parameters used during init.
Three related hardening changes:
1. DH public key validation (WolfCryptKeyAgreement, Dh.java):
- Store dhParamP/dhParamG from the private key at init time
- In engineDoPhase compare peer's {p,g} against stored values;
throw InvalidKeyException on mismatch (prevents cross-group
attacks)
- Call new Dh.checkPublicKey() wrapping wc_DhCheckPubKey to
reject out-of-range public key values
2. ECDH hardening (WolfCryptKeyAgreement):
- Release and recreate ecPrivate/ecPublic native structs in
wcInitECDHParams so engineInit() can be called multiple times
on the same object without leaking the prior session
- Wrap publicKeyDecode WolfCryptException as InvalidKeyException
- Call ecPublic.checkKey() and wrap failure as InvalidKeyException
- Compare getCurveId() for public and private keys; throw
InvalidKeyException on curve mismatch before makeSharedSecret
- Wrap makeSharedSecret WolfCryptException as IllegalStateException
3. WolfCryptECKeyFactory: wrap IllegalArgumentException from
validateParameters() as InvalidKeySpecException at both the
generatePublic and generatePrivate call sites
Two independent improvements to WolfCryptSignature and a new
AlgorithmParametersSpi for OAEP:
1. RSA CRT consistency check (WolfCryptSignature): in
wolfCryptInitPrivateKey, export n/p/q via exportRawPrivateKey
and verify n == p*q before accepting the key. A fault-injection
or corrupt key that has mismatched CRT components could expose
the private key via differential fault analysis; this check
rejects such keys at init time.
2. CRT component zeroing (WolfCryptSignature): d, p, q, dP, dQ,
and qInv (u) are exported into byte[] locals during validation.
A try-finally zeroes all six arrays with zeroArray() regardless
of outcome so private key material does not linger on the heap.
3. WolfCryptOaepParameters: new AlgorithmParametersSpi implementing
the "OAEP" algorithm name, supporting init/getEncoded/getSpec
for OAEPParameterSpec. Registered in WolfCryptProvider so that
AlgorithmParameters.getInstance("OAEP") resolves to this class.
Remove try/finally { finalized = true } from both engineDoFinal()
overloads. wolfCryptFinal() already resets cipher state for reuse,
matching the JCE contract that doFinal() allows subsequent
update()/doFinal() without re-init.
Wrap long lines in JNI declarations, Javadoc, and Java source
to stay within the 80-character project limit.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
MarkAtwood
force-pushed
the
fix/wycheproof-v2
branch
from
c74aa28 to
eb0c522
Compare
Closed
6 tasks
There was a problem hiding this comment.
Pull request overview
This PR hardens wolfJCE against several Wycheproof findings by adding AES-GCM streaming encryption support (JNI + Java), tightening Cipher lifecycle/nonce-reuse behavior, strengthening DH/ECDH validation/exception mapping, and adding OAEP AlgorithmParameters support.
Changes:
- Add AES-GCM streaming encrypt JNI wrappers and Java APIs, and integrate streaming encryption into
WolfCryptCipherwhen available. - Improve key agreement and key factory behavior (DH
{p,g}matching + pubkey validation; ECDH point/curve validation; wrap parameter validation errors as JCE exceptions). - Add RSA CRT consistency checking/zeroing and implement/register
AlgorithmParameters.OAEP.
Reviewed changes
Copilot reviewed 8 out of 9 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| src/main/java/com/wolfssl/wolfcrypt/AesGcm.java | Adds Java streaming AES-GCM init/update/final API with a small state machine. |
| jni/jni_aesgcm.c | Implements streaming AES-GCM JNI bindings to wolfCrypt init/update/final APIs. |
| jni/include/com_wolfssl_wolfcrypt_AesGcm.h | Declares new JNI methods for streaming AES-GCM encrypt. |
| src/main/java/com/wolfssl/provider/jce/WolfCryptCipher.java | Integrates AES-GCM streaming encrypt, adds AEAD size limits, and adds a post-doFinal lifecycle guard. |
| src/main/java/com/wolfssl/provider/jce/WolfCryptKeyAgreement.java | Adds DH parameter matching, ECDH point/curve validation, and improves exception propagation. |
| src/main/java/com/wolfssl/provider/jce/WolfCryptECKeyFactory.java | Wraps unsupported-curve IllegalArgumentException as InvalidKeySpecException. |
| src/main/java/com/wolfssl/provider/jce/WolfCryptSignature.java | Adds RSA CRT consistency check and zeroes exported CRT components. |
| src/main/java/com/wolfssl/provider/jce/WolfCryptOaepParameters.java | Adds AlgorithmParametersSpi implementation for OAEP parameters. |
| src/main/java/com/wolfssl/provider/jce/WolfCryptProvider.java | Registers AlgorithmParameters.OAEP when RSA-OAEP is enabled. |
Files not reviewed (1)
- jni/include/com_wolfssl_wolfcrypt_AesGcm.h: Generated file
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+300
to
+308
| checkStateAndInitialize(); | ||
| throwIfKeyNotLoaded(); | ||
|
|
||
| /* Allowed from IDLE or STREAMING (re-init resets a prior stream) */ | ||
| streamingState = StreamingState.STREAMING; | ||
|
|
||
| synchronized (pointerLock) { | ||
| wc_AesGcmEncryptInitStreaming(iv); | ||
| } |
Comment on lines
+66
to
+70
| String mgfDigest = ((MGF1ParameterSpec) mgfParams).getDigestAlgorithm(); | ||
| if (!isDigestSupported(mgfDigest)) { | ||
| throw new InvalidParameterSpecException( | ||
| "Unsupported MGF digest: " + mgfDigest); | ||
| } |
Comment on lines
+72
to
+79
| PSource pSource = spec.getPSource(); | ||
| if (pSource != null && pSource instanceof PSource.PSpecified) { | ||
| byte[] label = ((PSource.PSpecified) pSource).getValue(); | ||
| if (label != null && label.length > 0) { | ||
| throw new InvalidParameterSpecException( | ||
| "OAEP label (PSource) must be empty"); | ||
| } | ||
| } |
Comment on lines
+439
to
+458
| if (ivArr != NULL) { | ||
| iv = (const byte*)(*env)->GetByteArrayElements(env, ivArr, NULL); | ||
| ivSz = (*env)->GetArrayLength(env, ivArr); | ||
| } | ||
|
|
||
| if (iv == NULL || ivSz == 0) { | ||
| ret = BAD_FUNC_ARG; | ||
| } | ||
|
|
||
| /* | ||
| * Pass NULL key (key already loaded via wc_AesGcmSetKey). | ||
| * wc_AesGcmEncryptInit only sets key when key != NULL. | ||
| */ | ||
| if (ret == 0) { | ||
| ret = wc_AesGcmEncryptInit(aes, NULL, 0, iv, ivSz); | ||
| } | ||
|
|
||
| if (ivArr != NULL) { | ||
| (*env)->ReleaseByteArrayElements(env, ivArr, (jbyte*)iv, JNI_ABORT); | ||
| } |
Comment on lines
1780
to
1782
| this.operationStarted = false; | ||
| this.cipherInitialized = true; | ||
|
|
Comment on lines
+498
to
+512
| if (inputArr != NULL) { | ||
| in = (const byte*)(*env)->GetByteArrayElements(env, inputArr, NULL); | ||
| inLen = (*env)->GetArrayLength(env, inputArr); | ||
| if ((inLen > 0) && (in == NULL)) { | ||
| ret = BAD_FUNC_ARG; | ||
| } | ||
| } | ||
| if ((ret == 0) && (authInArr != NULL)) { | ||
| authIn = (const byte*)(*env)->GetByteArrayElements(env, | ||
| authInArr, NULL); | ||
| authInSz = (*env)->GetArrayLength(env, authInArr); | ||
| if ((authInSz > 0) && (authIn == NULL)) { | ||
| ret = BAD_FUNC_ARG; | ||
| } | ||
| } |
Comment on lines
+546
to
+552
| if (inputArr != NULL) { | ||
| (*env)->ReleaseByteArrayElements(env, inputArr, (jbyte*)in, JNI_ABORT); | ||
| } | ||
| if (authInArr != NULL) { | ||
| (*env)->ReleaseByteArrayElements(env, authInArr, (jbyte*)authIn, | ||
| JNI_ABORT); | ||
| } |
Comment on lines
+536
to
+542
| if ((*env)->ExceptionOccurred(env)) { | ||
| (*env)->ExceptionDescribe(env); | ||
| (*env)->ExceptionClear(env); | ||
| (*env)->DeleteLocalRef(env, outArr); | ||
| outArr = NULL; | ||
| ret = -1; | ||
| } |
Comment on lines
+560
to
+563
| if (ret != 0) { | ||
| throwWolfCryptExceptionFromError(env, ret); | ||
| return NULL; | ||
| } |
Comment on lines
+609
to
+635
| if (ret == 0) { | ||
| tagArr = (*env)->NewByteArray(env, tagLen); | ||
| if (tagArr == NULL) { | ||
| ret = MEMORY_E; | ||
| } | ||
| else { | ||
| (*env)->SetByteArrayRegion(env, tagArr, 0, tagLen, (jbyte*)tag); | ||
| if ((*env)->ExceptionOccurred(env)) { | ||
| (*env)->ExceptionDescribe(env); | ||
| (*env)->ExceptionClear(env); | ||
| (*env)->DeleteLocalRef(env, tagArr); | ||
| tagArr = NULL; | ||
| ret = -1; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| XFREE(tag, NULL, DYNAMIC_TYPE_TMP_BUFFER); | ||
|
|
||
| LogStr("wc_AesGcmEncryptFinal(aes = %p, tagLen = %d)\n", aes, tagLen); | ||
|
|
||
| if (ret != 0) { | ||
| throwWolfCryptExceptionFromError(env, ret); | ||
| return NULL; | ||
| } | ||
|
|
||
| return tagArr; |
The 3 AES-GCM update/getOutputSize assertions hard-coded buffered behavior and fail on builds with WOLFSSL_AESGCM_STREAM, where encrypt update() legally emits ciphertext incrementally (as SunJCE/BouncyCastle do). Gate them on FeatureDetect.AesGcmStreamEnabled() so each build asserts its correct behavior; the ciphertext/tag vector oracle is unchanged. Decrypt stays buffered (unchanged). Ref JENKINS-lq6k.
buffered.length + len is int arithmetic that overflows and can never exceed AEAD_MAX_PLAINTEXT (Integer.MAX_VALUE), so the guard was a vacuous always-false comparison (SpotBugs INT_VACUOUS_COMPARISON) that also missed the overflow it meant to catch. Widen the sum to long. Ref JENKINS-lq6k.
Infer flags a null dereference in the OAEP branch of
engineGetParameters (added by this PR): params.init() after
AlgorithmParameters.getInstance("OAEP"). getInstance() is non-null-
or-throws so it cannot actually NPE, but the sibling CBC/CTR/OFB
branch already guards with 'if (params != null)' before init() --
mirror that for consistency and to clear the Infer finding.
Behavior-preserving; verified OAEP getParameters still returns a
usable OAEPParameterSpec. Ref JENKINS-lq6k.
The RSA key-consistency validation added in this PR calls exportRawPrivateKey (native wc_RsaExportCrtKey), which some wolfSSL builds don't compile in and return 'Feature not compiled in'. That propagated as InvalidKeyException and broke signing on those builds (5.8.2-stable --enable-jni, Windows Normal non-FIPS). Catch the WolfCryptException from the export and skip the defensive check -- the key already imported via decodePrivateKeyPKCS8; a real n!=p*q mismatch still throws InvalidKeyException. Ref JENKINS-lq6k.
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.
Summary
Findings from running the Wycheproof
test suite against wolfJCE. Adds streaming AES-GCM encryption, hardens the
Cipher, KeyAgreement, and Signature providers, and makes the provider behave
consistently whether or not the underlying wolfSSL build enables a given
feature.
AES-GCM streaming
wc_AesGcmEncryptInit/Update/Final(jni_aesgcm.c) andAesGcm.javaJava bindings with anIDLE/STREAMINGstate machine thatrejects out-of-order calls and resets state after finalization.
WolfCryptCipheruses the streaming API forAES/GCM/NoPaddingencryptionwhen the native library is built with
WOLFSSL_AESGCM_STREAM, instead ofbuffering the entire plaintext; it falls back to the buffered path otherwise.
The NIST SP 800-38D plaintext limit is enforced, and AAD is always supplied
before plaintext.
Cipher hardening (
WolfCryptCipher)update,wrap, andunwrapthrowIllegalStateExceptionif called afterdoFinalwithout re-initializing,closing a window where a GCM nonce could be reused.
BadPaddingExceptionper the JCE contract (with a generic message, forpadding-oracle hygiene).
OAEPParameterSpecis exposed viaengineGetParameters().ECDH / DH validation (
WolfCryptKeyAgreement){p, g}are validated against the private key's parametersand out-of-range peer public keys are rejected (small-subgroup /
cross-group), throwing
InvalidKeyExceptionon mismatch.init()may be calledrepeatedly on the same object, and native failures are mapped to the
appropriate JCE exceptions.
RSA / OAEP (
WolfCryptSignature,WolfCryptOaepParameters)and
n == p*qis verified, rejecting corrupt or fault-injected keys; thesensitive components are zeroed afterward.
AlgorithmParametersSpiimplementing theOAEPalgorithm name,registered in
WolfCryptProvider.Behavior across build configurations
The provider and its tests adapt to what the native wolfSSL build actually
compiles in, so the suite is green on minimal and full builds alike:
update()output (incremental when streaming, buffered otherwise) isgated on
FeatureDetect.AesGcmStreamEnabled().(
wc_RsaExportKey) is not compiled in — the key still imports; a genuinen != p*qmismatch is still rejected.Testing
(
--enable-jni).and without
WOLFSSL_AESGCM_STREAM.Supersedes #224 (rebased onto current master).