[Android] Clear pending JNI exceptions in pal_rsa.c to avoid CheckJNI aborts

[simonrozsival]

Note

This PR description was drafted with help from GitHub Copilot.

Summary

Closes #128660.

Fixes a CheckJNI SIGABRT in the Android System.Security.Cryptography native library when importing or working with RSA keys that the Conscrypt + BoringSSL provider on the target device refuses to construct — most notably 16384-bit RSA on Pixel 6a / Android 16.

Root cause

AndroidCryptoNative_SetRsaParameters issued a chain of JNI calls without checking for pending Java exceptions in between. The crash from issue #128660 follows this exact pattern:

1. KeyFactory.generatePrivate(rsaPrivateKeySpec) is called for a 16384-bit private key.

2. Conscrypt + BoringSSL rejects the key and throws InvalidKeySpecException (BoringSSL caps RSA at 8192 bits on this device). The exception is left pending on the JNI thread.

3. The next JNI call — NewObject(g_RSAPublicCrtKeySpecClass, …) — runs with that pending exception, which is a JNI spec violation.

4. ART CheckJNI detects the violation and aborts the process:

   JNI ERROR (app bug): JNI NewObject called with pending exception java.security.spec.InvalidKeySpecException

The crash bypasses any managed-side catch (CryptographicException), so callers cannot recover, and any test relying on a graceful failure for unsupported key sizes (e.g. RSAKeyFileTests.Supports16384) blocks clean CI.

Because we only have a stack trace and no repro device, the fix is defensive: every JNI-using function in pal_rsa.c now clears any pending exception between JNI calls and propagates the failure to the managed caller as FAIL, which the existing C# code already converts to a recoverable CryptographicException.

Changes

Single file: src/native/libs/System.Security.Cryptography.Native.Android/pal_rsa.c.

Nine functions hardened to:

• check for pending JNI exceptions between JNI calls using ON_EXCEPTION_PRINT_AND_GOTO(cleanup) (which calls ExceptionDescribe + ExceptionClear);
• converge on a single cleanup: (or error:) label;
• use NULL-safe ReleaseLRef / ReleaseGRef helpers instead of hand-rolled if (x) DeleteLocalRef(…) chains;
• initialize all output handles up front so partial failures leave caller-visible state in a defined NULL.

Hardened functions:

Function	Why it could leave a pending exception
AndroidCryptoNative_SetRsaParameters	Primary fix. KeyFactory.generatePrivate(...) / generatePublic(...) reject keys the provider can't handle (e.g. 16384-bit RSA).
AndroidCryptoNative_GetRsaParameters	RSAPrivateCrtKey / RSAPublicKey field accessors are JNI calls; the error: path now also releases each output GRef and resets the slot to NULL so the managed-side SafeBignumHandle.Dispose() becomes a safe no-op (no double-free, no leak — IsInvalid returns true for IntPtr.Zero).
AndroidCryptoNative_RsaPublicEncrypt	Cipher.getInstance(...) / Cipher.init(...) / Cipher.doFinal(...) can throw NoSuchAlgorithmException, InvalidKeyException, IllegalBlockSizeException, BadPaddingException.
AndroidCryptoNative_RsaPrivateDecrypt	Same as above.
AndroidCryptoNative_RsaSignPrimitive	Same as above.
AndroidCryptoNative_RsaVerificationPrimitive	Same as above.
AndroidCryptoNative_DecodeRsaSubjectPublicKeyInfo	KeyFactory.getInstance(...) / new X509EncodedKeySpec(...) / KeyFactory.generatePublic(...) can throw NoSuchAlgorithmException, InvalidKeySpecException.
AndroidCryptoNative_RsaGenerateKeyEx	KeyPairGenerator.getInstance(...) / initialize(bits) / genKeyPair() can throw NoSuchAlgorithmException, InvalidParameterException.
AndroidCryptoNative_NewRsaFromKeys	RSAKey.getModulus() is a JNI call that should not be assumed exception-free.

No managed-side or API changes; behavior is identical on the success path. On failure, the functions now return the existing FAIL / RSA_FAIL sentinel without aborting the process, and the managed callers (RSAAndroid, Interop.AndroidCrypto.ExportRsaParameters) already convert those into CryptographicException.

Test plan

System.Security.Cryptography.Tests (the suite that contains the RSA importer tests in #128660) was run twice on Android arm64 with the change applied:

Device	Tests run	Passed	Failed	Skipped
Samsung Galaxy A16 5G (Android 16, arm64-v8a)	11715	10738	0	977
Android emulator (API 36, arm64-v8a)	12373	10738	0	977

Both runs match the pre-change baseline (no regressions). The 16384-bit KeyFactory.generatePrivate failure path could not be exercised directly because the Conscrypt build on these particular devices accepts 16384-bit RSA — neither hardware reproduces the abort from #128660. The change is justified by the stack trace alone: adding the exception check at every JNI call site in pal_rsa.c is the standard hardening pattern used elsewhere in this directory (see pal_dsa.c, pal_x509.c).

Once merged, RSAKeyFileTests.Supports16384 will fall back to Imported = false cleanly on devices where Conscrypt rejects the key, which unblocks CI on the affected hardware without changing behavior on devices that accept the key.

Risk

Low. Single file; no managed-side or API changes; success paths are unchanged; failure paths only switch from a process abort to a recoverable CryptographicException.

Notes for reviewers

• pal_dsa.c, pal_ecc_import_export.c, and other sibling files in the same directory have a few similar patterns (chained JNI calls without intermediate exception checks). I deliberately scoped this PR to pal_rsa.c to keep the fix small and easy to backport; a follow-up sweep would be a sensible next step but is out of scope here.
• Some pre-existing minor oddities in pal_rsa.c (e.g. the redundant oaepParameterSpec != NULL && oaepParameterSpec != FAIL check in RsaPublicEncrypt, since FAIL == NULL for jobject) were left untouched to keep the diff focused on the bug.

[dotnet-policy-service]

Tagging subscribers to this area: @bartonjs, @vcsjones, @dotnet/area-system-security
See info in area-owners.md if you want to be subscribed.

[Copilot]

Pull request overview

This PR hardens the Android RSA native interop layer (pal_rsa.c) by adding consistent “check-and-clear pending JNI exception” points between JNI calls, so Java-side failures (e.g., provider rejecting a key) don’t cascade into CheckJNI process aborts and instead flow back to managed code via existing FAIL/RSA_FAIL return conventions.

Changes:

• Added ON_EXCEPTION_PRINT_AND_GOTO(...) checks after JNI calls in the RSA encrypt/decrypt/sign/verify, import/decode, keygen, and parameter get/set paths.
• Refactored several functions to a single cleanup:/error: label and centralized local/global ref release via ReleaseLRef / ReleaseGRef.
• Ensured RSA parameter “out” handles are initialized and reliably cleared on error to keep managed cleanup safe.

[simonrozsival]

Note

This comment was generated with GitHub Copilot.

Addressed the two ⚠️ findings from the automated Code Review pass about partial state on failure:

• RsaGenerateKeyEx now stages the new private/public key GRefs in locals (newPrivateKey/newPublicKey) and commits to `rsa->privateKey`/`publicKey`/`keyWidthInBits` only after both `getPrivate` and `getPublic` succeed. Cleanup releases any staged GRefs that weren't transferred.
• `SetRsaParameters` does the same: both new keys are staged, then committed atomically (`keyWidthInBits` is updated only on success too). Pre-existing `dLength == 0` semantics are preserved (private key is left untouched when the input doesn't contain one.

Verified on emulator-5554 (API 36, arm64-v8a): 12373/10738/0 failed/977 skipped — unchanged from the previous run.
EOF
)

[github-actions]

🤖 Copilot Code Review — PR #128747

Holistic Assessment

Motivation: Justified. The root cause is clear from the stack trace: chained JNI calls without intermediate exception checks violate the JNI spec and cause unrecoverable SIGABRT under CheckJNI. This is a real bug that blocks CI on affected hardware.

Approach: Correct. Adding ON_EXCEPTION_PRINT_AND_GOTO between JNI calls is the established hardening pattern in this directory, and the "stage into temporaries, assign only on full success" pattern for SetRsaParameters / RsaGenerateKeyEx prevents partial mutation of the RSA struct on failure. The scope is appropriately limited to pal_rsa.c.

Summary: ✅ LGTM. The change is defensive, consistent with sibling files, and correct on both the success and failure paths. One minor observation noted below but nothing blocking.

---

Detailed Findings

✅ Exception checking coverage

All nine hardened functions now check for pending JNI exceptions after every JNI call that can throw. The ON_EXCEPTION_PRINT_AND_GOTO macro correctly calls CheckJNIExceptions(env) which does ExceptionDescribe + ExceptionClear before the goto, ensuring no pending exception is left when the next JNI call (in cleanup or caller) executes.

✅ Partial-failure safety in SetRsaParameters and RsaGenerateKeyEx

Both functions now stage new keys into local variables (newPrivateKey, newPublicKey) and only assign them to rsa-> fields after all JNI calls succeed. On failure, the temporaries are released via ReleaseGRef and the RSA struct is left unchanged. This is the correct pattern — the old code could leave rsa->privateKey updated but rsa->publicKey stale if the second KeyFactory.generatePublic() threw.

✅ Output parameter initialization in GetRsaParameters

All eight output pointers are now initialized to NULL up front (line 431–438), and the error: label releases any partially-assigned GRefs and resets them to NULL. This ensures the managed-side SafeBignumHandle.Dispose() is a safe no-op (IsInvalid returns true for IntPtr.Zero).

✅ Resource cleanup consolidation

RsaPrivateDecrypt and others consolidate from multiple hand-rolled DeleteLocalRef chains into a single cleanup: label with ReleaseLRef calls. Since ReleaseLRef is NULL-safe, this is correct for all exit paths.

💡 RsaVerificationPrimitive returns FAIL (0) vs RSA_FAIL (-1)

RsaVerificationPrimitive initializes ret = FAIL (0) while its sibling functions (RsaSignPrimitive, RsaPublicEncrypt, RsaPrivateDecrypt) use RSA_FAIL (-1). This is a pre-existing inconsistency (the old code also returned FAIL), so it's correctly out of scope for this PR, but worth noting for a future cleanup.

✅ No managed-side or API changes

The change is entirely within pal_rsa.c. Success paths are behaviorally identical. Failure paths now return the existing FAIL / RSA_FAIL sentinel instead of aborting, which the managed callers already convert to CryptographicException.

Generated by Code Review for issue #128747 · ● 2M · ◷

[vcsjones]

This implies we need to fix RSA. LegalKeySizes on Android. I think we can do this separately from this PR but we may want to make a tracking issue for it.

[simonrozsival]

/azp run runtime-android

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).