Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(60)

#22928: HTTP header injection in urrlib2/urllib/httplib/http.client

Can't Edit
Can't Publish+Mail
Start Review
Created:
3 years ago by guidovranken
Modified:
2 years, 9 months ago
Reviewers:
vadmium+py, demianbrecht, storchaka
CC:
Georg, orsenthil, haypo, larry, Benjamin Peterson, ned.deily, Arfrever, r.david.murray, devnull_psf.upfronthosting.co.za, berkerpeksag, Martin Panter, storchaka, koobs, demian, Guido, vlad-python_acheronmedia.com
Visibility:
Public.

Patch Set 1 #

Patch Set 2 #

Total comments: 4

Patch Set 3 #

Patch Set 4 #

Total comments: 4

Patch Set 5 #

Total comments: 7

Patch Set 6 #

Total comments: 6

Patch Set 7 #

Total comments: 3

Patch Set 8 #

Total comments: 5
Unified diffs Side-by-side diffs Delta from patch set Stats Patch
Lib/http/client.py View 1 2 3 4 5 6 7 3 chunks +38 lines, -0 lines 5 comments Download
Lib/test/test_httplib.py View 1 2 3 4 5 6 7 2 chunks +41 lines, -0 lines 0 comments Download

Messages

Total messages: 18
Martin Panter
https://bugs.python.org/review/22928/diff/13932/Lib/http/client.py File Lib/http/client.py (right): https://bugs.python.org/review/22928/diff/13932/Lib/http/client.py#newcode136 Lib/http/client.py:136: _HEADER_LEGAL_VALUE = re.compile(b'^\s*[\x21-\x7E]*\s*$') It looks like this disallows embedded ...
2 years, 10 months ago #1
demian
https://bugs.python.org/review/22928/diff/13932/Lib/http/client.py File Lib/http/client.py (right): https://bugs.python.org/review/22928/diff/13932/Lib/http/client.py#newcode136 Lib/http/client.py:136: _HEADER_LEGAL_VALUE = re.compile(b'^\s*[\x21-\x7E]*\s*$') On 2015/02/14 02:52:17, vadmium wrote: > ...
2 years, 10 months ago #2
demian
On 2015/02/17 16:53:33, demian wrote: > Yep, spaces were definitely an oversight on my part, ...
2 years, 10 months ago #3
Martin Panter
https://bugs.python.org/review/22928/diff/13957/Lib/http/client.py File Lib/http/client.py (right): https://bugs.python.org/review/22928/diff/13957/Lib/http/client.py#newcode1053 Lib/http/client.py:1053: 'Invalid header value {}'.format(encoded_value)) Sorry to be pedantic, but ...
2 years, 9 months ago #4
Martin Panter
http://bugs.python.org/review/22928/diff/13957/Lib/http/client.py File Lib/http/client.py (right): http://bugs.python.org/review/22928/diff/13957/Lib/http/client.py#newcode140 Lib/http/client.py:140: _HEADER_LEGAL_VALUE = re.compile(b'^\s*[\x20-\x7E\xA0-\xFF]*\s*$') The comment above says obs-text = ...
2 years, 9 months ago #5
demian
http://bugs.python.org/review/22928/diff/13957/Lib/http/client.py File Lib/http/client.py (right): http://bugs.python.org/review/22928/diff/13957/Lib/http/client.py#newcode140 Lib/http/client.py:140: _HEADER_LEGAL_VALUE = re.compile(b'^\s*[\x20-\x7E\xA0-\xFF]*\s*$') On 2015/02/20 14:13:14, vadmium wrote: > ...
2 years, 9 months ago #6
storchaka_gmail.com
http://bugs.python.org/review/22928/diff/13980/Lib/http/client.py File Lib/http/client.py (right): http://bugs.python.org/review/22928/diff/13980/Lib/http/client.py#newcode136 Lib/http/client.py:136: _HEADER_LEGAL_NAME = re.compile(b'^[!#$%&\'*+-.^_`|~a-zA-z0-9]+$') " is not legal character. So ...
2 years, 9 months ago #7
Martin Panter
https://bugs.python.org/review/22928/diff/13980/Lib/http/client.py File Lib/http/client.py (right): https://bugs.python.org/review/22928/diff/13980/Lib/http/client.py#newcode141 Lib/http/client.py:141: _HEADER_LEGAL_VALUE = re.compile(b'^[ \t]*[\x20-\x7E\xA0-\xFF]*[ \t]*$') On 2015/03/07 15:35:17, storchaka ...
2 years, 9 months ago #8
storchaka_gmail.com
http://bugs.python.org/review/22928/diff/14134/Lib/http/client.py File Lib/http/client.py (right): http://bugs.python.org/review/22928/diff/14134/Lib/http/client.py#newcode138 Lib/http/client.py:138: _is_legal_header_name = re.compile(b'(.(?!\r))*').fullmatch Legal header name shouldn't be empty, ...
2 years, 9 months ago #9
demian
http://bugs.python.org/review/22928/diff/13980/Lib/http/client.py File Lib/http/client.py (right): http://bugs.python.org/review/22928/diff/13980/Lib/http/client.py#newcode1037 Lib/http/client.py:1037: if not _HEADER_LEGAL_NAME.match(header): On 2015/03/07 15:35:17, storchaka wrote: > ...
2 years, 9 months ago #10
storchaka_gmail.com
http://bugs.python.org/review/22928/diff/14134/Lib/http/client.py File Lib/http/client.py (right): http://bugs.python.org/review/22928/diff/14134/Lib/http/client.py#newcode138 Lib/http/client.py:138: _is_legal_header_name = re.compile(b'(.(?!\r))*').fullmatch On 2015/03/09 20:37:44, demian wrote: > ...
2 years, 9 months ago #11
demian
http://bugs.python.org/review/22928/diff/14134/Lib/http/client.py File Lib/http/client.py (right): http://bugs.python.org/review/22928/diff/14134/Lib/http/client.py#newcode138 Lib/http/client.py:138: _is_legal_header_name = re.compile(b'(.(?!\r))*').fullmatch On 2015/03/10 20:50:10, storchaka wrote: > ...
2 years, 9 months ago #12
storchaka_gmail.com
https://bugs.python.org/review/22928/diff/14155/Lib/http/client.py File Lib/http/client.py (right): https://bugs.python.org/review/22928/diff/14155/Lib/http/client.py#newcode138 Lib/http/client.py:138: _is_legal_header_name = re.compile(rb'[^\s:]+').fullmatch I think spaces in the middle ...
2 years, 9 months ago #13
demian
http://bugs.python.org/review/22928/diff/14155/Lib/test/test_httplib.py File Lib/test/test_httplib.py (right): http://bugs.python.org/review/22928/diff/14155/Lib/test/test_httplib.py#newcode182 Lib/test/test_httplib.py:182: conn.putheader('LatinHeader', b'\xFF') On 2015/03/11 07:12:03, storchaka wrote: > Could ...
2 years, 9 months ago #14
demian
http://bugs.python.org/review/22928/diff/14166/Lib/http/client.py File Lib/http/client.py (right): http://bugs.python.org/review/22928/diff/14166/Lib/http/client.py#newcode139 Lib/http/client.py:139: _is_illegal_header_value = re.compile(rb'\r\n([^ \t]|\Z)').search I /slightly/ relaxed the constraint ...
2 years, 9 months ago #15
storchaka_gmail.com
http://bugs.python.org/review/22928/diff/14166/Lib/http/client.py File Lib/http/client.py (right): http://bugs.python.org/review/22928/diff/14166/Lib/http/client.py#newcode138 Lib/http/client.py:138: _is_illegal_header_name = re.compile(rb'^\s|[:\r\n]').search I'll tweak this pattern for speed. ...
2 years, 9 months ago #16
storchaka_gmail.com
http://bugs.python.org/review/22928/diff/14166/Lib/http/client.py File Lib/http/client.py (right): http://bugs.python.org/review/22928/diff/14166/Lib/http/client.py#newcode139 Lib/http/client.py:139: _is_illegal_header_value = re.compile(rb'\r\n([^ \t]|\Z)').search On 2015/03/12 07:54:34, storchaka wrote: ...
2 years, 9 months ago #17
storchaka_gmail.com
2 years, 9 months ago #18
http://bugs.python.org/review/22928/diff/14166/Lib/http/client.py
File Lib/http/client.py (right):

http://bugs.python.org/review/22928/diff/14166/Lib/http/client.py#newcode139
Lib/http/client.py:139: _is_illegal_header_value = re.compile(rb'\r\n([^
\t]|\Z)').search
No, the previous pattern was not correct. I think following pattern is good
compromise:

_is_illegal_header_value = re.compile(rb'\n(?![ \t])|\r(?![ \t\n])').search
Sign in to reply to this message.

RSS Feeds Recent Issues | This issue
This is Rietveld 894c83f36cb7