[Android] Improvements to remote certificate verification in SslStream (

#77386) * Extract existing validation code into a separate class * Implement AndroidDexBuilderTask * Implement TrustManager proxy * Integrate the trust manager proxy with SslStream on Android * Update tests * Update System.Net.Http tests * Update System.Net.Security tests * Fix packaging * Propagate caught exceptions * Build and pack .jar * Optimize allocation and deallocation of memory for certificate data * Fix building .jar * Cleanup * Remove complicated certificate copying * Remove unnecessary JNI classes and methods * Simplify and fix the core implementation * Update enabled and disabled tests * Cleanup * Renaming * Remove unnecessary changes * Fix invoking validation even when the Java callbacks aren't called (no peer certificate to validate) * Minor refactoring * Enable more unnecessarily disabled tests * Refactor exception handling * Update disabled tests * Renaming * Remove network security config workarounds * Keep existing active issue * Remove unnecessary changes * Remove unnecessary code * Enable more disabled tests * Fix throwing exception * Fix intptr_t cast to Java * Remove initialization lock * Update naming * Fix type casting * Improve throwing validation exception * Experiment with code structure * Fix repeated calls to beginHandshake * Make SslStream proxy mandatory * Add missing attributes * Free temporary buffer * Update src/native/libs/System.Security.Cryptography.Native.Android/pal_sslstream.c Co-authored-by: Elinor Fung <[email protected]> * Refactor creating array of trust managers * Add comments and clean up pal_sslstream.c * Revert experimental change * Remove special case for IPv6 addresses as hostnames and disable affected tests * Fix duplicate variable after merge * Improve code formatting * Remove the hack with SafeDeleteContextStub * Enable passing test * Remove unnecessary factory * Move clearing selected client certificate out of the remote certificate verification method * Fix typo in comment * Add comment with java equivalent * Move Android specific runtime files into a separate item group * Apply suggestions from code review Co-authored-by: Alexander Köplinger <[email protected]> * Update src/native/libs/build-native.proj Co-authored-by: Alexander Köplinger <[email protected]> * Disable test that fails on Android emualtors Co-authored-by: Elinor Fung <[email protected]> Co-authored-by: Alexander Köplinger <[email protected]>
dotnet · Jan 16, 2023 · c435061 · c435061
1 parent 4cf8c07
commit c435061
Show file tree

Hide file tree

Showing 50 changed files with 809 additions and 192 deletions.
diff --git a/eng/liveBuilds.targets b/eng/liveBuilds.targets
@@ -179,6 +179,11 @@
         $(LibrariesNativeArtifactsPath)*.pdb"
         IsNative="true"
         Exclude="@(ExcludeNativeLibrariesRuntimeFiles)" />
+      <LibrariesRuntimeFiles Condition="'$(TargetOS)' == 'android'"
+                             Include="
+        $(LibrariesNativeArtifactsPath)*.dex;
+        $(LibrariesNativeArtifactsPath)*.jar;"
+        IsNative="true" />
       <LibrariesRuntimeFiles Condition="'$(TargetOS)' == 'browser'"
                              Include="
         $(LibrariesNativeArtifactsPath)dotnet.js;

diff --git a/src/installer/pkg/sfx/Microsoft.NETCore.App/Directory.Build.props b/src/installer/pkg/sfx/Microsoft.NETCore.App/Directory.Build.props
@@ -95,6 +95,8 @@
     <PlatformManifestFileEntry Include="libSystem.Security.Cryptography.Native.Apple.dylib" IsNative="true" />
     <PlatformManifestFileEntry Include="libSystem.Security.Cryptography.Native.Android.a" IsNative="true" />
     <PlatformManifestFileEntry Include="libSystem.Security.Cryptography.Native.Android.so" IsNative="true" />
+    <PlatformManifestFileEntry Include="libSystem.Security.Cryptography.Native.Android.dex" IsNative="true" />
+    <PlatformManifestFileEntry Include="libSystem.Security.Cryptography.Native.Android.jar" IsNative="true" />
     <PlatformManifestFileEntry Include="libSystem.Security.Cryptography.Native.OpenSsl.a" IsNative="true" />
     <PlatformManifestFileEntry Include="libSystem.Security.Cryptography.Native.OpenSsl.dylib" IsNative="true" />
     <PlatformManifestFileEntry Include="libSystem.Security.Cryptography.Native.OpenSsl.so" IsNative="true" />

diff --git a/...ies/Common/src/Interop/Android/System.Security.Cryptography.Native.Android/Interop.Ssl.cs b/...ies/Common/src/Interop/Android/System.Security.Cryptography.Native.Android/Interop.Ssl.cs
@@ -29,25 +29,37 @@ internal enum PAL_SSLStreamStatus
         };
 
         [LibraryImport(Interop.Libraries.AndroidCryptoNative, EntryPoint = "AndroidCryptoNative_SSLStreamCreate")]
-        internal static partial SafeSslHandle SSLStreamCreate();
+        private static partial SafeSslHandle SSLStreamCreate(IntPtr sslStreamProxyHandle);
+        internal static SafeSslHandle SSLStreamCreate(SslStream.JavaProxy sslStreamProxy)
+            => SSLStreamCreate(sslStreamProxy.Handle);
 
         [LibraryImport(Interop.Libraries.AndroidCryptoNative, EntryPoint = "AndroidCryptoNative_SSLStreamCreateWithCertificates")]
         private static partial SafeSslHandle SSLStreamCreateWithCertificates(
+            IntPtr sslStreamProxyHandle,
             ref byte pkcs8PrivateKey,
             int pkcs8PrivateKeyLen,
             PAL_KeyAlgorithm algorithm,
             IntPtr[] certs,
             int certsLen);
-        internal static SafeSslHandle SSLStreamCreateWithCertificates(ReadOnlySpan<byte> pkcs8PrivateKey, PAL_KeyAlgorithm algorithm, IntPtr[] certificates)
+        internal static SafeSslHandle SSLStreamCreateWithCertificates(
+            SslStream.JavaProxy sslStreamProxy,
+            ReadOnlySpan<byte> pkcs8PrivateKey,
+            PAL_KeyAlgorithm algorithm,
+            IntPtr[] certificates)
         {
             return SSLStreamCreateWithCertificates(
+                sslStreamProxy.Handle,
                 ref MemoryMarshal.GetReference(pkcs8PrivateKey),
                 pkcs8PrivateKey.Length,
                 algorithm,
                 certificates,
                 certificates.Length);
         }
 
+        [LibraryImport(Interop.Libraries.AndroidCryptoNative, EntryPoint = "AndroidCryptoNative_RegisterRemoteCertificateValidationCallback")]
+        internal static unsafe partial void RegisterRemoteCertificateValidationCallback(
+            delegate* unmanaged<IntPtr, bool> verifyRemoteCertificate);
+
         [LibraryImport(Interop.Libraries.AndroidCryptoNative, EntryPoint = "AndroidCryptoNative_SSLStreamInitialize")]
         private static unsafe partial int SSLStreamInitializeImpl(
             SafeSslHandle sslHandle,

diff --git a/src/libraries/Common/tests/System/Net/Http/HttpClientHandlerTest.AcceptAllCerts.cs b/src/libraries/Common/tests/System/Net/Http/HttpClientHandlerTest.AcceptAllCerts.cs
@@ -96,7 +96,6 @@ await TestHelper.WhenAllCompletedOrAnyFailed(
         [OuterLoop]
         [ConditionalTheory(nameof(ClientSupportsDHECipherSuites))]
         [MemberData(nameof(InvalidCertificateServers))]
-        [SkipOnPlatform(TestPlatforms.Android, "Android rejects the certificate, the custom validation callback in .NET cannot override OS behavior in the current implementation")]
         public async Task InvalidCertificateServers_CertificateValidationDisabled_Succeeds(string url)
         {
             using (HttpClientHandler handler = CreateHttpClientHandler(allowAllCertificates: true))

diff --git a/src/libraries/Common/tests/System/Net/Http/HttpClientHandlerTest.ServerCertificates.cs b/src/libraries/Common/tests/System/Net/Http/HttpClientHandlerTest.ServerCertificates.cs
@@ -286,7 +286,6 @@ private async Task UseCallback_BadCertificate_ExpectedPolicyErrors_Helper(string
         [OuterLoop("Uses external servers")]
         [Theory]
         [MemberData(nameof(CertificateValidationServersAndExpectedPolicies))]
-        [SkipOnPlatform(TestPlatforms.Android, "Android rejects the certificate, the custom validation callback in .NET cannot override OS behavior in the current implementation")]
         public async Task UseCallback_BadCertificate_ExpectedPolicyErrors(string url, SslPolicyErrors expectedErrors)
         {
             const int SEC_E_BUFFER_TOO_SMALL = unchecked((int)0x80090321);
@@ -310,7 +309,6 @@ public async Task UseCallback_BadCertificate_ExpectedPolicyErrors(string url, Ss
         }
 
         [Fact]
-        [SkipOnPlatform(TestPlatforms.Android, "Android rejects the certificate, the custom validation callback in .NET cannot override OS behavior in the current implementation")]
         public async Task UseCallback_SelfSignedCertificate_ExpectedPolicyErrors()
         {
             using (HttpClientHandler handler = CreateHttpClientHandler())

diff --git a/src/libraries/Common/tests/System/Net/Http/HttpClientHandlerTest.cs b/src/libraries/Common/tests/System/Net/Http/HttpClientHandlerTest.cs
@@ -153,7 +153,7 @@ public void Properties_AddItemToDictionary_ItemPresent()
 
         [ConditionalFact]
         [SkipOnPlatform(TestPlatforms.Browser, "ServerCertificateCustomValidationCallback not supported on Browser")]
-        [SkipOnPlatform(TestPlatforms.Android, "IPv6 loopback with SSL doesn't work on Android")]
+        [SkipOnPlatform(TestPlatforms.Android, "TargetHost cannot be set to an IPv6 address on Android because the string doesn't conform to the STD 3 ASCII rules")]
         public async Task GetAsync_IPv6LinkLocalAddressUri_Success()
         {
             if (IsWinHttpHandler && UseVersion >= HttpVersion20.Value)
@@ -205,7 +205,7 @@ public async Task GetAsync_IPBasedUri_Success(IPAddress address)
 
             if (PlatformDetection.IsAndroid && options.UseSsl && address == IPAddress.IPv6Loopback)
             {
-                throw new SkipTestException("IPv6 loopback with SSL doesn't work on Android");
+                throw new SkipTestException("TargetHost cannot be set to an IPv6 address on Android because the string doesn't conform to the STD 3 ASCII rules");
             }
 
             await LoopbackServerFactory.CreateServerAsync(async (server, url) =>
@@ -287,7 +287,7 @@ public async Task GetAsync_SecureAndNonSecureIPBasedUri_CorrectlyFormatted(IPAdd
 
             if (PlatformDetection.IsAndroid && useSsl && address == IPAddress.IPv6Loopback)
             {
-                throw new SkipTestException("IPv6 loopback with SSL doesn't work on Android");
+                throw new SkipTestException("TargetHost cannot be set to an IPv6 address on Android because the string doesn't conform to the STD 3 ASCII rules");
             }
 
             var options = new LoopbackServer.Options { Address = address, UseSsl = useSsl };

diff --git a/src/libraries/Common/tests/System/Net/Http/TestHelper.cs b/src/libraries/Common/tests/System/Net/Http/TestHelper.cs
@@ -173,9 +173,6 @@ public static SocketsHttpHandler CreateSocketsHttpHandler(bool allowAllCertifica
             // Browser doesn't support ServerCertificateCustomValidationCallback
             if (allowAllCertificates && PlatformDetection.IsNotBrowser)
             {
-                // On Android, it is not enough to set the custom validation callback, the certificates also need to be trusted by the OS.
-                // See HttpClientHandlerTestBase.SocketsHttpHandler.cs:CreateHttpClientHandler for more details.
-
                 handler.SslOptions.RemoteCertificateValidationCallback = delegate { return true; };
             }
 

diff --git a/...ies/System.Net.Http/tests/FunctionalTests/HttpClientHandlerTestBase.SocketsHttpHandler.cs b/...ies/System.Net.Http/tests/FunctionalTests/HttpClientHandlerTestBase.SocketsHttpHandler.cs
@@ -37,13 +37,6 @@ protected static HttpClientHandler CreateHttpClientHandler(Version useVersion =
             // Browser doesn't support ServerCertificateCustomValidationCallback
             if (allowAllCertificates && PlatformDetection.IsNotBrowser)
             {
-                // On Android, it is not enough to set the custom validation callback, the certificates also need to be trusted by the OS.
-                // The public keys of our self-signed certificates that are used by the loopback server are part of the System.Net.TestData
-                // package and they can be included in a the Android test apk by adding the following property to the test's .csproj:
-                //
-                //    <IncludeNetworkSecurityConfig Condition="'$(TargetOS)' == 'android'">true</IncludeNetworkSecurityConfig>
-                //
-
                 handler.ServerCertificateCustomValidationCallback = TestHelper.AllowAllCertificates;
             }
 

diff --git a/src/libraries/System.Net.Http/tests/FunctionalTests/SocketsHttpHandlerTest.cs b/src/libraries/System.Net.Http/tests/FunctionalTests/SocketsHttpHandlerTest.cs
@@ -4076,7 +4076,6 @@ public abstract class SocketsHttpHandler_SecurityTest : HttpClientHandlerTestBas
         public SocketsHttpHandler_SecurityTest(ITestOutputHelper output) : base(output) { }
 
         [ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsNotWindows7))]
-        [SkipOnPlatform(TestPlatforms.Android, "Self-signed certificates are rejected by Android before the .NET validation is reached")]
         public async Task SslOptions_CustomTrust_Ok()
         {
             X509Certificate2Collection caCerts = new X509Certificate2Collection();
@@ -4113,7 +4112,6 @@ await LoopbackServerFactory.CreateClientAndServerAsync(
         }
 
         [Fact]
-        [SkipOnPlatform(TestPlatforms.Android, "Self-signed certificates are rejected by Android before the .NET validation is reached")]
         public async Task SslOptions_InvalidName_Throws()
         {
             X509Certificate2Collection caCerts = new X509Certificate2Collection();
@@ -4144,7 +4142,6 @@ await LoopbackServerFactory.CreateClientAndServerAsync(
         }
 
         [Fact]
-        [SkipOnPlatform(TestPlatforms.Android, "Self-signed certificates are rejected by Android before the .NET validation is reached")]
         public async Task SslOptions_CustomPolicy_IgnoresNameMismatch()
         {
             X509Certificate2Collection caCerts = new X509Certificate2Collection();

diff --git a/src/libraries/System.Net.Http/tests/FunctionalTests/SocksProxyTest.cs b/src/libraries/System.Net.Http/tests/FunctionalTests/SocksProxyTest.cs
@@ -37,7 +37,7 @@ public async Task TestLoopbackAsync(string scheme, bool useSsl, bool useAuth, st
 
             if (PlatformDetection.IsAndroid && useSsl && host == "::1")
             {
-                throw new SkipTestException("IPv6 loopback with SSL doesn't work on Android");
+                throw new SkipTestException("TargetHost cannot be set to an IPv6 address on Android because the string doesn't conform to the STD 3 ASCII rules");
             }
 
             await LoopbackServerFactory.CreateClientAndServerAsync(

diff --git a/src/libraries/System.Net.Http/tests/FunctionalTests/System.Net.Http.Functional.Tests.csproj b/src/libraries/System.Net.Http/tests/FunctionalTests/System.Net.Http.Functional.Tests.csproj
@@ -9,8 +9,6 @@
     <TargetFrameworks>$(NetCoreAppCurrent)-windows;$(NetCoreAppCurrent)-linux;$(NetCoreAppCurrent)-browser;$(NetCoreAppCurrent)-osx</TargetFrameworks>
     <EnableLibraryImportGenerator>true</EnableLibraryImportGenerator>
     <EnablePreviewFeatures>true</EnablePreviewFeatures>
-    <!-- the res/xml/network_security_config.xml file comes from the System.Net.TestData package -->
-    <IncludeNetworkSecurityConfig Condition="'$(TargetOS)' == 'android'">true</IncludeNetworkSecurityConfig>
     <EventSourceSupport Condition="'$(TestNativeAot)' == 'true'">true</EventSourceSupport>
   </PropertyGroup>
 

diff --git a/src/libraries/System.Net.Requests/tests/System.Net.Requests.Tests.csproj b/src/libraries/System.Net.Requests/tests/System.Net.Requests.Tests.csproj
@@ -7,8 +7,6 @@
     <IgnoreForCI Condition="'$(TargetOS)' == 'browser'">true</IgnoreForCI>
     <!-- SYSLIB0014: WebRequest, HttpWebRequest, ServicePoint, and WebClient are obsolete. Use HttpClient instead. -->
     <NoWarn>$(NoWarn);SYSLIB0014</NoWarn>
-    <!-- the res/xml/network_security_config.xml file comes from the System.Net.TestData package -->
-    <IncludeNetworkSecurityConfig Condition="'$(TargetOS)' == 'android'">true</IncludeNetworkSecurityConfig>
     <EnablePreviewFeatures>true</EnablePreviewFeatures>
     <EventSourceSupport Condition="'$(TestNativeAot)' == 'true'">true</EventSourceSupport>
   </PropertyGroup>

diff --git a/src/libraries/System.Net.Security/src/System.Net.Security.csproj b/src/libraries/System.Net.Security/src/System.Net.Security.csproj
@@ -11,6 +11,7 @@
     <TargetPlatformIdentifier>$([MSBuild]::GetTargetPlatformIdentifier('$(TargetFramework)'))</TargetPlatformIdentifier>
     <GeneratePlatformNotSupportedAssemblyMessage Condition="'$(TargetPlatformIdentifier)' == ''">SR.SystemNetSecurity_PlatformNotSupported</GeneratePlatformNotSupportedAssemblyMessage>
     <DefineConstants Condition="'$(TargetPlatformIdentifier)' == 'windows'">$(DefineConstants);TARGET_WINDOWS</DefineConstants>
+    <DefineConstants Condition="'$(TargetPlatformIdentifier)' == 'android'">$(DefineConstants);TARGET_ANDROID</DefineConstants>
     <UseAndroidCrypto Condition="'$(TargetPlatformIdentifier)' == 'android'">true</UseAndroidCrypto>
     <UseAppleCrypto Condition="'$(TargetPlatformIdentifier)' == 'osx' or '$(TargetPlatformIdentifier)' == 'ios' or '$(TargetPlatformIdentifier)' == 'tvos'">true</UseAppleCrypto>
     <UseManagedNtlm Condition="'$(TargetPlatformIdentifier)' == 'android' or '$(TargetPlatformIdentifier)' == 'tvos'">true</UseManagedNtlm>
@@ -102,7 +103,7 @@
              Link="Common\System\Net\SecurityStatusPal.cs" />
     <Compile Include="$(CommonPath)System\HexConverter.cs"
              Link="Common\System\HexConverter.cs" />
-    <Compile Include="$(CommonPath)System\Obsoletions.cs" 
+    <Compile Include="$(CommonPath)System\Obsoletions.cs"
              Link="Common\System\Obsoletions.cs" />
   </ItemGroup>
   <!-- This file depends on IANA registry. We do not want anyone's build to break after the update -->
@@ -382,6 +383,7 @@
     <Compile Include="System\Net\Security\Pal.Android\SafeDeleteSslContext.cs" />
     <Compile Include="System\Net\Security\Pal.Managed\SslProtocolsValidation.cs" />
     <Compile Include="System\Net\Security\SslConnectionInfo.Android.cs" />
+    <Compile Include="System\Net\Security\SslStream.Android.cs" />
     <Compile Include="System\Net\Security\SslStreamCertificateContext.Android.cs" />
     <Compile Include="System\Net\Security\SslStreamPal.Android.cs" />
     <Compile Include="System\Net\Security\StreamSizes.Unix.cs" />

diff --git a/...libraries/System.Net.Security/src/System/Net/Security/Pal.Android/SafeDeleteSslContext.cs b/...libraries/System.Net.Security/src/System/Net/Security/Pal.Android/SafeDeleteSslContext.cs
@@ -34,14 +34,19 @@ internal sealed class SafeDeleteSslContext : SafeDeleteContext
         private ArrayBuffer _inputBuffer = new ArrayBuffer(InitialBufferSize);
         private ArrayBuffer _outputBuffer = new ArrayBuffer(InitialBufferSize);
 
+        public SslStream.JavaProxy SslStreamProxy { get; }
+
         public SafeSslHandle SslContext => _sslContext;
 
         public SafeDeleteSslContext(SslAuthenticationOptions authOptions)
             : base(IntPtr.Zero)
         {
+            SslStreamProxy = authOptions.SslStreamProxy
+                ?? throw new ArgumentNullException(nameof(authOptions.SslStreamProxy));
+
             try
             {
-                _sslContext = CreateSslContext(authOptions);
+                _sslContext = CreateSslContext(SslStreamProxy, authOptions);
                 InitializeSslContext(_sslContext, authOptions);
             }
             catch (Exception ex)
@@ -58,8 +63,7 @@ protected override void Dispose(bool disposing)
         {
             if (disposing)
             {
-                SafeSslHandle sslContext = _sslContext;
-                if (sslContext != null)
+                if (_sslContext is SafeSslHandle sslContext)
                 {
                     _inputBuffer.Dispose();
                     _outputBuffer.Dispose();
@@ -145,11 +149,11 @@ internal int ReadPendingWrites(byte[] buf, int offset, int count)
             return limit;
         }
 
-        private static SafeSslHandle CreateSslContext(SslAuthenticationOptions authOptions)
+        private static SafeSslHandle CreateSslContext(SslStream.JavaProxy sslStreamProxy, SslAuthenticationOptions authOptions)
         {
             if (authOptions.CertificateContext == null)
             {
-                return Interop.AndroidCrypto.SSLStreamCreate();
+                return Interop.AndroidCrypto.SSLStreamCreate(sslStreamProxy);
             }
 
             SslStreamCertificateContext context = authOptions.CertificateContext;
@@ -169,7 +173,7 @@ private static SafeSslHandle CreateSslContext(SslAuthenticationOptions authOptio
                 ptrs[i + 1] = context.IntermediateCertificates[i].Handle;
             }
 
-            return Interop.AndroidCrypto.SSLStreamCreateWithCertificates(keyBytes, algorithm, ptrs);
+            return Interop.AndroidCrypto.SSLStreamCreateWithCertificates(sslStreamProxy, keyBytes, algorithm, ptrs);
         }
 
         private static AsymmetricAlgorithm GetPrivateKeyAlgorithm(X509Certificate2 cert, out PAL_KeyAlgorithm algorithm)

diff --git a/src/libraries/System.Net.Security/src/System/Net/Security/SslAuthenticationOptions.cs b/src/libraries/System.Net.Security/src/System/Net/Security/SslAuthenticationOptions.cs
@@ -182,5 +182,9 @@ private static SslProtocols FilterOutIncompatibleSslProtocols(SslProtocols proto
         internal object? UserState { get; set; }
         internal ServerOptionsSelectionCallback? ServerOptionDelegate { get; set; }
         internal X509ChainPolicy? CertificateChainPolicy { get; set; }
+
+#if TARGET_ANDROID
+        internal SslStream.JavaProxy? SslStreamProxy { get; set; }
+#endif
     }
 }