search

dotnet / aspnetcore

ASP.NET Core is a cross-platform .NET framework for building modern cloud-based web applications on Windows, Mac, or Linux.
https://asp.net
MIT License
35.46k stars 10.03k forks source link

Kestrel allows invalid characters in request header names #28571

Open Szer opened 3 years ago

Szer commented 3 years ago

Describe the bug

According to HTTP spec header grammar consists of:

header-field   = field-name ":" OWS field-value OWS
field-name     = token
token          = 1*tchar
tchar          = "!" / "#" / "$" / "%" / "&" / "'" / "*"
                    / "+" / "-" / "." / "^" / "_" / "`" / "|" / "~"
                    / DIGIT / ALPHA
ALPHA (letters)  ; visible [USASCII] (printing) characters
DIGIT (decimal 0-9)

If I will send raw TCP request with invalid char in header name Kestrel will respond with 200 Kestrel shouldn't even start invoking any middleware if request header is invalid, but as you see from repro below it does.

To Reproduce

https://gist.github.com/Szer/d834674e0621d2100a61cd76dcadbcf4

Further technical details

Runtime Environment: OS Name: Windows OS Version: 10.0.19042 OS Platform: Windows RID: win10-x64 Base Path: C:\Program Files\dotnet\sdk\5.0.101\

Host (useful for support): Version: 5.0.1 Commit: b02e13abab

.NET SDKs installed: 3.1.403 [C:\Program Files\dotnet\sdk] 3.1.404 [C:\Program Files\dotnet\sdk] 5.0.101 [C:\Program Files\dotnet\sdk]

.NET runtimes installed: Microsoft.AspNetCore.All 2.1.23 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.All] Microsoft.AspNetCore.App 2.1.23 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App] Microsoft.AspNetCore.App 3.1.10 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App] Microsoft.AspNetCore.App 5.0.1 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App] Microsoft.NETCore.App 2.1.23 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App] Microsoft.NETCore.App 3.1.10 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App] Microsoft.NETCore.App 5.0.1 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App] Microsoft.WindowsDesktop.App 3.1.10 [C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App] Microsoft.WindowsDesktop.App 5.0.1 [C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App]

shirhatti commented 3 years ago

Can you share Kestrel connection logs? https://docs.microsoft.com/aspnet/core/fundamentals/servers/kestrel?view=aspnetcore-5.0#connection-logging

Szer commented 3 years ago

@shirhatti sure

For request with 0x01 in ASCII (unprintable symbol)

(SOH here is unprintable symbol)

GET / HTTP/1.1
Host: localhost 
User-Agent: test r
Accept: */*
T(SOH)est: 123

dbug: Microsoft.AspNetCore.Hosting.Diagnostics[3]
      Hosting starting
dbug: Microsoft.AspNetCore.Hosting.Diagnostics[4]
      Hosting started
dbug: Microsoft.AspNetCore.Hosting.Diagnostics[0]
      Loaded hosting startup assembly ConsoleApp1
dbug: Microsoft.AspNetCore.Server.Kestrel[39]
      Connection id "0HM507K3JHR0O" accepted.
dbug: Microsoft.AspNetCore.Server.Kestrel[1]
      Connection id "0HM507K3JHR0O" started.
dbug: Microsoft.AspNetCore.Server.Kestrel.Core.Internal.LoggingConnectionMiddleware[0]
      ReadAsync[81]
      47 45 54 20 2F 20 48 54  54 50 2F 31 2E 31 0D 0A   GET / HT TP/1.1..
      48 6F 73 74 3A 20 6C 6F  63 61 6C 68 6F 73 74 20   Host: lo calhost
      0D 0A 55 73 65 72 2D 41  67 65 6E 74 3A 20 74 65   ..User-A gent: te
      73 74 20 72 0D 0A 41 63  63 65 70 74 3A 20 2A 2F   st r..Ac cept: */
      2A 0D 0A 54 01 65 73 74  3A 20 31 32 33 0D 0A 0D   *..T.est : 123...
      0A                                                 .
info: Microsoft.AspNetCore.Hosting.Diagnostics[1]
      Request starting HTTP/1.1 GET http://localhost/ - -
dbug: Microsoft.AspNetCore.Server.Kestrel.Core.Internal.LoggingConnectionMiddleware[0]
      WriteAsync[160]
      48 54 54 50 2F 31 2E 31  20 32 30 30 20 4F 4B 0D   HTTP/1.1  200 OK.
      0A 44 61 74 65 3A 20 4D  6F 6E 2C 20 31 34 20 44   .Date: M on, 14 D
      65 63 20 32 30 32 30 20  31 34 3A 33 32 3A 30 34   ec 2020  14:32:04
      20 47 4D 54 0D 0A 43 6F  6E 74 65 6E 74 2D 54 79    GMT..Co ntent-Ty
      70 65 3A 20 74 65 78 74  2F 70 6C 61 69 6E 0D 0A   pe: text /plain..
      53 65 72 76 65 72 3A 20  4B 65 73 74 72 65 6C 0D   Server:  Kestrel.
      0A 43 6F 6E 74 65 6E 74  2D 4C 65 6E 67 74 68 3A   .Content -Length:
      20 34 31 0D 0A 0D 0A 5B  22 41 63 63 65 70 74 22    41....[ "Accept"
      3B 20 22 48 6F 73 74 22  3B 20 22 55 73 65 72 2D   ; "Host" ; "User-
      41 67 65 6E 74 22 3B 20  22 54 01 65 73 74 22 5D   Agent";  "T.est"]

dbug: Microsoft.AspNetCore.Server.Kestrel[9]
      Connection id "0HM507K3JHR0O" completed keep alive response.
info: Microsoft.AspNetCore.Hosting.Diagnostics[2]
      Request finished HTTP/1.1 GET http://localhost/ - - - 200 41 text/plain 47.5235ms

Process finished with exit code 0.
Tratcher commented 3 years ago

To summarize the data from your gist, the following octets in a header name cause it to be rejected: 0x00 (null), 0x09 (tab), 0x0A (line feed), 0x0D (carriage return), 0x20 (space).

This behavior is currently by design (though we can debate the merits of that design). The parser focuses on the values that are required to parse the structure of the message correctly and is fairly permissive about anything else.

Here's the check for space and tab: https://github.com/dotnet/aspnetcore/blob/4a7ed303fff26e4070e02e385fb22df484f6b81f/src/Servers/Kestrel/Core/src/Internal/Http/HttpParser.cs#L339-L363

The new line characters mess up the line parsing and everything gets rejected as malformed. https://github.com/dotnet/aspnetcore/blob/4a7ed303fff26e4070e02e385fb22df484f6b81f/src/Servers/Kestrel/Core/src/Internal/Http/HttpParser.cs#L216-L220

I don't see the check for the null character.

ghost commented 3 years ago

We've moved this issue to the Backlog milestone. This means that it is not going to be worked on for the coming release. We will reassess the backlog following the current release and consider this item at that time. To learn more about our issue management process and to have better expectation regarding different types of issues you can read our Triage Process.

halter73 commented 3 years ago

Thanks for the awesome gist to repro the issue and the connection log! That really helped make it easier to understand what's going on.

We do reject non-ASCII characters in header names. We have a test that verifies we at least reject '\x80' here in our BadHttpRequestTests. kestrel_check.fs incorrectly makes it appear that Kestrel is accepting header names containing non-ASCII bytes (0x80-0xFF) because ASCIIEncoding.GetBytes encodes any Unicode character greater than U+007F as the ASCII question mark ('?'). This can be seen in the connection logs:

dbug: Microsoft.AspNetCore.Server.Kestrel.Core.Internal.LoggingConnectionMiddleware[0]
      ReadAsync[78]
      47 45 54 20 2F 20 48 54  54 50 2F 31 2E 31 0D 0A   GET / HT TP/1.1..
      48 6F 73 74 3A 20 6C 6F  63 61 6C 68 6F 73 74 20   Host: lo calhost
      0D 0A 55 73 65 72 2D 41  67 65 6E 74 3A 20 74 65   ..User-A gent: te
      73 74 20 0D 0A 41 63 63  65 70 74 3A 20 2A 2F 2A   st ..Acc ept: */*
      0D 0A 54 3F 65 73 74 3A  20 31 32 33 0D 0A      ..T?est:  123..

Changing Encoding.ASCII.GetBytes to Encoding.GetEncoding("ISO-8859-1").GetBytes in the msg function should fix this.

The actual code that does the validation is here in StringUtilities which is called by HttpUtilites.GetHeaderName(). As you can see, this code is heavily optimized. It might be difficult to maintain perf while rejecting every character not allowed by the HTTP spec.

halter73 commented 3 years ago

I don't see the check for the null character.

That's done as part of StringUtilities.TryGetAsciiString(). The various CheckBytesInAsciiRange overloads reject null characters.