Repository navigation
Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates the Android RSA parameter export path to avoid calling RSAPrivateCrtKey APIs on KeyStore-backed private key instances that don’t implement that interface, and adds coverage for the KeyStore scenario. The native layer now gates CRT-only parameter extraction behind an explicit includePrivateParameters flag and type checks, enabling public-parameter export without triggering a JNI abort.
Changes:
- Add an
includePrivateParametersargument toAndroidCryptoNative_GetRsaParametersand thread it through the managed interop. - In the native implementation, only export CRT private parameters when explicitly requested and the private key is an
RSAPrivateCrtKey; otherwise export public parameters from the public key. - Add an Android-specific X509 store regression test ensuring public export succeeds and private export throws.
Show a summary per file
| File | Description |
|---|---|
| src/native/libs/System.Security.Cryptography.Native.Android/pal_rsa.h | Updates RSA key-type comments and adds includePrivateParameters to the exported parameters API signature. |
| src/native/libs/System.Security.Cryptography.Native.Android/pal_rsa.c | Implements conditional RSA parameter export: CRT-only private export when requested; otherwise export via public key or fail safely. |
| src/libraries/Common/src/Interop/Android/System.Security.Cryptography.Native.Android/Interop.Rsa.cs | Updates the P/Invoke signature and passes the new includePrivateParameters flag to native. |
| src/libraries/System.Security.Cryptography/tests/X509Certificates/X509StoreMutableTests.Android.cs | Adds a regression test for KeyStore-backed RSA public export and private export failure behavior. |
Copilot's findings
- Files reviewed: 4/4 changed files
- Comments generated: 1
bartonjs
left a comment
bartonjs
left a comment
There was a problem hiding this comment.
I don't remember off the top of my head of FindByThumprint clones the cert instances or not. If Copilot's right, then, yeah, dispose storeCert. (If it's wrong, the extra dispose doesn't really hurt, but isn't needed)
It doesn't. |
|
Tagging subscribers to 'arch-android': @vitek-karas, @simonrozsival, @steveisok, @akoeplinger |
|
/azp run runtime-android |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
@vcsjones FYI, merge conflicts exists now. |
|
Yeah I think it makes sense to wait for #128777 to merge first. |
…-key-export # Conflicts: # src/native/libs/System.Security.Cryptography.Native.Android/pal_rsa.c
|
/azp run runtime-android |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
|
/azp list |
|
@simonrozsival is there a way to run Android with OuterLoop? |
|
Workflow state for the Holistic Review Orchestrator. {
"version": 5,
"last_dispatched_commit": "e1093ef232b06338aa59a91e6563c7e75ec0db81",
"last_dispatched_base_ref": "main",
"last_dispatched_base_sha": "c86503b722d7358981cff5a18711ee55fc54e8fe",
"last_reviewed_commit": "e1093ef232b06338aa59a91e6563c7e75ec0db81",
"last_reviewed_base_ref": "main",
"last_reviewed_base_sha": "c86503b722d7358981cff5a18711ee55fc54e8fe",
"last_recorded_worker_run_id": "29684015108",
"review_attempt_commit": "",
"review_attempt_base_ref": "",
"review_attempt_count": 0,
"max_review_attempts": 5,
"review_history_format": "holistic-review-disclosure-v1",
"review_history": [
{
"commit": "e1093ef232b06338aa59a91e6563c7e75ec0db81",
"review_id": 4730632365
}
]
} |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Holistic Review
Motivation: On Android, an RSA private key retrieved from the KeyStore is represented by android.security.keystore.AndroidKeyStoreRSAPrivateKey, which does not implement java.security.interfaces.RSAPrivateCrtKey. The existing exporter unconditionally invoked RSAPrivateCrtKey methods (e.g. getPublicExponent()) whenever a private key object was present, causing a hard JNI abort crash of the Android runtime (issue #119924) rather than a recoverable managed exception.
Approach: AndroidCryptoNative_GetRsaParameters gains an includePrivateParameters flag plumbed through from the managed ExportRsaParameters/GetRsaParameters P/Invoke. When private parameters are requested, the code now requires the private key to actually be an RSAPrivateCrtKey (via IsInstanceOf) and returns FAIL otherwise, which surfaces as a CryptographicException. For public-only export, it prefers the publicKey, and falls back to extracting the public modulus/exponent from a CRT private key when no public key object is available. The struct field comments are relaxed from RSAPrivateCrtKey/RSAPublicCrtKey to the more accurate RSAPrivateKey/RSAPublicKey. A new Android test asserts public export succeeds and private export throws CryptographicException for a KeyStore-backed key.
Summary: This is a correct, well-scoped fix that converts a fatal native abort into a recoverable managed exception while preserving the documented limitation that KeyStore keys cannot export private material. The IsInstanceOf guard is applied consistently to every path that dereferences CRT-specific methods, out parameters are initialized up front, and the error: cleanup path releases global refs properly. The managed marshalling (includePrivateParameters ? 1 : 0 to an int) matches the native int32_t signature, and the added regression test exercises the intended behavior. I found no correctness, resource, or consistency issues in the changed lines. LGTM.
Note
This review was generated by this repository's Holistic Review agentic workflow to complement the built-in Copilot review.
Generated by Holistic Review · 57.2 AIC · ⌖ 10.4 AIC · ⊞ 10K
Android has multiple ways to represent a private key. Our RSAParameters exporter assumed that we would always export off an instance of
java.security.interfaces.RSAPrivateCrtKey. But that is not true if the private key was retrieved from the key store - the private key is represented byandroid.security.keystore.AndroidKeyStoreRSAPrivateKey.This would crash the Android runtime with something like:
AndroidKeyStoreRSAPrivateKeydo not permit exporting the private key because they are likely in the TEE. We can only get the public parameters.This does mean, unfortunately, that
ExportParameters(includePrivateParameters: true)will fail on KeyStore keys because Android doesn't let us get the key material back. We can get the public key back withincludePrivateParameters: false.This at least fixes the hard android runtime crash and turns it in to a CryptographicException.
Fixes #119924