homepage

Initially spotted on Requests GitHub bugtracker: https://github.com/kennethreitz/requests/issues/1804
On receiving an HTTP response with an invalid header, httplib stops parsing the headers and attempts to receive the rest of the message as body content. Normally that would be fine, but problems occur if later on in the headers "Transfer-Encoding: chunked" is declared. This leads to a hang while reading the body content until the remote end forcibly closes the connection.
This bug certainly affects versions 2.7 through 3.3.
To reproduce (note that we need to request gzip to get the server to send the bad header):
 import http.client
 h = http.client.HTTPConnection('www.sainsburysbank.co.uk')
 h.request('GET', '/', headers={'Accept-Encoding': 'gzip'})
 r = h.getresponse()
 hdrs = r.getheaders()
 body = r.read() # Hang here.
cURL configured equivalently doesn't exhibit this problem, that is the following works fine:
curl --compressed http://www.sainsburysbank.co.uk/
It's not clear to me that this behaviour is wrong. The server is definitely violating RFC 2616 which expressly forbids empty header names. I'm open to consultation about what the correct fix should be here (which may be nothing at all).

Well, having it hang forever is a potential DOS attack, so something needs to be fixed, I think.

The easiest way to 'fix' the DoS problem is to throw an exception if an invalid header is parsed. That's a backwards-compatibility problem though: things that previously 'worked' now won't. That presumably limits the ability to back-apply this fix to 2.7.7.
An alternative option is to speculatively attempt to parse the next line for headers or the end of the header block. I'm not sure this is a great idea: at this stage all we know is that the header block is malformed, so it's not clear that 'doing our best' is a good idea either, especially since that attitude got us here to begin with.
The best 'middle of the road' option is to abort message parsing at this stage without throwing an exception. This leads to truncated headers and no body, where previously we'd have got truncated headers and a body that potentially included the missing headers. We could also potentially add a warning about the problem.
Are there any preferences for a fix here, or a better solution than the above (none of which I'm wild about)?

I haven't looked at the code, but could we preserve the existing behavior but apply a timeout to mitigate the DOS?
On the other hand, the fact that curl manages to return something indicates there is probably an error recovery strategy that would work. I'm not sure if we have an error reporting mechanism in httplib if we do error recovery. We do in the email module, and httplib uses the email code for headers, I think, so there might be a way to leverage that if there is no existing mechanism. But of course even deciding to do that requires some discussion :)

Maybe. If we do it we have to apply that timeout to all the socket actions on that HTTP connection. This would have the effect of changing the default value of the timeout parameter on the HTTPConnection object from socket._GLOBAL_DEFAULT_TIMEOUT to whatever value was chosen. We could do this for reads only, and avoid applying the timeout to connect() calls, but that's kind of weird.
We hit the same problem though: by default, HTTPConnections block indefinitely on all socket calls: we'd be changing that default to some finite timeout instead. Does that sound like a good way to go?
As for curl's error recovery strategy, I'm pretty sure it just keeps parsing the header block. That can definitely be done here. We do have an error reporting mechanism as well (sort of): we set the HTTPMessage.status field to some error string. We could do that, and continue to parse the header block: that's probably the least destructive way to fix this.

An update: in Python 2.7 through 3.3, fixing this should only affect http.client/httplib, because they do most of their header parsing themselves. Fixing this in later versions of Python is more interesting, as http.client got rewritten to use email.parser (which uses email.feedparser). This means any change to fix this problem in HTTP will also affect anything else that uses this module. Not sure how problematic that is yet.

Actually, that might be OK. I don't know the email package at all, but I suspect being able to handle empty header keys (by ignoring them) is a reasonable thing to do in the email case as well. Thoughts?

Alright, here's a patch for the current tip. I'll need to prepare a different patch for earlier versions of Python, which will take me a little while longer to do (maybe not today). I've also signed a contributor agreement, but it doesn't look like that's propagated here yet.

Heh. A missing header *name* was something I never considered in the email package tests. So yeah, that's worth handing one way or another. I'll put reviewing this on my TODO list, since I'm the maintainer of the email package.
I'm updating the versions per our usual practice: the versions in which we want to fix it.

Ok, here's a patch for 2.7 as well.
I decided to allow the empty header names in rfc822.py as well, if only because I wanted the changed parsing code to match. If anyone thinks that's an excessive change, I'll happily remove it.

Is there anything I can do to help move this forward? I appreciate you're all busy so I'm happy for this to take as long as it takes, I just wanted to make sure it's not blocked behind me.

import http.client
 h = http.client.HTTPConnection('http://www.einstantloan.co.uk/')
 h.request('GET', '/', headers={'Accept-Encoding': 'gzip'})
 r = h.getresponse()
 hdrs = r.getheaders()
 body = r.read() # Hang here.
curl --compressed http://www.einstantloan.co.uk/ 

I took the patches and verified that;
* running the new tests without the changed code in Lib/email/feedparser.py (head) and Lib/httplib.py, Lib/rfc822.py (2.7) fails both the new tests.
* running the new tests with the changed code passes the tests (on both head and 2.7).
Sainsburys Bank site seems to have been fixed thus verification in the first comment does not work - but the test I think emulates that well enough.

New changeset 25ecf3d0ea03 by Benjamin Peterson in branch '3.4':
handle headers with no key (closes #19996)
https://hg.python.org/cpython/rev/25ecf3d0ea03
New changeset 29923a9987be by Benjamin Peterson in branch 'default':
merge 3.4 (#19996)
https://hg.python.org/cpython/rev/29923a9987be
New changeset 9a4882b12218 by Benjamin Peterson in branch '2.7':
simply ignore headers with no name (#19996)
https://hg.python.org/cpython/rev/9a4882b12218