Repository navigation
httplib fails to handle semivalid HTTP headers #68551
Description
Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.
Show more details
GitHub fields:
assignee = None
closed_at = None
created_at = <Date 2015-06-02.15:06:03.770>
labels = ['3.7', '3.8', 'type-bug', 'library', 'expert-email']
title = 'httplib fails to handle semivalid HTTP headers'
updated_at = <Date 2019-09-16.10:07:45.075>
user = 'https://bugs.python.org/mgdelmonte'bugs.python.org fields:
activity = <Date 2019-09-16.10:07:45.075>
actor = 'cschmidbauer'
assignee = 'none'
closed = False
closed_date = None
closer = None
components = ['Library (Lib)', 'email']
creation = <Date 2015-06-02.15:06:03.770>
creator = 'mgdelmonte'
dependencies = []
files = ['44087', '44482', '44524', '44527', '46400']
hgrepos = []
issue_num = 24363
keywords = ['patch']
message_count = 35.0
messages = ['244672', '244673', '244674', '244675', '244676', '244678', '244703', '244704', '244719', '244720', '244721', '244722', '244732', '244733', '244752', '244753', '255557', '268408', '268544', '272537', '275244', '275366', '275368', '275370', '275473', '275484', '275582', '275607', '276670', '276672', '276896', '286138', '287228', '287237', '352448']
nosy_count = 12.0
nosy_names = ['barry', 'orsenthil', 'r.david.murray', 'python-dev', 'martin.panter', 'piotr.dobrogost', 'icordasc', 'demian.brecht', 'maxking', 'Lukasa', 'mgdelmonte', 'gboudreau']
pr_nums = ['12214']
priority = 'normal'
resolution = None
stage = 'patch review'
status = 'open'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue24363'
versions = ['Python 3.7', 'Python 3.8']
Activity
mgdelmonte commentedon Jun 2, 2015
Initially reported at https://github.com/kennethreitz/requests/issues/2622
Closely related to http://bugs.python.org/issue19996
An HTTP response with an invalid header line that contains non-blank characters but *no* colon (contrast http://bugs.python.org/issue19996 in which it contained a colon as the first character) causes the same behavior.
httplib.HTTPMessage.readheaders() oddly does not appear even to attempt to follow RFC 2616, which requires the header to terminate with a blank line. The invalid header line, which admittedly also breaks RFC 2616, is at least non-blank and should not terminate the header. Yet readheaders() takes it as an indicator that the header is over and then fails properly to process the rest of the response.
The problem is exacerbated by a chunked encoding, which will not be properly received if the encoding header is not seen because readheaders() terminates early. An example (why are banks always the miscreants here?) is:
My recommended fix would be to insert these lines at httplib:327
This would cause readheaders() to terminate only on a non-blank non-header non-comment line, in accordance with RFC 2616.
sigmavirus24 commentedon Jun 2, 2015
FWIW, the proper section to reference now is 3.2 in RFC 7230 (https://tools.ietf.org/html/rfc7230#section-3.2)
sigmavirus24 commentedon Jun 2, 2015
Also I'm marking this as affecting 3.3, 3.4, and 3.5. I haven't tested against 3.5, but it definitely fails on 3.4. I hope to be able to test against 3.5.0b2 tonight
mgdelmonte commentedon Jun 2, 2015
Thanks. Also I meant to have said, "...to terminate only on a *blank* non-header non-comment line, in accordance with RFC 2616 (and 7230)."
I note that the RFCs require CRLF to terminate but in my experience you can get all manner of "blank" lines, so accepting len(line.strip())==0 is going to accommodate servers that give CRCR or LFLF or (and I have seen this) LFCRLF.
bitdancer commentedon Jun 2, 2015
The current behavior probably comes out of the RFC822 world, where when seeing a line that doesn't look like a header the error recovery is to treat that line as the beginning of the body (ie: assume the blank line is missing).
Is there in fact any guidance on what the optimal error recovery strategy is for http? I've actually considered trying to make the error recovery in email smarter, by seeing if the *next* line looks like a header before making the decision.
bitdancer commentedon Jun 2, 2015
Ah, in fact that's exactly where it comes from, since httplib uses the email header parsing code. In python3 we are actually using the email package to parse the headers (which is sensible) (in 2.7 it is a copy of code from the old mimelib with some tweaks). Fixing this in python3 is best done by making the error recovery enhancement(s) I mentioned in the email package.
Please note that changing this behavior has the potential to break working code. That is, just as we have here a server that is out of spec and sending invalid links in the middle of headers, we may have a server that is out of spec by not sending the blank delimiter, which is currently being handled "correctly". Thus I don't think your simple fix is advisable, instead I think we should pursue the more complicated "look ahead" fix.
Lukasa commentedon Jun 2, 2015
This is one of those bugs that's actually super tricky to correctly fix.
The correct path is to have the goal of conservatively accepting as many headers as possible. Probably this means looking ahead to the next few lines and seeing if they appear to roughly keep a header structure (\r\n line breaks and colon separated values). However, I'm not entirely sure how to structure that sentiment in code at this time.
bitdancer commentedon Jun 2, 2015
I think there may be a way to accomplish this in a reasonably straightforward fashion in python3 given that feedparser has an 'unreadline' function. The python2 case is probably going to be a more complicated change. And I agree that multiple lines should be examined.
There is also the question of what to do with the extra data. I think the correct approach is to treat a line that does not conform as a line that is missing the leading whitespace that would make it a continuation line. It looks like that would certainly be correct for the case in hand.
mgdelmonte commentedon Jun 3, 2015
I don't want to speak out of school and you guys certainly know what you're doing, but it seems a shame to go through these gyrations -- lookahead plus "unreading" lines -- only to preserve the ability to parse email headers, when HTTP really does follow a different spec. My suggestion would be to examine the header and decide, if it's HTTP, to just ignore nonconforming lines; and if it's email, then the problem is already solved (as email doesn't have encoding rules that would cause problems later).
My fear would be that you'll eventually get that nonconforming line with leading whitespace, which will lead right back to the same error.
bitdancer commentedon Jun 3, 2015
No, the point is to do "best practical" error recovery when faced with dirty data that may be dirty in various ways, and it doesn't really matter whether it is http headers or email headers. A line with leading whitespace is treated as part of the preceding header line now, and this is the way it should behave, since the older http standards adopted that behavior from rfc822. You will note that the standard referenced by Ian is explicit about that, in the obs-fold clause. That is, we are required by the standard to support that behavior, which is why I posit that the best recovery is to assume an invalid line followed by what look like headers is in fact an incorrectly folded obs-fold continuation line. That this will also conform to the email standard is a not-accidental consequence of how these standards evolved. (That is, email and http header handling are *not* "different" specs in the sense of being disjoint, they are derivatives of a common ancestor spec and some effort is spent keeping them interoperable, to my understanding.)
vadmium commentedon Jun 3, 2015
Regarding the suggested fix for Python 2, make sure it does not prematurely end the parsing on empty folded lines (having only tabs and spaces in them). E.g. according to RFC 7230 this should be a single header field:
b"Header: obsolete but\r\n"
b" \r\n"
b" still valid\r\n"
I suspect the RFC doesn’t say anything specifically about this case. In general the guidance seems to be things like:
In the case of the bank web site, the last lines of the header are:
X-Frame-Options: SAMEORIGIN\r\n
Set-Cookie: mb-CookieP=; HttpOnly; \r\n
Secure\r\n
Set-Cookie: mb-CookieP=; HttpOnly; Secure\r\n
\r\n
It is obvious that this case could be treated as a folded (continuation) line. But in general I think it would be better to ignore the erroneous line, or to record it as a defect so that the server module or other user can check it.
Looking at the Python 3 code, both the client and server call http.client._parse_headers(), which sensibly reads each line until it sees a blank line (Lib/http/client.py:197). But then after jumping through some more hoops we parse it again until we find a line that fails the regular expression at Lib/email/feedparser.py:37. The remaining lines become the “payload” of the HTTP header:
What might be nice is a way to reuse the email header field parsing code, without worrying about the “From” line stuff, or the payload stuff.
13 remaining items
vadmium commentedon Sep 9, 2016
I guess we could add this secret policy flag that the email parser checks. The solution should still be applied as a bug fix to 3.5 as well as 3.6+. I would have to make the flag “very” unique, to reduce the chance of it breaking user code. I.e. adding policy._strict_end_of_headers might interfere with somebody’s user-defined policy class, but maybe policy.__strict_end_of_headers__ is safe, since user code is not supposed to invent such attribute names. Or do you think it “_strict_end_of_headers” name is fair game since the policy stuff is/was provisional?
It would be good to fix 2.7. This bug, and the duplicate report, were both originally reported for 2.7. I haven’t looked at the situation there in detail, but it looks completely different. HTTPMessage is based on the “mimetools” package, and there is a lot more parsing code already in “httplib”.
bitdancer commentedon Sep 9, 2016
Oh, yes, I forgot 2.7 was using the older code.
Sure, that flag name sounds fine. I'm not worrying about policy flag name collisions, but perhaps I should be. How about _py_strict_end_of_headers? I don't really care what it is named for the bug fix, I'll think about the exposed name for 3.7.
vadmium commentedon Sep 10, 2016
Here is a fix using a policy flag. I called it “_py_body_detached”.
vadmium commentedon Sep 10, 2016
Patch for Python 2
python-dev commentedon Sep 16, 2016
New changeset c38e10ad7c89 by Martin Panter in branch '2.7':
Issue bpo-24363: Continue parsing HTTP header in spite of invalid lines
https://hg.python.org/cpython/rev/c38e10ad7c89
vadmium commentedon Sep 16, 2016
I pushed my Py 2 patch, since it is simpler and does not interfere with other modules. But it would still be good to get feedback on policy-flag.patch for Python 3.
bitdancer commentedon Sep 18, 2016
I will try to review this in the not too distant future. You can ping me if I don't get to it by next Saturday.
I think I'll probably prefer to call the flag something like _greedy_header_parsing, to reflect a change from assuming we've got body on a non-header-like line to assuming we've still got header if we haven't seen a blank line yet. What we call the private version of the flag doesn't matter all that much, though.
vadmium commentedon Jan 24, 2017
Just a minor update with an extra get_payload() test I missed before
gboudreau commentedon Feb 7, 2017
Any chance this could get reviewed and merged soon? I got hit by a similar issue (see bpo-29445) where the server, which I don't control, sends me invalid HTTP headers, and the prevents all the headers that follow it to not be parsed.
The latest attached patch fixed the issue for me.
Thanks.
bitdancer commentedon Feb 7, 2017
Yeah, I'm going to try to get to this this weekend.
maxking commentedon Sep 14, 2019
Martin: Can you please create a PR for the added patch? If you are busy, I can do that for you, just wanted to ask before I do :)
I am going to remove "easy" label from this issue, which IMO it clearly isn't given 4 years of history to catch up on and a few other related bugs.
Since some time has passed since the patch was posted, I am going to target this to 3.8 and 3.7 since others are in security fix mode only.