[Android] Handle X509ChainContext creation failures

[simonrozsival]

Note

This PR description was drafted with GitHub Copilot.

An Android production app reported a native abort while building an X.509 chain on arm64. The available tombstone snippet showed the process aborting in AndroidCryptoNative_X509ChainBuild from pal_x509chain.c, with the native guard reporting that parameter ctx was not a valid pointer. The report did not include a repro or full tombstone, but the observed failure mode means managed code reached the native build entry point with a null X509ChainContext*.

Thread
/__w/1/s/src/native/libs/System.Security.Cryptography.Native.Android/pal_x509chain.c:113 (AndroidCryptoNative_X509ChainBuild): Parameter 'ctx' must be a valid pointer
 
*** *** *** *** *** *** *** *** *** *** *** *** *** *** *** ***
pid: 0, tid: 31609 >>> com.app.name <<<
 
backtrace:
  #00  pc 0x000000000002232c  /system/lib64/libc.so (abort+116)
  #01  pc 0x0000000000021fe8  [removed]-KwPZdoEumri00C7kBm3pQw==/lib/arm64/libSystem.Security.Cryptography.Native.Android.so
  #02  pc 0x00000000000220b0  [removed]-KwPZdoEumri00C7kBm3pQw==/lib/arm64/libSystem.Security.Cryptography.Native.Android.so (AndroidCryptoNative_X509ChainBuild+88)
  #03  pc 0x000000000000cfcc

X509ChainContext is created by AndroidCryptoNative_X509ChainCreateContext. That initialization can fail if Android certificate store setup or PKIX parameter construction throws, or if required JNI global references cannot be created. Previously, the managed Android chain path stored the returned SafeHandle without checking whether context creation failed, so a later build could pass a null native context to AndroidCryptoNative_X509ChainBuild and terminate the app process.

This change makes context creation fail gracefully:

• The native create path checks Java exceptions around object creation and method calls more consistently.
• Partial native contexts are destroyed if global-reference creation fails.
• The managed Android chain path checks the returned chain context immediately and throws CryptographicException if initialization failed.
• The managed exception points developers to logcat for native diagnostics.

No regression test is included because the reliable failure modes depend on Android platform/provider state or artificial fault injection, and a test hook would be fragile and not representative.

[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 X.509 chain-building pipeline so that failures during native X509ChainContext initialization don’t later surface as a process-terminating native abort, and instead fail fast with a managed CryptographicException.

Changes:

• Add more consistent JNI exception handling during native X509ChainContext creation and pre-create the Java errorList.
• Validate the returned Android chain context immediately after creation on the managed side and throw a CryptographicException when initialization fails.
• Add a new localized resource string for the managed exception message.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File	Description
src/native/libs/System.Security.Cryptography.Native.Android/pal_x509chain.c	Adds additional JNI exception checks and changes how errorList is created/held during chain context initialization.
src/libraries/System.Security.Cryptography/src/System/Security/Cryptography/X509Certificates/ChainPal.Android.cs	Throws early if the returned chain context handle is invalid, preventing later native calls with a bad context.
src/libraries/System.Security.Cryptography/src/Resources/Strings.resx	Adds the new error message resource used by the managed exception.

[simonrozsival]

Note

This reply was generated by GitHub Copilot.

Addressed in 1b689fb. AndroidCryptoNative_X509ChainCreateContext now checks both global-reference creations after AddGRef. If either params or errorList cannot be promoted to a global ref, it prints/clears any pending JNI exception, releases any partial native state, frees the context, and returns NULL so managed code fails gracefully instead of receiving a partially initialized context.

[simonrozsival]

Note

This reply was generated by GitHub Copilot.

Addressed in 7de43db. I moved Cryptography_AndroidX509ChainContextInitializationFailed into the sorted Cryptography_* position, after Cryptography_AlgorithmTypesMustBeVisible and before Cryptography_ArgDSARequiresDSAKey.

[vcsjones]

We typically try to put these kinds of checks as close to the interop boundary as possible now. For example:

runtime/src/libraries/Common/src/Interop/Unix/System.Security.Cryptography.Native/Interop.EvpPkey.X25519.cs

Lines 30 to 31 in 50b8d1a

	[LibraryImport(Libraries.CryptoNative, EntryPoint = "CryptoNative_X25519GenerateKey")]
	private static partial SafeEvpPKeyHandle CryptoNative_X25519GenerateKey();

runtime/src/libraries/Common/src/Interop/Unix/System.Security.Cryptography.Native/Interop.EvpPkey.X25519.cs

Lines 91 to 103 in 50b8d1a

	internal static SafeEvpPKeyHandle X25519GenerateKey()
	{
	SafeEvpPKeyHandle key = CryptoNative_X25519GenerateKey();
	if (key.IsInvalid)
	{
	Exception ex = CreateOpenSslCryptographicException();
	key.Dispose();
	throw ex;
	}
	return key;
	}

So maybe the right thing to do is to make the actual LibraryImport call private and have the internal one check it. (Also make sure you dispose the handle)

[simonrozsival]

Note

This reply was generated by GitHub Copilot.

Addressed in 7de43db. The raw LibraryImport is now private (X509ChainCreateContext_private), and the internal X509ChainCreateContext wrapper performs the invalid-handle check at the interop boundary. If the handle is invalid, the wrapper disposes it before throwing, and the higher-level ChainPal.Android check was removed.

[vcsjones]

Does android not have an exception throw helper that includes exception messages from the JNI / Java side? It feels weird to say "See logcat for details" in the exception message.

[simonrozsival]

Note

This reply was generated by GitHub Copilot.

Addressed in 7de43db. I did not find an existing managed Android crypto helper that propagates the Java/JNI exception message here; the native helper prints and clears JNI exceptions. To avoid the odd guidance, I simplified the resource string to: The Android X.509 certificate chain could not be initialized.

[github-actions]

Caution

Security scanning requires review for Code Review

Details

The threat detection results could not be parsed. The workflow output should be reviewed before merging.

Review the workflow run logs for details.

🤖 Copilot Code Review — PR #128651

Note

This review was generated by GitHub Copilot.

Holistic Assessment

Motivation: Justified reliability fix for a real production native abort on Android when X509ChainContext creation fails and a null pointer is later passed to AndroidCryptoNative_X509ChainBuild.

Approach: Correct two-layer defense: harden the native JNI code with consistent exception checks and partial-context cleanup, then validate at the managed interop boundary with a clear CryptographicException. This follows the established pattern (e.g., Interop.EvpPkey.X25519.cs) as requested by the area maintainer.

Summary: ✅ LGTM. The change is small, focused, and addresses all prior review feedback. The native code now consistently checks JNI exceptions, properly cleans up partial state on AddGRef failure, and the managed wrapper validates the handle at the interop boundary before it can propagate.

---

Detailed Findings

✅ Native JNI exception handling — Correct and complete

All JNI calls that previously lacked exception checks (NewObject for X509CertSelector, CallVoidMethod for setCertificate, CallBooleanMethod for ArrayList.add, CallVoidMethod for addCertStore) now have ON_EXCEPTION_PRINT_AND_GOTO(cleanup). This prevents undefined behavior from subsequent JNI calls when an exception is pending.

✅ AddGRef null checks — Addresses previous gap

The latest commit adds explicit null checks after AddGRef for both ret->params and ret->errorList. On failure, it clears JNI exceptions, releases any already-acquired global refs, frees the context, and returns NULL. This eliminates the scenario where a partially-initialized context reaches managed code.

✅ errorList lifecycle — Correct improvement

Moving errorList creation into INIT_LOCALS ensures the local ref is properly released via RELEASE_LOCALS on all paths (including early goto cleanup). The old ToGRef pattern would have leaked the local ref if NewGlobalRef failed.

✅ Managed interop wrapper — Follows established conventions

The private/internal split pattern (X509ChainCreateContext_private + public wrapper) matches the Unix crypto interop style. The wrapper checks IsInvalid, disposes the handle, and throws with a descriptive resource string. This converts a deferred native abort into an immediate managed exception.

✅ No test — Justified

The failure modes require JNI OOM or Android provider failures that cannot be reliably reproduced in CI. The PR description adequately explains this.

Generated by Code Review for PR #128651 · ◷

Generated by Code Review for issue #128651 · ● 1.4M · ◷

[Copilot]

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.