diff --git a/src/Servers/HttpSys/src/LoggerEventIds.cs b/src/Servers/HttpSys/src/LoggerEventIds.cs index 87f8ea56ee9f..5bc0b6b65ed6 100644 --- a/src/Servers/HttpSys/src/LoggerEventIds.cs +++ b/src/Servers/HttpSys/src/LoggerEventIds.cs @@ -58,4 +58,5 @@ internal static class LoggerEventIds public const int AcceptSetExpectationMismatch = 51; public const int AcceptCancelExpectationMismatch = 52; public const int AcceptObserveExpectationMismatch = 53; + public const int RequestParsingError = 54; } diff --git a/src/Servers/HttpSys/src/RequestProcessing/Request.cs b/src/Servers/HttpSys/src/RequestProcessing/Request.cs index 290218a7e3b8..f02507251cab 100644 --- a/src/Servers/HttpSys/src/RequestProcessing/Request.cs +++ b/src/Servers/HttpSys/src/RequestProcessing/Request.cs @@ -38,145 +38,152 @@ internal Request(RequestContext requestContext) { // TODO: Verbose log RequestContext = requestContext; - _contentBoundaryType = BoundaryType.None; - RequestId = requestContext.RequestId; - // For HTTP/2 Http.Sys assigns each request a unique connection id for use with API calls, but the RawConnectionId represents the real connection. - UConnectionId = requestContext.ConnectionId; - RawConnectionId = requestContext.RawConnectionId; - SslStatus = requestContext.SslStatus; + try + { + _contentBoundaryType = BoundaryType.None; - KnownMethod = requestContext.VerbId; - Method = requestContext.GetVerb()!; + RequestId = requestContext.RequestId; + // For HTTP/2 Http.Sys assigns each request a unique connection id for use with API calls, but the RawConnectionId represents the real connection. + UConnectionId = requestContext.ConnectionId; + RawConnectionId = requestContext.RawConnectionId; + SslStatus = requestContext.SslStatus; - RawUrl = requestContext.GetRawUrl()!; + KnownMethod = requestContext.VerbId; + Method = requestContext.GetVerb()!; - var cookedUrl = requestContext.GetCookedUrl(); - QueryString = cookedUrl.GetQueryString() ?? string.Empty; + RawUrl = requestContext.GetRawUrl()!; - var rawUrlInBytes = requestContext.GetRawUrlInBytes(); - var originalPath = RequestUriBuilder.DecodeAndUnescapePath(rawUrlInBytes); + var cookedUrl = requestContext.GetCookedUrl(); + QueryString = cookedUrl.GetQueryString() ?? string.Empty; - PathBase = string.Empty; - Path = originalPath; - var prefix = requestContext.Server.Options.UrlPrefixes.GetPrefix((int)requestContext.UrlContext); + var rawUrlInBytes = requestContext.GetRawUrlInBytes(); + var originalPath = RequestUriBuilder.DecodeAndUnescapePath(rawUrlInBytes); - // 'OPTIONS * HTTP/1.1' - if (KnownMethod == HTTP_VERB.HttpVerbOPTIONS && string.Equals(RawUrl, "*", StringComparison.Ordinal)) - { PathBase = string.Empty; - Path = string.Empty; - } - // Prefix may be null if the requested has been transferred to our queue - else if (prefix is not null) - { - var pathBase = prefix.PathWithoutTrailingSlash; + Path = originalPath; + var prefix = requestContext.Server.Options.UrlPrefixes.GetPrefix((int)requestContext.UrlContext); - // url: /base/path, prefix: /base/, base: /base, path: /path - // url: /, prefix: /, base: , path: / - if (originalPath.Equals(pathBase, StringComparison.Ordinal)) - { - // Exact match, no need to preserve the casing - PathBase = pathBase; - Path = string.Empty; - } - else if (originalPath.Equals(pathBase, StringComparison.OrdinalIgnoreCase)) + // 'OPTIONS * HTTP/1.1' + if (KnownMethod == HTTP_VERB.HttpVerbOPTIONS && string.Equals(RawUrl, "*", StringComparison.Ordinal)) { - // Preserve the user input casing - PathBase = originalPath; + PathBase = string.Empty; Path = string.Empty; } - else if (originalPath.StartsWith(prefix.Path, StringComparison.Ordinal)) - { - // Exact match, no need to preserve the casing - PathBase = pathBase; - Path = originalPath[pathBase.Length..]; - } - else if (originalPath.StartsWith(prefix.Path, StringComparison.OrdinalIgnoreCase)) + // Prefix may be null if the requested has been transferred to our queue + else if (prefix is not null) { - // Preserve the user input casing - PathBase = originalPath[..pathBase.Length]; - Path = originalPath[pathBase.Length..]; - } - else - { - // Http.Sys path base matching is based on the cooked url which applies some non-standard normalizations that we don't use - // like collapsing duplicate slashes "//", converting '\' to '/', and un-escaping "%2F" to '/'. Find the right split and - // ignore the normalizations. - var originalOffset = 0; - var baseOffset = 0; - while (originalOffset < originalPath.Length && baseOffset < pathBase.Length) + var pathBase = prefix.PathWithoutTrailingSlash; + + // url: /base/path, prefix: /base/, base: /base, path: /path + // url: /, prefix: /, base: , path: / + if (originalPath.Equals(pathBase, StringComparison.Ordinal)) { - var baseValue = pathBase[baseOffset]; - var offsetValue = originalPath[originalOffset]; - if (baseValue == offsetValue - || char.ToUpperInvariant(baseValue) == char.ToUpperInvariant(offsetValue)) - { - // case-insensitive match, continue - originalOffset++; - baseOffset++; - } - else if (baseValue == '/' && offsetValue == '\\') - { - // Http.Sys considers these equivalent - originalOffset++; - baseOffset++; - } - else if (baseValue == '/' && originalPath.AsSpan(originalOffset).StartsWith("%2F", StringComparison.OrdinalIgnoreCase)) - { - // Http.Sys un-escapes this - originalOffset += 3; - baseOffset++; - } - else if (baseOffset > 0 && pathBase[baseOffset - 1] == '/' - && (offsetValue == '/' || offsetValue == '\\')) - { - // Duplicate slash, skip - originalOffset++; - } - else if (baseOffset > 0 && pathBase[baseOffset - 1] == '/' - && originalPath.AsSpan(originalOffset).StartsWith("%2F", StringComparison.OrdinalIgnoreCase)) - { - // Duplicate slash equivalent, skip - originalOffset += 3; - } - else + // Exact match, no need to preserve the casing + PathBase = pathBase; + Path = string.Empty; + } + else if (originalPath.Equals(pathBase, StringComparison.OrdinalIgnoreCase)) + { + // Preserve the user input casing + PathBase = originalPath; + Path = string.Empty; + } + else if (originalPath.StartsWith(prefix.Path, StringComparison.Ordinal)) + { + // Exact match, no need to preserve the casing + PathBase = pathBase; + Path = originalPath[pathBase.Length..]; + } + else if (originalPath.StartsWith(prefix.Path, StringComparison.OrdinalIgnoreCase)) + { + // Preserve the user input casing + PathBase = originalPath[..pathBase.Length]; + Path = originalPath[pathBase.Length..]; + } + else + { + // Http.Sys path base matching is based on the cooked url which applies some non-standard normalizations that we don't use + // like collapsing duplicate slashes "//", converting '\' to '/', and un-escaping "%2F" to '/'. Find the right split and + // ignore the normalizations. + var originalOffset = 0; + var baseOffset = 0; + while (originalOffset < originalPath.Length && baseOffset < pathBase.Length) { - // Mismatch, fall back - // The failing test case here is "/base/call//../bat//path1//path2", reduced to "/base/call/bat//path1//path2", - // where http.sys collapses "//" before "../", but we do "../" first. We've lost the context that there were dot segments, - // or duplicate slashes, how do we figure out that "call/" can be eliminated? - originalOffset = 0; - break; + var baseValue = pathBase[baseOffset]; + var offsetValue = originalPath[originalOffset]; + if (baseValue == offsetValue + || char.ToUpperInvariant(baseValue) == char.ToUpperInvariant(offsetValue)) + { + // case-insensitive match, continue + originalOffset++; + baseOffset++; + } + else if (baseValue == '/' && offsetValue == '\\') + { + // Http.Sys considers these equivalent + originalOffset++; + baseOffset++; + } + else if (baseValue == '/' && originalPath.AsSpan(originalOffset).StartsWith("%2F", StringComparison.OrdinalIgnoreCase)) + { + // Http.Sys un-escapes this + originalOffset += 3; + baseOffset++; + } + else if (baseOffset > 0 && pathBase[baseOffset - 1] == '/' + && (offsetValue == '/' || offsetValue == '\\')) + { + // Duplicate slash, skip + originalOffset++; + } + else if (baseOffset > 0 && pathBase[baseOffset - 1] == '/' + && originalPath.AsSpan(originalOffset).StartsWith("%2F", StringComparison.OrdinalIgnoreCase)) + { + // Duplicate slash equivalent, skip + originalOffset += 3; + } + else + { + // Mismatch, fall back + // The failing test case here is "/base/call//../bat//path1//path2", reduced to "/base/call/bat//path1//path2", + // where http.sys collapses "//" before "../", but we do "../" first. We've lost the context that there were dot segments, + // or duplicate slashes, how do we figure out that "call/" can be eliminated? + originalOffset = 0; + break; + } } + PathBase = originalPath[..originalOffset]; + Path = originalPath[originalOffset..]; } - PathBase = originalPath[..originalOffset]; - Path = originalPath[originalOffset..]; } - } - else if (requestContext.Server.Options.UrlPrefixes.TryMatchLongestPrefix(IsHttps, cookedUrl.GetHost()!, originalPath, out var pathBase, out var path)) - { - PathBase = pathBase; - Path = path; - } + else if (requestContext.Server.Options.UrlPrefixes.TryMatchLongestPrefix(IsHttps, cookedUrl.GetHost()!, originalPath, out var pathBase, out var path)) + { + PathBase = pathBase; + Path = path; + } - ProtocolVersion = RequestContext.GetVersion(); + ProtocolVersion = RequestContext.GetVersion(); - Headers = new RequestHeaders(RequestContext); + Headers = new RequestHeaders(RequestContext); - User = RequestContext.GetUser(); + User = RequestContext.GetUser(); - SniHostName = string.Empty; - if (IsHttps) - { - GetTlsHandshakeResults(); - } + SniHostName = string.Empty; + if (IsHttps) + { + GetTlsHandshakeResults(); + } - // GetTlsTokenBindingInfo(); TODO: https://github.com/aspnet/HttpSysServer/issues/231 + // GetTlsTokenBindingInfo(); TODO: https://github.com/aspnet/HttpSysServer/issues/231 - // Finished directly accessing the HTTP_REQUEST structure. - RequestContext.ReleasePins(); - // TODO: Verbose log parameters + } + finally + { + // Finished directly accessing the HTTP_REQUEST structure. + RequestContext.ReleasePins(); + // TODO: Verbose log parameters + } RemoveContentLengthIfTransferEncodingContainsChunked(); } diff --git a/src/Servers/HttpSys/src/RequestProcessing/RequestContext.FeatureCollection.cs b/src/Servers/HttpSys/src/RequestProcessing/RequestContext.FeatureCollection.cs index 2671bc71c8bb..e1931dc0fc6b 100644 --- a/src/Servers/HttpSys/src/RequestProcessing/RequestContext.FeatureCollection.cs +++ b/src/Servers/HttpSys/src/RequestProcessing/RequestContext.FeatureCollection.cs @@ -64,6 +64,7 @@ internal partial class RequestContext : private bool _bodyCompleted; private IHeaderDictionary _responseHeaders = default!; private IHeaderDictionary? _responseTrailers; + private ulong? _requestId; private Fields _initializedFields; @@ -98,11 +99,26 @@ private enum Fields TraceIdentifier = 0x200, } - protected internal void InitializeFeatures() + protected internal bool InitializeFeatures() { _initialized = true; - Request = new Request(this); + // Get the ID before creating the Request object as the Request ctor releases the native memory + // We want the ID in case request processing fails and we need the ID to cancel the native request + _requestId = RequestId; + try + { + Request = new Request(this); + } + catch (Exception ex) + { + Log.RequestParsingError(Logger, ex); + // Synchronously calls Http.Sys and tells it to send an http response + // No one has written to the response yet (haven't even created the response object below) + Server.SendError(_requestId.Value, StatusCodes.Status400BadRequest, authChallenges: null); + return false; + } + Response = new Response(this); _features = new FeatureCollection(new StandardFeatureCollection(this)); @@ -124,6 +140,7 @@ protected internal void InitializeFeatures() _responseStream = new ResponseStream(Response.Body, OnResponseStart); _responseHeaders = Response.Headers; + return true; } private bool IsNotInitialized(Fields field) diff --git a/src/Servers/HttpSys/src/RequestProcessing/RequestContext.Log.cs b/src/Servers/HttpSys/src/RequestProcessing/RequestContext.Log.cs index 2c587b98a401..41b1cf480d5a 100644 --- a/src/Servers/HttpSys/src/RequestProcessing/RequestContext.Log.cs +++ b/src/Servers/HttpSys/src/RequestProcessing/RequestContext.Log.cs @@ -17,5 +17,8 @@ private static partial class Log [LoggerMessage(LoggerEventIds.ChannelBindingRetrieved, LogLevel.Debug, "Channel binding retrieved.", EventName = "ChannelBindingRetrieved")] public static partial void ChannelBindingRetrieved(ILogger logger); + + [LoggerMessage(LoggerEventIds.RequestParsingError, LogLevel.Debug, "Failed to parse request.", EventName = "RequestParsingError")] + public static partial void RequestParsingError(ILogger logger, Exception exception); } } diff --git a/src/Servers/HttpSys/src/RequestProcessing/RequestContext.cs b/src/Servers/HttpSys/src/RequestProcessing/RequestContext.cs index 9faaab9e831d..20cd8056b04f 100644 --- a/src/Servers/HttpSys/src/RequestProcessing/RequestContext.cs +++ b/src/Servers/HttpSys/src/RequestProcessing/RequestContext.cs @@ -171,9 +171,11 @@ public void Abort() _disconnectToken = new CancellationToken(canceled: true); } ForceCancelRequest(); - Request.Dispose(); + // Request and/or Response can be null (even though the property doesn't say it can) + // if the constructor throws (can happen for invalid path format) + Request?.Dispose(); // Only Abort, Response.Dispose() tries a graceful flush - Response.Abort(); + Response?.Abort(); } private static void Abort(object? state) @@ -192,15 +194,22 @@ internal void ForceCancelRequest() { try { + // Shouldn't be able to get here when this is null, but just in case we'll noop + if (_requestId is null) + { + return; + } + var statusCode = PInvoke.HttpCancelHttpRequest(Server.RequestQueue.Handle, - Request.RequestId, default); + _requestId.Value, default); // Either the connection has already dropped, or the last write is in progress. // The requestId becomes invalid as soon as the last Content-Length write starts. // The only way to cancel now is with CancelIoEx. if (statusCode == ErrorCodes.ERROR_CONNECTION_INVALID) { - Response.CancelLastWrite(); + // Can be null if processing the request threw and the response object was never created. + Response?.CancelLastWrite(); } } catch (ObjectDisposedException) diff --git a/src/Servers/HttpSys/src/RequestProcessing/RequestContextOfT.cs b/src/Servers/HttpSys/src/RequestProcessing/RequestContextOfT.cs index f661bf1fd018..2a1d06a06d26 100644 --- a/src/Servers/HttpSys/src/RequestProcessing/RequestContextOfT.cs +++ b/src/Servers/HttpSys/src/RequestProcessing/RequestContextOfT.cs @@ -27,7 +27,11 @@ public override async Task ExecuteAsync() try { - InitializeFeatures(); + if (!InitializeFeatures()) + { + Abort(); + return; + } if (messagePump.Stopping) { diff --git a/src/Servers/HttpSys/test/FunctionalTests/Listener/RequestTests.cs b/src/Servers/HttpSys/test/FunctionalTests/Listener/RequestTests.cs index 87cc013e4e8c..7a9897245e5a 100644 --- a/src/Servers/HttpSys/test/FunctionalTests/Listener/RequestTests.cs +++ b/src/Servers/HttpSys/test/FunctionalTests/Listener/RequestTests.cs @@ -1,15 +1,9 @@ // 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.IO; -using System.Net.Http; using System.Net.Sockets; using System.Text; -using System.Threading.Tasks; using Microsoft.AspNetCore.InternalTesting; -using Microsoft.Extensions.Logging; -using Xunit; namespace Microsoft.AspNetCore.Server.HttpSys.Listener; @@ -142,6 +136,7 @@ public async Task Request_OverlongUTF8Path(string requestPath, string expectedPa [InlineData("/", "/", "", "/")] [InlineData("/base", "/base", "/base", "")] [InlineData("/base", "/baSe", "/baSe", "")] + [InlineData("/base", "/baSe/", "/baSe", "/")] [InlineData("/base", "/base/path", "/base", "/path")] [InlineData("/base", "///base/path1/path2", "///base", "/path1/path2")] [InlineData("/base/ball", @"/baSe\ball//path1//path2", @"/baSe\ball", "//path1//path2")] diff --git a/src/Servers/HttpSys/test/FunctionalTests/RequestTests.cs b/src/Servers/HttpSys/test/FunctionalTests/RequestTests.cs index b34f57b6172a..daa659c9d2a8 100644 --- a/src/Servers/HttpSys/test/FunctionalTests/RequestTests.cs +++ b/src/Servers/HttpSys/test/FunctionalTests/RequestTests.cs @@ -1,24 +1,17 @@ // 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.Globalization; -using System.IO; using System.Net; using System.Net.Http; using System.Net.Sockets; using System.Text; -using System.Threading; -using System.Threading.Tasks; -using Microsoft.AspNetCore.Authentication; using Microsoft.AspNetCore.Hosting.Server; using Microsoft.AspNetCore.Http; using Microsoft.AspNetCore.Http.Features; using Microsoft.AspNetCore.HttpSys.Internal; using Microsoft.AspNetCore.InternalTesting; using Microsoft.Extensions.Logging; -using Microsoft.Extensions.Options; -using Xunit; namespace Microsoft.AspNetCore.Server.HttpSys; @@ -368,6 +361,43 @@ public async Task Request_UrlUnescaping() } } + [Fact] + public async Task Latin1UrlIsRejected() + { + string root; + using (var server = Utilities.CreateHttpServerReturnRoot("/", out root, httpContext => + { + Assert.Fail("Request should not reach here"); + return Task.FromResult(0); + }, LoggerFactory)) + { + var uri = new Uri(root); + StringBuilder builder = new StringBuilder(); + builder.AppendLine(FormattableString.Invariant($"GET /a HTTP/1.1")); + builder.AppendLine("Connection: close"); + builder.Append("HOST: "); + builder.AppendLine(uri.Authority); + builder.AppendLine(); + byte[] request = Encoding.ASCII.GetBytes(builder.ToString()); + // Replace the 'a' in the path with a Latin1 value + request[5] = 0xe1; + + using (var socket = new Socket(SocketType.Stream, ProtocolType.Tcp)) + { + socket.Connect(uri.Host, uri.Port); + socket.Send(request); + var response = new byte[12]; + await Task.Run(() => socket.Receive(response)); + + var statusCode = Encoding.UTF8.GetString(response).Substring(9); + Assert.Equal("400", statusCode); + } + } + + var errorLogs = TestSink.Writes.Where(w => w.LogLevel >= LogLevel.Error).Select(w => w.Exception); + Assert.False(errorLogs.Any(), string.Join(" ", errorLogs)); + } + [ConditionalFact] public async Task Request_WithDoubleSlashes_LeftAlone() { diff --git a/src/Servers/HttpSys/test/FunctionalTests/ResponseCachingTests.cs b/src/Servers/HttpSys/test/FunctionalTests/ResponseCachingTests.cs index e590ac7dda31..cb71f06346cc 100644 --- a/src/Servers/HttpSys/test/FunctionalTests/ResponseCachingTests.cs +++ b/src/Servers/HttpSys/test/FunctionalTests/ResponseCachingTests.cs @@ -22,8 +22,23 @@ public class ResponseCachingTests : LoggedTest public ResponseCachingTests() { - _absoluteFilePath = Path.Combine(Directory.GetCurrentDirectory(), "Microsoft.AspNetCore.Server.HttpSys.dll"); - _fileLength = new FileInfo(_absoluteFilePath).Length; + _absoluteFilePath = Path.Combine(Directory.GetCurrentDirectory(), Path.GetRandomFileName()); + using var file = File.Create(_absoluteFilePath); + // HttpSys will cache responses up to ~260k, keep this value below that + // 30k is an arbitrary choice + file.Write(new byte[30000]); + _fileLength = 30000; + } + + public override void Dispose() + { + try + { + File.Delete(_absoluteFilePath); + } + catch { } + + base.Dispose(); } [ConditionalFact] @@ -383,7 +398,6 @@ public async Task Caching_SendFileNoContentLength_NotCached() } } - [QuarantinedTest("new issue")] [ConditionalFact] public async Task Caching_SendFileWithFullContentLength_Cached() { diff --git a/src/Servers/HttpSys/test/FunctionalTests/ResponseSendFileTests.cs b/src/Servers/HttpSys/test/FunctionalTests/ResponseSendFileTests.cs index 74233fe9df5f..fd9fa5b6f83c 100644 --- a/src/Servers/HttpSys/test/FunctionalTests/ResponseSendFileTests.cs +++ b/src/Servers/HttpSys/test/FunctionalTests/ResponseSendFileTests.cs @@ -30,6 +30,7 @@ public ResponseSendFileTests() AbsoluteFilePath = Directory.GetFiles(Directory.GetCurrentDirectory()).First(); RelativeFilePath = Path.GetFileName(AbsoluteFilePath); FileLength = new FileInfo(AbsoluteFilePath).Length; + Assert.True(FileLength > 0, "FileLength is 0"); } [ConditionalFact]