Improper handling of `errSSLClosedGraceful` returned from `SSLWrite` on macOS

[comex]

Description

This issue applies to the original Secure Transport-based SSL implementation on macOS. It does not apply to the new Network.framework-based implementation which seems slated to replace it, but for now Secure Transport is still the default implementation.

If you try to write to a System.Net.Security.SslStream after the other side has sent a TLS close_notify alert (which is a standard way to close a connection), you'll get an unexpected exception:

Unhandled exception. System.IO.IOException: The encryption operation failed, see inner exception.
 ---> System.ComponentModel.Win32Exception (14): Bad address

Cause

There are actually two bugs here.

1. No check for ClosedGracefully

In this case, the native function SSLWrite is returning errSSLClosedGraceful, which Interop.AppleCrypto.SslWrite translates to PAL_TlsIo.ClosedGracefully. But the caller, SslStreamPal.EncryptMessage (in SslStreamPal.OSX.cs), doesn't have a case for this error code. DecryptMessage does have a case for it:

                            case PAL_TlsIo.ClosedGracefully:
                                return new SecurityStatusPal(SecurityStatusPalErrorCode.ContextExpired);

But EncryptMessage doesn’t, so we fall into:

                            default:
                                Debug.Fail($"Unknown status value: {status}");
                                token.Status = new SecurityStatusPal(SecurityStatusPalErrorCode.InternalError);
                                break;

Note that Debug.Fail crashes the program on debug builds of System.Net.Security (but does nothing on release builds). Clearly we don’t want to fall into this case in any legitimate circumstance.

2. Misinterpreted error code

If we’re on a release build, then we proceed past the Debug.Fail to token.Status = new SecurityStatusPal(SecurityStatusPalErrorCode.InternalError); However, this is later translated to an exception with the wrong message: “Win32Exception (14): Bad address”. There is no actual bad address here. Instead, this function in SslStreamPal.OSX.cs is extracting the numeric value of SecurityStatusPalErrorCode.InternalError, 14, and passing it to new Win32Exception:

        public static Exception GetException(SecurityStatusPal status)
        {
            return status.Exception ?? new Win32Exception((int)status.ErrorCode);
        }

Which picks this constructor:

        public Win32Exception(int error) : this(error, Marshal.GetPInvokeErrorMessage(error))

But GetPInvokeErrorMessage interprets the integer as a Unix errno value and calls out to strerror to get a string description. "Bad address" is the description of EFAULT, which just happens to be the errno value with numeric value 14 on macOS.

This should be fixed in addition to the first bug, since it could still apply to other error cases.

Test case

To reproduce, compile and run Program.cs.

(On macOS this will use Secure Transport by default. To use Network Framework, change the SslProtocols argument to either SslProtocols.Tls13 | SslProtocols.Tls12 or SslProtocols.Tls13, and pass DOTNET_SYSTEM_NET_SECURITY_USENETWORKFRAMEWORK=1. If you only pass SslProtocols.Tls12 then it will fall back to Secure Transport regardless of the environment variable, so to test Network.framework with TLS 1.2 you need to pass SslProtocols.Tls13 | SslProtocols.Tls12 and connect to a server that doesn’t support TLS 1.3. For convenience, I’m running such a server on port 444 at the same host from the test case.)

Here are some pcaps taken from running the program on various systems. They have the TLS secrets injected, so they should be viewable in Wireshark without any additional files. However, to view these properly, you should go to Preferences -> Protocols -> TLS and uncheck “Reassemble TLS Application Data spanning multiple TLS records”. Otherwise, some of the close_notify alerts will not be decoded properly. This appears to be a Wireshark bug.

What do we expect to happen?

This section is merely informative.

For practical compatibility, it would be nice if the specific exception thrown was the same on all platforms, but this is probably not achievable.

In my testing, on both Linux and Windows, SslStream disregards the close_notify alert and attempts to send data anyway. Therefore, the first Write call succeeds and sends an Application Data packet to the server; the server responds with a TCP RST, and subsequent Write calls fail with an error in Socket.Send:

Unhandled exception. System.IO.IOException: Unable to write data to the transport connection: An established connection was aborted by the software in your host machine..
 ---> System.Net.Sockets.SocketException (10053): An established connection was aborted by the software in your host machine.
   at System.Net.Sockets.Socket.Send(ReadOnlySpan`1 buffer, SocketFlags socketFlags)

Interestingly, the semantics here changed between TLS 1.2 and TLS 1.3. According to the TLS 1.2 RFC, after a close_notify alert is sent, “[t]he other party MUST respond with a close_notify alert of its own and close down the connection immediately, discarding any pending writes.” (source) In the TLS 1.3 RFC, on the other hand, “Each party MUST send a "close_notify" alert before closing its write side of the connection”, but “[t]his does not have any effect on its read side of the connection.” (source) In other words, trying to send more data after receiving close_notify is legitimate in TLS 1.3, but not in TLS 1.2.

However, my test case forces the use of TLS 1.2 on all platforms, so it’s not a protocol difference, just a case of SecureTransport enforcing the TLS 1.2 semantics while OpenSSL and Schannel apparently don’t.

Meanwhile, when using Network Framework with either TLS 1.2 or TLS 1.3, the client automatically responds to the close_notify with its own close_notify, and produces a different exception:

Unhandled exception. System.IO.IOException: The write operation failed, see inner exception.
 ---> Interop+NetworkFramework+NetworkFrameworkException: Operation canceled
   at System.Net.Quic.ResettableValueTaskSource.TryComplete(Exception exception, Boolean final)
   at System.Net.Security.SafeDeleteNwContext.<WriteAsync>g__CompletionCallback|40_0(IntPtr context, NetworkFrameworkError* error)
--- End of stack trace from previous location ---
   at System.Net.Quic.ResettableValueTaskSource.System.Threading.Tasks.Sources.IValueTaskSource.GetResult(Int16 token)
   at System.Net.Security.SafeDeleteNwContext.WriteAsync(ReadOnlyMemory`1 buffer, CancellationToken cancellationToken)
   at System.Net.Security.SafeDeleteNwContext.WriteAsync(ReadOnlyMemory`1 buffer, CancellationToken cancellationToken)
   at System.Net.SyncReadWriteAdapter.WaitAsync(Task task)
   at System.Net.Security.SslStream.WriteAsyncInternal[TIOAdapter](ReadOnlyMemory`1 buffer, CancellationToken cancellationToken), ErrorCode: 89, ErrorDomain: POSIX
   --- End of inner exception stack trace ---
   at System.Net.Security.SslStream.WriteAsyncInternal[TIOAdapter](ReadOnlyMemory`1 buffer, CancellationToken cancellationToken)
   at System.Net.Security.SslStream.Write(Byte[] buffer, Int32 offset, Int32 count)
   at Program.<Main>$(String[] args) in /Users/comex/c/ssltest/Program.cs:line 36

[dotnet-policy-service]

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

[wfurt]

Thanks for great report @comex. I think We should fix it in next release. The new framework is still quite experimental and the fix should be simple and safe.