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

Add host validation requirement. #1970

Open
ioquatix opened this issue Oct 3, 2022 · 8 comments
Open

Add host validation requirement. #1970

ioquatix opened this issue Oct 3, 2022 · 8 comments
Assignees

Comments

@ioquatix
Copy link
Member

ioquatix commented Oct 3, 2022

puma/puma#2977 for original discussion.

Servers should reject bad requests with invalid host headers.

Invalid means doesn't conform to the syntax described by https://www.rfc-editor.org/rfc/rfc9110.html#section-7.2 which itself is described by https://www.rfc-editor.org/rfc/rfc3986#section-3.2.2.

In theory we can do this in a middleware to ease compatibility.

We need to consider performance impact. Limiting the scope of the validation for performance reasons is acceptable.

cc @MSP-Greg

@jeremyevans
Copy link
Contributor

In terms of SPEC, I don't think we should require host validation. I think is better to keep this optional, up to the server.

If you are using a front end web server (e.g. Apache/Nginx), and proxying to a Ruby web server (e.g. Puma/Unicorn), the front end web server will generally perform the host check in order to know which backend web server to route to, in which case a check on the Ruby web server side is redundant. That's my environment, so this change would not improve security in any of my applications, it would only slow them down.

If we think validation is important enough to security to require it, it seems odd to limit the validation scope for performance reasons.

SPEC doesn't currently specify the format of any HTTP_* values, so doing so for HTTP_HOST seems inconsistent. We don't check that HTTP_FORWARDED_FOR is a valid IP or list of IPs, that HTTP_COOKIE returns a valid cookie, HTTP_ACCEPT_ENCODING and HTTP_ACCEPT_LANGUAGE contain valid values, etc.. I understand that HTTP_HOST has security issues that other environment keys may not have, though.

I don't think adding this to SPEC will help security, assuming by enforcement that means Lint is doing the check. In development, you probably aren't getting such attacks, and in production, you aren't using Lint.

I'm positive for shipping a middleware that does this check. That allows users/frameworks who want to use this support to do so, without negatively affecting users who would not benefit from such checks.

We should be aware when discussing this that just because a host is valid according to the RFC, does not been it is valid for the app. There are all kinds of host rebinding attacks you can do with valid hostnames, you don't need an invalid hostname for a rebinding attack. In general, applications should be configured to only accept a specific host or list of hosts if they want to avoid rebinding attacks, assuming they are using the host information in a a way that makes them vulnerable to a rebinding attack in the first place.

@MSP-Greg
Copy link
Contributor

MSP-Greg commented Oct 3, 2022

As mentioned in the above Puma issue, I was seeing what could be done to minimize performance degradation.

In general, applications should be configured to only accept a specific host or list of hosts

But, the best place to check for the above is in the server, as there's no reason for the request to 'stay alive' if the host is invalid.

So, we may look at adding a config option for valid hosts, and if used, Puma could perform a full validation, and reject the request before it even reads the request body, if included...

@ioquatix
Copy link
Member Author

ioquatix commented Oct 3, 2022

The reason why host validation will fail (according to some definition of hosts/patterns) is highly likely to be because of an attack. Stopping this as early as possible has got to be a significant win for security. Doing this in the middleware has already exposed a significant part of the server code to malicious input, so I agree, the earlier this is done, the better. The server is in the best position to prevent these attacks.

@jeremyevans
Copy link
Contributor

I agree that it makes sense for the server to prevent such attacks. As @MSP-Greg mentioned, a server config option for the actual valid hosts for the app seems like the best way to handle this. I don't think it's worth specifying in rack.

@ioquatix
Copy link
Member Author

ioquatix commented Oct 3, 2022

I think we should consider adding options like:

# config.ru

# Should we consider server agnostic options/configurations?
option :allowed_hosts, ["myhost.com"]

@tenderlove
Copy link
Member

I agree this shouldn't be part of SPEC. Unsure if we should add support for this in the rackup world. If we did that, it would mean that the RU file would become not portable between webservers (or not make sense given a particular webserver).

@MSP-Greg
Copy link
Contributor

MSP-Greg commented Oct 5, 2022

I agree this shouldn't be part of SPEC

I agree. Some systems may want checks like host.end_with? 'github.io', some may want to check a single string, some may want an array of strings, etc.

So, re Puma (or servers in general), I think support for configuring a block/lambda as a check might be helpful. If an app is using the header, it's probably already checking it, so adding checks to a server would only benefit when a large number of invalid requests are being made.

@ioquatix
Copy link
Member Author

ioquatix commented Oct 5, 2022

If we did that, it would mean that the RU file would become not portable between webservers (or not make sense given a particular webserver).

I don't understand, the entire point is to have a generic configuration rather than having to write the same config for N different servers, e.g. config/puma.rb, config/falcon.rb config/unicorn.rb.

You are right, severs would need to support it, but that's true no matter what.

We could do it as a middleware, but that has a different set of trade offs - adoption - too late to block the attack completely - etc.

Some systems may want checks like host.end_with? 'github.io', some may want to check a single string, some may want an array of strings, etc.

That's out of the bounds of the RFC validation of a valid header format. We enforce that certain headers should be present, why should we accept them with invalid values? It's fine to leave it up to the server, but filtering out invalid requests as early as possible makes sense to me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants