Fix X509 test failures on Android (#50301)

* Disable tests on Android that use unsupported features (Brainpool curve, PSS padding in cert signatures) * Handle JNI thread shutdown * On older Android API levels, ECPrivateKey does not implement Destroyable * On older API levels, we can end up getting a certificate with a null public key. Return early with null instead of crashing with an assert. * Condition another test on SupportsBrainpool for clarity. * Fix X509 test failures found when running against an API Level 21 Android simulator. * Reuse existing item groups when possible. * Remove extraneous test suppression and handle exceptions. * Fix bug in x509 get certs. * Disable X509 tests on brower with ProjectExclusions. * Fix ChainTests.TestResetMethod test fix. * PR feedback. * Fix test project path.
dotnet · Apr 6, 2021 · c0a710f · c0a710f
1 parent bc8be66
commit c0a710f
Show file tree

Hide file tree

Showing 17 changed files with 139 additions and 25 deletions.
diff --git a/...mmon/src/Interop/Android/System.Security.Cryptography.Native.Android/Interop.X509Chain.cs b/...mmon/src/Interop/Android/System.Security.Cryptography.Native.Android/Interop.X509Chain.cs
@@ -3,6 +3,7 @@
 
 using System;
 using System.Collections.Generic;
+using System.Diagnostics;
 using System.Runtime.InteropServices;
 using System.Security.Cryptography;
 using System.Security.Cryptography.X509Certificates;
@@ -41,16 +42,23 @@ internal static X509Certificate2[] X509ChainGetCertificates(SafeX509ChainContext
             var certPtrs = new IntPtr[count];
 
             int res = Interop.AndroidCrypto.X509ChainGetCertificates(ctx, certPtrs, certPtrs.Length);
-            if (res != SUCCESS)
+            if (res == 0)
                 throw new CryptographicException();
 
+            Debug.Assert(res <= certPtrs.Length);
+
             var certs = new X509Certificate2[certPtrs.Length];
-            for (int i = 0; i < certs.Length; i++)
+            for (int i = 0; i < res; i++)
             {
                 certs[i] = new X509Certificate2(certPtrs[i]);
             }
 
-            return certs;
+            if (res == certPtrs.Length)
+            {
+                return certs;
+            }
+
+            return certs[0..res];
         }
 
         [StructLayout(LayoutKind.Sequential)]
@@ -90,6 +98,10 @@ internal static extern int X509ChainSetCustomTrustStore(
             IntPtr[] customTrustStore,
             int customTrustStoreLen);
 
+        [DllImport(Libraries.CryptoNative, EntryPoint = "AndroidCryptoNative_X509ChainSupportsRevocationOptions")]
+        [return:MarshalAs(UnmanagedType.U1)]
+        internal static extern bool X509ChainSupportsRevocationOptions();
+
         [DllImport(Libraries.CryptoNative, EntryPoint = "AndroidCryptoNative_X509ChainValidate")]
         internal static extern int X509ChainValidate(
             SafeX509ChainContextHandle ctx,

diff --git a/...libraries/Native/Unix/System.Security.Cryptography.Native.Android/pal_ecc_import_export.c b/...libraries/Native/Unix/System.Security.Cryptography.Native.Android/pal_ecc_import_export.c
@@ -317,7 +317,7 @@ static jobject AndroidCryptoNative_CreateKeyPairFromCurveParameters(
     goto cleanup;
 
 error:
-    if (loc[privateKey])
+    if (loc[privateKey] && (*env)->IsInstanceOf(env, loc[privateKey], g_DestroyableClass))
     {
         // Destroy the private key data.
         (*env)->CallVoidMethod(env, loc[privateKey], g_destroy);

diff --git a/src/libraries/Native/Unix/System.Security.Cryptography.Native.Android/pal_eckey.c b/src/libraries/Native/Unix/System.Security.Cryptography.Native.Android/pal_eckey.c
@@ -45,7 +45,7 @@ void AndroidCryptoNative_EcKeyDestroy(EC_KEY* r)
             {
                 // Destroy the private key data.
                 jobject privateKey = (*env)->CallObjectMethod(env, r->keyPair, g_keyPairGetPrivateMethod);
-                if (privateKey)
+                if (privateKey && (*env)->IsInstanceOf(env, privateKey, g_DestroyableClass))
                 {
                     (*env)->CallVoidMethod(env, privateKey, g_destroy);
                     ReleaseLRef(env, privateKey);

diff --git a/src/libraries/Native/Unix/System.Security.Cryptography.Native.Android/pal_jni.c b/src/libraries/Native/Unix/System.Security.Cryptography.Native.Android/pal_jni.c
@@ -2,6 +2,7 @@
 // The .NET Foundation licenses this file to you under the MIT license.
 
 #include "pal_jni.h"
+#include <pthread.h>
 
 JavaVM* gJvm;
 
@@ -476,6 +477,8 @@ static jclass GetOptionalClassGRef(JNIEnv *env, const char* name)
     if (!TryGetClassGRef(env, name, &klass))
     {
         LOG_DEBUG("optional class %s was not found", name);
+        // Failing to find an optional class causes an exception state, which we need to clear.
+        TryClearJNIExceptions(env);
     }
 
     return klass;
@@ -551,6 +554,8 @@ jmethodID GetOptionalMethod(JNIEnv *env, bool isStatic, jclass klass, const char
     jmethodID mid = isStatic ? (*env)->GetStaticMethodID(env, klass, name, sig) : (*env)->GetMethodID(env, klass, name, sig);
     if (!mid) {
         LOG_INFO("optional method %s %s was not found", name, sig);
+        // Failing to find an optional method causes an exception state, which we need to clear.
+        TryClearJNIExceptions(env);
     }
     return mid;
 }
@@ -566,13 +571,34 @@ jfieldID GetField(JNIEnv *env, bool isStatic, jclass klass, const char* name, co
     return fid;
 }
 
+static void DetatchThreadFromJNI(void* unused)
+{
+    LOG_DEBUG("Detaching thread from JNI");
+    (void)unused;
+    (*gJvm)->DetachCurrentThread(gJvm);
+}
+
+static pthread_key_t threadLocalEnvKey;
+static pthread_once_t threadLocalEnvInitKey = PTHREAD_ONCE_INIT;
+
+static void
+make_key()
+{
+    (void) pthread_key_create(&threadLocalEnvKey, &DetatchThreadFromJNI);
+}
+
 JNIEnv* GetJNIEnv()
 {
     JNIEnv *env;
     (*gJvm)->GetEnv(gJvm, (void**)&env, JNI_VERSION_1_6);
     if (env)
         return env;
     jint ret = (*gJvm)->AttachCurrentThreadAsDaemon(gJvm, &env, NULL);
+
+    (void) pthread_once(&threadLocalEnvInitKey, make_key);
+    LOG_DEBUG("Registering JNI thread detach. env ptr %p. Key: %ld", (void*)env, (long)threadLocalEnvKey);
+    pthread_setspecific(threadLocalEnvKey, env);
+
     assert(ret == JNI_OK && "Unable to attach thread to JVM");
     (void)ret;
     return env;

diff --git a/src/libraries/Native/Unix/System.Security.Cryptography.Native.Android/pal_x509.c b/src/libraries/Native/Unix/System.Security.Cryptography.Native.Android/pal_x509.c
@@ -250,6 +250,10 @@ void* AndroidCryptoNative_X509PublicKey(jobject /*X509Certificate*/ cert, PAL_Ke
 
     void* keyHandle;
     jobject key = (*env)->CallObjectMethod(env, cert, g_X509CertGetPublicKey);
+    if (CheckJNIExceptions(env) || !key)
+    {
+        return NULL;
+    }
     switch (algorithm)
     {
         case PAL_EC:

diff --git a/src/libraries/Native/Unix/System.Security.Cryptography.Native.Android/pal_x509chain.c b/src/libraries/Native/Unix/System.Security.Cryptography.Native.Android/pal_x509chain.c
@@ -192,9 +192,16 @@ int32_t AndroidCryptoNative_X509ChainGetCertificates(X509ChainContext* ctx,
     // Certificate trustedCert = trustAnchor.getTrustedCert();
     // certs[i] = trustedCert;
     jobject trustedCert = (*env)->CallObjectMethod(env, ctx->trustAnchor, g_TrustAnchorGetTrustedCert);
-    certs[i] = ToGRef(env, trustedCert);
-
-    ret = SUCCESS;
+    if (i == 0 || !(*env)->IsSameObject(env, certs[i-1], trustedCert))
+    {
+        certs[i] = ToGRef(env, trustedCert);
+        ret = i + 1;
+    }
+    else
+    {
+        ret = i;
+        certs[i] = NULL;
+    }
 
 cleanup:
     (*env)->DeleteLocalRef(env, certPathList);
@@ -404,7 +411,7 @@ int32_t AndroidCryptoNative_X509ChainSetCustomTrustStore(X509ChainContext* ctx,
     return CheckJNIExceptions(env) ? FAIL : SUCCESS;
 }
 
-static bool X509ChainSupportsRevocationOptions(void)
+bool AndroidCryptoNative_X509ChainSupportsRevocationOptions(void)
 {
     return g_CertPathValidatorGetRevocationChecker != NULL && g_PKIXRevocationCheckerClass != NULL;
 }
@@ -483,7 +490,7 @@ static int32_t ValidateWithRevocation(JNIEnv* env,
         else
         {
             certPathToUse = ctx->certPath;
-            if (X509ChainSupportsRevocationOptions())
+            if (AndroidCryptoNative_X509ChainSupportsRevocationOptions())
             {
                 // Only add the ONLY_END_ENTITY if we are not just checking the trust anchor. If ONLY_END_ENTITY is
                 // specified, revocation checking will skip the trust anchor even if it is the only certificate.
@@ -512,7 +519,7 @@ static int32_t ValidateWithRevocation(JNIEnv* env,
     }
 
     jobject params = ctx->params;
-    if (X509ChainSupportsRevocationOptions())
+    if (AndroidCryptoNative_X509ChainSupportsRevocationOptions())
     {
         // PKIXRevocationChecker checker = validator.getRevocationChecker();
         loc[checker] = (*env)->CallObjectMethod(env, validator, g_CertPathValidatorGetRevocationChecker);

diff --git a/src/libraries/Native/Unix/System.Security.Cryptography.Native.Android/pal_x509chain.h b/src/libraries/Native/Unix/System.Security.Cryptography.Native.Android/pal_x509chain.h
@@ -33,9 +33,9 @@ Return the number of certificates in the path
 PALEXPORT int32_t AndroidCryptoNative_X509ChainGetCertificateCount(X509ChainContext* ctx);
 
 /*
-Get the certificates in the path
+Get the certificates in the path.
 
-Returns 1 on success, 0 otherwise
+Returns the number of certificates exported.
 */
 PALEXPORT int32_t AndroidCryptoNative_X509ChainGetCertificates(X509ChainContext* ctx,
                                                                jobject* /*X509Certificate[]*/ certs,
@@ -62,6 +62,11 @@ PALEXPORT int32_t AndroidCryptoNative_X509ChainSetCustomTrustStore(X509ChainCont
                                                                    jobject* /*X509Certificate[]*/ customTrustStore,
                                                                    int32_t customTrustStoreLen);
 
+/*
+Returns true if revocation checking is supported. Returns false otherwise.
+*/
+PALEXPORT bool AndroidCryptoNative_X509ChainSupportsRevocationOptions(void);
+
 // Matches managed X509RevocationMode enum
 enum
 {

diff --git a/src/libraries/Native/Unix/System.Security.Cryptography.Native.Android/pal_x509store.c b/src/libraries/Native/Unix/System.Security.Cryptography.Native.Android/pal_x509store.c
@@ -98,6 +98,7 @@ int32_t AndroidCryptoNative_X509StoreAddCertificate(jobject /*KeyStore*/ store,
     EntryFlags flags;
     if (ContainsEntryForAlias(env, store, cert, alias, &flags))
     {
+        ReleaseLRef(env, alias);
         EntryFlags matchesFlags = EntryFlags_HasCertificate & EntryFlags_MatchesCertificate;
         if ((flags & matchesFlags) != matchesFlags)
         {
@@ -141,20 +142,22 @@ int32_t AndroidCryptoNative_X509StoreAddCertificateWithPrivateKey(jobject /*KeyS
         EntryFlags matchesFlags = EntryFlags_HasCertificate & EntryFlags_MatchesCertificate;
         if ((flags & matchesFlags) != matchesFlags)
         {
+            RELEASE_LOCALS(loc, env);
             LOG_ERROR("Store already contains alias with entry that does not match the expected certificate");
             return FAIL;
         }
 
         if ((flags & EntryFlags_HasPrivateKey) == EntryFlags_HasPrivateKey)
         {
+            RELEASE_LOCALS(loc, env);
             // Certificate with private key is already in store - nothing to do
             LOG_DEBUG("Store already contains certificate with private key");
             return SUCCESS;
         }
 
         // Delete existing entry. We will replace the existing cert with the cert + private key.
         // store.deleteEntry(alias);
-        (*env)->CallVoidMethod(env, store, g_KeyStoreDeleteEntry, alias);
+        (*env)->CallVoidMethod(env, store, g_KeyStoreDeleteEntry, loc[alias]);
     }
 
     bool releasePrivateKey = true;

diff --git a/src/libraries/System.Security.Cryptography.X509Certificates/tests/AssemblyInfo.cs b/src/libraries/System.Security.Cryptography.X509Certificates/tests/AssemblyInfo.cs
diff --git a/...y.Cryptography.X509Certificates/tests/CertificateCreation/CertificateRequestChainTests.cs b/...y.Cryptography.X509Certificates/tests/CertificateCreation/CertificateRequestChainTests.cs
@@ -526,6 +526,13 @@ public static void CreateChain_RSAPSS()
 
         private static bool DetectPssSupport()
         {
+            if (PlatformDetection.IsAndroid)
+            {
+                // Android supports PSS at the algorithms layer, but does not support it
+                // being used in cert chains.
+                return false;
+            }
+
             using (X509Certificate2 cert = new X509Certificate2(TestData.PfxData, TestData.PfxDataPassword))
             using (RSA rsa = cert.GetRSAPrivateKey())
             {

diff --git a/src/libraries/System.Security.Cryptography.X509Certificates/tests/ChainTests.cs b/src/libraries/System.Security.Cryptography.X509Certificates/tests/ChainTests.cs
@@ -210,10 +210,13 @@ public static void TestResetMethod()
 
         /// <summary>
         /// Tests that when a certificate chain has a root certification which is not trusted by the trust provider,
-        /// Build returns false and a ChainStatus returns UntrustedRoot
+        /// Build returns false and a ChainStatus returns UntrustedRoot.
+        /// Android does not support the detailed status in this test. It always validates time
+        /// and trusted root. It will fail to build any chain if those are not valid.
         /// </summary>
         [Fact]
         [OuterLoop]
+        [PlatformSpecific(~TestPlatforms.Android)]
         public static void BuildChainExtraStoreUntrustedRoot()
         {
             using (var testCert = new X509Certificate2(TestFiles.ChainPfxFile, TestData.ChainPfxPassword))

diff --git a/src/libraries/System.Security.Cryptography.X509Certificates/tests/PublicKeyTests.cs b/src/libraries/System.Security.Cryptography.X509Certificates/tests/PublicKeyTests.cs
@@ -489,7 +489,12 @@ public static void TestECDsaPublicKey_BrainpoolP160r1_ValidatesSignature(byte[]
                         Assert.Equal("1.2.840.10045.2.1", cert.PublicKey.Oid.Value);
 
                         bool isSignatureValid = publicKey.VerifyData(helloBytes, existingSignature, HashAlgorithmName.SHA256);
-                        Assert.True(isSignatureValid, "isSignatureValid");
+
+                        if (!isSignatureValid)
+                        {
+                            Assert.True(PlatformDetection.IsAndroid, "signature invalid on Android only");
+                            return;
+                        }
 
                         unchecked
                         {

diff --git a/...ity.Cryptography.X509Certificates/tests/RevocationTests/DynamicRevocationTests.Android.cs b/...ity.Cryptography.X509Certificates/tests/RevocationTests/DynamicRevocationTests.Android.cs
@@ -0,0 +1,16 @@
+// Licensed to the .NET Foundation under one or more agreements.
+// The .NET Foundation licenses this file to you under the MIT license.
+
+using System.Collections.Generic;
+using System.Linq;
+using System.Runtime.CompilerServices;
+using System.Security.Cryptography.X509Certificates.Tests.Common;
+using Xunit;
+
+namespace System.Security.Cryptography.X509Certificates.Tests.RevocationTests
+{
+    public static partial class DynamicRevocationTests
+    {
+        public static bool SupportsDynamicRevocation { get; } = Interop.AndroidCrypto.X509ChainSupportsRevocationOptions();
+    }
+}
diff --git a/...ity.Cryptography.X509Certificates/tests/RevocationTests/DynamicRevocationTests.Default.cs b/...ity.Cryptography.X509Certificates/tests/RevocationTests/DynamicRevocationTests.Default.cs
@@ -0,0 +1,16 @@
+// Licensed to the .NET Foundation under one or more agreements.
+// The .NET Foundation licenses this file to you under the MIT license.
+
+using System.Collections.Generic;
+using System.Linq;
+using System.Runtime.CompilerServices;
+using System.Security.Cryptography.X509Certificates.Tests.Common;
+using Xunit;
+
+namespace System.Security.Cryptography.X509Certificates.Tests.RevocationTests
+{
+    public static partial class DynamicRevocationTests
+    {
+        public static bool SupportsDynamicRevocation => true;
+    }
+}
diff --git a/...em.Security.Cryptography.X509Certificates/tests/RevocationTests/DynamicRevocationTests.cs b/...em.Security.Cryptography.X509Certificates/tests/RevocationTests/DynamicRevocationTests.cs
@@ -10,7 +10,8 @@
 namespace System.Security.Cryptography.X509Certificates.Tests.RevocationTests
 {
     [OuterLoop("These tests run serially at about 1 second each, and the code shouldn't change that often.")]
-    public static class DynamicRevocationTests
+    [ConditionalClass(typeof(DynamicRevocationTests), nameof(SupportsDynamicRevocation))]
+    public static partial class DynamicRevocationTests
     {
         // The CI machines are doing an awful lot of things at once, be generous with the timeout;
         internal static readonly TimeSpan s_urlRetrievalLimit = TimeSpan.FromSeconds(15);

diff --git a/...ography.X509Certificates/tests/System.Security.Cryptography.X509Certificates.Tests.csproj b/...ography.X509Certificates/tests/System.Security.Cryptography.X509Certificates.Tests.csproj
@@ -12,7 +12,6 @@
   </PropertyGroup>
   <Import Project="$(CommonPath)System\Security\Cryptography\Asn1Reader\System.Security.Cryptography.Asn1Reader.Shared.projitems" />
   <ItemGroup>
-    <Compile Include="AssemblyInfo.cs" />
     <Compile Include="$(CommonPath)System\Memory\PointerMemoryManager.cs"
              Link="Common\System\Memory\PointerMemoryManager.cs" />
     <Compile Include="$(CommonPath)System\Security\Cryptography\CryptoPool.cs"
@@ -99,8 +98,22 @@
   <ItemGroup Condition="'$(UseAndroidCrypto)' == 'true' or '$(UseAppleCrypto)' == 'true'">
     <Compile Include="X509StoreMutableTests.cs" />
   </ItemGroup>
+  <ItemGroup Condition="'$(UseAndroidCrypto)' != 'true'">
+    <Compile Include="RevocationTests\DynamicRevocationTests.Default.cs" />
+  </ItemGroup>
   <ItemGroup Condition="'$(UseAndroidCrypto)' == 'true'">
     <Compile Include="X509StoreMutableTests.Android.cs" />
+    <Compile Include="RevocationTests\DynamicRevocationTests.Android.cs" />
+    <Compile Include="$(CommonPath)Interop\Android\System.Security.Cryptography.Native.Android\Interop.X509.cs"
+             Link="Common\Interop\Android\System.Security.Cryptography.Native.Android\Interop.X509.cs" />
+    <Compile Include="$(CommonPath)Interop\Android\System.Security.Cryptography.Native.Android\Interop.X509Chain.cs"
+             Link="Common\Interop\Android\System.Security.Cryptography.Native.Android\Interop.X509Chain.cs" />
+    <Compile Include="$(CommonPath)Interop\Android\Interop.JObjectLifetime.cs"
+             Link="Common\Interop\Android\Interop.JObjectLifetime.cs" />
+    <Compile Include="$(CommonPath)Interop\Android\Interop.Libraries.cs"
+             Link="Common\Interop\Android\Interop.Libraries.cs" />
+    <Compile Include="$(CommonPath)Interop\Unix\Interop.Libraries.cs"
+             Link="Common\Interop\Unix\Interop.Libraries.cs" />
   </ItemGroup>
   <ItemGroup Condition="'$(UseAppleCrypto)' == 'true'">
     <Compile Include="X509StoreMutableTests.OSX.cs" />

diff --git a/src/libraries/tests.proj b/src/libraries/tests.proj
@@ -203,6 +203,9 @@
 
     <ProjectExclusions Include="$(MSBuildThisFileDirectory)System.Linq.Parallel\tests\System.Linq.Parallel.Tests.csproj" />
     <ProjectExclusions Include="$(MSBuildThisFileDirectory)System.Net.Http\tests\FunctionalTests\System.Net.Http.Functional.Tests.csproj" />
+
+    <!-- https://github.com/dotnet/runtime/issues/37669 -->
+    <ProjectExclusions Include="$(MSBuildThisFileDirectory)System.Security.Cryptography.X509Certificates\tests\System.Security.Cryptography.X509Certificates.Tests.csproj" />
   </ItemGroup>
 
   <ItemGroup Condition="'$(TargetOS)' == 'Browser' and '$(RunAOTCompilation)' == 'true' and '$(RunDisabledWasmTests)' != 'true'">