Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix Kestrel host header mismatch handling when port in Url #59352

Merged
merged 2 commits into from
Dec 9, 2024
Merged

Fix Kestrel host header mismatch handling when port in Url #59352

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5ec6c22
Fix Kestrel host header mismatch handling when port in Url
BrennanConroy Dec 6, 2024
e36d998
avoid some allocs
BrennanConroy Dec 6, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 7 additions & 3 deletions src/Servers/Kestrel/Core/src/Internal/Http/Http1Connection.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@

using System.Buffers;
using System.Diagnostics;
using System.Globalization;
using System.IO.Pipelines;
using Microsoft.AspNetCore.Connections;
using Microsoft.AspNetCore.Connections.Features;
Expand Down Expand Up @@ -649,11 +648,16 @@ private void ValidateNonOriginHostHeader(string hostText)
if (hostText != _absoluteRequestTarget!.Authority)
{
if (!_absoluteRequestTarget.IsDefaultPort
|| hostText != _absoluteRequestTarget.Authority + ":" + _absoluteRequestTarget.Port.ToString(CultureInfo.InvariantCulture))
|| hostText != $"{_absoluteRequestTarget.Authority}:{_absoluteRequestTarget.Port}")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unrelated to the bug, but this temporary string allocation seems like it could be optimized away.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dotnet/runtime#110427

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could do it ourselves. e.g. check that content before : matches _absoluteRequestTarget.Authority and content after : parses to an int and matches _absoluteRequestTarget.Port.

But if this path isn't that hot then probably not worth the complexity.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it run per request ?

Copy link
Member Author

@BrennanConroy BrennanConroy Dec 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the host doesn't match the authority.

{
if (_context.ServiceContext.ServerOptions.AllowHostHeaderOverride)
{
hostText = _absoluteRequestTarget.Authority + ":" + _absoluteRequestTarget.Port.ToString(CultureInfo.InvariantCulture);
// No need to include the port here, it's either already in the Authority
// or it's the default port
// see: https://datatracker.ietf.org/doc/html/rfc2616/#section-14.23
// A "host" without any trailing port information implies the default
// port for the service requested (e.g., "80" for an HTTP URL).
hostText = _absoluteRequestTarget.Authority;
HttpRequestHeaders.HeaderHost = hostText;
}
else
Expand Down
7 changes: 6 additions & 1 deletion src/Servers/Kestrel/shared/test/HttpParsingData.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -497,8 +497,10 @@ public static TheoryData<string, string> HostHeaderData
{ "GET /pub/WWW/", "www.example.org" },
{ "GET http://localhost/", "localhost" },
{ "GET http://localhost:80/", "localhost:80" },
{ "GET http://localhost:80/", "localhost" },
{ "GET https://localhost/", "localhost" },
{ "GET https://localhost:443/", "localhost:443" },
{ "GET https://localhost:443/", "localhost" },
{ "CONNECT asp.net:80", "asp.net:80" },
{ "CONNECT asp.net:443", "asp.net:443" },
{ "CONNECT user-images.githubusercontent.com:443", "user-images.githubusercontent.com:443" },
Expand Down Expand Up @@ -534,10 +536,13 @@ public static TheoryData<string, string> HostHeaderInvalidData
data.Add("CONNECT contoso.com", host);
}

// port mismatch when target contains port
// port mismatch when target contains default https port
data.Add("GET https://contoso.com:443/", "contoso.com:5000");
data.Add("CONNECT contoso.com:443", "contoso.com:5000");

// port mismatch when target contains default http port
data.Add("GET http://contoso.com:80/", "contoso.com:5000");

return data;
}
}
Expand Down
13 changes: 8 additions & 5 deletions src/Servers/Kestrel/test/InMemory.FunctionalTests/BadHttpRequestTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -153,9 +153,12 @@ public Task BadRequestIfHostHeaderDoesNotMatchRequestTarget(string requestTarget
}

[Theory]
[InlineData("Host: www.foo.comConnection: keep-alive")] // Corrupted - missing line-break
[InlineData("Host: www.notfoo.com")] // Syntactically correct but not matching
public async Task CanOptOutOfBadRequestIfHostHeaderDoesNotMatchRequestTarget(string hostHeader)
[InlineData("http://www.foo.com", "Host: www.foo.comConnection: keep-alive", "www.foo.com")] // Corrupted - missing line-break
[InlineData("http://www.foo.com/", "Host: www.notfoo.com", "www.foo.com")] // Syntactically correct but not matching
[InlineData("http://www.foo.com:80", "Host: www.notfoo.com", "www.foo.com")] // Explicit default port in request string
[InlineData("http://www.foo.com:5129", "Host: www.foo.com", "www.foo.com:5129")] // Non-default port in request string
[InlineData("http://www.foo.com:5129", "Host: www.foo.com:5128", "www.foo.com:5129")] // Different port in host header
public async Task CanOptOutOfBadRequestIfHostHeaderDoesNotMatchRequestTarget(string requestString, string hostHeader, string expectedHost)
{
var testMeterFactory = new TestMeterFactory();
using var connectionDuration = new MetricCollector<double>(testMeterFactory, "Microsoft.AspNetCore.Server.Kestrel", "kestrel.connection.duration");
Expand All @@ -175,13 +178,13 @@ public async Task CanOptOutOfBadRequestIfHostHeaderDoesNotMatchRequestTarget(str
{
using (var client = server.CreateConnection())
{
await client.SendAll($"GET http://www.foo.com/api/data HTTP/1.1\r\n{hostHeader}\r\n\r\n");
await client.SendAll($"GET {requestString} HTTP/1.1\r\n{hostHeader}\r\n\r\n");

await client.Receive("HTTP/1.1 200 OK");
}
}

Assert.Equal("www.foo.com:80", receivedHost);
Assert.Equal(expectedHost, receivedHost);

Assert.Collection(connectionDuration.GetMeasurementSnapshot(), m => MetricsAssert.NoError(m.Tags));
}
Expand Down
Loading