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

HTTP Response Splitting - Status/Header Names #122

Closed
jamadden opened this Issue Mar 17, 2016 · 4 comments

Comments

Projects
None yet
3 participants
@jamadden
Contributor

jamadden commented Mar 17, 2016

#117 was about HTTP response splitting in header values. Should this also cover the status line and the header names? The same thing can happen with those.

All of these examples are on waitress 0.9.0b0:

$ pip freeze | grep waitress
waitress==0.9.0b0

Here's an app that uses a bad status line:

def app(environ, start_response):
    start_response("200 Evil\r\nContent-Length: 0\r\nConnection: close\r\n\r\n", [])
    return [b"This should be the body!"]

And the output of connecting to it:

$ http 127.0.0.1:8080
HTTP/1.1 200 Evil
Connection: close
Content-Length: 0


$

And here's one that uses a bad header name:

def app(environ, start_response):
    start_response("200 BadName",
                   [("\r\n", "Content-length: 0")])
    return [b"This should be the body!"]

And its output, which hangs the client until a timeout, leaving a dangling socket:

$ http 127.0.0.1:8080
HTTP/1.1 200 BadName


http: error: ConnectionError: HTTPConnectionPool(host='127.0.0.1', port=8080): Read timed out.

I realize that HTTP status messages and header names are probably less likely to allow user input than header values, but if they do, bad things can still happen.

@bertjwregeer

This comment has been minimized.

Show comment
Hide comment
@bertjwregeer

bertjwregeer Mar 17, 2016

Member

I used mod_wsgi as a source of inspiration for where to add the check, but additional checks can and probably should be added. Thanks for this report, I will work on getting these fixed.

Member

bertjwregeer commented Mar 17, 2016

I used mod_wsgi as a source of inspiration for where to add the check, but additional checks can and probably should be added. Thanks for this report, I will work on getting these fixed.

@bertjwregeer bertjwregeer added this to the 0.9.0 milestone Mar 18, 2016

@jamadden

This comment has been minimized.

Show comment
Hide comment
@jamadden

jamadden Mar 19, 2016

Contributor

I think PR #124 should cover it.

Contributor

jamadden commented Mar 19, 2016

I think PR #124 should cover it.

bertjwregeer added a commit that referenced this issue Mar 26, 2016

Merge pull request #124 from NextThought/fix.122
Check header names and status for line feed/carriage return.

Fixes #122
@GrahamDumpleton

This comment has been minimized.

Show comment
Hide comment
@GrahamDumpleton

GrahamDumpleton Apr 10, 2016

For the status line, mod_wsgi is probably relying on Apache rejecting it. If Apache isn't failing it in some way I should add a check. There should already be checks in mod_wsgi for HTTP header names and values.

GrahamDumpleton commented Apr 10, 2016

For the status line, mod_wsgi is probably relying on Apache rejecting it. If Apache isn't failing it in some way I should add a check. There should already be checks in mod_wsgi for HTTP header names and values.

@GrahamDumpleton

This comment has been minimized.

Show comment
Hide comment
@GrahamDumpleton

GrahamDumpleton Apr 10, 2016

Nope, mod_wsgi has:

so it should be validating the status line and rejecting he presence of any control characters.

I should check that works. :-)

GrahamDumpleton commented Apr 10, 2016

Nope, mod_wsgi has:

so it should be validating the status line and rejecting he presence of any control characters.

I should check that works. :-)

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