diff --git a/src/Servers/HttpSys/src/RequestProcessing/Request.cs b/src/Servers/HttpSys/src/RequestProcessing/Request.cs index 47e40d6b28d3..71f3dbfeb624 100644 --- a/src/Servers/HttpSys/src/RequestProcessing/Request.cs +++ b/src/Servers/HttpSys/src/RequestProcessing/Request.cs @@ -21,6 +21,8 @@ namespace Microsoft.AspNetCore.Server.HttpSys; internal sealed partial class Request { + private static readonly bool AllowKeepAliveAfterCLTE = AppContext.TryGetSwitch("Microsoft.AspNetCore.Server.HttpSys.AllowKeepAliveAfterCLTE", out var value) && value; + private X509Certificate2? _clientCert; // TODO: https://github.com/aspnet/HttpSysServer/issues/231 // private byte[] _providedTokenBindingId; @@ -203,6 +205,8 @@ internal Request(RequestContext requestContext) private RequestContext RequestContext { get; } + public bool KeepAlive { get; private set; } = true; + // With the leading ?, if any public string QueryString { get; } @@ -514,24 +518,36 @@ private void RemoveContentLengthIfTransferEncodingContainsChunked() return; } - // https://datatracker.ietf.org/doc/html/rfc7230#section-3.3.2 + // https://www.rfc-editor.org/rfc/rfc9112#section-6.2 // A sender MUST NOT send a Content-Length header field in any message // that contains a Transfer-Encoding header field. - // https://datatracker.ietf.org/doc/html/rfc7230#section-3.3.3 + // https://www.rfc-editor.org/rfc/rfc9112#section-6.3-2.3 // If a message is received with both a Transfer-Encoding and a // Content-Length header field, the Transfer-Encoding overrides the - // Content-Length. Such a message might indicate an attempt to - // perform request smuggling (Section 9.5) or response splitting - // (Section 9.4) and ought to be handled as an error. A sender MUST - // remove the received Content-Length field prior to forwarding such - // a message downstream. + // Content-Length. Such a message might indicate an attempt to + // perform request smuggling (Section 11.2) or response splitting + // (Section 11.1) and ought to be handled as an error. An intermediary + // that chooses to forward the message MUST first remove the received + // Content-Length field and process the Transfer-Encoding + // (as described below) prior to forwarding the message downstream. // We should remove the Content-Length request header in this case, for compatibility - // reasons, include X-Content-Length so that the original Content-Length is still available. + // reasons, include x-Content-Length so that the original Content-Length is still available. IHeaderDictionary headerDictionary = Headers; // dont overwrite if user explicitly set X-Content-Length _ = headerDictionary.TryAdd("X-Content-Length", headerDictionary[HeaderNames.ContentLength]); Headers.ContentLength = StringValues.Empty; + + if (!AllowKeepAliveAfterCLTE) + { + // https://www.rfc-editor.org/rfc/rfc9112#section-6.1 + // A server MAY reject a request that contains both Content-Length + // and Transfer-Encoding or process such a request in accordance + // with the Transfer-Encoding alone. Regardless, the server MUST + // close the connection after responding to such a request to + // avoid the potential attacks. + KeepAlive = false; + } } private static bool IsChunked(string? transferEncoding) diff --git a/src/Servers/HttpSys/src/RequestProcessing/Response.cs b/src/Servers/HttpSys/src/RequestProcessing/Response.cs index c9533fa925aa..4a671166c42d 100644 --- a/src/Servers/HttpSys/src/RequestProcessing/Response.cs +++ b/src/Servers/HttpSys/src/RequestProcessing/Response.cs @@ -419,7 +419,7 @@ internal uint ComputeHeaders(long writeCount, bool endOfRequest = false) var statusCanHaveBody = CanSendResponseBody(RequestContext.Response.StatusCode); // Determine if the connection will be kept alive or closed. - var keepConnectionAlive = true; + var keepConnectionAlive = Request.KeepAlive; // An HTTP/1.1 server may also establish persistent connections with // HTTP/1.0 clients upon receipt of a Keep-Alive connection token. diff --git a/src/Servers/HttpSys/src/RequestProcessing/ResponseBody.cs b/src/Servers/HttpSys/src/RequestProcessing/ResponseBody.cs index 0a75f67adccb..0103b3582d42 100644 --- a/src/Servers/HttpSys/src/RequestProcessing/ResponseBody.cs +++ b/src/Servers/HttpSys/src/RequestProcessing/ResponseBody.cs @@ -475,6 +475,11 @@ private uint ComputeLeftToWrite(long writeCount, bool endOfRequest = false) } } + if (!_requestContext.Request.KeepAlive) + { + flags |= PInvoke.HTTP_SEND_RESPONSE_FLAG_DISCONNECT; + } + if (endOfRequest && _requestContext.Response.BoundaryType == BoundaryType.Close) { flags |= PInvoke.HTTP_SEND_RESPONSE_FLAG_DISCONNECT; diff --git a/src/Servers/HttpSys/test/FunctionalTests/RequestHeaderTests.cs b/src/Servers/HttpSys/test/FunctionalTests/RequestHeaderTests.cs index 2e2081a1e4a7..f439460a4def 100644 --- a/src/Servers/HttpSys/test/FunctionalTests/RequestHeaderTests.cs +++ b/src/Servers/HttpSys/test/FunctionalTests/RequestHeaderTests.cs @@ -210,6 +210,79 @@ public async Task RequestHeaders_ClientSendTransferEncodingAndContentLengthAndXC } } + [ConditionalFact] + public async Task CloseConnectionAfterProcessingContentLengthPlusChunkedRequest() + { + string address; + using (Utilities.CreateHttpServer(out address, async httpContext => + { + var requestHeaders = httpContext.Request.Headers; + var request = httpContext.Features.Get().Request; + Assert.Single(requestHeaders["Transfer-Encoding"]); + Assert.Equal("chunked", requestHeaders.TransferEncoding); + + Assert.Null(request.ContentLength); + Assert.True(request.HasEntityBody); + + Assert.False(requestHeaders.ContainsKey("Content-Length")); + Assert.Null(requestHeaders.ContentLength); + + Assert.Single(requestHeaders["X-Content-Length"]); + Assert.Equal("1", requestHeaders["X-Content-Length"]); + + await httpContext.Response.WriteAsync("Hello World"); + }, LoggerFactory)) + { + var uri = new Uri(address); + using (Socket socket = new Socket(SocketType.Stream, ProtocolType.Tcp)) + { + socket.Connect(uri.Host, uri.Port); + // Send 2 requests with both CL and TE header + // expect the second to not be processed and the connection to be closed + for (var i = 0; i < 2; i++) + { + socket.Send(Encoding.ASCII.GetBytes(string.Join("\r\n", + "POST / HTTP/1.1", + $"Host: {uri.Authority}", + "Transfer-Encoding: chunked", + "Connection: keep-alive", + "Content-Length: 1", + "", + "5", "Hello", + "6", " World", + "0", + "", + ""))); + } + byte[] response = new byte[1024 * 5]; + var sb = new StringBuilder(); + + int read = 0; + do + { + read = await Task.Run(() => socket.Receive(response)); + var s = Encoding.ASCII.GetString(response, 0, read); + sb.Append(s); + } while (read != 0); + + var resp = sb.ToString(); + + Assert.Matches(@"HTTP/1\.1 200 OK +Transfer-Encoding: chunked +Server: Microsoft-HTTPAPI/2\.0 +Date: .+ +Connection: close + +B +Hello World +0 + +$", + resp); + } + } + } + [ConditionalFact] public async Task RequestHeaders_AllKnownHeadersKeys_Received() { diff --git a/src/Servers/IIS/AspNetCoreModuleV2/InProcessRequestHandler/managedexports.cpp b/src/Servers/IIS/AspNetCoreModuleV2/InProcessRequestHandler/managedexports.cpp index 056ba647f4ba..ee42f5d0500d 100644 --- a/src/Servers/IIS/AspNetCoreModuleV2/InProcessRequestHandler/managedexports.cpp +++ b/src/Servers/IIS/AspNetCoreModuleV2/InProcessRequestHandler/managedexports.cpp @@ -437,6 +437,16 @@ http_close_connection( return S_OK; } +EXTERN_C __declspec(dllexport) +HRESULT +http_set_close( + _In_ IN_PROCESS_HANDLER* pInProcessHandler +) +{ + pInProcessHandler->QueryHttpContext()->GetResponse()->SetNeedDisconnect(); + return S_OK; +} + EXTERN_C __declspec(dllexport) HRESULT http_response_set_unknown_header( diff --git a/src/Servers/IIS/IIS/src/Core/IISHttpContext.cs b/src/Servers/IIS/IIS/src/Core/IISHttpContext.cs index 21b5bac7f4f6..2f24f0a0642b 100644 --- a/src/Servers/IIS/IIS/src/Core/IISHttpContext.cs +++ b/src/Servers/IIS/IIS/src/Core/IISHttpContext.cs @@ -30,6 +30,8 @@ namespace Microsoft.AspNetCore.Server.IIS.Core; internal abstract partial class IISHttpContext : NativeRequestContext, IThreadPoolWorkItem, IDisposable { + private static readonly bool AllowKeepAliveAfterCLTE = AppContext.TryGetSwitch("Microsoft.AspNetCore.Server.IIS.AllowKeepAliveAfterCLTE", out var value) && value; + private const int MinAllocBufferSize = 2048; protected readonly NativeSafeHandle _requestNativeHandle; @@ -42,6 +44,7 @@ internal abstract partial class IISHttpContext : NativeRequestContext, IThreadPo private int _statusCode; private string? _reasonPhrase; + // Used to synchronize callback registration and native method calls internal readonly object _contextLock = new object(); @@ -387,24 +390,36 @@ private bool CheckRequestCanHaveBody() string transferEncoding = RequestHeaders.TransferEncoding.ToString(); if (IsChunked(transferEncoding)) { - // https://datatracker.ietf.org/doc/html/rfc7230#section-3.3.2 + // https://www.rfc-editor.org/rfc/rfc9112#section-6.2 // A sender MUST NOT send a Content-Length header field in any message // that contains a Transfer-Encoding header field. - // https://datatracker.ietf.org/doc/html/rfc7230#section-3.3.3 + // https://www.rfc-editor.org/rfc/rfc9112#section-6.3-2.3 // If a message is received with both a Transfer-Encoding and a // Content-Length header field, the Transfer-Encoding overrides the - // Content-Length. Such a message might indicate an attempt to - // perform request smuggling (Section 9.5) or response splitting - // (Section 9.4) and ought to be handled as an error. A sender MUST - // remove the received Content-Length field prior to forwarding such - // a message downstream. + // Content-Length. Such a message might indicate an attempt to + // perform request smuggling (Section 11.2) or response splitting + // (Section 11.1) and ought to be handled as an error. An intermediary + // that chooses to forward the message MUST first remove the received + // Content-Length field and process the Transfer-Encoding + // (as described below) prior to forwarding the message downstream. // We should remove the Content-Length request header in this case, for compatibility // reasons, include X-Content-Length so that the original Content-Length is still available. - if (RequestHeaders.ContentLength.HasValue) + if (RequestHeaders.TryGetValue(HeaderNames.ContentLength, out var contentLength)) { // if user already passed X-Content-Length, we won't overwrite it - _ = RequestHeaders.TryAdd("X-Content-Length", RequestHeaders[HeaderNames.ContentLength]); + _ = RequestHeaders.TryAdd("X-Content-Length", contentLength); RequestHeaders.ContentLength = null; + + if (!AllowKeepAliveAfterCLTE) + { + // https://www.rfc-editor.org/rfc/rfc9112#section-6.1 + // A server MAY reject a request that contains both Content-Length + // and Transfer-Encoding or process such a request in accordance + // with the Transfer-Encoding alone. Regardless, the server MUST + // close the connection after responding to such a request to + // avoid the potential attacks. + NativeMethods.HttpSetClose(_requestNativeHandle); + } } return true; } diff --git a/src/Servers/IIS/IIS/src/NativeMethods.cs b/src/Servers/IIS/IIS/src/NativeMethods.cs index 6aa3d68ed35e..1fe994ec4ff4 100644 --- a/src/Servers/IIS/IIS/src/NativeMethods.cs +++ b/src/Servers/IIS/IIS/src/NativeMethods.cs @@ -143,6 +143,9 @@ private static unsafe partial int http_websockets_write_bytes( [LibraryImport(AspNetCoreModuleDll)] private static partial int http_close_connection(NativeSafeHandle pInProcessHandler); + [LibraryImport(AspNetCoreModuleDll)] + private static partial int http_set_close(NativeSafeHandle pInProcessHandler); + [LibraryImport(AspNetCoreModuleDll)] private static partial int http_response_set_need_goaway(NativeSafeHandle pInProcessHandler); @@ -306,6 +309,11 @@ public static void HttpCloseConnection(NativeSafeHandle pInProcessHandler) Validate(http_close_connection(pInProcessHandler)); } + public static void HttpSetClose(NativeSafeHandle pInProcessHandler) + { + Validate(http_set_close(pInProcessHandler)); + } + public static unsafe void HttpResponseSetUnknownHeader(NativeSafeHandle pInProcessHandler, byte* pszHeaderName, byte* pszHeaderValue, ushort usHeaderValueLength, bool fReplace) { Validate(http_response_set_unknown_header(pInProcessHandler, pszHeaderName, pszHeaderValue, usHeaderValueLength, fReplace)); diff --git a/src/Servers/IIS/IIS/test/Common.FunctionalTests/RequestResponseTests.cs b/src/Servers/IIS/IIS/test/Common.FunctionalTests/RequestResponseTests.cs index 4179953dcff5..8865ae76b78f 100644 --- a/src/Servers/IIS/IIS/test/Common.FunctionalTests/RequestResponseTests.cs +++ b/src/Servers/IIS/IIS/test/Common.FunctionalTests/RequestResponseTests.cs @@ -10,8 +10,8 @@ using System.Net.Http.Headers; using System.Text; using System.Threading.Tasks; -using Microsoft.AspNetCore.Server.IntegrationTesting; using Microsoft.AspNetCore.InternalTesting; +using Microsoft.AspNetCore.Server.IntegrationTesting; using Xunit; #if !IIS_FUNCTIONALS @@ -788,6 +788,62 @@ await connection.Receive( } } + [ConditionalFact] + public async Task CloseConnectionAfterProcessingContentLengthPlusChunkedRequest() + { + using (var connection = _fixture.CreateTestConnection()) + { + for (var i = 0; i < 2; i++) + { + await connection.Send( + "POST /ReadAndWriteEcho HTTP/1.1", + "Host: localhost", + "Transfer-Encoding: chunked", + "Connection: keep-alive", + "Content-Length: 25", + "", + "5", "Hello", + "6", " World", + "0", + "", + ""); + } + + await connection.Receive( + "HTTP/1.1 200 OK", + ""); + var headers = await connection.ReceiveHeaders(); + + // RFC 9112 ยง6.1: the server MUST close the connection after responding + // to a request that contained both Content-Length and Transfer-Encoding. + Assert.Contains("Connection: close", headers); + Assert.Contains("Server: Microsoft-IIS/10.0", headers); + + if (headers.Contains("Transfer-Encoding: chunked")) + { + await connection.Receive( + "B", + "Hello World", + ""); + await connection.Receive( + "0", + "", + ""); + } + else + { + // Either framed by Content-Length: 11 or by connection close + // (no framing header). Either way the body is exactly "Hello World" + // and WaitForConnectionClose below verifies nothing else follows. + await connection.Receive("Hello World"); + } + + // Verify the second request was not processed and that the server closed + // the connection (no extra bytes are sent). + await connection.WaitForConnectionClose(); + } + } + private async Task<(int Status, string Body)> SendSocketRequestAsync(string path) { using (var connection = _fixture.CreateTestConnection()) diff --git a/src/Servers/Kestrel/Core/src/Internal/Http/Http1MessageBody.cs b/src/Servers/Kestrel/Core/src/Internal/Http/Http1MessageBody.cs index 26d4996e416b..1722febc22bb 100644 --- a/src/Servers/Kestrel/Core/src/Internal/Http/Http1MessageBody.cs +++ b/src/Servers/Kestrel/Core/src/Internal/Http/Http1MessageBody.cs @@ -15,6 +15,8 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http; internal abstract class Http1MessageBody : MessageBody { + private static readonly bool ContinueProcessingAfterCLTE = AppContext.TryGetSwitch("Microsoft.AspNetCore.Server.Kestrel.AllowKeepAliveAfterCLTE", out var value) && value; + protected readonly Http1Connection _context; private bool _readerCompleted; @@ -166,17 +168,18 @@ public static MessageBody For( KestrelBadHttpRequestException.Throw(RequestRejectionReason.FinalTransferCodingNotChunked, transferEncoding); } - // https://datatracker.ietf.org/doc/html/rfc7230#section-3.3.2 + // https://www.rfc-editor.org/rfc/rfc9112#section-6.2 // A sender MUST NOT send a Content-Length header field in any message // that contains a Transfer-Encoding header field. - // https://datatracker.ietf.org/doc/html/rfc7230#section-3.3.3 + // https://www.rfc-editor.org/rfc/rfc9112#section-6.3-2.3 // If a message is received with both a Transfer-Encoding and a // Content-Length header field, the Transfer-Encoding overrides the - // Content-Length. Such a message might indicate an attempt to - // perform request smuggling (Section 9.5) or response splitting - // (Section 9.4) and ought to be handled as an error. A sender MUST - // remove the received Content-Length field prior to forwarding such - // a message downstream. + // Content-Length. Such a message might indicate an attempt to + // perform request smuggling (Section 11.2) or response splitting + // (Section 11.1) and ought to be handled as an error. An intermediary + // that chooses to forward the message MUST first remove the received + // Content-Length field and process the Transfer-Encoding + // (as described below) prior to forwarding the message downstream. // We should remove the Content-Length request header in this case, for compatibility // reasons, include x-Content-Length so that the original Content-Length is still available. if (headers.ContentLength.HasValue) @@ -186,6 +189,17 @@ public static MessageBody For( // if user already passed X-Content-Length, we won't overwrite it _ = headerDictionary.TryAdd("X-Content-Length", headerDictionary[HeaderNames.ContentLength]); headers.ContentLength = null; + + if (!ContinueProcessingAfterCLTE) + { + // https://www.rfc-editor.org/rfc/rfc9112#section-6.1 + // A server MAY reject a request that contains both Content-Length + // and Transfer-Encoding or process such a request in accordance + // with the Transfer-Encoding alone. Regardless, the server MUST + // close the connection after responding to such a request to + // avoid the potential attacks. + keepAlive = false; + } } // TODO may push more into the wrapper rather than just calling into the message body diff --git a/src/Servers/Kestrel/test/InMemory.FunctionalTests/ChunkedRequestTests.cs b/src/Servers/Kestrel/test/InMemory.FunctionalTests/ChunkedRequestTests.cs index 0e37009b4544..c36a68dba0d6 100644 --- a/src/Servers/Kestrel/test/InMemory.FunctionalTests/ChunkedRequestTests.cs +++ b/src/Servers/Kestrel/test/InMemory.FunctionalTests/ChunkedRequestTests.cs @@ -11,6 +11,7 @@ using Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http; using Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Infrastructure; using Microsoft.AspNetCore.Server.Kestrel.InMemory.FunctionalTests.TestTransport; +using Microsoft.DotNet.RemoteExecutor; using Microsoft.Extensions.Diagnostics.Metrics.Testing; using Microsoft.Extensions.Logging; using BadHttpRequestException = Microsoft.AspNetCore.Server.Kestrel.Core.BadHttpRequestException; @@ -119,7 +120,7 @@ private async Task PipeApp(HttpContext httpContext) } } - private async Task AppChunked(HttpContext httpContext) + private static async Task AppChunked(HttpContext httpContext) { var request = httpContext.Request; var response = httpContext.Response; @@ -1267,4 +1268,88 @@ await connection.ReceiveEnd( } } } + + [Fact] + public async Task CloseConnectionAfterProcessingContentLengthPlusChunkedRequest() + { + var testContext = new TestServiceContext(LoggerFactory); + + await using (var server = new TestServer(AppChunked, testContext)) + { + using (var connection = server.CreateConnection()) + { + for (var i = 0; i < 2; i++) + { + await connection.Send( + "POST / HTTP/1.1", + "Host:", + "Transfer-Encoding: chunked", + "Connection: keep-alive", + "Content-Length: 7", + "", + "5", "Hello", + "6", " World", + "0", + "", + ""); + } + + await connection.ReceiveEnd( + "HTTP/1.1 200 OK", + "Content-Length: 11", + "Connection: close", + $"Date: {testContext.DateHeaderValue}", + "", + "Hello World"); + } + } + } + + [ConditionalFact] + [RemoteExecutionSupported] + public void CanKeepProcessingRequestsAfterContentLengthPlusChunkedRequest_WithAppContext() + { + var options = new RemoteInvokeOptions(); + options.RuntimeConfigurationOptions.Add("Microsoft.AspNetCore.Server.Kestrel.AllowKeepAliveAfterCLTE", "true"); + + using var remoteHandle = RemoteExecutor.Invoke(static async () => + { + var testContext = new TestServiceContext(); + + await using (var server = new TestServer(AppChunked, testContext)) + { + using (var connection = server.CreateConnection()) + { + for (var i = 0; i < 2; i++) + { + await connection.Send( + "POST / HTTP/1.1", + "Host:", + "Transfer-Encoding: chunked", + "Connection: keep-alive", + "Content-Length: 7", + "", + "5", "Hello", + "6", " World", + "0", + "", + ""); + } + + await connection.Receive( + "HTTP/1.1 200 OK", + "Content-Length: 11", + $"Date: {testContext.DateHeaderValue}", + "", + "Hello World"); + await connection.Receive( + "HTTP/1.1 200 OK", + "Content-Length: 11", + $"Date: {testContext.DateHeaderValue}", + "", + "Hello World"); + } + } + }, options); + } }