diff --git a/src/Servers/HttpSys/src/RequestProcessing/Request.cs b/src/Servers/HttpSys/src/RequestProcessing/Request.cs index d0fb349f324f..cbdfd0d8cfa1 100644 --- a/src/Servers/HttpSys/src/RequestProcessing/Request.cs +++ b/src/Servers/HttpSys/src/RequestProcessing/Request.cs @@ -8,8 +8,10 @@ using System.Security.Cryptography; using System.Security.Cryptography.X509Certificates; using System.Security.Principal; +using Microsoft.AspNetCore.Http; using Microsoft.AspNetCore.HttpSys.Internal; using Microsoft.Extensions.Logging; +using Microsoft.Extensions.Primitives; using Microsoft.Net.Http.Headers; namespace Microsoft.AspNetCore.Server.HttpSys; @@ -108,6 +110,8 @@ internal Request(RequestContext requestContext) // Finished directly accessing the HTTP_REQUEST structure. RequestContext.ReleasePins(); // TODO: Verbose log parameters + + RemoveContentLengthIfTransferEncodingContainsChunked(); } internal ulong UConnectionId { get; } @@ -116,7 +120,7 @@ internal Request(RequestContext requestContext) // No ulongs in public APIs... public long ConnectionId => RawConnectionId != 0 ? (long)RawConnectionId : (long)UConnectionId; - + internal ulong RequestId { get; } private SslStatus SslStatus { get; } @@ -134,7 +138,7 @@ public long? ContentLength { // Note Http.Sys adds the Transfer-Encoding: chunked header to HTTP/2 requests with bodies for back compat. var transferEncoding = Headers[HeaderNames.TransferEncoding].ToString(); - if (string.Equals("chunked", transferEncoding.Trim(), StringComparison.OrdinalIgnoreCase)) + if (transferEncoding != null && string.Equals("chunked", transferEncoding.Split(',')[^1].Trim(), StringComparison.OrdinalIgnoreCase)) { _contentBoundaryType = BoundaryType.Chunked; } @@ -458,4 +462,32 @@ private static partial class Log [LoggerMessage(LoggerEventIds.ErrorInReadingCertificate, LogLevel.Debug, "An error occurred reading the client certificate.", EventName = "ErrorInReadingCertificate")] public static partial void ErrorInReadingCertificate(ILogger logger, Exception exception); } + + private void RemoveContentLengthIfTransferEncodingContainsChunked() + { + if (StringValues.IsNullOrEmpty(Headers.ContentLength)) { return; } + + var transferEncoding = Headers[HeaderNames.TransferEncoding].ToString(); + if (transferEncoding == null || !string.Equals("chunked", transferEncoding.Split(',')[^1].Trim(), StringComparison.OrdinalIgnoreCase)) + { + return; + } + + // https://datatracker.ietf.org/doc/html/rfc7230#section-3.3.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 + // 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. + // 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. + IHeaderDictionary headerDictionary = Headers; + headerDictionary.Add("X-Content-Length", headerDictionary[HeaderNames.ContentLength]); + Headers.ContentLength = StringValues.Empty; + } } diff --git a/src/Servers/HttpSys/test/FunctionalTests/RequestHeaderTests.cs b/src/Servers/HttpSys/test/FunctionalTests/RequestHeaderTests.cs index 3f03b9b4f8a8..8498ca254d9c 100644 --- a/src/Servers/HttpSys/test/FunctionalTests/RequestHeaderTests.cs +++ b/src/Servers/HttpSys/test/FunctionalTests/RequestHeaderTests.cs @@ -1,15 +1,12 @@ // Licensed to the .NET Foundation under one or more agreements. // The .NET Foundation licenses this file to you under the MIT license. -using System; -using System.Collections.Generic; using System.Net.Http; using System.Net.Sockets; using System.Text; -using System.Threading.Tasks; +using Microsoft.AspNetCore.Http; using Microsoft.AspNetCore.Testing; using Microsoft.Extensions.Primitives; -using Xunit; namespace Microsoft.AspNetCore.Server.HttpSys; @@ -78,6 +75,84 @@ public async Task RequestHeaders_ServerAddsCustomHeaders_Success() } } + [ConditionalFact] + public async Task RequestHeaders_ClientSendTransferEncodingHeaders() + { + string address; + using (Utilities.CreateHttpServer(out address, httpContext => + { + var requestHeaders = httpContext.Request.Headers; + var request = httpContext.Features.Get().Request; + Assert.Single(requestHeaders["Transfer-Encoding"]); + Assert.Equal("chunked", requestHeaders.TransferEncoding); + Assert.True(request.HasEntityBody); + return Task.FromResult(0); + })) + { + var headerDictionary = new HeaderDictionary(new Dictionary { + { "Transfer-Encoding", "chunked" } + }); + var response = await SendRequestAsync(address, headerDictionary); + var responseStatusCode = response.Substring(9, 3); // Skip "HTTP/1.1 " + Assert.Equal("200", responseStatusCode); + } + } + + [ConditionalFact] + public async Task RequestHeaders_ClientSendTransferEncodingHeadersWithMultipleValues() + { + string address; + using (Utilities.CreateHttpServer(out address, httpContext => + { + var requestHeaders = httpContext.Request.Headers; + var request = httpContext.Features.Get().Request; + Assert.Single(requestHeaders["Transfer-Encoding"]); + Assert.Equal("gzip, chunked", requestHeaders.TransferEncoding); + Assert.True(request.HasEntityBody); + return Task.FromResult(0); + })) + { + var headerDictionary = new HeaderDictionary(new Dictionary { + { "Transfer-Encoding", new string[] { "gzip", "chunked" } } + }); + var response = await SendRequestAsync(address, headerDictionary); + var responseStatusCode = response.Substring(9, 3); // Skip "HTTP/1.1 " + Assert.Equal("200", responseStatusCode); + } + } + + [ConditionalFact] + public async Task RequestHeaders_ClientSendTransferEncodingAndContentLength_ContentLengthShouldBeRemoved() + { + string address; + using (Utilities.CreateHttpServer(out address, httpContext => + { + var requestHeaders = httpContext.Request.Headers; + var request = httpContext.Features.Get().Request; + Assert.Single(requestHeaders["Transfer-Encoding"]); + Assert.Equal("gzip, 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"]); + return Task.FromResult(0); + })) + { + var headerDictionary = new HeaderDictionary(new Dictionary { + { "Transfer-Encoding", new string[] { "gzip", "chunked" } }, + { "Content-Length", "1" }, + }); + var response = await SendRequestAsync(address, headerDictionary); + var responseStatusCode = response.Substring(9, 3); // Skip "HTTP/1.1 " + Assert.Equal("200", responseStatusCode); + } + } + private async Task SendRequestAsync(string uri) { using (HttpClient client = new HttpClient()) @@ -114,4 +189,35 @@ private async Task SendRequestAsync(string address, string customHeader, string[ await Task.Run(() => socket.Receive(response)); socket.Dispose(); } + + private async Task SendRequestAsync(string address, IHeaderDictionary headers) + { + var uri = new Uri(address); + StringBuilder builder = new StringBuilder(); + builder.AppendLine("POST / HTTP/1.1"); + builder.AppendLine("Connection: close"); + builder.Append("HOST: "); + builder.AppendLine(uri.Authority); + foreach (var header in headers) + { + foreach (var value in header.Value) + { + builder.Append(header.Key); + builder.Append(": "); + builder.AppendLine(value); + } + } + builder.AppendLine(); + + byte[] request = Encoding.ASCII.GetBytes(builder.ToString()); + + using (Socket socket = new Socket(SocketType.Stream, ProtocolType.Tcp)) + { + socket.Connect(uri.Host, uri.Port); + socket.Send(request); + byte[] response = new byte[1024 * 5]; + var read = await Task.Run(() => socket.Receive(response)); + return Encoding.ASCII.GetString(response, 0, read); + } + } } diff --git a/src/Servers/IIS/IIS/src/Core/IISHttpContext.cs b/src/Servers/IIS/IIS/src/Core/IISHttpContext.cs index e94cd37896cc..3654ba6999a3 100644 --- a/src/Servers/IIS/IIS/src/Core/IISHttpContext.cs +++ b/src/Servers/IIS/IIS/src/Core/IISHttpContext.cs @@ -259,8 +259,26 @@ private bool CheckRequestCanHaveBody() // Note Http.Sys adds the Transfer-Encoding: chunked header to HTTP/2 requests with bodies for back compat. // Transfer-Encoding takes priority over Content-Length. string transferEncoding = RequestHeaders.TransferEncoding.ToString(); - if (string.Equals("chunked", transferEncoding?.Trim(), StringComparison.OrdinalIgnoreCase)) - { + if (transferEncoding != null && string.Equals("chunked", transferEncoding.Split(',')[^1].Trim(), StringComparison.OrdinalIgnoreCase)) + { + // https://datatracker.ietf.org/doc/html/rfc7230#section-3.3.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 + // 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. + // 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) + { + RequestHeaders.Add("X-Content-Length", RequestHeaders[HeaderNames.ContentLength]); + RequestHeaders.ContentLength = null; + } return true; } diff --git a/src/Servers/IIS/IIS/test/Common.FunctionalTests/RequestResponseTests.cs b/src/Servers/IIS/IIS/test/Common.FunctionalTests/RequestResponseTests.cs index b40b82e624ed..e5c3139677d0 100644 --- a/src/Servers/IIS/IIS/test/Common.FunctionalTests/RequestResponseTests.cs +++ b/src/Servers/IIS/IIS/test/Common.FunctionalTests/RequestResponseTests.cs @@ -718,6 +718,47 @@ await connection.Send( await Task.WhenAll(tasks); } + [ConditionalFact] + [RequiresNewHandler] + public async Task SendTransferEncodingHeadersWithMultipleValues() + { + using (var connection = _fixture.CreateTestConnection()) + { + await connection.Send( + "POST /TransferEncodingHeadersWithMultipleValues HTTP/1.1", + "Transfer-Encoding: gzip, chunked", + "Host: localhost", + "Connection: close", + "", + ""); + + await connection.Receive( + "HTTP/1.1 200 OK", + ""); + } + } + + [ConditionalFact] + [RequiresNewHandler] + public async Task SendTransferEncodingAndContentLength_ContentLengthShouldBeRemoved() + { + using (var connection = _fixture.CreateTestConnection()) + { + await connection.Send( + "POST /TransferEncodingAndContentLengthShouldBeRemove HTTP/1.1", + "Transfer-Encoding: gzip, chunked", + "Content-Length: 5", + "Host: localhost", + "Connection: close", + "", + ""); + + await connection.Receive( + "HTTP/1.1 200 OK", + ""); + } + } + private async Task<(int Status, string Body)> SendSocketRequestAsync(string path) { using (var connection = _fixture.CreateTestConnection()) diff --git a/src/Servers/IIS/IIS/test/testassets/InProcessWebSite/Startup.cs b/src/Servers/IIS/IIS/test/testassets/InProcessWebSite/Startup.cs index d394828b9ece..1a9bd5982ce7 100644 --- a/src/Servers/IIS/IIS/test/testassets/InProcessWebSite/Startup.cs +++ b/src/Servers/IIS/IIS/test/testassets/InProcessWebSite/Startup.cs @@ -1061,6 +1061,45 @@ public Task InvalidCharacter(HttpContext context) return Task.CompletedTask; } + private async Task TransferEncodingHeadersWithMultipleValues(HttpContext ctx) + { + try + { +#if !FORWARDCOMPAT + Assert.True(ctx.Request.CanHaveBody()); +#endif + Assert.True(ctx.Request.Headers.ContainsKey("Transfer-Encoding")); + Assert.Equal("gzip, chunked", ctx.Request.Headers["Transfer-Encoding"]); + return; + } + catch (Exception exception) + { + ctx.Response.StatusCode = 500; + await ctx.Response.WriteAsync(exception.ToString()); + } + } + + private async Task TransferEncodingAndContentLengthShouldBeRemove(HttpContext ctx) + { + try + { +#if !FORWARDCOMPAT + Assert.True(ctx.Request.CanHaveBody()); +#endif + Assert.True(ctx.Request.Headers.ContainsKey("Transfer-Encoding")); + Assert.Equal("gzip, chunked", ctx.Request.Headers["Transfer-Encoding"]); + Assert.False(ctx.Request.Headers.ContainsKey("Content-Length")); + Assert.True(ctx.Request.Headers.ContainsKey("X-Content-Length")); + Assert.Equal("5", ctx.Request.Headers["X-Content-Length"]); + return; + } + catch (Exception exception) + { + ctx.Response.StatusCode = 500; + await ctx.Response.WriteAsync(exception.ToString()); + } + } + #if !FORWARDCOMPAT public Task ResponseTrailers_HTTP2_TrailersAvailable(HttpContext context) {