Fix matching algo, remove pin because of client buffer use, moved gchandle to ssl...

Commit migrated from https://github.com/dotnet/corefx/commit/01709e234375bf1aecd9d2362d4a311a8094114e

diff --git a/src/libraries/Common/src/Interop/Unix/System.Security.Cryptography.Native/Interop.OpenSsl.cs b/src/libraries/Common/src/Interop/Unix/System.Security.Cryptography.Native/Interop.OpenSsl.cs
index 7374827..da26895 100644 (file)
--- a/src/libraries/Common/src/Interop/Unix/System.Security.Cryptography.Native/Interop.OpenSsl.cs
+++ b/src/libraries/Common/src/Interop/Unix/System.Security.Cryptography.Native/Interop.OpenSsl.cs
@@ -21,7 +21,7 @@ internal static partial class Interop
     internal static partial class OpenSsl
     {
         private static readonly Ssl.SslCtxSetVerifyCallback s_verifyClientCertificate = VerifyClientCertificate;
-        private static readonly Ssl.SslCtxSetAlpnCallback s_alpnServerCallback = AlpnServerSelectCallback;
+        private unsafe static readonly Ssl.SslCtxSetAlpnCallback s_alpnServerCallback = AlpnServerSelectCallback;
 
         #region internal methods
 
@@ -96,52 +96,64 @@ internal static partial class Interop
                     //update the client CA list 
                     UpdateCAListFromRootStore(innerContext);
                 }
-
-                if (sslAuthenticationOptions.ApplicationProtocols != null)
+                GCHandle alpnHandle = default;
+                try
                 {
-                    if (sslAuthenticationOptions.IsServer)
+                    if (sslAuthenticationOptions.ApplicationProtocols != null)
                     {
-                        byte[] protos = Interop.Ssl.ConvertAlpnProtocolListToByteArray(sslAuthenticationOptions.ApplicationProtocols);
-                        sslAuthenticationOptions.AlpnProtocolsHandle = GCHandle.Alloc(protos, GCHandleType.Pinned);
-                        Interop.Ssl.SslCtxSetAlpnSelectCb(innerContext, s_alpnServerCallback, GCHandle.ToIntPtr(sslAuthenticationOptions.AlpnProtocolsHandle));
-                    }
-                    else
-                    {
-                        if (Interop.Ssl.SslCtxSetAlpnProtos(innerContext, sslAuthenticationOptions.ApplicationProtocols) != 0)
+                        if (sslAuthenticationOptions.IsServer)
                         {
-                            throw CreateSslException(SR.net_alpn_config_failed);
+                            alpnHandle = GCHandle.Alloc(sslAuthenticationOptions.ApplicationProtocols);
+                            Interop.Ssl.SslCtxSetAlpnSelectCb(innerContext, s_alpnServerCallback, GCHandle.ToIntPtr(alpnHandle));
+                        }
+                        else
+                        {
+                            if (Interop.Ssl.SslCtxSetAlpnProtos(innerContext, sslAuthenticationOptions.ApplicationProtocols) != 0)
+                            {
+                                throw CreateSslException(SR.net_alpn_config_failed);
+                            }
                         }
                     }
-                }
 
-                context = SafeSslHandle.Create(innerContext, sslAuthenticationOptions.IsServer);
-                Debug.Assert(context != null, "Expected non-null return value from SafeSslHandle.Create");
-                if (context.IsInvalid)
-                {
-                    context.Dispose();
-                    throw CreateSslException(SR.net_allocate_ssl_context_failed);
-                }
+                    context = SafeSslHandle.Create(innerContext, sslAuthenticationOptions.IsServer);
+                    Debug.Assert(context != null, "Expected non-null return value from SafeSslHandle.Create");
+                    if (context.IsInvalid)
+                    {
+                        context.Dispose();
+                        throw CreateSslException(SR.net_allocate_ssl_context_failed);
+                    }
 
-                if (hasCertificateAndKey)
-                {
-                    bool hasCertReference = false;
-                    try
+                    if (hasCertificateAndKey)
                     {
-                        certHandle.DangerousAddRef(ref hasCertReference);
-                        using (X509Certificate2 cert = new X509Certificate2(certHandle.DangerousGetHandle()))
+                        bool hasCertReference = false;
+                        try
                         {
-                            using (X509Chain chain = TLSCertificateExtensions.BuildNewChain(cert, includeClientApplicationPolicy: false))
+                            certHandle.DangerousAddRef(ref hasCertReference);
+                            using (X509Certificate2 cert = new X509Certificate2(certHandle.DangerousGetHandle()))
                             {
-                                if (chain != null && !Ssl.AddExtraChainCertificates(context, chain))
-                                    throw CreateSslException(SR.net_ssl_use_cert_failed);
+                                using (X509Chain chain = TLSCertificateExtensions.BuildNewChain(cert, includeClientApplicationPolicy: false))
+                                {
+                                    if (chain != null && !Ssl.AddExtraChainCertificates(context, chain))
+                                        throw CreateSslException(SR.net_ssl_use_cert_failed);
+                                }
                             }
                         }
+                        finally
+                        {
+                            if (hasCertReference)
+                                certHandle.DangerousRelease();
+                        }
                     }
-                    finally
+                    context.AlpnHandle = alpnHandle;
+                }
+                catch
+                {
+                    if (alpnHandle.IsAllocated)
                     {
-                        if (hasCertReference)
-                            certHandle.DangerousRelease();
+                        alpnHandle.Free();
                     }
+
+                    throw;
                 }
             }
 
@@ -330,18 +342,43 @@ internal static partial class Interop
             return OpenSslSuccess;
         }
 
-        private static unsafe int AlpnServerSelectCallback(IntPtr ssl, out IntPtr outp, out byte outlen, IntPtr inp, uint inlen, IntPtr arg)
+        private static unsafe int AlpnServerSelectCallback(IntPtr ssl, out byte* outp, out byte outlen, byte* inp, uint inlen, IntPtr arg)
         {
-            outp = IntPtr.Zero;
+            outp = null;
             outlen = 0;
-                        
-            GCHandle protocols = GCHandle.FromIntPtr(arg);
-            Debug.Assert(protocols.IsAllocated && protocols.Target != null);
 
-            byte[] server = (byte[])protocols.Target;
+            GCHandle protocolHandle = GCHandle.FromIntPtr(arg);
+            if (!(protocolHandle.Target is List<SslApplicationProtocol> protocolList))
+            {
+                return Ssl.SSL_TLSEXT_ERR_NOACK;
+            }
+
+            try
+            {
+                for (int i = 0; i < protocolList.Count; i++)
+                {
+                    Span<byte> clientList = new Span<byte>(inp, (int)inlen);
+                    while (clientList.Length > 0)
+                    {
+                        byte length = clientList[0];
+                        Span<byte> clientProto = clientList.Slice(1, length);
+                        if (clientProto.SequenceEqual(protocolList[i].Protocol.Span))
+                        {
+                            outp = inp + (int)inlen - clientList.Length + 1;
+                            outlen = length;
+                            return Ssl.SSL_TLSEXT_ERR_OK;
+                        }
+
+                        clientList = clientList.Slice(1 + length);
+                    }
+                }
+            }
+            catch
+            {
+                return Ssl.SSL_TLSEXT_ERR_NOACK;
+            }
 
-            return Ssl.SslSelectNextProto(out outp, out outlen, protocols.AddrOfPinnedObject(), (uint)server.Length, inp, inlen) == Ssl.OPENSSL_NPN_NEGOTIATED ?
-                    Ssl.SSL_TLSEXT_ERR_OK : Ssl.SSL_TLSEXT_ERR_NOACK;
+            return Ssl.SSL_TLSEXT_ERR_NOACK;
         }
 
         private static void UpdateCAListFromRootStore(SafeSslContextHandle context)

diff --git a/src/libraries/Common/src/Interop/Unix/System.Security.Cryptography.Native/Interop.Ssl.cs b/src/libraries/Common/src/Interop/Unix/System.Security.Cryptography.Native/Interop.Ssl.cs
index 2828856..019f68f 100644 (file)
--- a/src/libraries/Common/src/Interop/Unix/System.Security.Cryptography.Native/Interop.Ssl.cs
+++ b/src/libraries/Common/src/Interop/Unix/System.Security.Cryptography.Native/Interop.Ssl.cs
@@ -47,10 +47,7 @@ internal static partial class Interop
 
         [DllImport(Libraries.CryptoNative, EntryPoint = "CryptoNative_SslGetVersion")]
         private static extern IntPtr SslGetVersion(SafeSslHandle ssl);
-
-        [DllImport(Libraries.CryptoNative, EntryPoint = "CryptoNative_SslSelectNextProto")]
-        internal static extern int SslSelectNextProto(out IntPtr outp, out byte outlen, IntPtr server, uint serverlen, IntPtr client, uint clientlen);
-
+                
         [DllImport(Libraries.CryptoNative, EntryPoint = "CryptoNative_SslGet0AlpnSelected")]
         internal static extern void SslGetAlpnSelected(SafeSslHandle ssl, out IntPtr protocol, out int len);
 
@@ -198,6 +195,9 @@ namespace Microsoft.Win32.SafeHandles
         private SafeBioHandle _writeBio;
         private bool _isServer;
         private bool _handshakeCompleted = false;
+        private GCHandle _alpnHandle;
+
+        public GCHandle AlpnHandle { set => _alpnHandle = value; }
 
         public bool IsServer
         {
@@ -279,6 +279,12 @@ namespace Microsoft.Win32.SafeHandles
                 _readBio?.Dispose();
                 _writeBio?.Dispose();
             }
+
+            if(_alpnHandle.IsAllocated)
+            {
+                _alpnHandle.Free();
+            }
+
             base.Dispose(disposing);
         }
 

diff --git a/src/libraries/Common/src/Interop/Unix/System.Security.Cryptography.Native/Interop.SslCtx.cs b/src/libraries/Common/src/Interop/Unix/System.Security.Cryptography.Native/Interop.SslCtx.cs
index 229f31a..b199687 100644 (file)
--- a/src/libraries/Common/src/Interop/Unix/System.Security.Cryptography.Native/Interop.SslCtx.cs
+++ b/src/libraries/Common/src/Interop/Unix/System.Security.Cryptography.Native/Interop.SslCtx.cs
@@ -15,7 +15,7 @@ internal static partial class Interop
     {
         internal delegate int AppVerifyCallback(IntPtr storeCtx, IntPtr arg);
         internal delegate int ClientCertCallback(IntPtr ssl, out IntPtr x509, out IntPtr pkey);
-        internal delegate int SslCtxSetAlpnCallback(IntPtr ssl, out IntPtr outp, out byte outlen, IntPtr inp, uint inlen, IntPtr arg);
+        internal unsafe delegate int SslCtxSetAlpnCallback(IntPtr ssl, out byte* outp, out byte outlen, byte* inp, uint inlen, IntPtr arg);
 
         [DllImport(Libraries.CryptoNative, EntryPoint = "CryptoNative_SslCtxCreate")]
         internal static extern SafeSslContextHandle SslCtxCreate(IntPtr method);

diff --git a/src/libraries/Native/Unix/System.Security.Cryptography.Native/opensslshim.h b/src/libraries/Native/Unix/System.Security.Cryptography.Native/opensslshim.h
index ee211be..46d3359 100644 (file)
--- a/src/libraries/Native/Unix/System.Security.Cryptography.Native/opensslshim.h
+++ b/src/libraries/Native/Unix/System.Security.Cryptography.Native/opensslshim.h
@@ -61,7 +61,6 @@ void SSL_CTX_set_alpn_select_cb(SSL_CTX* ctx, int (*cb) (SSL *ssl,
                                             unsigned int inlen,
                                             void *arg), void *arg);
 void SSL_get0_alpn_selected(const SSL* ssl, const unsigned char** protocol, unsigned int* len);
-int32_t SSL_select_next_proto(unsigned char** out, unsigned char* outlen, const unsigned char* server, unsigned int server_len, const unsigned char* client, unsigned int client_len);
 #endif
 
 #define API_EXISTS(fn) (fn != nullptr)
@@ -292,7 +291,6 @@ int32_t SSL_select_next_proto(unsigned char** out, unsigned char* outlen, const
     PER_FUNCTION_BLOCK(SSL_new, true) \
     PER_FUNCTION_BLOCK(SSL_read, true) \
     PER_FUNCTION_BLOCK(SSL_renegotiate_pending, true) \
-    PER_FUNCTION_BLOCK(SSL_select_next_proto, false) \
     PER_FUNCTION_BLOCK(SSL_set_accept_state, true) \
     PER_FUNCTION_BLOCK(SSL_set_bio, true) \
     PER_FUNCTION_BLOCK(SSL_set_connect_state, true) \
@@ -586,7 +584,6 @@ FOR_ALL_OPENSSL_FUNCTIONS
 #define SSL_new SSL_new_ptr
 #define SSL_read SSL_read_ptr
 #define SSL_renegotiate_pending SSL_renegotiate_pending_ptr
-#define SSL_select_next_proto SSL_select_next_proto_ptr
 #define SSL_set_accept_state SSL_set_accept_state_ptr
 #define SSL_set_bio SSL_set_bio_ptr
 #define SSL_set_connect_state SSL_set_connect_state_ptr

diff --git a/src/libraries/Native/Unix/System.Security.Cryptography.Native/pal_ssl.cpp b/src/libraries/Native/Unix/System.Security.Cryptography.Native/pal_ssl.cpp
index d056e22..281ae54 100644 (file)
--- a/src/libraries/Native/Unix/System.Security.Cryptography.Native/pal_ssl.cpp
+++ b/src/libraries/Native/Unix/System.Security.Cryptography.Native/pal_ssl.cpp
@@ -525,20 +525,6 @@ extern "C" int32_t CryptoNative_SslAddExtraChainCert(SSL* ssl, X509* x509)
     return 0;
 }
 
-extern "C" int32_t CryptoNative_SslSelectNextProto(uint8_t** out, uint8_t* outlen, const uint8_t* server, uint32_t server_len, const uint8_t* client, uint32_t client_len)
-{
-#ifdef HAVE_OPENSSL_ALPN
-    if (API_EXISTS(SSL_select_next_proto))
-    {
-        return SSL_select_next_proto(out, outlen, server, server_len, client, client_len);
-    }
-    else
-#endif
-    {
-        return -1;
-    }
-}
-
 extern "C" void CryptoNative_SslCtxSetAlpnSelectCb(SSL_CTX* ctx, SslCtxSetAlpnCallback cb, void* arg)
 {
 #ifdef HAVE_OPENSSL_ALPN

diff --git a/src/libraries/Native/Unix/System.Security.Cryptography.Native/pal_ssl.h b/src/libraries/Native/Unix/System.Security.Cryptography.Native/pal_ssl.h
index 10e6aca..3a179e0 100644 (file)
--- a/src/libraries/Native/Unix/System.Security.Cryptography.Native/pal_ssl.h
+++ b/src/libraries/Native/Unix/System.Security.Cryptography.Native/pal_ssl.h
@@ -375,12 +375,6 @@ Returns 1 if success and 0 in case of failure
 extern "C" int32_t CryptoNative_SslAddExtraChainCert(SSL* ssl, X509* x509);
 
 /*
-Shims the SSL_select_next_proto method.
-Returns 1 on success, 0 on failure.
-*/
-extern "C" int32_t CryptoNative_SslSelectNextProto(uint8_t** out, uint8_t* outlen, const uint8_t* server, uint32_t server_len, const uint8_t* client, uint32_t client_len);
-
-/*
 Shims the ssl_ctx_set_alpn_select_cb method.
 */
 extern "C" void CryptoNative_SslCtxSetAlpnSelectCb(SSL_CTX* ctx, SslCtxSetAlpnCallback cb, void *arg);

diff --git a/src/libraries/System.Net.Security/src/System/Net/Security/SecureChannel.cs b/src/libraries/System.Net.Security/src/System/Net/Security/SecureChannel.cs
index f3a8d0a..ac86fde 100644 (file)
--- a/src/libraries/System.Net.Security/src/System/Net/Security/SecureChannel.cs
+++ b/src/libraries/System.Net.Security/src/System/Net/Security/SecureChannel.cs
@@ -183,21 +183,8 @@ namespace System.Net.Security
             _refreshCredentialNeeded = true;
         }
 
-        ~SecureChannel()
-        {
-            if (_sslAuthenticationOptions.AlpnProtocolsHandle.IsAllocated)
-            {
-                _sslAuthenticationOptions.AlpnProtocolsHandle.Free();
-            }
-        }
-
         internal void Close()
         {
-            if (_sslAuthenticationOptions.AlpnProtocolsHandle.IsAllocated)
-            {
-                _sslAuthenticationOptions.AlpnProtocolsHandle.Free();
-            }
-
             if (_securityContext != null)
             {
                 _securityContext.Dispose();
@@ -938,7 +925,7 @@ namespace System.Net.Security
             }
 
             byte[] writeBuffer = output;
-                        
+
             SecurityStatusPal secStatus = SslStreamPal.EncryptMessage(
                 _securityContext,
                 buffer,

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
index d2d5bb9..609666a 100644 (file)
--- a/src/libraries/System.Net.Security/src/System/Net/Security/SslAuthenticationOptions.cs
+++ b/src/libraries/System.Net.Security/src/System/Net/Security/SslAuthenticationOptions.cs
@@ -66,7 +66,6 @@ namespace System.Net.Security
         internal bool CheckCertName { get; set; }
         internal RemoteCertValidationCallback CertValidationDelegate { get; set; }
         internal LocalCertSelectionCallback CertSelectionDelegate { get; set; }
-        internal GCHandle AlpnProtocolsHandle { get; set; }
     }
 }
 

diff --git a/src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamAlpnTests.cs b/src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamAlpnTests.cs
index 68d7e18..f32743f 100644 (file)
--- a/src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamAlpnTests.cs
+++ b/src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamAlpnTests.cs
@@ -89,7 +89,6 @@ namespace System.Net.Security.Tests
         }
 
         [Theory]
-        [ActiveIssue(24866, TestPlatforms.Linux)]
         [MemberData(nameof(Alpn_TestData))]
         public void SslStream_StreamToStream_Alpn_Success(List<SslApplicationProtocol> clientProtocols, List<SslApplicationProtocol> serverProtocols, SslApplicationProtocol expected)
         {