diff --git a/src/Servers/HttpSys/src/LoggerEventIds.cs b/src/Servers/HttpSys/src/LoggerEventIds.cs index d345691fcae4..f7d73cb51a79 100644 --- a/src/Servers/HttpSys/src/LoggerEventIds.cs +++ b/src/Servers/HttpSys/src/LoggerEventIds.cs @@ -55,5 +55,6 @@ internal static class LoggerEventIds public static readonly EventId ListenerDisposing = new EventId(46, "ListenerDisposing"); public static readonly EventId RequestValidationFailed = new EventId(47, "RequestValidationFailed"); public static readonly EventId CreateDisconnectTokenError = new EventId(48, "CreateDisconnectTokenError"); + public static readonly EventId RequestParsingError = new EventId(54, "RequestParsingError"); } } diff --git a/src/Servers/HttpSys/src/RequestProcessing/Request.cs b/src/Servers/HttpSys/src/RequestProcessing/Request.cs index 210fef9d652f..a7a8a5245fe8 100644 --- a/src/Servers/HttpSys/src/RequestProcessing/Request.cs +++ b/src/Servers/HttpSys/src/RequestProcessing/Request.cs @@ -42,143 +42,150 @@ 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 == HttpApiTypes.HTTP_VERB.HttpVerbOPTIONS && string.Equals(RawUrl, "*", StringComparison.Ordinal)) - { PathBase = string.Empty; - Path = string.Empty; - } - // Prefix may be null if the request has been transfered to our queue - else if (prefix is not null) - { - var pathBase = prefix.PathWithoutTrailingSlash; - // 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)) + Path = originalPath; + var prefix = requestContext.Server.Options.UrlPrefixes.GetPrefix((int)requestContext.UrlContext); + + // 'OPTIONS * HTTP/1.1' + if (KnownMethod == HttpApiTypes.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)) - { - // Preserve the user input casing - PathBase = originalPath[..pathBase.Length]; - Path = originalPath[pathBase.Length..]; - } - else + // Prefix may be null if the request has been transfered to our queue + else if (prefix is not null) { - // 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//../ball//path1//path2", reduced to "/base/call/ball//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//../ball//path1//path2", reduced to "/base/call/ball//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(); - if (IsHttps) - { - GetTlsHandshakeResults(); - } + 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 + } } internal ulong UConnectionId { get; } diff --git a/src/Servers/HttpSys/src/RequestProcessing/RequestContext.FeatureCollection.cs b/src/Servers/HttpSys/src/RequestProcessing/RequestContext.FeatureCollection.cs index a07642a9abb7..8f3774a91d12 100644 --- a/src/Servers/HttpSys/src/RequestProcessing/RequestContext.FeatureCollection.cs +++ b/src/Servers/HttpSys/src/RequestProcessing/RequestContext.FeatureCollection.cs @@ -69,6 +69,7 @@ internal partial class RequestContext : private bool _bodyCompleted; private IHeaderDictionary _responseHeaders = default!; private IHeaderDictionary? _responseTrailers; + private ulong? _requestId; private Fields _initializedFields; @@ -103,11 +104,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 it 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)); @@ -129,6 +145,7 @@ protected internal void InitializeFeatures() _responseStream = new ResponseStream(Response.Body, OnResponseStart); _responseHeaders = Response.Headers; + return true; } diff --git a/src/Servers/HttpSys/src/RequestProcessing/RequestContext.Log.cs b/src/Servers/HttpSys/src/RequestProcessing/RequestContext.Log.cs index 6378593fab70..0cca9570838c 100644 --- a/src/Servers/HttpSys/src/RequestProcessing/RequestContext.Log.cs +++ b/src/Servers/HttpSys/src/RequestProcessing/RequestContext.Log.cs @@ -19,6 +19,9 @@ private static class Log private static readonly Action _channelBindingRetrieved = LoggerMessage.Define(LogLevel.Debug, LoggerEventIds.ChannelBindingRetrieved, "Channel binding retrieved."); + private static readonly Action _requestParsingError = + LoggerMessage.Define(LogLevel.Debug, LoggerEventIds.RequestParsingError, "Failed to parse request."); + public static void AbortError(ILogger logger, Exception exception) { _abortError(logger, exception); @@ -33,6 +36,11 @@ public static void ChannelBindingRetrieved(ILogger logger) { _channelBindingRetrieved(logger, null); } + + public static void RequestParsingError(ILogger logger, Exception exception) + { + _requestParsingError(logger, exception); + } } } } diff --git a/src/Servers/HttpSys/src/RequestProcessing/RequestContext.cs b/src/Servers/HttpSys/src/RequestProcessing/RequestContext.cs index 9c45a262dac8..61cf0670345a 100644 --- a/src/Servers/HttpSys/src/RequestProcessing/RequestContext.cs +++ b/src/Servers/HttpSys/src/RequestProcessing/RequestContext.cs @@ -186,9 +186,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) @@ -207,15 +209,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 = HttpApi.HttpCancelHttpRequest(Server.RequestQueue.Handle, - Request.RequestId, IntPtr.Zero); + _requestId.Value, IntPtr.Zero); // 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 == UnsafeNclNativeMethods.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 0f827aac53ce..e741d00e9956 100644 --- a/src/Servers/HttpSys/src/RequestProcessing/RequestContextOfT.cs +++ b/src/Servers/HttpSys/src/RequestProcessing/RequestContextOfT.cs @@ -29,7 +29,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 be57b06b5c3d..78c0000e08b0 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.Testing; -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 605298947fe9..70a9b4ad1a27 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.Testing; using Microsoft.Extensions.Logging; -using Microsoft.Extensions.Options; -using Xunit; namespace Microsoft.AspNetCore.Server.HttpSys { @@ -368,6 +361,39 @@ public async Task Request_UrlUnescaping() } } + [Fact] + public async Task Latin1UrlIsRejected() + { + string root; + using (var server = Utilities.CreateHttpServerReturnRoot("/", out root, httpContext => + { + throw new NotImplementedException(); + })) + { + 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); + } + } + } + [ConditionalFact] public async Task Request_WithDoubleSlashes_LeftAlone() { diff --git a/src/Servers/HttpSys/test/FunctionalTests/ResponseSendFileTests.cs b/src/Servers/HttpSys/test/FunctionalTests/ResponseSendFileTests.cs index 621a6c2648e6..413d8ffe89dc 100644 --- a/src/Servers/HttpSys/test/FunctionalTests/ResponseSendFileTests.cs +++ b/src/Servers/HttpSys/test/FunctionalTests/ResponseSendFileTests.cs @@ -29,6 +29,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]