improve handling of handshake failure (#35549)

* improve handling of handshake failure

* more cleanup

* remove console reference

* add quotes around parameter

* enable ServerAsyncAuthenticate_MismatchProtocols_Fails

* cleanup ssl2 test

* fix http2

* feedback from review

* add back two missing empty lines

Co-authored-by: Tomas Weinfurt <[email protected]>

Author: wfurt

src/libraries/System.Net.Security/src/Resources/Strings.resx

@@ -212,6 +212,9 @@
   <data name="net_auth_eof" xml:space="preserve">
     <value>Authentication failed because the remote party has closed the transport stream.</value>
   </data>
+  <data name="net_auth_tls_alert" xml:space="preserve">
+    <value>Authentication failed because the remote party sent a TLS alert: '{0}'.</value>
+  </data>
   <data name="net_auth_alert" xml:space="preserve">
     <value>Authentication failed on the remote side (the stream might still be available for additional authentication attempts).</value>
   </data>

src/libraries/System.Net.Security/src/System.Net.Security.csproj

@@ -34,6 +34,7 @@
     <Compile Include="System\Net\Security\StreamSizes.cs" />
     <Compile Include="System\Net\Security\TlsAlertType.cs" />
     <Compile Include="System\Net\Security\TlsAlertMessage.cs" />
+    <Compile Include="System\Net\Security\TlsFrameHelper.cs" />
     <Compile Include="System\Security\Authentication\AuthenticationException.cs" />
     <!-- NegotiateStream -->
     <Compile Include="System\Net\BufferAsyncResult.cs" />

src/libraries/System.Net.Security/src/System/Net/Security/SecureChannel.cs

@@ -625,7 +625,7 @@ private bool AcquireClientCredentials(ref byte[]? thumbPrint)
         //
         // Acquire Server Side Certificate information and set it on the class.
         //
-        private bool AcquireServerCredentials(ref byte[]? thumbPrint, ReadOnlySpan<byte> clientHello)
+        private bool AcquireServerCredentials(ref byte[]? thumbPrint)
         {
             if (NetEventSource.IsEnabled)
                 NetEventSource.Enter(this);
@@ -639,13 +639,13 @@ private bool AcquireServerCredentials(ref byte[]? thumbPrint, ReadOnlySpan<byte>
             // with .NET Framework), and if neither is set we fall back to using ServerCertificate.
             if (_sslAuthenticationOptions.ServerCertSelectionDelegate != null)
             {
-                string? serverIdentity = SniHelper.GetServerName(clientHello);
-                localCertificate = _sslAuthenticationOptions.ServerCertSelectionDelegate(serverIdentity);
-
+                localCertificate = _sslAuthenticationOptions.ServerCertSelectionDelegate(_sslAuthenticationOptions.TargetHost);
                 if (localCertificate == null)
                 {
                     throw new AuthenticationException(SR.net_ssl_io_no_server_cert);
                 }
+                if (NetEventSource.IsEnabled)
+                    NetEventSource.Info(this, "Use delegate selected Cert");
             }
             else if (_sslAuthenticationOptions.CertSelectionDelegate != null)
             {
@@ -784,7 +784,7 @@ private SecurityStatusPal GenerateToken(ReadOnlySpan<byte> inputBuffer, ref byte
                     if (_refreshCredentialNeeded)
                     {
                         cachedCreds = _sslAuthenticationOptions.IsServer
-                                        ? AcquireServerCredentials(ref thumbPrint, inputBuffer)
+                                        ? AcquireServerCredentials(ref thumbPrint)
                                         : AcquireClientCredentials(ref thumbPrint);
                     }
 

src/libraries/System.Net.Security/src/System/Net/Security/SniHelper.cs

@@ -26,13 +26,13 @@ internal class SniHelper
             //     opaque fragment[SSLPlaintext.length];
             // } SSLPlaintext;
             const int ContentTypeOffset = 0;
-            const int ProtocolVersionOffset = ContentTypeOffset + sizeof(ContentType);
+            const int ProtocolVersionOffset = ContentTypeOffset + sizeof(TlsContentType);
             const int LengthOffset = ProtocolVersionOffset + ProtocolVersionSize;
             const int HandshakeOffset = LengthOffset + sizeof(ushort);
 
             // SSL v2's ContentType has 0x80 bit set.
             // We do not care about SSL v2 here because it does not support client hello extensions
-            if (sslPlainText.Length < HandshakeOffset || (ContentType)sslPlainText[ContentTypeOffset] != ContentType.Handshake)
+            if (sslPlainText.Length < HandshakeOffset || (TlsContentType)sslPlainText[ContentTypeOffset] != TlsContentType.Handshake)
             {
                 return null;
             }
@@ -62,10 +62,10 @@ internal class SniHelper
             //     } body;
             // } Handshake;
             const int HandshakeTypeOffset = 0;
-            const int ClientHelloLengthOffset = HandshakeTypeOffset + sizeof(HandshakeType);
+            const int ClientHelloLengthOffset = HandshakeTypeOffset + sizeof(TlsHandshakeType);
             const int ClientHelloOffset = ClientHelloLengthOffset + UInt24Size;
 
-            if (sslHandshake.Length < ClientHelloOffset || (HandshakeType)sslHandshake[HandshakeTypeOffset] != HandshakeType.ClientHello)
+            if (sslHandshake.Length < ClientHelloOffset || (TlsHandshakeType)sslHandshake[HandshakeTypeOffset] != TlsHandshakeType.ClientHello)
             {
                 return null;
             }
@@ -363,16 +363,6 @@ private static Encoding CreateEncoding()
             return Encoding.GetEncoding("utf-8", new EncoderExceptionFallback(), new DecoderExceptionFallback());
         }
 
-        private enum ContentType : byte
-        {
-            Handshake = 0x16
-        }
-
-        private enum HandshakeType : byte
-        {
-            ClientHello = 0x01
-        }
-
         private enum ExtensionType : ushort
         {
             ServerName = 0x00

src/libraries/System.Net.Security/src/System/Net/Security/SslStream.Implementation.cs

@@ -34,14 +34,8 @@ private enum Framing
         // This is set on the first packet to figure out the framing style.
         private Framing _framing = Framing.Unknown;
 
-        // SSL3/TLS protocol frames definitions.
-        private enum FrameType : byte
-        {
-            ChangeCipherSpec = 20,
-            Alert = 21,
-            Handshake = 22,
-            AppData = 23
-        }
+        private TlsAlertDescription _lastAlertDescription;
+        private TlsFrameHandshakeInfo _lastFrame;
 
         private readonly object _handshakeLock = new object();
         private volatile TaskCompletionSource<bool>? _handshakeWaiter;
@@ -274,7 +268,6 @@ private async Task ForceAuthenticationAsync<TIOAdapter>(TIOAdapter adapter, bool
                 {
                     // get ready to receive first frame
                     _handshakeBuffer = new ArrayBuffer(InitialHandshakeBufferSize);
-                    _framing = Framing.Unknown;
                 }
 
                 while (!handshakeCompleted)
@@ -288,6 +281,19 @@ private async Task ForceAuthenticationAsync<TIOAdapter>(TIOAdapter adapter, bool
 
                     if (message.Failed)
                     {
+                        if (_lastFrame.Header.Type == TlsContentType.Handshake && message.Size == 0)
+                        {
+                            // If we failed without OS sending out alert, inject one here to be consistent across platforms.
+                            byte[] alert = TlsFrameHelper.CreateAlertFrame(_lastFrame.Header.Version, TlsAlertDescription.ProtocolVersion);
+                            await adapter.WriteAsync(alert, 0, alert.Length).ConfigureAwait(false);
+                        }
+                        else if (_lastFrame.Header.Type == TlsContentType.Alert && _lastAlertDescription != TlsAlertDescription.CloseNotify &&
+                                 message.Status.ErrorCode == SecurityStatusPalErrorCode.IllegalMessage)
+                        {
+                            // Improve generic message and show details if we failed because of TLS Alert.
+                            throw new AuthenticationException(SR.Format(SR.net_auth_tls_alert, _lastAlertDescription.ToString()), message.GetException());
+                        }
+
                         throw new AuthenticationException(SR.net_auth_SSPI, message.GetException());
                     }
                     else if (message.Status.ErrorCode == SecurityStatusPalErrorCode.OK)
@@ -346,17 +352,49 @@ private async ValueTask<ProtocolToken> ReceiveBlobAsync<TIOAdapter>(TIOAdapter a
                 _framing = DetectFraming(_handshakeBuffer.ActiveReadOnlySpan);
             }
 
-            int frameSize = GetFrameSize(_handshakeBuffer.ActiveReadOnlySpan);
-            if (frameSize < 0)
+            if (_framing == Framing.BeforeSSL3)
+            {
+#pragma warning disable 0618
+                _lastFrame.Header.Version = SslProtocols.Ssl2;
+#pragma warning restore 0618
+                _lastFrame.Header.Length = GetFrameSize(_handshakeBuffer.ActiveReadOnlySpan);
+            }
+            else
+            {
+                TlsFrameHelper.TryGetFrameHeader(_handshakeBuffer.ActiveReadOnlySpan, ref _lastFrame.Header);
+            }
+
+            if (_lastFrame.Header.Length < 0)
             {
                 throw new IOException(SR.net_frame_read_size);
             }
 
+            // Header length is content only so we must add header size as well.
+            int frameSize = _lastFrame.Header.Length + TlsFrameHelper.HeaderSize;
             if (_handshakeBuffer.ActiveLength < frameSize)
             {
                 await FillHandshakeBufferAsync(adapter, frameSize).ConfigureAwait(false);
             }
+
             // At this point, we have at least one TLS frame.
+            if (_lastFrame.Header.Type == TlsContentType.Alert)
+            {
+                TlsAlertLevel level = 0;
+                if (TlsFrameHelper.TryGetAlertInfo(_handshakeBuffer.ActiveReadOnlySpan, ref level, ref _lastAlertDescription))
+                {
+                    if (NetEventSource.IsEnabled && _lastAlertDescription != TlsAlertDescription.CloseNotify) NetEventSource.Fail(this, $"Received TLS alert {_lastAlertDescription}");
+                }
+            }
+            else if (_lastFrame.Header.Type == TlsContentType.Handshake)
+            {
+                if (_handshakeBuffer.ActiveReadOnlySpan[TlsFrameHelper.HeaderSize] == (byte)TlsHandshakeType.ClientHello &&
+                    _sslAuthenticationOptions!.ServerCertSelectionDelegate != null)
+                {
+                    // Process SNI from Client Hello message
+                    TlsFrameHelper.TryGetHandshakeInfo(_handshakeBuffer.ActiveReadOnlySpan, ref _lastFrame);
+                    _sslAuthenticationOptions.TargetHost = _lastFrame.TargetName;
+                }
+            }
 
             return ProcessBlob(frameSize);
         }
@@ -372,23 +410,24 @@ private ProtocolToken ProcessBlob(int frameSize)
             _handshakeBuffer.Discard(frameSize);
 
             // Often more TLS messages fit into same packet. Get as many complete frames as we can.
-            while (_handshakeBuffer.ActiveLength > SecureChannel.ReadHeaderSize)
+            while (_handshakeBuffer.ActiveLength > TlsFrameHelper.HeaderSize)
             {
-                ReadOnlySpan<byte> remainingData = _handshakeBuffer.ActiveReadOnlySpan;
-                if (remainingData[0] >= (int)FrameType.AppData)
+                TlsFrameHeader nextHeader = default;
+
+                if (!TlsFrameHelper.TryGetFrameHeader(_handshakeBuffer.ActiveReadOnlySpan, ref nextHeader))
                 {
                     break;
                 }
 
-                frameSize = GetFrameSize(remainingData);
-                if (_handshakeBuffer.ActiveLength >= frameSize)
+                frameSize = nextHeader.Length + TlsFrameHelper.HeaderSize;
+                if (nextHeader.Type == TlsContentType.AppData || frameSize > _handshakeBuffer.ActiveLength)
                 {
-                    chunkSize += frameSize;
-                    _handshakeBuffer.Discard(frameSize);
-                    continue;
+                    // We don't have full frame left or we already have app data which needs to be processed by decrypt.
+                    break;
                 }
 
-                break;
+                chunkSize += frameSize;
+                _handshakeBuffer.Discard(frameSize);
             }
 
             return _context!.NextMessage(availableData.Slice(0, chunkSize));
@@ -645,7 +684,7 @@ private async ValueTask<int> ReadAsyncInternal<TIOAdapter>(TIOAdapter adapter, M
                     Debug.Assert(_internalBufferCount >= SecureChannel.ReadHeaderSize);
 
                     // Parse the frame header to determine the payload size (which includes the header size).
-                    int payloadBytes = GetFrameSize(_internalBuffer.AsSpan(_internalOffset));
+                    int payloadBytes = TlsFrameHelper.GetFrameSize(_internalBuffer.AsSpan(_internalOffset));
                     if (payloadBytes < 0)
                     {
                         throw new IOException(SR.net_frame_read_size);
@@ -913,6 +952,7 @@ private static byte[] EnsureBufferSize(byte[] buffer, int copyCount, int size)
                     Buffer.BlockCopy(saved, 0, buffer, 0, copyCount);
                 }
             }
+
             return buffer;
         }
 
@@ -1003,8 +1043,8 @@ private Framing DetectFraming(ReadOnlySpan<byte> bytes)
             }
 
             // If the first byte is SSL3 HandShake, then check if we have a SSLv3 Type3 client hello.
-            if (bytes[0] == (byte)FrameType.Handshake || bytes[0] == (byte)FrameType.AppData
-                || bytes[0] == (byte)FrameType.Alert)
+            if (bytes[0] == (byte)TlsContentType.Handshake || bytes[0] == (byte)TlsContentType.AppData
+                || bytes[0] == (byte)TlsContentType.Alert)
             {
                 if (bytes.Length < 3)
                 {

src/libraries/System.Net.Security/src/System/Net/Security/SslStreamPal.OSX.cs

@@ -237,9 +237,8 @@ private static SecurityStatusPal HandshakeInternal(
                     sslContext = new SafeDeleteSslContext((credential as SafeFreeSslCredentials)!, sslAuthenticationOptions);
                     context = sslContext;
 
-                    if (!string.IsNullOrEmpty(sslAuthenticationOptions.TargetHost))
+                    if (!string.IsNullOrEmpty(sslAuthenticationOptions.TargetHost) && !sslAuthenticationOptions.IsServer)
                     {
-                        Debug.Assert(!sslAuthenticationOptions.IsServer, "targetName should not be set for server-side handshakes");
                         Interop.AppleCrypto.SslSetTargetName(sslContext.SslContext, sslAuthenticationOptions.TargetHost);
                     }