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.

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

caddyhttp: Introduce strict HTTP mode (WIP) #4841

Closed
wants to merge 2 commits into from
Closed

caddyhttp: Introduce strict HTTP mode (WIP) #4841

wants to merge 2 commits into from

Conversation

mholt
Copy link
Member

@mholt mholt commented Jun 14, 2022

It has come up several times that across various parts of the HTTP app, malformed requests can cause problems. They're not necessarily direct security issues, but can have unintended consequences for upstream/backend applications that are being proxied to, or file systems with flimsy rules for filepaths, etc., when combined with the wrong configs.

In an effort to reduce possible misconfigurations and edge cases with various other systems like backend services and file systems, I'm proposing a kind of "strict HTTP mode" to be enabled by default. (This PR is still in a draft/WIP state. Feedback and discussion is invited.)

It will reject requests with HTTP 400 that have:

Basically the idea of strict HTTP is to enforce clean, spec-compliant requests and reject all others.

To clarify, I'm not saying this is a good idea or that the web server/proxy should be responsible for this. In fact I am pretty sure this will break a few legitimate use cases and applications, which do want underscores in some header fields or which use semicolons in the query string or which require double slashes for some sort of semantic interpretation.

As mentioned, these qualities of an HTTP requests are not in and of themselves security vulnerabilities, but they can present problems from poorly-designed or buggy applications that depend on these parts of HTTP requests. (For example, a file server that does not sanitize the path could be vulnerable to directory traversal by blindly evaluating .. components in the path. Or Django apps can be misled by clients adding a HOST_HEADER header field. Or Windows file systems do unexpected things with NUL bytes. Merely having .. in a path is not a security problem for Caddy, nor is having a header called HOST_HEADER, and so on.)

This implementation is currently WIP. It enables the enforcement of these traits by default, but can be disabled entirely or piecewise by being lenient on query strings, paths, or headers according to your configuration.

Upsides:

  • This change can prevent some subtle misconfigurations and security flaws when interacting with other systems. They do NOT patch any known vulnerability in Caddy itself.

Downsides:

  • This will inevitably take some users by surprise and break legitimate use cases.
  • Protections can't currently be overridden per-request; currently you can turn off strict mode (entirely or for certain request parts), but there's not necessarily a way to re-enable them in your HTTP routes for certain requests. You could probably get clever to re-implement them, for example using headers: Support wildcards for delete ops #4831 to drop headers that have underscores when going to a backend.
  • This adds trivial latency to all requests.
  • This doesn't actually solve any security issues, it just covers them.

The whole change is not my favorite, and that last point is really bothering me.

Instead of this change, I recommend that applications actually patch their vulnerabilities.

@mholt mholt added feature ⚙️ New feature or request discussion 💬 The right solution needs to be found do not merge ⛔ Not ready yet! labels Jun 14, 2022
@mholt mholt requested a review from francislavoie June 14, 2022 17:45
@mholt
Copy link
Member Author

mholt commented Jul 28, 2022

If we do enable this, it'll be optional (opt-in, not opt-out).

More likely to be a separate plugin too.

@mholt
Copy link
Member Author

mholt commented Sep 2, 2022

Shelving this. May come back to it later if there's good reason or high demand.

@mholt mholt closed this Sep 2, 2022
@mholt mholt added the deferred ⏰ We'll come back to this later label Sep 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deferred ⏰ We'll come back to this later discussion 💬 The right solution needs to be found do not merge ⛔ Not ready yet! feature ⚙️ New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant