Unsafe pattern for port restriction in documentation

[hacst]

Imo the recommendations given for port restriction in https://learn.microsoft.com/en-us/aspnet/core/host-and-deploy/health-checks?view=aspnetcore-7.0#require-host are unsafe. They rely solely on the Host header transmitted by the client. This header often is fully client controlled. E.g. if a host has an exposed port 80 and an internal management port 5001 which is not exposed to the internet with endpoints restricted by using .RequireHost("*:5001"), this can be trivially circumvented by doing something like curl --header "Host: whatever:5001" http://thepublicserver/healthz which connects on port 80 but stating port 5001 in the host header passing the RequireHost condition.

The linked https://learn.microsoft.com/en-us/aspnet/core/fundamentals/routing?view=aspnetcore-7.0#host-matching-in-routes-with-requirehost and the extension method documentation in https://learn.microsoft.com/en-us/dotnet/api/microsoft.aspnetcore.builder.routingendpointconventionbuilderextensions.requirehost?view=aspnetcore-7.0 also do not make it clear that this relies on the potentially user controlled Host header field.

I think I've seen this implementation pattern recommended in other locations in the documentation and samples where the goal is to restrict endpoints to a given port. At the very least there should be a warning, that this is generally not a safe way to achieve such a restriction. Ideally a safe pattern would be suggested. Even better would be having some built-in way to achieve this common requirement in a safe way and documenting that. E.g. some RequireLocalPort, RequireLocalHost etc. that actually checks the local port / local IP in the http context instead of the host header. It seems this was suggested in dotnet/aspnetcore#46057 which is now closed & locked, though I am not sure the security angle was given sufficient appreciation there.

---

Document Details

⚠ Do not edit this section. It is required for learn.microsoft.com ➟ GitHub issue linking.

• ID: c0cbb094-29e9-740a-9fd6-db372641d753
• Version Independent ID: 52d8efc9-a336-71d9-6d90-98173cbf6bf4
• Content: Health checks in ASP.NET Core
• Content Source: aspnetcore/host-and-deploy/health-checks.md
• Product: aspnet-core
• Technology: aspnetcore-hostdeploy
• GitHub Login: @Rick-Anderson
• Microsoft Alias: riande

---

Associated WorkItem - 98270

[guardrex]

I have one Blazor example that filters requests for middleware that relies on HttpContext.Request.Host.Port. The example works well with HttpContext.Request.HttpContext.Connection.LocalPort, too. However, it also uses HttpContext.Request.Host to match the host. I'm considering a NOTE for that example to raise awareness on this.

For ...

the extension method documentation in https://learn.microsoft.com/en-us/dotnet/api/microsoft.aspnetcore.builder.routingendpointconventionbuilderextensions.requirehost?view=aspnetcore-7.0

... we don't control that here. That documentation is driven directly by the API itself, so you'd need to open an issue for the product unit asking them about making a change to the API remarks that generate the docs. I think you should do that and re-raise the whole subject with them ...

https://github.com/dotnet/aspnetcore/issues

Besides anything they might do with API remarks, I think the more important point is that dotnet/aspnetcore#46057 closed automatically just as it was about to reach critical mass 💣 and turn into a work item. If you open a new issue asking them to consider proceeding with new API, would you mind tagging me in your opening comment? I'd like to follow the discussion.

Please add ...

cc: @guardrex https://github.com/dotnet/AspNetCore.Docs/issues/29399

Rick, we might be able to craft a one/two-line NOTE for the repo-wide INCLUDES that can appear anywhere. I'm going to wait and see how this plays out before I do anything for that Blazor doc. 👂

[guardrex]

Language ideas for discussion ...

API that relies on the Host header, such as RequireHost or HttpRequest.Host, is subject to potential spoofing by clients.

In high security scenarios, consider assigning a value to HttpRequest.Host in middleware before UseRouting is called and using HttpContext.Connection in places where either or both the IP address and port are checked. A host (domain name) can only be checked via a reverse DNS lookup using a registered IP address.

...

> [!WARNING]
> API that relies on the [Host header](https://developer.mozilla.org/docs/Web/HTTP/Headers/Host), such as <xref:Microsoft.AspNetCore.Builder.RoutingEndpointConventionBuilderExtensions.RequireHost%2A> or <xref:Microsoft.AspNetCore.Http.HttpRequest.Host%2A?displayProperty=nameWithType>, is subject to potential spoofing by clients.
>
> In high security scenarios, consider assigning a value to <xref:Microsoft.AspNetCore.Http.HttpRequest.Host?displayProperty=nameWithType> in [middleware](xref:fundamentals/middleware/write) before <xref:Microsoft.AspNetCore.Builder.EndpointRoutingApplicationBuilderExtensions.UseRouting%2A> is called and using <xref:Microsoft.AspNetCore.Http.HttpContext.Connection%2A?displayProperty=nameWithType> in places where either or both the IP address and port are checked. A host (domain name) can only be checked via a reverse DNS lookup using a registered IP address.

BUT ... if this subject is important enough to add a section to the main repo's Routing article, then the NOTE INCLUDE would just have a one-liner with a cross-link to the new section.

[Rick-Anderson]

... we don't control that here. That documentation is driven directly by the API itself, so you'd need to open an issue for the product unit asking them about making a change to the API remarks that generate the docs. I think you should do that and re-raise the whole subject with them ...

@david-acker I think an issue should be opened and a PR made to these API's so the API docs warn about host spoofing. That could be done after the PU (Product Unit) approves our warning language.