diff --git a/src/Microsoft.AspNetCore.Server.Kestrel/BadHttpRequestException.cs b/src/Microsoft.AspNetCore.Server.Kestrel/BadHttpRequestException.cs index f5981b5607..775fb2344d 100644 --- a/src/Microsoft.AspNetCore.Server.Kestrel/BadHttpRequestException.cs +++ b/src/Microsoft.AspNetCore.Server.Kestrel/BadHttpRequestException.cs @@ -21,24 +21,6 @@ namespace Microsoft.AspNetCore.Server.Kestrel BadHttpRequestException ex; switch (reason) { - case RequestRejectionReason.MissingMethod: - ex = new BadHttpRequestException("Missing method.", 400); - break; - case RequestRejectionReason.InvalidMethod: - ex = new BadHttpRequestException("Invalid method.", 400); - break; - case RequestRejectionReason.MissingRequestTarget: - ex = new BadHttpRequestException("Missing request target.", 400); - break; - case RequestRejectionReason.MissingHTTPVersion: - ex = new BadHttpRequestException("Missing HTTP version.", 400); - break; - case RequestRejectionReason.UnrecognizedHTTPVersion: - ex = new BadHttpRequestException("Unrecognized HTTP version.", 505); - break; - case RequestRejectionReason.MissingLFInRequestLine: - ex = new BadHttpRequestException("Missing LF in request line.", 400); - break; case RequestRejectionReason.HeadersCorruptedInvalidHeaderSequence: ex = new BadHttpRequestException("Headers corrupted, invalid header sequence.", 400); break; @@ -84,15 +66,6 @@ namespace Microsoft.AspNetCore.Server.Kestrel case RequestRejectionReason.RequestLineTooLong: ex = new BadHttpRequestException("Request line too long.", 414); break; - case RequestRejectionReason.MissingSpaceAfterMethod: - ex = new BadHttpRequestException("No space character found after method in request line.", 400); - break; - case RequestRejectionReason.MissingSpaceAfterTarget: - ex = new BadHttpRequestException("No space character found after target in request line.", 400); - break; - case RequestRejectionReason.MissingCrAfterVersion: - ex = new BadHttpRequestException("Missing CR in request line.", 400); - break; case RequestRejectionReason.HeadersExceedMaxTotalSize: ex = new BadHttpRequestException("Request headers too long.", 431); break; @@ -117,12 +90,15 @@ namespace Microsoft.AspNetCore.Server.Kestrel BadHttpRequestException ex; switch (reason) { - case RequestRejectionReason.MalformedRequestLineStatus: + case RequestRejectionReason.InvalidRequestLine: ex = new BadHttpRequestException($"Invalid request line: {value}", 400); break; case RequestRejectionReason.InvalidContentLength: ex = new BadHttpRequestException($"Invalid content length: {value}", 400); break; + case RequestRejectionReason.UnrecognizedHTTPVersion: + ex = new BadHttpRequestException($"Unrecognized HTTP version: {value}", 505); + break; default: ex = new BadHttpRequestException("Bad request.", 400); break; diff --git a/src/Microsoft.AspNetCore.Server.Kestrel/Internal/Http/Frame.cs b/src/Microsoft.AspNetCore.Server.Kestrel/Internal/Http/Frame.cs index f3c8dc8dbb..9a13ab2cb7 100644 --- a/src/Microsoft.AspNetCore.Server.Kestrel/Internal/Http/Frame.cs +++ b/src/Microsoft.AspNetCore.Server.Kestrel/Internal/Http/Frame.cs @@ -857,7 +857,10 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Internal.Http public RequestLineStatus TakeStartLine(SocketInput input) { + const int MaxInvalidRequestLineChars = 32; + var scan = input.ConsumingStart(); + var start = scan; var consumed = scan; var end = scan; @@ -890,6 +893,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Internal.Http return RequestLineStatus.Incomplete; } } + end.Take(); string method; var begin = scan; @@ -897,14 +901,16 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Internal.Http { if (scan.Seek(ref _vectorSpaces, ref end) == -1) { - RejectRequest(RequestRejectionReason.MissingSpaceAfterMethod); + RejectRequest(RequestRejectionReason.InvalidRequestLine, + Log.IsEnabled(LogLevel.Information) ? start.GetAsciiStringEscaped(end, MaxInvalidRequestLineChars) : string.Empty); } method = begin.GetAsciiString(scan); if (method == null) { - RejectRequest(RequestRejectionReason.MissingMethod); + RejectRequest(RequestRejectionReason.InvalidRequestLine, + Log.IsEnabled(LogLevel.Information) ? start.GetAsciiStringEscaped(end, MaxInvalidRequestLineChars) : string.Empty); } // Note: We're not in the fast path any more (GetKnownMethod should have handled any HTTP Method we're aware of) @@ -913,7 +919,8 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Internal.Http { if (!IsValidTokenChar(method[i])) { - RejectRequest(RequestRejectionReason.InvalidMethod); + RejectRequest(RequestRejectionReason.InvalidRequestLine, + Log.IsEnabled(LogLevel.Information) ? start.GetAsciiStringEscaped(end, MaxInvalidRequestLineChars) : string.Empty); } } } @@ -928,7 +935,8 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Internal.Http var chFound = scan.Seek(ref _vectorSpaces, ref _vectorQuestionMarks, ref _vectorPercentages, ref end); if (chFound == -1) { - RejectRequest(RequestRejectionReason.MissingSpaceAfterTarget); + RejectRequest(RequestRejectionReason.InvalidRequestLine, + Log.IsEnabled(LogLevel.Information) ? start.GetAsciiStringEscaped(end, MaxInvalidRequestLineChars) : string.Empty); } else if (chFound == '%') { @@ -936,7 +944,8 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Internal.Http chFound = scan.Seek(ref _vectorSpaces, ref _vectorQuestionMarks, ref end); if (chFound == -1) { - RejectRequest(RequestRejectionReason.MissingSpaceAfterTarget); + RejectRequest(RequestRejectionReason.InvalidRequestLine, + Log.IsEnabled(LogLevel.Information) ? start.GetAsciiStringEscaped(end, MaxInvalidRequestLineChars) : string.Empty); } } @@ -949,7 +958,8 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Internal.Http begin = scan; if (scan.Seek(ref _vectorSpaces, ref end) == -1) { - RejectRequest(RequestRejectionReason.MissingSpaceAfterTarget); + RejectRequest(RequestRejectionReason.InvalidRequestLine, + Log.IsEnabled(LogLevel.Information) ? start.GetAsciiStringEscaped(end, MaxInvalidRequestLineChars) : string.Empty); } queryString = begin.GetAsciiString(scan); } @@ -958,43 +968,40 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Internal.Http if (pathBegin.Peek() == ' ') { - RejectRequest(RequestRejectionReason.MissingRequestTarget); + RejectRequest(RequestRejectionReason.InvalidRequestLine, + Log.IsEnabled(LogLevel.Information) ? start.GetAsciiStringEscaped(end, MaxInvalidRequestLineChars) : string.Empty); } scan.Take(); begin = scan; if (scan.Seek(ref _vectorCRs, ref end) == -1) { - RejectRequest(RequestRejectionReason.MissingCrAfterVersion); + RejectRequest(RequestRejectionReason.InvalidRequestLine, + Log.IsEnabled(LogLevel.Information) ? start.GetAsciiStringEscaped(end, MaxInvalidRequestLineChars) : string.Empty); } string httpVersion; if (!begin.GetKnownVersion(out httpVersion)) { - // A slower fallback is necessary since the iterator's PeekLong() method - // used in GetKnownVersion() only examines two memory blocks at most. - // Although unlikely, it is possible that the 8 bytes forming the version - // could be spread out on more than two blocks, if the connection - // happens to be unusually slow. - httpVersion = begin.GetAsciiString(scan); + httpVersion = begin.GetAsciiStringEscaped(scan, 9); - if (httpVersion == null) + if (httpVersion == string.Empty) { - RejectRequest(RequestRejectionReason.MissingHTTPVersion); + RejectRequest(RequestRejectionReason.InvalidRequestLine, + Log.IsEnabled(LogLevel.Information) ? start.GetAsciiStringEscaped(end, MaxInvalidRequestLineChars) : string.Empty); } - else if (httpVersion != "HTTP/1.0" && httpVersion != "HTTP/1.1") + else { - RejectRequest(RequestRejectionReason.UnrecognizedHTTPVersion); + RejectRequest(RequestRejectionReason.UnrecognizedHTTPVersion, httpVersion); } } scan.Take(); // consume CR - if (scan.Block != end.Block || scan.Index != end.Index) + if (scan.Take() != '\n') { - RejectRequest(RequestRejectionReason.MissingLFInRequestLine); + RejectRequest(RequestRejectionReason.InvalidRequestLine, + Log.IsEnabled(LogLevel.Information) ? start.GetAsciiStringEscaped(end, MaxInvalidRequestLineChars) : string.Empty); } - scan.Take(); // consume LF - end = scan; // URIs are always encoded/escaped to ASCII https://tools.ietf.org/html/rfc3986#page-11 // Multibyte Internationalized Resource Identifiers (IRIs) are first converted to utf8; diff --git a/src/Microsoft.AspNetCore.Server.Kestrel/Internal/Http/FrameOfT.cs b/src/Microsoft.AspNetCore.Server.Kestrel/Internal/Http/FrameOfT.cs index 203e97152c..eeba9695df 100644 --- a/src/Microsoft.AspNetCore.Server.Kestrel/Internal/Http/FrameOfT.cs +++ b/src/Microsoft.AspNetCore.Server.Kestrel/Internal/Http/FrameOfT.cs @@ -51,7 +51,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Internal.Http if (requestLineStatus != RequestLineStatus.Done) { - RejectRequest(RequestRejectionReason.MalformedRequestLineStatus, requestLineStatus.ToString()); + RejectRequest(RequestRejectionReason.InvalidRequestLine, requestLineStatus.ToString()); } break; diff --git a/src/Microsoft.AspNetCore.Server.Kestrel/Internal/Http/RequestRejectionReason.cs b/src/Microsoft.AspNetCore.Server.Kestrel/Internal/Http/RequestRejectionReason.cs index 3bd3aecaf5..07ed7444e4 100644 --- a/src/Microsoft.AspNetCore.Server.Kestrel/Internal/Http/RequestRejectionReason.cs +++ b/src/Microsoft.AspNetCore.Server.Kestrel/Internal/Http/RequestRejectionReason.cs @@ -5,19 +5,14 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Internal.Http { public enum RequestRejectionReason { - MissingMethod, - InvalidMethod, - MissingRequestTarget, - MissingHTTPVersion, UnrecognizedHTTPVersion, - MissingLFInRequestLine, HeadersCorruptedInvalidHeaderSequence, HeaderLineMustNotStartWithWhitespace, NoColonCharacterFoundInHeaderLine, WhitespaceIsNotAllowedInHeaderName, HeaderValueMustNotContainCR, HeaderValueLineFoldingNotSupported, - MalformedRequestLineStatus, + InvalidRequestLine, MalformedRequestInvalidHeaders, InvalidContentLength, UnexpectedEndOfRequestContent, @@ -28,9 +23,6 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Internal.Http InvalidCharactersInHeaderName, NonAsciiOrNullCharactersInInputString, RequestLineTooLong, - MissingSpaceAfterMethod, - MissingSpaceAfterTarget, - MissingCrAfterVersion, HeadersExceedMaxTotalSize, MissingCRInHeaderLine, TooManyHeaders, diff --git a/src/Microsoft.AspNetCore.Server.Kestrel/Internal/Infrastructure/MemoryPoolIteratorExtensions.cs b/src/Microsoft.AspNetCore.Server.Kestrel/Internal/Infrastructure/MemoryPoolIteratorExtensions.cs index 59978837fd..146d9f127a 100644 --- a/src/Microsoft.AspNetCore.Server.Kestrel/Internal/Infrastructure/MemoryPoolIteratorExtensions.cs +++ b/src/Microsoft.AspNetCore.Server.Kestrel/Internal/Infrastructure/MemoryPoolIteratorExtensions.cs @@ -127,6 +127,26 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Internal.Infrastructure return asciiString; } + public static string GetAsciiStringEscaped(this MemoryPoolIterator start, MemoryPoolIterator end, int maxChars) + { + var sb = new StringBuilder(); + var scan = start; + + while (maxChars > 0 && (scan.Block != end.Block || scan.Index != end.Index)) + { + var ch = scan.Take(); + sb.Append(ch < 0x20 || ch >= 0x7F ? $"<0x{ch.ToString("X2")}>" : ((char)ch).ToString()); + maxChars--; + } + + if (scan.Block != end.Block || scan.Index != end.Index) + { + sb.Append("..."); + } + + return sb.ToString(); + } + public static string GetUtf8String(this MemoryPoolIterator start, MemoryPoolIterator end) { if (start.IsDefault || end.IsDefault) diff --git a/test/Microsoft.AspNetCore.Server.KestrelTests/FrameTests.cs b/test/Microsoft.AspNetCore.Server.KestrelTests/FrameTests.cs index a131af3e48..b411dc6b8f 100644 --- a/test/Microsoft.AspNetCore.Server.KestrelTests/FrameTests.cs +++ b/test/Microsoft.AspNetCore.Server.KestrelTests/FrameTests.cs @@ -1057,17 +1057,17 @@ namespace Microsoft.AspNetCore.Server.KestrelTests } [Theory] - [InlineData("GET/HTTP/1.1\r\n", "No space character found after method in request line.")] - [InlineData(" / HTTP/1.1\r\n", "Missing method.")] - [InlineData("GET? / HTTP/1.1\r\n", "Invalid method.")] - [InlineData("GET /HTTP/1.1\r\n", "No space character found after target in request line.")] - [InlineData("GET /a?b=cHTTP/1.1\r\n", "No space character found after target in request line.")] - [InlineData("GET /a%20bHTTP/1.1\r\n", "No space character found after target in request line.")] - [InlineData("GET /a%20b?c=dHTTP/1.1\r\n", "No space character found after target in request line.")] - [InlineData("GET HTTP/1.1\r\n", "Missing request target.")] - [InlineData("GET / HTTP/1.1\n", "Missing CR in request line.")] - [InlineData("GET / \r\n", "Missing HTTP version.")] - [InlineData("GET / HTTP/1.1\ra\n", "Missing LF in request line.")] + [InlineData("GET/HTTP/1.1\r\n", "Invalid request line: GET/HTTP/1.1<0x0D><0x0A>")] + [InlineData(" / HTTP/1.1\r\n", "Invalid request line: / HTTP/1.1<0x0D><0x0A>")] + [InlineData("GET? / HTTP/1.1\r\n", "Invalid request line: GET? / HTTP/1.1<0x0D><0x0A>")] + [InlineData("GET /HTTP/1.1\r\n", "Invalid request line: GET /HTTP/1.1<0x0D><0x0A>")] + [InlineData("GET /a?b=cHTTP/1.1\r\n", "Invalid request line: GET /a?b=cHTTP/1.1<0x0D><0x0A>")] + [InlineData("GET /a%20bHTTP/1.1\r\n", "Invalid request line: GET /a%20bHTTP/1.1<0x0D><0x0A>")] + [InlineData("GET /a%20b?c=dHTTP/1.1\r\n", "Invalid request line: GET /a%20b?c=dHTTP/1.1<0x0D><0x0A>")] + [InlineData("GET HTTP/1.1\r\n", "Invalid request line: GET HTTP/1.1<0x0D><0x0A>")] + [InlineData("GET / HTTP/1.1\n", "Invalid request line: GET / HTTP/1.1<0x0A>")] + [InlineData("GET / \r\n", "Invalid request line: GET / <0x0D><0x0A>")] + [InlineData("GET / HTTP/1.1\ra\n", "Invalid request line: GET / HTTP/1.1<0x0D>a<0x0A>")] public void TakeStartLineThrowsWhenInvalid(string requestLine, string expectedExceptionMessage) { var trace = new KestrelTrace(new TestKestrelTrace()); @@ -1130,7 +1130,41 @@ namespace Microsoft.AspNetCore.Server.KestrelTests socketInput.IncomingData(requestLineBytes, 0, requestLineBytes.Length); var exception = Assert.Throws(() => frame.TakeStartLine(socketInput)); - Assert.Equal("Unrecognized HTTP version.", exception.Message); + Assert.Equal("Unrecognized HTTP version: HTTP/1.2", exception.Message); + Assert.Equal(505, exception.StatusCode); + } + } + + [Fact] + public void TakeStartLineThrowsOnUnsupportedHttpVersionLongerThanEigthCharacters() + { + var trace = new KestrelTrace(new TestKestrelTrace()); + var ltp = new LoggingThreadPool(trace); + using (var pool = new MemoryPool()) + using (var socketInput = new SocketInput(pool, ltp)) + { + var serviceContext = new ServiceContext + { + DateHeaderValueManager = new DateHeaderValueManager(), + ServerOptions = new KestrelServerOptions(), + Log = trace + }; + var listenerContext = new ListenerContext(serviceContext) + { + ServerAddress = ServerAddress.FromUrl("http://localhost:5000") + }; + var connectionContext = new ConnectionContext(listenerContext) + { + ConnectionControl = Mock.Of(), + }; + var frame = new Frame(application: null, context: connectionContext); + frame.Reset(); + + var requestLineBytes = Encoding.ASCII.GetBytes("GET / HTTP/1.1ab\r\n"); + socketInput.IncomingData(requestLineBytes, 0, requestLineBytes.Length); + + var exception = Assert.Throws(() => frame.TakeStartLine(socketInput)); + Assert.Equal("Unrecognized HTTP version: HTTP/1.1a...", exception.Message); Assert.Equal(505, exception.StatusCode); } } diff --git a/test/Microsoft.AspNetCore.Server.KestrelTests/MemoryPoolIteratorTests.cs b/test/Microsoft.AspNetCore.Server.KestrelTests/MemoryPoolIteratorTests.cs index dd4edd4529..ae539b10f5 100644 --- a/test/Microsoft.AspNetCore.Server.KestrelTests/MemoryPoolIteratorTests.cs +++ b/test/Microsoft.AspNetCore.Server.KestrelTests/MemoryPoolIteratorTests.cs @@ -502,6 +502,52 @@ namespace Microsoft.AspNetCore.Server.KestrelTests TestKnownStringsInterning(input, expected, MemoryPoolIteratorExtensions.GetKnownVersion); } + [Theory] + [InlineData("", "HTTP/1.1\r")] + [InlineData("H", "TTP/1.1\r")] + [InlineData("HT", "TP/1.1\r")] + [InlineData("HTT", "P/1.1\r")] + [InlineData("HTTP", "/1.1\r")] + [InlineData("HTTP/", "1.1\r")] + [InlineData("HTTP/1", ".1\r")] + [InlineData("HTTP/1.", "1\r")] + [InlineData("HTTP/1.1", "\r")] + [InlineData("HTTP/1.1\r", "")] + public void KnownVersionCanBeReadAtAnyBlockBoundary(string block1Input, string block2Input) + { + MemoryPoolBlock block1 = null; + MemoryPoolBlock block2 = null; + + try + { + // Arrange + var chars1 = block1Input.ToCharArray().Select(c => (byte)c).ToArray(); + var chars2 = block2Input.ToCharArray().Select(c => (byte)c).ToArray(); + block1 = _pool.Lease(); + block2 = _pool.Lease(); + Buffer.BlockCopy(chars1, 0, block1.Array, block1.Start, chars1.Length); + Buffer.BlockCopy(chars2, 0, block2.Array, block2.Start, chars2.Length); + block1.End += chars1.Length; + block2.End += chars2.Length; + block1.Next = block2; + var iterator = block1.GetIterator(); + + // Act + string knownVersion; + var result = iterator.GetKnownVersion(out knownVersion); + + // Assert + Assert.True(result); + Assert.Equal("HTTP/1.1", knownVersion); + } + finally + { + // Cleanup + if (block1 != null) _pool.Return(block1); + if (block2 != null) _pool.Return(block2); + } + } + [Theory] [InlineData("CONNECT / HTTP/1.1", "CONNECT")] [InlineData("DELETE / HTTP/1.1", "DELETE")] @@ -766,6 +812,39 @@ namespace Microsoft.AspNetCore.Server.KestrelTests } } + [Theory] + [InlineData("a", "a", 1)] + [InlineData("ab", "a...", 1)] + [InlineData("abcde", "abcde", 5)] + [InlineData("abcde", "abcd...", 4)] + [InlineData("abcde", "abcde", 6)] + public void TestGetAsciiStringEscaped(string input, string expected, int maxChars) + { + MemoryPoolBlock block = null; + + try + { + // Arrange + block = _pool.Lease(); + var chars = input.ToCharArray().Select(c => (byte)c).ToArray(); + Buffer.BlockCopy(chars, 0, block.Array, block.Start, chars.Length); + block.End += chars.Length; + var start = block.GetIterator(); + var end = start; + end.Skip(input.Length); + + // Act + var result = start.GetAsciiStringEscaped(end, maxChars); + + // Assert + Assert.Equal(expected, result); + } + finally + { + if (block != null) _pool.Return(block); + } + } + private delegate bool GetKnownString(MemoryPoolIterator iter, out string result); private void TestKnownStringsInterning(string input, string expected, GetKnownString action)