Implement ECC APIs on Android using Android-built-in Crypto APIs #48348

jkoritzinsky · 2021-02-16T18:12:48Z

Implement the ECC key management, import/export, ECDSA, and ECDH APIs using the Java-based Crypto APIs available on Android.

cc: @bartonjs

…ly implemented CryptoNative_EcKeyCreateByExplicitParameters.

…handling still todo.

ghost · 2021-02-16T18:12:51Z

Tagging subscribers to this area: @bartonjs, @vcsjones, @krwq, @GrabYourPitchforks
See info in area-owners.md if you want to be subscribed.

Issue Details

Implement the ECC key management, import/export, and ECDSA APIs using the Java-based Crypto APIs available on Android.

cc: @bartonjs

Author:	jkoritzinsky
Assignees:	-
Labels:	`area-System.Security`
Milestone:	6.0.0

src/libraries/Native/Unix/System.Security.Cryptography.Native.Android/pal_evp_cipher.c

src/libraries/Native/Unix/System.Security.Cryptography.Native.Android/pal_ecc_import_export.c

src/libraries/Native/Unix/System.Security.Cryptography.Native.Android/pal_eckey.h

src/libraries/Native/Unix/System.Security.Cryptography.Native.Android/pal_ecdsa.h

src/libraries/Native/Unix/System.Security.Cryptography.Native.Android/pal_ecdsa.c

AaronRobinsonMSFT · 2021-02-16T18:25:19Z

src/libraries/Native/Unix/System.Security.Cryptography.Native.Android/pal_ecdsa.c

+    jstring algorithmName = JSTRING("SHA1withECDSA");
+    jobject signatureObject = (*env)->CallStaticObjectMethod(env, g_SignatureClass, g_SignatureGetInstance, algorithmName);
+    (*env)->DeleteLocalRef(algorithmName);
+    if (CheckJNIExceptions()) return NULL;


Are we not printing the exception here? Does this indicate a sort of catastrophic failure?

The CheckJNIExceptions() method prints out the exceptions to the Android logs.

Okay. So NULL is okay here then, but we immediately return in all places where NULL is returned but don't record in that scenario. It seems to me that this code should be as boiler plate as possible and this function handles errors but does the caller so I am just trying to understand how reconciles ownership of when to report and when not to. Also, I think we can pass in env right?

I like having the CheckJNIExceptions() call as close to the throwing Java method call as possible for clarity. Since this method calls the Java Signature.getInstance method, it checks if it threw an exception.

The callers of this method don't need to check if it threw an exception since we already checked that.

I think a good way to encapsulate the model for JNI exception checking in the PAL is, "a JNI exception will never be left pending across a PAL function return." Or at least that's my mental model of it.

I think what I'm getting at is this function seems like 3 lines in 2 places and can be removed entirely. Just my opinion. Up to you.

Yeah that's true. I'll think about removing it. We might want something similar for RSA signing, so I might generalize it for that.

src/libraries/Native/Unix/System.Security.Cryptography.Native.Android/pal_eckey.c

src/libraries/Native/Unix/System.Security.Cryptography.Native.Android/pal_ecc_import_export.h

AaronRobinsonMSFT · 2021-02-16T18:29:47Z

src/libraries/Native/Unix/System.Security.Cryptography.Native.Android/pal_ecc_import_export.c

+    jobject* qy, int32_t* cbQy,
+    jobject* d, int32_t* cbD)
+{
+    assert(qx != NULL);


What is the logic to assert here as opposed to return an error code? In other functions it seems we do a hard check instead of simply asserting? Perhaps this is an internal function? I would recommend we be consistent here.

We guard in the error case for d but not here?

I guess the asserts are copied from OpenSSL's CryptoNative_GetECKeyParameters

The shims are considered 100% as internal functions. We're inconsistent of asserting vs error checking (largely, IIRC, error checking was first, when we had the possibility of torn state of the native shim package and the caller package... now that we have shared frameworks some of the newer things assert)

The shims are considered 100% as internal functions.
now that we have shared frameworks some of the newer things assert

W00t. If these functions are internal only I would prefer to limit this sort of validation to asserts only. My assumption is we will always call these functions correctly and find incorrect internal usage with testing.

I've noticed that a lot of places in the crypto tests push the validation down to the shim/native library, so we can't use asserts for those ones since they'll take down the process.

It all comes back to the current "time-saving" effort of reusing the OpenSSL-based implementations. The tests (generally) don't care where things fail, just that CryptographicException-derived exceptions happen. If you make ECDsaAndroid and it does more work in managed code you can do more asserting and less runtime error checking.

If there are tests that care about specifics for the exceptions I believe those should only care about using the concrete public ECDsaOpenSsl type, which should NOT work on Android since it can't speak pointers in an actually interoperable way (and the pointer interop is the only reason the type is public).

AaronRobinsonMSFT · 2021-02-16T18:33:23Z

src/libraries/Native/Unix/System.Security.Cryptography.Native.Android/pal_ecc_import_export.c

+    const EC_KEY* key,
+    int32_t includePrivate,
+    ECCurveType* curveType,
+    jobject* qx, int32_t* cbQx,


Might it make sense to create a blittable in/out value type for these large pointer sets? I assume all these functions are for internal usage and not external consumption.

My answer to this, and everything else in the shim API, is "do whatever makes sense for this interop". Just make a new ECDsaAndroid/ECDiffieHellmanAndroid as internal types in System.Security.Cryptography.Algorithms and own the interop for them entirely. There's no reason to stay aligned with the OpenSSL shim API... especially since I've already started the process of rewriting that one 😄.

src/libraries/Native/Unix/System.Security.Cryptography.Native.Android/pal_ecc_import_export.c

EgorBo · 2021-02-16T19:11:41Z

My two cents (unrelated to this PR)

I'd rewrite C to C++ honestly, it'd be less verbose e.g. (*env)->Foo(env) to env->Foo() at a cost of a minor size regression.
I'd make the declarations of classes/methods to be table-based on macros, e.g. something like this: https://github.com/dotnet/runtime/blob/master/src/coreclr/jit/instrsxarch.h

src/libraries/Native/Unix/System.Security.Cryptography.Native.Android/pal_ecc_import_export.c

AaronRobinsonMSFT · 2021-02-16T19:20:51Z

I'd rewrite C to C++ honestly, it'd be less verbose e.g. (*env)->Foo(env) to env->Foo() at a cost of a minor size regression.

I don't think that is an option here - at least I was under the impression we were required to use pure C.

I'd make the declarations of classes/methods to be table-based on macros, e.g. something like this: https://github.com/dotnet/runtime/blob/master/src/coreclr/jit/instrsxarch.h

I think we can consider that pattern if we are keeping with C. However, if we are going with C++ that sort of macro magic should be avoided as it can't be debugged and modern C++ has mechanisms to avoid those patterns.

EgorBo · 2021-02-16T19:24:40Z

I don't think that is an option here - at least I was under the impression we were required to use pure C.

AFAIK, System.Native.* libs were initially C++ in corefx but then were rewritten to C to be: 1) Mono-friendly (Mono used to be C-only and couldn't consume/build C++ files) 2) to save some space (not much) by dropping stdlib.

AaronRobinsonMSFT · 2021-02-16T19:26:47Z

Mono-friendly (Mono used to be C-only and couldn't consume/build C++ files)

Yep. That is what I was alluding to. Does Mono now permit C++ in their scenarios?

EgorBo · 2021-02-16T19:28:18Z

Mono-friendly (Mono used to be C-only and couldn't consume/build C++ files)

Yep. That is what I was alluding to. Does Mono now permit C++ in their scenarios?

Well, this lib is a standalone dynamic lib so it doesn't matter at all - cmake will just build a *.so and Mono will be able to pinvoke it.
UPD even if it's linked statically - we still use Android NDK's C++ compiler to build the mono runtime as far as I remember.
cc-ying @akoeplinger @lambdageek to verify my statements regarding C++

AaronRobinsonMSFT · 2021-02-16T19:30:28Z

Well, this lib is a standalone dynamic lib so it doesn't matter at all - cmake will just build a *.so and Mono will be able to pinvoke it.

I am fine with that if it is okay. We will have to make sure the target platforms provide the C++ runtime and ensure no exceptions cross the binary boundary but other than those two considerations I am all for it.

filipnavara · 2021-02-16T19:34:51Z

There were some other reasons for the C++ > C conversion. Maybe @am11 will remember them? It may not matter in this case if it's specific to Android platform.

AaronRobinsonMSFT · 2021-02-16T19:46:06Z

@filipnavara @am11 @VSadov Was the decision based on the Single File + self-contained work?

src/libraries/Native/Unix/System.Security.Cryptography.Native.Android/pal_ecc_import_export.c

src/libraries/Native/Unix/System.Security.Cryptography.Native.Android/pal_ecdsa.c

...ries/Common/src/Interop/Android/System.Security.Cryptography.Native.Android/Interop.EcKey.cs

src/libraries/Common/src/Interop/Unix/System.Security.Cryptography.Native/Interop.Dsa.cs

src/libraries/Native/Unix/System.Security.Cryptography.Native.Android/pal_ecc_import_export.c

src/libraries/Native/Unix/System.Security.Cryptography.Native.Android/pal_eckey.c

src/libraries/Native/Unix/System.Security.Cryptography.Native.Android/pal_jni.h

....Security.Cryptography.Algorithms/src/System/Security/Cryptography/ECAndroid.ImportExport.cs

…id-ecc

...ty.Cryptography.Algorithms/src/System/Security/Cryptography/ECDiffieHellmanAndroid.Derive.cs

src/libraries/Native/Unix/System.Security.Cryptography.Native.Android/pal_eckey.c

...ty.Cryptography.Algorithms/src/System/Security/Cryptography/ECDiffieHellmanAndroid.Derive.cs

bartonjs

Two outstanding bits of housekeeping:

Use BitsToBytes instead of repeating the formula in managed code
After verifying how leading zeros work, either assert or handle the case where bytesWritten in ECDH isn't the expected value.

...n/tests/System/Security/Cryptography/AlgorithmImplementations/ECDsa/ECDsaTests.netcoreapp.cs

…droid-ecc

ghost · 2021-02-26T23:12:10Z

Hello @jkoritzinsky!

Because this pull request has the auto-merge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.

jkoritzinsky · 2021-02-26T23:53:46Z

Merging in as this PR has no effect on iOS or the standard Android build with OpenSSL.

jkoritzinsky added 8 commits February 10, 2021 16:01

Copy over OpenSSL ECC impl and start reimplementing with JNI. Current…

796242c

…ly implemented CryptoNative_EcKeyCreateByExplicitParameters.

Implement pal_eckey APIs and stub out required JNI interfaces. Error …

24a6097

…handling still todo.

Fill in implementations of ECC export methods.

bed8251

Fix C build (linking still tbd).

7dcb934

Fix linking and initialize JNI.

bf14c95

Handle lifetime and exceptions.

a4a8048

Implement ECDSA APIs.

82b18db

Add some needed exception handling and fix incorrect member name.

459d821

jkoritzinsky added the area-System.Security label Feb 16, 2021

jkoritzinsky added this to the 6.0.0 milestone Feb 16, 2021

jkoritzinsky requested review from AaronRobinsonMSFT, elinor-fung and steveisok February 16, 2021 18:12

Merge branch 'master' into android-ecc

46cb701

jkoritzinsky commented Feb 16, 2021

View reviewed changes

src/libraries/Native/Unix/System.Security.Cryptography.Native.Android/pal_evp_cipher.c Outdated Show resolved Hide resolved

filipnavara reviewed Feb 16, 2021

View reviewed changes

src/libraries/Native/Unix/System.Security.Cryptography.Native.Android/pal_ecc_import_export.c Show resolved Hide resolved

AaronRobinsonMSFT reviewed Feb 16, 2021

View reviewed changes

EgorBo reviewed Feb 16, 2021

View reviewed changes

src/libraries/Native/Unix/System.Security.Cryptography.Native.Android/pal_ecc_import_export.c Outdated Show resolved Hide resolved

runfoapp bot mentioned this pull request Feb 16, 2021

System.Threading.Tasks.Tests timed out on net5.0-Linux-Debug-arm64-Mono_release #42024

Closed

bartonjs reviewed Feb 16, 2021

View reviewed changes

src/libraries/Native/Unix/System.Security.Cryptography.Native.Android/pal_ecc_import_export.c Show resolved Hide resolved

bartonjs reviewed Feb 16, 2021

View reviewed changes

src/libraries/Native/Unix/System.Security.Cryptography.Native.Android/pal_ecdsa.c Outdated Show resolved Hide resolved