dotnet / aspnetcore

ASP.NET Core is a cross-platform .NET framework for building modern cloud-based web applications on Windows, Mac, or Linux.
https://asp.net
MIT License
35.19k stars 9.93k forks source link

Kestrel server returns 500 status code (instead of 400) if client corrupts TLS encrypted data #46778

Open gkostal opened 1 year ago

gkostal commented 1 year ago

Is there an existing issue for this?

Describe the bug

The Kestrel server in ASP.NET Core will return a 500 HTTP status code response if a client garbles the TLS encrypted data for the body of a POST request sent to the service.

A 500 HTTP status code response indicates a service malfunction -- and if the same request is sent again in the future it might succeed. This is not the case for the described circumstance. If the client repeatedly garbles the TLS encrypted data for the POST request body, the service will continue to fail in processing the request.

The issue is that the client is sending bad data -- hence, the HTTP status code response should not be 500. Instead, it should be something like 400 (bad request).

Here's the definition of the 400 status code. Note the warning that the client shouldn't repeat the request without modification, exactly matching the circumstances described here.

image

Expected Behavior

The Kestrel server in ASP.NET Core should return a HTTP 400 status code response if the client sends garbled TLS encrypted data for the request body for a POST request.

Steps To Reproduce

A simple repro has been created and published on GitHub.

cec System.IO.IOException: The decryption operation failed, see inner exception. ---> System.ComponentModel.Win32Exception (0x80090330): The specified data could not be decrypted. --- End of inner exception stack trace --- at System.Net.Security.SslStream.ReadAsyncInternal[TIOAdapter](TIOAdapter adapter, Memory1 buffer) at System.IO.Pipelines.StreamPipeReader.<ReadAsync>g__Core|36_0(StreamPipeReader reader, CancellationTokenSource tokenSource, CancellationToken cancellationToken) at Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http.Http1ContentLengthMessageBody.ReadAsyncInternal(CancellationToken cancellationToken) at System.Runtime.CompilerServices.PoolingAsyncValueTaskMethodBuilder1.StateMachineBox1.System.Threading.Tasks.Sources.IValueTaskSource<TResult>.GetResult(Int16 token) at Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http.HttpRequestStream.ReadAsyncInternal(Memory1 destination, CancellationToken cancellationToken) at System.Text.Json.JsonSerializer.ReadFromStreamAsync(Stream utf8Json, ReadBufferState bufferState, CancellationToken cancellationToken) at System.Text.Json.JsonSerializer.ReadAllAsync[TValue](Stream utf8Json, JsonTypeInfo jsonTypeInfo, CancellationToken cancellationToken) at Microsoft.AspNetCore.Mvc.Formatters.SystemTextJsonInputFormatter.ReadRequestBodyAsync(InputFormatterContext context, Encoding encoding) at Microsoft.AspNetCore.Mvc.Formatters.SystemTextJsonInputFormatter.ReadRequestBodyAsync(InputFormatterContext context, Encoding encoding) at Microsoft.AspNetCore.Mvc.ModelBinding.Binders.BodyModelBinder.BindModelAsync(ModelBindingContext bindingContext) at Microsoft.AspNetCore.Mvc.ModelBinding.ParameterBinder.BindModelAsync(ActionContext actionContext, IModelBinder modelBinder, IValueProvider valueProvider, ParameterDescriptor parameter, ModelMetadata metadata, Object value, Object container) at Microsoft.AspNetCore.Mvc.Controllers.ControllerBinderDelegateProvider.<>cDisplayClass0_0.<gBind|0>d.MoveNext() --- End of stack trace from previous location --- at Microsoft.AspNetCore.Mvc.Infrastructure.ControllerActionInvoker.gAwaited|13_0(ControllerActionInvoker invoker, Task lastTask, State next, Scope scope, Object state, Boolean isCompleted) at Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.gAwaited|20_0(ResourceInvoker invoker, Task lastTask, State next, Scope scope, Object state, Boolean isCompleted) at Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.gLogged|17_1(ResourceInvoker invoker) at Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.g__Logged|17_1(ResourceInvoker invoker) at Microsoft.AspNetCore.Routing.EndpointMiddleware.gAwaitRequestTask|6_0(Endpoint endpoint, Task requestTask, ILogger logger) at Microsoft.AspNetCore.Authorization.AuthorizationMiddleware.Invoke(HttpContext context) at Microsoft.AspNetCore.HttpLogging.HttpLoggingMiddleware.InvokeInternal(HttpContext context) at Microsoft.AspNetCore.Diagnostics.DeveloperExceptionPageMiddleware.Invoke(HttpContext context)

HEADERS

Host: 127.0.0.1:7000 Content-Type: application/json; charset=utf-8 Content-Length: 1073

0



### Exceptions (if any)

_No response_

### .NET Version

6.0

### Anything else?

_No response_
Tratcher commented 1 year ago

IOExceptions almost always indicate a client disconnect so we're usually less concerned about the status code, it doesn't reach the client.

If your transport (TCP/TLS) is experiencing corruption then all bets are off at the application layer. I'm a little surprised you were even able to respond.

Maybe we should make a point of killing the connection if there are TLS errors.

gkostal commented 1 year ago

IOExceptions almost always indicate a client disconnect so we're usually less concerned about the status code, it doesn't reach the client.

If your transport (TCP/TLS) is experiencing corruption then all bets are off at the application layer. I'm a little surprised you were even able to respond.

Maybe we should make a point of killing the connection if there are TLS errors.

If you run the repro, you can see that the client does receive a 500 response from the ASP.NET Core Kestrel service. The specific error code in this circumstance is decrypt from the server's perspective -- so, the client didn't disconnect and responses can make it back to the client.

Tratcher commented 1 year ago

Related: https://github.com/dotnet/aspnetcore/issues/43831. It's interesting this isn't wrapped as a BadHttpRequestException.

Tratcher commented 1 year ago

Repro notes: Remove the DeveloperExceptionPageMiddleware, assume Kestrel is catching the exception in production.

Tratcher commented 1 year ago

Also related: https://github.com/dotnet/aspnetcore/issues/13333, having model binding catch IOExceptions and produce 400s.

adityamandaleeka commented 1 year ago

Triage: we should catch IOExceptions in the https transport and wrap them in BadHttpRequestExceptions so we return 400 (and make sure the connection gets closed).

amcasey commented 1 year ago

Some things I noticed while investigating:

  1. If you send the request as a single string, rather than line-by-line, you get no response at all. I think it's just handling the exception by disconnecting.
  2. If you send the request line-by-line, Connection Logging shows the 500 response, but HTTP Logging does not, since it is generated at a lower level (i.e. one not hooked by the HTTP Logging middleware).
amcasey commented 1 year ago

On Ubuntu (WSL), I'm seeing

fail: Microsoft.AspNetCore.Diagnostics.DeveloperExceptionPageMiddleware[1]
      An unhandled exception has occurred while executing the request.
      System.IO.IOException: The decryption operation failed, see inner exception.
       ---> Interop+OpenSsl+SslException: Decrypt failed with OpenSSL error - SSL_ERROR_SSL.
       ---> Interop+Crypto+OpenSslCryptographicException: error:0A000119:SSL routines::decryption failed or bad record mac
         --- End of inner exception stack trace ---
         at Interop.OpenSsl.Decrypt(SafeSslHandle context, Span`1 buffer, SslErrorCode& errorCode)
         at System.Net.Security.SslStreamPal.DecryptMessage(SafeDeleteSslContext securityContext, Span`1 buffer, Int32& offset, Int32& count)
         --- End of inner exception stack trace ---
         at System.Net.Security.SslStream.ReadAsyncInternal[TIOAdapter](Memory`1 buffer, CancellationToken cancellationToken)

It's still an IOException, but I'm not convinced we want to treat every IOException as a client error. @Tratcher may know more?

amcasey commented 1 year ago

I don't think we can detect decryption failures in a cross-platform way, since SslStream throws the rather generic IOException. Since that effectively makes the connection unusable, the server is doing mostly the right thing. At this point, improving the status code is a nice-to-have, since this scenario is relatively rare.

ghost commented 1 year ago

We've moved this issue to the Backlog milestone. This means that it is not going to be worked on for the coming release. We will reassess the backlog following the current release and consider this item at that time. To learn more about our issue management process and to have better expectation regarding different types of issues you can read our Triage Process.