Refactor request line validation and error reporting (#1116).

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;

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
+                    httpVersion = begin.GetAsciiStringEscaped(scan, 9);
-                    // 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);
 
-                    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;

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;

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,

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)

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("GET/HTTP/1.1\r\n", "Invalid request line: GET/HTTP/1.1<0x0D><0x0A>")]
-        [InlineData(" / HTTP/1.1\r\n", "Missing method.")]
+        [InlineData(" / HTTP/1.1\r\n", "Invalid request line:  / HTTP/1.1<0x0D><0x0A>")]
-        [InlineData("GET? / HTTP/1.1\r\n", "Invalid method.")]
+        [InlineData("GET? / HTTP/1.1\r\n", "Invalid request line: GET? / HTTP/1.1<0x0D><0x0A>")]
-        [InlineData("GET /HTTP/1.1\r\n", "No space character found after target in request line.")]
+        [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", "No space character found after target in request line.")]
+        [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", "No space character found after target in request line.")]
+        [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", "No space character found after target in request line.")]
+        [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", "Missing request target.")]
+        [InlineData("GET  HTTP/1.1\r\n", "Invalid request line: GET  HTTP/1.1<0x0D><0x0A>")]
-        [InlineData("GET / HTTP/1.1\n", "Missing CR in request line.")]
+        [InlineData("GET / HTTP/1.1\n", "Invalid request line: GET / HTTP/1.1<0x0A>")]
-        [InlineData("GET / \r\n", "Missing HTTP version.")]
+        [InlineData("GET / \r\n", "Invalid request line: GET / <0x0D><0x0A>")]
-        [InlineData("GET / HTTP/1.1\ra\n", "Missing LF in request line.")]
+        [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<BadHttpRequestException>(() => 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<IConnectionControl>(),
+                };
+                var frame = new Frame<object>(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<BadHttpRequestException>(() => frame.TakeStartLine(socketInput));
+                Assert.Equal("Unrecognized HTTP version: HTTP/1.1a...", exception.Message);
                 Assert.Equal(505, exception.StatusCode);
             }
         }

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)