Skip to content

HttpsRedirectionMiddleware unsafe fallback behavior when app is misconfigured #27951

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.

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

Closed
davidni opened this issue Nov 18, 2020 · 5 comments · Fixed by #29166
Closed

HttpsRedirectionMiddleware unsafe fallback behavior when app is misconfigured #27951

davidni opened this issue Nov 18, 2020 · 5 comments · Fixed by #29166
Assignees
Labels
area-middleware Includes: URL rewrite, redirect, response cache/compression, session, and other general middlewares
Milestone

Comments

@davidni
Copy link

davidni commented Nov 18, 2020

Describe the bug

Applications that explicitly include app.UseHttpsRedirection() to their pipeline should (reasonably) expect that anything beyond that point would be HTTPS only, and may make security assumptions based on that expected fact.

However, the implementation of HttpsRedirectionMiddleware in ASP .NET Core 3.1, as well as latest master, have an unsafe, unreasonable and unexpected fallback when the application is misconfigured and the middleware is unable to determine the target HTTPS port. See

if (port == PortNotFound)
{
return _next(context);
}
.

Ideally, misconfiguration that triggers that condition should be detected at startup -- fail fast, making it trivial for someone to analyze and fix a potential vulnerability.

Failing that, the condition should be detected at runtime, causing the pipeline to abort (throw new InvalidOperationException(...) of some kind).

However, ASP .NET Core today simply skips HTTPS redirection, allowing the request to proceed over an insecure HTTP channel**. This is a significant issue and may lead to inadvertent transmission of sensitive data that can be easily intercepted.

Under no circumstances should the framework allow an insecure request to proceed when the app explicitly added a step to enforce HTTPS to their pipeline. This is a an unreasonable and unsafe fallback that puts entire applications at risk.

Example scenario where this can be particularly problematic -- a seemingly innocuous change to add a second HTTPS listener address on Kestrel means that, all of a sudden, the HttpsRedirectionMiddleware is no longer able to determine a single HTTPS target port, making an entire application insecure just because a new endpoint was added. This behavior violates the Principle of Least Astonishment.

To Reproduce

  • Create an application that listens e.g. on http://localhost:80, https://localhost:443, https://localhost:444
  • Make sure the app includes app.UseHttpsRedirection() early in its pipeline, without any additional custom configuration
  • Run, then issue a request to http://localhost:80
  • Notice that the request executes to completion, and it is NOT redirected to HTTPS

Relevant source:

// If we find multiple different https ports specified, throw
if (nullablePort.HasValue && nullablePort != bindingAddress.Port)
{
_logger.FailedMultiplePorts();
return PortNotFound;
}

Notice the comment says If we find multiple different https ports specified, throw, yet the actual code does not throw, and instead proceeds.

Exceptions (if any)

N/A

Further technical details

  • ASP.NET Core version: 3.1.8 up to latest master
@jkotalik jkotalik assigned Tratcher and ChrisAhna and unassigned ChrisAhna Dec 4, 2020
@jkotalik jkotalik added this to the Backlog milestone Dec 4, 2020
@ghost
Copy link

ghost commented Dec 4, 2020

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.

@Tratcher
Copy link
Member

Tratcher commented Dec 4, 2020

Yes, the code comment is outdated and should be corrected. The default behavior during the initial development was to throw but that caused significant usability issues for apps that didn't have https enabled at all.

I think we can split this conversation into three scenarios:

  1. The server doesn't have https enabled and/or is behind a TLS terminator. That was a significant source of usability issues that lead to the no-op design. I don't anticipate making changes for this scenario.
  2. The issue you've outlined above where the middleware auto-detects more than one https endpoint. I think we could make this scenario fail (5XX?) rather than no-op by default. I'll check with the team.
  3. Explicitly configuring multiple http/https pairs to resolve the ambiguities from scenario (2). This is tracked by Enhance HTTPS redirect middleware for multi-hostname scenarios #21291

When we talked offline you mentioned that you were lucky not to require this (3), you were able to select one of your https ports and ignore the other one. Any improvements here would not be to unblock you, but to prevent the unwary from stumbling into the same issue.

@blowdart blowdart modified the milestones: Backlog, Next sprint planning Jan 7, 2021
@ghost
Copy link

ghost commented Jan 7, 2021

Thanks for contacting us.
We're moving this issue to the Next sprint planning milestone for future evaluation / consideration. We will evaluate the request when we are planning the work for the next milestone. To learn more about what to expect next and how this issue will be handled you can read more about our triage process here.

@Tratcher
Copy link
Member

Tratcher commented Jan 7, 2021

Triage: Let's try throwing a runtime error when there are multiple https endpoints. To resolve the ambiguity the developer can explicitly set the port (if there's only one valid port) or we can implement #21291 for more complex mappings.

@davidni
Copy link
Author

davidni commented Jan 11, 2021

@Tratcher +1, your proposal is reasonable and fully addresses my concern.

@ghost ghost locked as resolved and limited conversation to collaborators Feb 27, 2021
@amcasey amcasey added area-middleware Includes: URL rewrite, redirect, response cache/compression, session, and other general middlewares and removed area-runtime labels Jun 2, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-middleware Includes: URL rewrite, redirect, response cache/compression, session, and other general middlewares
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants