Potential HTTP Response Splitting Vulnerability

[jamadden]

This header injection vulnerability was recently reported to and fixed in both waitress (WSGI server) and WebOb (and waitress has an outstanding issues to discuss further possible vulnerabilities). I also made changes to gevent for these issues.

gunicorn seems to be partly (but not entirely) vulnerable to these issues as well, across at least the sync and gevent workers.

Here are three apps, each producing different kinds of bad output. The badvalue case (invalid values in the header values) is probably the most likely injection vulnerability, with the next being the badname case (invalid value in the header names) and the least likely being the badstatus case (invalid value in the status line):

def badvalue(environ, start_response):
    start_response("200 BadValue",
                   [("Test", "Hi\r\nContent-length: 0\r\nConnection: keep-alive\r\nTransfer-Encoding: identity\r\n\r\n\r\n")])
    return [b"This should be the body!"]

def badname(environ, start_response):
    start_response("200 BadName",
                   [("\r\n\r\n\r\nContent-length: 0\r\nConnection: close\r\n\r\n", "Nothing")])
    return [b"This should be the body!"]

def badstatus(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!"]

Here's what happens hitting badvalue. We get a bunch of duplicate headers that might confuse clients:

$ http 127.0.0.1:8000
HTTP/1.1 200 BadValue
Connection: close
Connection: keep-alive
Content-length: 0
Date: Thu, 17 Mar 2016 13:33:39 GMT
Server: gunicorn/19.4.5
Test: Hi
Transfer-Encoding: chunked
Transfer-Encoding: identity

This should be the body!

Here's what happens hitting badname. The worst I was able to do was produce malformed HTTP; on other servers I was able to cause clients to hang, but I haven't reproduced that with gunicorn (yet):

$ http 127.0.0.1:8000
HTTP/1.1 200 BadName
Connection: close
Connection: close: Nothing
Content-length: 0
Date: Thu, 17 Mar 2016 13:36:39 GMT
Server: gunicorn/19.4.5
Transfer-Encoding: chunked

This should be the body!

Finally, here's badstatus. Here, we completely override the rest of the response:

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

Should gunicorn check for and raise exceptions for these type of malformed values in start_response?

[jamadden]

Actually, with the badstatus line, the httpie client was hiding what was actually on the wire, based on the headers. In a case of pipelining/keep-alive connections, that could confuse clients. Here's what's actually on the wire:

$ echo -e "GET / HTTP/1.0\r\n\r\n" | nc localhost 8000
HTTP/1.0 200 Evil
Content-Length: 0
Connection: close


Server: gunicorn/19.4.5
Date: Thu, 17 Mar 2016 18:19:45 GMT
Connection: close

This should be the body!

[jamadden]

I'm happy to try to work up a pull request if that's the best way to solve this potential vulnerability. Though I do admit to not being super familiar with this area of gunicorn, so it may take some time.

[benoitc]

i fail to see how it affects gunicorn there. in the worst case it seems to accept 2 requests. What dis i missed?

[jamadden]

Well, I may not be a security expert. But this vulnerability has its own CVE number, not to mention the urgency in which it was fixed in the aforementioned webob and waitress projects. At a minimum that suggests to me it should be fixed in gunicorn. At worst, I think this allows hiding an entire bogus response inside an otherwise trusted response; webob explicitly checks the redirect headers for this, because they often allow user input--think a link shortener service that embeds a malicious page. (Then there's the DOS the other projects exhibit; I haven't produced them yet with gunicorn, but I also haven't proven them impossible.)

[benoitc]

ok i reread the issue. for us. the question we should address is if it's the role of the server to sanitise the response before sending it.

actually i'm not sure its our role. rather the role of the application. But i would like to hear what people think about it.

[jamadden]

Apache's mod_wsgi sanitizes. As does waitress and gevent. And PEP3333 does explicitly say that the server should sanitize at least some of these values (and the waitress devs agree that more sanitization should be done) (previously cited links). (Obviously I'm biased when it comes to gevent :)

[benoitc]

no PEP3333 says

Each header_value must not include any control characters, including carriage returns or linefeeds, either embedded or at the end. (These requirements are to minimize the complexity of any parsing that must be performed by servers, gateways, and intermediate response processors that need to inspect or modify response headers.)

So it's the role of the application to ensure about their format. The parts other refer is about checking we don't set hop headers or forgot one:

In general, the server or gateway is responsible for ensuring that correct headers are sent to the client: if the application omits a header required by HTTP (or other relevant specifications that are in effect), the server or gateway must add it. For example, the HTTP Date: and Server: headers would normally be supplied by the server or gateway.

it is not really efficient to do that work twice. especially since that error can only be created by the apllication. We need to think a little more about it imo :)

[jamadden]

PEP3333 also says

Servers should check for errors in the headers at the time start_response is called, so that an error can be raised while the application is still running.

And blatant but easily missed security vulnerabilities are pretty much an error, at least according to the interpretation of the other projects (that cite came from waitress, IIRC). Not to mention failing to comply with the spec being an error.

[benoitc]

Client should protect themselves. But anyway I was about to say that we may want to check if the header is valid and return an error in that case if not. It can easily be done there:
https://github.com/benoitc/gunicorn/blob/master/gunicorn/http/wsgi.py#L263

Let find an efficient way to check it.

[benoitc]

also while we are here we should not stop to this issue and check completely if the header is valid or not :)

[jamadden]

client should protect themselves.

In principle I agree. But in practice, do they? Almost certainly not. This seems to me similar to PEP475 about interrupted syscalls: everyone should check for it, but no one does. So it's much better to do it at the choke points once then force everyone to do it or pay the price.

[jamadden]

also while we are here we should not stop to this issue and check completely if the header is valid or not :)

If there are potential security vulnerabilities :) I will happily update gevent to do so as well.

[benoitc]

@jamadden opened the PR above. It should be compliant with the PEP3333. Additionally it makes sure that each header_value must not include any control characters, including carriage returns or linefeeds.

[benoitc]

cc @berkerpeksag @tilgovi

[jamadden]

@benoitc Thanks!

[RyPeck]

@benoitc glad to see this fixed quickly! Will you be pushing out a new release with this fix included soon?

[benoitc]

i will make a release tomorrow. Couldn't make it today :(