[Android] Add missing JNI exception checks across crypto PAL#130555
Draft
simonrozsival wants to merge 1 commit into
Draft
[Android] Add missing JNI exception checks across crypto PAL#130555simonrozsival wants to merge 1 commit into
simonrozsival wants to merge 1 commit into
Conversation
Adds missing JNI exception checks across the remaining functions in the Android cryptography PAL, based on a static audit of `Call*Method` / `NewObject` / `Get*ArrayRegion`-style JNI calls where a pending Java exception could otherwise leak back into managed code (undefined behavior for the next managed->native transition) or produce partial output. The touched functions follow the patterns already established elsewhere in the PAL: 1. `ON_EXCEPTION_PRINT_AND_GOTO(label)` immediately after each fallible JNI call. The macro both prints and clears the exception, so subsequent JNI calls on the cleanup path are safe. 2. `INIT_LOCALS(loc, ...)` + a single `cleanup:`/`RELEASE_LOCALS(loc, env)` tail for local-ref hygiene, replacing ad-hoc `jobject foo = NULL;` declarations and scattered `DeleteLocalRef` calls. 3. No partial output state on failure -- results are computed into locals and only promoted to caller-visible out-parameters / global refs on the success path. Files changed (all under `src/native/libs/System.Security.Cryptography.Native.Android/` unless noted): - `pal_x509chain.c` -- harden the functions not covered by the earlier `X509ChainCreateContext` fix (build, certificate/error enumeration, custom trust store, revocation validation) and clear exceptions on the error-capture paths. - `pal_bignum.c` -- check `toByteArray()` before using the result. - `pal_cipher.c` -- `CipherSetKeyAndIV`, `CipherUpdateAAD`, `CipherUpdate` (also releases the update output array on all paths). - `pal_ecdsa.c` -- clear the exception on the `getPrivate()` failure path in `EcDsaSign`. - `pal_eckey.c` -- `NewEcKeyFromKeys`, `EcKeyCreateByOid` (converted to `INIT_LOCALS` + per-call checks), `EcKeyGetCurveName`. - `pal_hmac.c` -- check `Mac.doFinal()` before reading the result array. - `pal_misc.c` -- `GetRandomBytes`. - `pal_pbkdf2.c` -- clear the pending exception when `NewDirectByteBuffer` fails. - `pal_sslstream.c` -- `SetTargetHost`, `SSLStreamRead`, `RequestClientAuthentication`, `SetApplicationProtocols`. - `pal_x509.c` -- check `SetByteArrayRegion` in the decode paths. Managed changes: - `Interop.X509Chain.cs` -- `X509ChainGetErrorCount` now returns a negative value when a JNI exception occurs (distinct from a legitimate count of 0); the wrapper throws `CryptographicException` in that case instead of silently reporting "no errors". - `Interop.Bignum.cs` -- `AndroidCryptoNative_BigNumToBinary` now returns -1 when a JNI exception occurs and writes nothing; `ExtractBignum` throws `CryptographicException` rather than returning a zero-filled buffer. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: bc96caa6-f528-4662-8cfa-df04e8a47475
simonrozsival
temporarily deployed
to
copilot-pat-pool
GitHub Actions
Inactive
simonrozsival
temporarily deployed
to
copilot-pat-pool
GitHub Actions
Inactive
simonrozsival
had a problem deploying
to
copilot-pat-pool
GitHub Actions
Failure
simonrozsival
temporarily deployed
to
copilot-pat-pool
GitHub Actions
Inactive
simonrozsival
temporarily deployed
to
copilot-pat-pool
GitHub Actions
Inactive
simonrozsival
temporarily deployed
to
copilot-pat-pool
GitHub Actions
Inactive
Contributor
There was a problem hiding this comment.
Pull request overview
This PR hardens the Android cryptography native PAL by adding missing JNI exception checks and tightening failure-path behavior so pending Java exceptions don’t leak into subsequent JNI transitions; it also updates managed interop to surface certain JNI failures as CryptographicException.
Changes:
- Added
ON_EXCEPTION_PRINT_AND_GOTO(...)/CheckJNIExceptions(...)checks after additional fallible JNI calls across the Android crypto PAL. - Adjusted native return conventions for some functions to better distinguish JNI-exception failures from legitimate “0 results”.
- Updated managed Android interop wrappers to throw when native indicates a JNI exception occurred (e.g., negative error counts / negative bignum conversions).
Show a summary per file
| File | Description |
|---|---|
| src/native/libs/System.Security.Cryptography.Native.Android/pal_x509chain.c | Adds JNI exception checks and tightens error materialization/cleanup when building/validating chains and collecting errors. |
| src/native/libs/System.Security.Cryptography.Native.Android/pal_x509.c | Adds missing exception checks after byte-array population JNI calls. |
| src/native/libs/System.Security.Cryptography.Native.Android/pal_sslstream.c | Adds exception checks around additional SSLEngine/ByteBuffer JNI operations to avoid leaking pending exceptions. |
| src/native/libs/System.Security.Cryptography.Native.Android/pal_pbkdf2.c | Adds exception check after NewDirectByteBuffer and other JNI byte-buffer operations. |
| src/native/libs/System.Security.Cryptography.Native.Android/pal_misc.c | Converts to a ret/cleanup pattern and adds some exception checks in GetRandomBytes. |
| src/native/libs/System.Security.Cryptography.Native.Android/pal_hmac.c | Adds exception checks in the HMAC finalization path to prevent pending exceptions from escaping. |
| src/native/libs/System.Security.Cryptography.Native.Android/pal_eckey.c | Improves JNI exception handling for EC key creation and curve name extraction. |
| src/native/libs/System.Security.Cryptography.Native.Android/pal_ecdsa.c | Ensures exception state is cleared/observed on getPrivate() failure path. |
| src/native/libs/System.Security.Cryptography.Native.Android/pal_cipher.c | Adds exception checks in cipher update/AAD paths and ensures local refs are released on all paths. |
| src/native/libs/System.Security.Cryptography.Native.Android/pal_bignum.c | Adds exception checks and returns a negative result on JNI failure for bignum-to-binary conversion. |
| src/libraries/Common/src/Interop/Android/System.Security.Cryptography.Native.Android/Interop.X509Chain.cs | Throws when native indicates a JNI exception during chain error-count retrieval. |
| src/libraries/Common/src/Interop/Android/System.Security.Cryptography.Native.Android/Interop.Bignum.cs | Propagates native “JNI exception occurred” via CryptographicException instead of returning corrupt/zero-filled data. |
Copilot's findings
- Files reviewed: 12/12 changed files
- Comments generated: 4
Comment on lines
23
to
+26
| jbyteArray buffArray = make_java_byte_array(env, len); | ||
| (*env)->SetByteArrayRegion(env, buffArray, 0, len, (jbyte*)buff); | ||
| (*env)->CallVoidMethod(env, randObj, g_randNextBytesMethod, buffArray); | ||
| ON_EXCEPTION_PRINT_AND_GOTO(cleanup); |
Comment on lines
271
to
+280
| JNIEnv* env = GetJNIEnv(); | ||
| int32_t ret = FAIL; | ||
| jbyteArray outDataBytes = NULL; | ||
| jbyteArray inDataBytes = make_java_byte_array(env, inl); | ||
| (*env)->SetByteArrayRegion(env, inDataBytes, 0, inl, (jbyte*)in); | ||
| ON_EXCEPTION_PRINT_AND_GOTO(cleanup); | ||
|
|
||
| *outl = 0; | ||
| jbyteArray outDataBytes = (jbyteArray)(*env)->CallObjectMethod(env, ctx->cipher, g_cipherUpdateMethod, inDataBytes); | ||
| outDataBytes = (jbyteArray)(*env)->CallObjectMethod(env, ctx->cipher, g_cipherUpdateMethod, inDataBytes); | ||
| ON_EXCEPTION_PRINT_AND_GOTO(cleanup); |
Comment on lines
493
to
+495
| (*env)->CallBooleanMethod(env, anchors, g_HashSetAdd, anchor); | ||
| (*env)->DeleteLocalRef(env, anchor); | ||
| ON_EXCEPTION_PRINT_AND_GOTO(cleanup); |
Comment on lines
+45
to
+47
| // A negative result means a pending JNI exception was cleared and no data was written. | ||
| // Fail loudly rather than silently returning a zero-filled (corrupt) buffer. | ||
| if (written < 0) |
Contributor
|
Tagging subscribers to 'arch-android': @vitek-karas, @simonrozsival, @steveisok, @akoeplinger |
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.
Note
This PR description was drafted by an AI assistant (GitHub Copilot CLI) based on the actual code changes in this branch. All code was authored interactively with Simon driving the design decisions.
Follow-up to #128777, which added the initial batch of JNI exception checks to the Android crypto PAL. This PR covers the remaining functions.
Summary
Adds missing JNI exception checks across the remaining functions in the Android cryptography PAL (
src/native/libs/System.Security.Cryptography.Native.Android/). The work is based on a static audit ofCall*Method/NewObject/Get*ArrayRegion-style JNI calls where a pending Java exception could otherwise leak back into managed code (undefined behavior for the next managed→native transition) or produce partial output.Beyond adding the checks, the touched functions are tightened to follow three patterns already established elsewhere in the PAL:
ON_EXCEPTION_PRINT_AND_GOTO(label)immediately after each fallible JNI call. The macro both prints and clears the exception, so subsequent JNI calls on thecleanuppath are safe.INIT_LOCALS(loc, …)+ a singlecleanup:/RELEASE_LOCALS(loc, env)tail for local-ref hygiene, replacing ad-hocjobject foo = NULL;declarations and scatteredDeleteLocalRefcalls.Native changes
All under
src/native/libs/System.Security.Cryptography.Native.Android/:pal_x509chain.cX509ChainBuild,X509ChainGetCertificateCount,X509ChainGetCertificates,X509ChainGetErrorCount,PopulateValidationError,X509ChainGetErrors,X509ChainSetCustomTrustStore,CreateCertPathFromAnchor,ValidateWithRevocation, and the error-capture branchespal_bignum.cBigNumToBinary,GetBigNumBytesIncludingPaddingByteForSignpal_cipher.cCipherSetKeyAndIV,CipherUpdateAAD,CipherUpdate(also releases the update output array on all paths)pal_ecdsa.cEcDsaSign(clear exception on thegetPrivate()failure path)pal_eckey.cNewEcKeyFromKeys,EcKeyCreateByOid(converted toINIT_LOCALS+ per-call checks),EcKeyGetCurveNamepal_hmac.cDoFinalpal_misc.cGetRandomBytespal_pbkdf2.cPbkdf2(clear the pending exception whenNewDirectByteBufferfails)pal_sslstream.cSSLStreamSetTargetHost,SSLStreamRead,SSLStreamRequestClientAuthentication,SSLStreamSetApplicationProtocolspal_x509.cX509Decode,X509DecodeCollection,X509GetContentType(checkSetByteArrayRegion)Managed changes
Interop.X509Chain.cs—X509ChainGetErrorCountnow returns a negative value when a JNI exception occurs (distinct from a legitimate count of0); the wrapper throwsCryptographicExceptionin that case instead of silently reporting "no errors" (which would leaveChainStatusempty for a failed validation).Interop.Bignum.cs—AndroidCryptoNative_BigNumToBinarynow returns-1and writes nothing when a JNI exception occurs;ExtractBignumthrowsCryptographicExceptionrather than returning a zero-filled (corrupt) buffer.Testing
libs -os android -a arm64 -rf CoreCLR(includesSystem.Security.Cryptography) — 0 warnings / 0 errors.CI will provide cross-architecture validation and device/emulator test runs.