improve validation of HTTP methods

Author: analogrelay

src/Microsoft.AspNetCore.Server.Kestrel/Http/Frame.cs

@@ -807,10 +807,21 @@ protected RequestLineStatus TakeStartLine(SocketInput input)
                     }
 
                     method = begin.GetAsciiString(scan);
+
                     if (method == null)
                     {
                         RejectRequest("Missing method.");
                     }
+
+                    // Note: We're not in the fast path any more (GetKnownMethod should have handled any HTTP Method we're aware of)
+                    // So we can be a tiny bit slower and more careful here.
+                    for (int i = 0; i < method.Length; i++)
+                    {
+                        if (!IsValidTokenChar(method[i]))
+                        {
+                            RejectRequest("Invalid method.");
+                        }
+                    }
                 }
                 else
                 {
@@ -938,6 +949,31 @@ protected RequestLineStatus TakeStartLine(SocketInput input)
             }
         }
 
+        private static bool IsValidTokenChar(char c)
+        {
+            // Determines if a character is valid as a 'token' as defined in the
+            // HTTP spec: https://tools.ietf.org/html/rfc7230#section-3.2.6
+            return
+                (c >= '0' && c <= '9') ||
+                (c >= 'A' && c <= 'Z') ||
+                (c >= 'a' && c <= 'z') ||
+                c == '!' ||
+                c == '#' ||
+                c == '$' ||
+                c == '%' ||
+                c == '&' ||
+                c == '\'' ||
+                c == '*' ||
+                c == '+' ||
+                c == '-' ||
+                c == '.' ||
+                c == '^' ||
+                c == '_' ||
+                c == '`' ||
+                c == '|' ||
+                c == '~';
+        }
+
         private bool RequestUrlStartsWithPathBase(string requestUrl, out bool caseMatches)
         {
             caseMatches = true;

test/Microsoft.AspNetCore.Server.KestrelTests/BadHttpRequestTests.cs

@@ -45,7 +45,7 @@ public class BadHttpRequestTests
         // Missing method
         [InlineData(" ")]
         // Missing second space
-        [InlineData("/ HTTP/1.1\r\n\r\n")]
+        [InlineData("/ ")] // This fails trying to read the '/' because that's invalid for an HTTP method
         [InlineData("GET /\r\n")]
         // Missing target
         [InlineData("GET  ")]
@@ -67,13 +67,33 @@ public class BadHttpRequestTests
         [InlineData("GET / 8charact\r")]
         // Missing LF
         [InlineData("GET / HTTP/1.0\rA")]
+        // Bad HTTP Methods (invalid according to RFC)
+        [InlineData("( / HTTP/1.0\r\n")]
+        [InlineData(") / HTTP/1.0\r\n")]
+        [InlineData("< / HTTP/1.0\r\n")]
+        [InlineData("> / HTTP/1.0\r\n")]
+        [InlineData("@ / HTTP/1.0\r\n")]
+        [InlineData(", / HTTP/1.0\r\n")]
+        [InlineData("; / HTTP/1.0\r\n")]
+        [InlineData(": / HTTP/1.0\r\n")]
+        [InlineData("\\ / HTTP/1.0\r\n")]
+        [InlineData("\" / HTTP/1.0\r\n")]
+        [InlineData("/ / HTTP/1.0\r\n")]
+        [InlineData("[ / HTTP/1.0\r\n")]
+        [InlineData("] / HTTP/1.0\r\n")]
+        [InlineData("? / HTTP/1.0\r\n")]
+        [InlineData("= / HTTP/1.0\r\n")]
+        [InlineData("{ / HTTP/1.0\r\n")]
+        [InlineData("} / HTTP/1.0\r\n")]
+        [InlineData("get@ / HTTP/1.0\r\n")]
+        [InlineData("post= / HTTP/1.0\r\n")]
         public async Task TestBadRequestLines(string request)
         {
             using (var server = new TestServer(context => TaskUtilities.CompletedTask))
             {
                 using (var connection = server.CreateConnection())
                 {
-                    await connection.SendEnd(request);
+                    await connection.SendAllEnd(request);
                     await ReceiveBadRequestResponse(connection);
                 }
             }
@@ -84,6 +104,26 @@ public async Task TestBadRequestLines(string request)
         [InlineData("GET  ")]
         [InlineData("GET / HTTP/1.2\r")]
         [InlineData("GET / HTTP/1.0\rA")]
+        // Bad HTTP Methods (invalid according to RFC)
+        [InlineData("( ")]
+        [InlineData(") ")]
+        [InlineData("< ")]
+        [InlineData("> ")]
+        [InlineData("@ ")]
+        [InlineData(", ")]
+        [InlineData("; ")]
+        [InlineData(": ")]
+        [InlineData("\\ ")]
+        [InlineData("\" ")]
+        [InlineData("/ ")]
+        [InlineData("[ ")]
+        [InlineData("] ")]
+        [InlineData("? ")]
+        [InlineData("= ")]
+        [InlineData("{ ")]
+        [InlineData("} ")]
+        [InlineData("get@ ")]
+        [InlineData("post= ")]
         public async Task ServerClosesConnectionAsSoonAsBadRequestLineIsDetected(string request)
         {
             using (var server = new TestServer(context => TaskUtilities.CompletedTask))

test/Microsoft.AspNetCore.Server.KestrelTests/TestConnection.cs

@@ -43,6 +43,21 @@ public void Dispose()
             _socket.Dispose();
         }
 
+        public async Task SendAll(params string[] lines)
+        {
+            var text = String.Join("\r\n", lines);
+            var writer = new StreamWriter(_stream, Encoding.ASCII);
+            await writer.WriteAsync(text);
+            writer.Flush();
+            _stream.Flush();
+        }
+
+        public async Task SendAllEnd(params string[] lines)
+        {
+            await SendAll(lines);
+            _socket.Shutdown(SocketShutdown.Send);
+        }
+
         public async Task Send(params string[] lines)
         {
             var text = String.Join("\r\n", lines);