HttpListener crashes application on Linux when SSL connection fails

[martin-frydl]

When I create HttpListener for https (using reflection since it is not supported yet - #19752) and connect to it via http (or client just does not trust the certificate), the exception is thrown from thread outside my control and crashes whole application.

This bug is known from mono https://bugzilla.xamarin.com/show_bug.cgi?id=52675 and was fixed there mono/mono@c1ddee1. But it reappeared here.

Attached is test project, just run with "dotnet run SslConnectionCrashBugDemo.csproj". If you connect via SSL and trust the certificate (curl -k -D - -s https://localhost:4040/), it works fine - returns response and exists. But if you connect via plain HTTP, the application crashes (curl -D - -s http://localhost:4040/):

Unhandled Exception: System.IO.IOException: The handshake failed due to an unexpected packet format.
   at System.Net.Security.SslState.StartReadFrame(Byte[] buffer, Int32 readBytes, AsyncProtocolRequest asyncRequest)
   at System.Net.Security.SslState.StartReceiveBlob(Byte[] buffer, AsyncProtocolRequest asyncRequest)
   at System.Net.Security.SslState.ForceAuthentication(Boolean receiveFirst, Byte[] buffer, AsyncProtocolRequest asyncRequest)
   at System.Net.Security.SslState.ProcessAuthentication(LazyAsyncResult lazyResult)
   at System.Net.Security.SslStream.AuthenticateAsServer(SslServerAuthenticationOptions sslServerAuthenticationOptions)
   at System.Net.Security.SslStream.AuthenticateAsServer(X509Certificate serverCertificate, Boolean clientCertificateRequired, SslProtocols enabledSslProtocols, Boolean checkCertificateRevocation)
   at System.Net.HttpConnection..ctor(Socket sock, HttpEndPointListener epl, Boolean secure, X509Certificate cert)
   at System.Net.HttpEndPointListener.ProcessAccept(SocketAsyncEventArgs args)
   at System.Net.HttpEndPointListener.OnAccept(Object sender, SocketAsyncEventArgs e)
   at System.Net.Sockets.SocketAsyncEventArgs.OnCompleted(SocketAsyncEventArgs e)
   at System.Net.Sockets.SocketAsyncEventArgs.ExecutionCallback(Object state)
   at System.Threading.ExecutionContext.RunInternal(ExecutionContext executionContext, ContextCallback callback, Object state)
--- End of stack trace from previous location where exception was thrown ---
   at System.Net.Sockets.SocketAsyncEventArgs.FinishOperationAsyncSuccess(Int32 bytesTransferred, SocketFlags flags)
   at System.Net.Sockets.SocketAsyncEventArgs.CompletionCallback(Int32 bytesTransferred, SocketFlags flags, SocketError socketError)
   at System.Net.Sockets.SocketAsyncEventArgs.AcceptCompletionCallback(IntPtr acceptedFileDescriptor, Byte[] socketAddress, Int32 socketAddressSize, SocketError socketError)
   at System.Net.Sockets.SocketAsyncContext.AcceptOperation.InvokeCallback(Boolean allowPooling)
   at System.Net.Sockets.SocketAsyncContext.OperationQueue`1.ProcessAsyncOperation(TOperation op)
   at System.Net.Sockets.SocketAsyncContext.OperationQueue`1.<>c.<.cctor>b__18_0(Object op)
   at System.Threading.ThreadPoolWorkQueue.Dispatch()

[karelz]

Given that you use unsupported set up via Reflection, I wonder if this is real thing we should change.
Will we hit it once we implement https? Shouldn't we make it part of that work?

[martin-frydl]

Yes, you will because I did. All you need to get SSL working is to set the certificate. Currently the only way to do that is to use reflection as I did. Then it works but using this bug anyone can crash the server.

When not fixing SSL bugs before it is implemented, why have you fixed dotnet/corefx#19487? That's also SSL only.

[pavelchuchma]

Voting for a fix of this bug.

We have a larger project written in C# and we added an ability to run it on Mono about 2 years ago. Now we started a migration from Mono to .net core to get better performance (especially GC), stability and use of live and fresh runtime. Unfortunately, it is sometimes a little bit fresher than we wish :-) Anyway, we are early adopters and such hits are part of the accepted game.

Of course, the proposed way is to use Kestrel instead of "hacked" SSL in this way. Unfortunately, our existing code uses HttpListeners in many places and we are not able to replace all of them in the first release supporting .net core.

This bug is currently the main blocker to use our product in production on .net core. A release of a product which can be killed by a single wget call is not possible :-(

The proposed fix is simple. It just catches an unexpected exception and makes a proper cleanup instead of a crash of the whole runtime. There is no side effect. It was tested and accepted by Mono and we use it in Mono for more than one year.

As SSL is not officially supported in HttpListener, there is no regression risk for you. But it will be a big help for us. Please consider this during the planning of the fix of this bug.

Thank you.

[karelz]

@martin-frydl we didn't fix dotnet/corefx#19487, we took contribution from Mono team to ease their way of reusing our libraries. cc @baulig as he may be interested in porting this fix from Mono to CoreFX as well.
I am not sure if there was any general sweep for differences between Mono and CoreFX in HttpListener area - @baulig @marek-safar?

@pavelchuchma technically, there is always risk for regression from any change. No matter how harmless it seems.
However, note that I am not arguing for not taking it based on regression risk. I am trying to understand how to prioritize it. Given that it is hacky (reflection-based) unsupported scenario at this moment, I cannot recommend to prioritize it at this moment over the other work in the space. However, I think we won't be against a contribution here (assuming the contributor can verify the fix with he repro above / with their implementation).

In general, I would be more interested in proper overall SSL support in HttpListener (#14691) which is useful e.g. for IoT scenarios. This bug fix will likely be part of that work (based on information from @martin-frydl).

[marek-safar]

We plan to share the implementation of HttpListener with CoreFX but we are not there yet

[martin-frydl]

I really don't know why fixing this is so problematic. Looking at your code https://github.com/dotnet/corefx/blob/master/src/System.Net.HttpListener/src/System/Net/Managed/HttpEndPointListener.cs#L119, it is more or less the same as Mono code https://github.com/mono/mono/blob/master/mcs/class/System/System.Net/EndPointListener.cs#L117. The only difference is that try catch with accepted.Close(). The SSL seems to work fine, only this bug causes the server vulnerable to attack because all the attacker has to do to kill the server is to connect to it via plain HTTP. It throws exception from thread I cannot handle and it stops whole application.

The fix is in Mono for some time now, you surely have tests for HttpListener so why not to make this minor fix? You should see if it break something in tests. The problem is that we have to spend lot of time learning Kestrel, hoping it supports what we need and then spend another time implementing it on every place it currently uses HttpListener. We have lot of work porting other parts from .NET Framework to .NET Core since there are many problems with APIs behaving differently on Linux/Core and Windows/Fwk and this would really help us a lot.

[karelz]

@martin-frydl I think we're running in circles here.
.NET Core does not support SSL in HttpListener on Linux. Using Reflection to hack-enable features is unsupported and we want to DISCOURAGE anyone from doing that. Fixing bugs on such code paths proactively is encouragement and goes against our goals. Hence, the priority is extremely low. We did not close the issue right away just because it will be needed one day for full SSL support in HttpListener on Linux (#19752).

The fix is in Mono for some time now, you surely have tests for HttpListener so why not to make this minor fix? You should see if it break something in tests.

First, HttpListener is legacy API - i.e. the bar for changes is higher. Second, I don't have high trust in our test bed coverage, especially for HttpListener given that it is legacy API and it didn't get that much attention (many of the tests actually came from community contributions).

The problem is that we have to spend lot of time learning Kestrel, hoping it supports what we need and then spend another time implementing it on every place it currently uses HttpListener.

Learning Kestrel and using Kestrel is a good future investment, given the status of HttpListener. The only place where HttpListener makes sense is IoT scenarios, or legacy scenarios without any high-perf / rich-features requirements.

We have lot of work porting other parts from .NET Framework to .NET Core since there are many problems with APIs behaving differently on Linux/Core and Windows/Fwk and this would really help us a lot.

That is a valid concern. From that perspective I would count it as +1 on proper SSL support in HttpListener on Linux (#19752 (please upvote the issue).
It would be also good to understand your HttpListener usage/scenario: What do you use it for? What kind of perf / feature richness do you expect from it? Why do you need to run it on Linux?
Either way, even if we had the "small" change in our code base, it is not something I would recommend any serious customer to use via Reflection as it is unsupported (i.e. we do not have tests, we would not fix bugs). From support-level point of view it would be as good as compiling the sources yourself with the modification / private API exposing the certs.

That said, I believe we would be ok with community contribution, assuming it is straight port of the Mono change and assuming the contributor is willing to test it locally (using the hacky Reflection way). Is it something you are interested in @martin-frydl?