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

New option to drop headers that contain underscores #17

Closed
mar10 opened this Issue Mar 7, 2017 · 5 comments

Comments

Projects
None yet
3 participants
@mar10
Contributor

mar10 commented Mar 7, 2017

I'd like to propose a new option to drop headers that contain underscores ('_').

Cheroot / CherryPy convert HTTP headers for WSGI to upper case and replace hyphen ('-') with underscore ('_'):

env.update(
    ('HTTP_' + bton(k).upper().replace('-', '_'), bton(v))
    for k, v in req.inheaders.items()
)

So this headers

headers = {"Sm-User": "val1",
           "Sm_User": "val2",}
res = requests.get("http://127.0.0.1:8080", headers=headers)

will result in a single WSGI environment entry "HTTP_SM_USER": "val1" or "HTTP_SM_USER": "val2".
(The concrete behavior is undefined, since the order of dict key iteration is hard to control.)

Proposed solution

This could be fixed by modifying the wsgi server code above, but I would suggest to drop those headers in the core code.

While underscores in header fields are allowed (rfc7230), they are uncommon, AFAIK.
For example Apache and nginx seem to drop them by default:
http://stackoverflow.com/a/18205049/19166
http://stackoverflow.com/a/22856867/19166

So a solution could look like this:

def read_headers(rfile, hdict=None, drop_underscores=False):
    ...
    while True:
        line = rfile.readline()
            ...
            if drop_underscores and '_' in k:
                continue
            k = k.strip().title()
            v = v.strip()
            hname = k
            ...

I tried to prepare a PR, but I could need some advice how and where to make this configurable:

        try:
            read_headers(self.rfile, self.inheaders)
            # Configurable using "`server.underscores_in_headers": False`
            drop_underscores = not self.server.server_adapter.underscores_in_headers
            read_headers(self.rfile, self.inheaders, drop_underscores)
        except ValueError:
            ex = sys.exc_info()[1]
            self.simple_response('400 Bad Request', ex.args[0])

For example I tried adding this to cheroot.HTTPServer:

class HTTPServer(object):
    ...
    underscores_in_headers = True
    """If False, silently discards headers containing underscores ('_')."""

and then make it configurable from cherrypy like so:

cherrypy.config.update({"server.underscores_in_headers": False})
cherrypy.quickstart(HelloWorld())

=> this seems to set the attribute on the ServerAdapter instead of the HttpServer.

or

cherrypy.quickstart(HelloWorld(), config={
    "/": {
        "server.underscores_in_headers": False,
        }
    })

doesn't work, because the servernamespace is not registered when quickstart is running?

@webknjaz

This comment has been minimized.

Member

webknjaz commented Mar 9, 2017

Hello Martin,

I like your idea overall. I'm currently not sure about the config, perhaps I'll figure this out later.
You may proceed with PR and decide about the feature toggle later during review.

@webknjaz webknjaz self-assigned this Mar 9, 2017

@webknjaz webknjaz added the enhancement label Mar 9, 2017

mar10 added a commit to mar10/cheroot that referenced this issue Mar 9, 2017

@mar10

This comment has been minimized.

Contributor

mar10 commented Mar 12, 2017

Maybe 'drop_underscore_headers' is a better option name than 'underscores_in_headers'.
Default should probably be False for now to avoid surprises. (But if I am not mistaken, Apache and nginx seem to have this enabled by default.)

The implementation is trivial, however I don't know what is the best way to configure it.
Configuration should be possible when Cheroot is used directly, but we should also offer to configure this through the CherryPy framework

We could

  • configure it as attribute of server.HTTPServer
    Don't know if this a good place, but max_request_header_sizelives there for example.
    But how could a user configure it using th config.ini or using@cherrypy.config(**{...})?
  • Configuration-wise I think that :
    @cherrypy.config(**{'server.drop_underscore_headers': True})
    would be intuitive for a CherryPy user.
    But how can this setting be accessed from Cheroot code?

Any advice?

@jaraco

This comment has been minimized.

Member

jaraco commented Mar 19, 2017

Can you update the original post to describe (or link to) the original problem description (explaining why someone would care about this issue)?

I can't help but feel like this issue is a problem with the WSGI spec, since it's WSGI that's informing the choice to translate lowercase and dash-separated names to uppercase, underscore-separated names. It seems to me the issue doesn't apply only to the dash/underscore translation but also with the case sensitivity. This translation doesn't happen in the base HTTPServer.

This finding makes me wonder if the wsgi spec has anything to say about the handling of conflated headers as you describe.

@jaraco

This comment has been minimized.

Member

jaraco commented Mar 19, 2017

Reading PEP 3333 gives me little insight.

@mar10

This comment has been minimized.

Contributor

mar10 commented Mar 19, 2017

I think it may have security implications:

Consider a CherryPy application running behind a proxy that does NTLM or SAML
authentication.
If the user is authenticated, a header field will be injected, otherwise this header is removed or emptied.
Since the app is only reachable through the proxy it will trust this field.

Assume the field is named X-Auth-User-Name.

An attacker that is not authorized or authorized with a restricted account could issue an ajax request with additional headers, e.g.
X_Auth-User_name: admin, X_Auth_User-name: admin, …
(Easy with browser plugins, for example.)

With a bit of luck, CherryPy will replace the protected field with the
elevated variant.

I don’t think that this is strictly a bug in Cheroot (since the proxy could filter this out), but the proposed change would make it easier to harden the application.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment