[issue22928] HTTP header injection in urrlib2/urllib/httplib/http.client (CVE-2016-5699)

[Ashwin Ramaswami]

Change by Ashwin Ramaswami :


--
pull_requests: +15023
pull_request: https://github.com/python/cpython/pull/15299

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue22928] HTTP header injection in urrlib2/urllib/httplib/http.client (CVE-2016-5699)

[Ned Deily]

Changes by Ned Deily :


--
assignee: georg.brandl -> 
priority: release blocker -> 
resolution:  -> fixed
stage: backport needed -> resolved
status: open -> closed

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue22928] HTTP header injection in urrlib2/urllib/httplib/http.client (CVE-2016-5699)

[Ned Deily]

Ned Deily added the comment:


New changeset 8e88f6b5e2a35ee458c161aa3f2b7f1f17fb45d1 by Ned Deily (Serhiy 
Storchaka) in branch '3.3':
[3.3] bpo-22928: Disabled HTTP header injections in http.client. (#2817)
https://github.com/python/cpython/commit/8e88f6b5e2a35ee458c161aa3f2b7f1f17fb45d1


--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue22928] HTTP header injection in urrlib2/urllib/httplib/http.client (CVE-2016-5699)

[Serhiy Storchaka]

Serhiy Storchaka added the comment:

\A is not needed. match() always matches from the start.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue22928] HTTP header injection in urrlib2/urllib/httplib/http.client (CVE-2016-5699)

[STINNER Victor]

STINNER Victor added the comment:

> What is the difference between PR 2817 and PR 2861?

Oh crap, I didn't know that you already created a PR. I compared the two PR:

* My PR adds \A at the start of:
   _is_legal_header_name = re.compile(rb'\A[^:\s][^:\r\n]*\Z').match 
* My PR uses blurb, yours modifies Misc/NEWS

The \A was copied from the Python 2.7 commit, since Python 3.3 doesn't have 
fullmatch().

If you are ok to add \A and convert the NEWS entry to blurb, I can abandon my 
duplicated PR.

--
nosy: +haypo

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue22928] HTTP header injection in urrlib2/urllib/httplib/http.client (CVE-2016-5699)

[Serhiy Storchaka]

Serhiy Storchaka added the comment:

What is the difference between PR 2817 and PR 2861?

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue22928] HTTP header injection in urrlib2/urllib/httplib/http.client (CVE-2016-5699)

[STINNER Victor]

Changes by STINNER Victor :


--
pull_requests: +2912

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue22928] HTTP header injection in urrlib2/urllib/httplib/http.client (CVE-2016-5699)

[Serhiy Storchaka]

Changes by Serhiy Storchaka :


--
nosy: +benjamin.peterson, larry
priority: normal -> release blocker

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue22928] HTTP header injection in urrlib2/urllib/httplib/http.client (CVE-2016-5699)

[Serhiy Storchaka]

Changes by Serhiy Storchaka :


--
pull_requests: +2870

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue22928] HTTP header injection in urrlib2/urllib/httplib/http.client (CVE-2016-5699)

[Kubilay Kocak]

Changes by Kubilay Kocak :


--
stage: resolved -> backport needed

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue22928] HTTP header injection in urrlib2/urllib/httplib/http.client (CVE-2016-5699)

[Ned Deily]

Ned Deily added the comment:

Getting to be the last chance to backport this for 3.3.x.

--
nosy: +ned.deily

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue22928] HTTP header injection in urrlib2/urllib/httplib/http.client (CVE-2016-5699)

[Serhiy Storchaka]

Changes by Serhiy Storchaka :


--
assignee: serhiy.storchaka -> georg.brandl

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue22928] HTTP header injection in urrlib2/urllib/httplib/http.client (CVE-2016-5699)

[koobs]

Changes by koobs :


--
versions: +Python 3.3

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue22928] HTTP header injection in urrlib2/urllib/httplib/http.client (CVE-2016-5699)

[koobs]

koobs added the comment:

3.3 is supported for security related fixes until September 2017 [1], but only 
3.4, 3.5 and 2.7 have received the backport, reopen for outstanding merge

[1] https://docs.python.org/devguide/#status-of-python-branches

Update summary to reflect the RedHat CVE that was assigned to this issue.

--
keywords: +security_issue
nosy: +koobs
resolution: fixed -> 
status: closed -> open
title: HTTP header injection in urrlib2/urllib/httplib/http.client -> HTTP 
header injection in urrlib2/urllib/httplib/http.client (CVE-2016-5699)

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue22928] HTTP header injection in urrlib2/urllib/httplib/http.client

[Serhiy Storchaka]

Changes by Serhiy Storchaka :


--
nosy: +georg.brandl

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue22928] HTTP header injection in urrlib2/urllib/httplib/http.client

[Vlad K.]

Vlad K. added the comment:

Doesn't this affect Python 3.3 as well, which is in security-only mode? 
Shouldn't that version be patched as well?

--
nosy: +vladk

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue22928] HTTP header injection in urrlib2/urllib/httplib/http.client

[Arfrever Frehtes Taifersar Arahesis]

Changes by Arfrever Frehtes Taifersar Arahesis arfrever@gmail.com:


--
nosy: +Arfrever

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue22928
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue22928] HTTP header injection in urrlib2/urllib/httplib/http.client

[Serhiy Storchaka]

Serhiy Storchaka added the comment:

Added new tests and tweaked regexpes. Thank you for your contribution Demian.

Now we can start long-standing deprecation process for conforming to RFC.

--
resolution:  - fixed
stage: commit review - resolved
status: open - closed
versions: +Python 2.7, Python 3.4

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue22928
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue22928] HTTP header injection in urrlib2/urllib/httplib/http.client

[Roundup Robot]

Roundup Robot added the comment:

New changeset 1c45047c5102 by Serhiy Storchaka in branch '2.7':
Issue #22928: Disabled HTTP header injections in httplib.
https://hg.python.org/cpython/rev/1c45047c5102

New changeset bf3e1c9b80e9 by Serhiy Storchaka in branch '3.4':
Issue #22928: Disabled HTTP header injections in http.client.
https://hg.python.org/cpython/rev/bf3e1c9b80e9

New changeset aa4c6992c126 by Serhiy Storchaka in branch 'default':
Issue #22928: Disabled HTTP header injections in http.client.
https://hg.python.org/cpython/rev/aa4c6992c126

--
nosy: +python-dev

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue22928
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue22928] HTTP header injection in urrlib2/urllib/httplib/http.client

[Demian Brecht]

Demian Brecht added the comment:

Thanks for the tweaks Serhiy, those seem reasonable to me.

--

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue22928
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue22928] HTTP header injection in urrlib2/urllib/httplib/http.client

[Serhiy Storchaka]

Serhiy Storchaka added the comment:

I'll return previous implementation of header value regex. All other LGTM.

--
stage: patch review - commit review

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue22928
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue22928] HTTP header injection in urrlib2/urllib/httplib/http.client

[Demian Brecht]

Changes by Demian Brecht demianbre...@gmail.com:


Added file: http://bugs.python.org/file38449/issue22928_6.patch

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue22928
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue22928] HTTP header injection in urrlib2/urllib/httplib/http.client

[Martin Panter]

Martin Panter added the comment:

I much prefer the new patch with better compatibility and flexibility :)

If you want to strengthen the tests to reflect some of the decisions made here 
you could add the following tests:

Positive tests:
* putheader('C1-Control', b'next\x85line')
* putheader('Embedded-Fold', 'is\r\n\tallowed')

Negative tests:
* putheader(' Leading-Space', 'value')
* putheader('Embedded: colon', 'extra value')

--

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue22928
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue22928] HTTP header injection in urrlib2/urllib/httplib/http.client

[Demian Brecht]

Demian Brecht added the comment:

Latest patch should now address all comments.

--
Added file: http://bugs.python.org/file38433/issue22928_5.patch

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue22928
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue22928] HTTP header injection in urrlib2/urllib/httplib/http.client

[Serhiy Storchaka]

Serhiy Storchaka added the comment:

May be not drop folding support, but just deprecate the putheader() 
multi-argument mode? And of course add sanity checks for separate putheader() 
arguments.

--

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue22928
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue22928] HTTP header injection in urrlib2/urllib/httplib/http.client

[Demian Brecht]

Demian Brecht added the comment:

After a chat with David and getting my head wrapped more around backwards 
compatibility, I also agree that the changes in the patch are far too strict. 
It's much more important to preserve backwards compatibility than to strictly 
conform to the RFC.

I've updated the patch to allow for (what should be) anything that was 
previously allowed as header name/value pairs minus carriage returns not 
immediately followed by a tab or space (obs-fold: CRLF 1*( SP / HTAB )). This 
change fixes the reported issue but should not otherwise affect backwards 
compatibility.

Additionally, even though line folding is deprecated by RFC 7230, I don't think 
it's necessarily an issue to support line folding until proven to be a problem 
in practice. With the current implementation, users have the ability to conform 
to the target server/proxy requirements, based on errors (if obs-fold isn't 
transparently dealt with as suggested) yielded by each as defined in the RFC. 
In light of that, I don't think that it's even worthwhile to start deprecating 
multi-parameter putheader at this point (but I'm open to argument on that one).

One note on the deprecation is that if we deprecate multi-parameter, we should 
also add a warning if an embedded line fold is detected in a single headers 
value.

--
Added file: http://bugs.python.org/file38399/issue22928_4.patch

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue22928
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue22928] HTTP header injection in urrlib2/urllib/httplib/http.client

[Serhiy Storchaka]

Serhiy Storchaka added the comment:

Added comments on Rietveld.

Is there a limit to the length of header line? Would not unfolding all header 
values exceed the limit?

--
assignee:  - serhiy.storchaka
nosy: +serhiy.storchaka

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue22928
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue22928] HTTP header injection in urrlib2/urllib/httplib/http.client

[Martin Panter]

Martin Panter added the comment:

Folded header fields are deprecated as of RFC 7230; see 
https://tools.ietf.org/html/rfc7230#section-3.2.4. The only reasons to fold 
them I can think of is for readability (debugging), when generating a 
messsage/http MIME message (which I don’t think the Python library supports), 
or maybe when dealing with a strange server limitation. Normally there is not 
meant to be a limit for lines in the HTTP header, although it is common to 
limit the total unfolded header field value.

If we go ahead and drop folding support, perhaps we should deprecate the 
putheader() multi-argument mode, rather than just document the arguments are 
now joined by spaces. It seems a pointless API now with this change.

--

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue22928
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue22928] HTTP header injection in urrlib2/urllib/httplib/http.client

[Berker Peksag]

Changes by Berker Peksag berker.pek...@gmail.com:


--
nosy: +berker.peksag
stage:  - patch review
versions:  -Python 3.6

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue22928
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue22928] HTTP header injection in urrlib2/urllib/httplib/http.client

[Demian Brecht]

Changes by Demian Brecht demianbre...@gmail.com:


Added file: http://bugs.python.org/file38190/issue22928_3.patch

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue22928
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue22928] HTTP header injection in urrlib2/urllib/httplib/http.client

[Demian Brecht]

Demian Brecht added the comment:

I’ve updated the patch to include the latin-1 charset in legal header values. 
It still uses a space as delimiter, but all other comments should now be 
addressed.

--
Added file: http://bugs.python.org/file38158/issue22928_2.patch

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue22928
___diff -r d12c7938c4b0 Doc/library/http.client.rst
--- a/Doc/library/http.client.rst   Mon Feb 16 13:33:32 2015 +0200
+++ b/Doc/library/http.client.rst   Tue Feb 17 07:55:09 2015 -0800
@@ -292,10 +292,10 @@
 
 .. method:: HTTPConnection.putheader(header, argument[, ...])
 
-   Send an :rfc:`822`\ -style header to the server.  It sends a line to the 
server
-   consisting of the header, a colon and a space, and the first argument.  If 
more
-   arguments are given, continuation lines are sent, each consisting of a tab 
and
-   an argument.
+   Send an :rfc:`7230`\ -style header to the server.  It sends a line to the 
server
+   consisting of the header, a colon and a space, and the first argument. If
+   more arguments are given, they are appended to the header value, each
+   prepended with a single space.
 
 
 .. method:: HTTPConnection.endheaders(message_body=None)
diff -r d12c7938c4b0 Lib/http/client.py
--- a/Lib/http/client.pyMon Feb 16 13:33:32 2015 +0200
+++ b/Lib/http/client.pyTue Feb 17 07:55:09 2015 -0800
@@ -71,6 +71,7 @@
 import http
 import io
 import os
+import re
 import socket
 import collections
 from urllib.parse import urlsplit
@@ -87,6 +88,7 @@
 
 _UNKNOWN = 'UNKNOWN'
 
+
 # connection states
 _CS_IDLE = 'Idle'
 _CS_REQ_STARTED = 'Request-started'
@@ -107,6 +109,36 @@
 _MAXLINE = 65536
 _MAXHEADERS = 100
 
+# Header name/value ABNF (http://tools.ietf.org/html/rfc7230#section-3.2)
+#
+# VCHAR  = %x21-7E
+# obs-text   = %x80-FF
+# header-field   = field-name : OWS field-value OWS
+# field-name = token
+# field-value= *( field-content / obs-fold )
+# field-content  = field-vchar [ 1*( SP / HTAB ) field-vchar ]
+# field-vchar= VCHAR / obs-text
+#
+# obs-fold   = CRLF 1*( SP / HTAB )
+#; obsolete line folding
+#; see Section 3.2.4
+
+# token  = 1*tchar
+#
+# tchar  = ! / # / $ / % /  / ' / *
+#/ + / - / . / ^ / _ / ` / | / ~
+#/ DIGIT / ALPHA
+#; any VCHAR, except delimiters
+#
+# VCHAR defined in http://tools.ietf.org/html/rfc5234#appendix-B.1
+
+_HEADER_LEGAL_NAME = re.compile(b'^[!#$%\'*+-.^_`|~a-zA-z0-9]+$')
+# 0x20 (sp) is included in the valid character range for backwards
+# compatibility, where header values with spaces (i.e. auth headers) are passed
+# through to putheader as single values. latin-1 charset is also legal as ASCII
+# is only suggested in RFC 7230
+_HEADER_LEGAL_VALUE = re.compile(b'^\s*[\x20-\x7E\xA0-\xFF]*\s*$')
+
 
 class HTTPMessage(email.message.Message):
 # XXX The only usage of this method is in
@@ -1000,13 +1032,35 @@
 
 if hasattr(header, 'encode'):
 header = header.encode('ascii')
+
+if not _HEADER_LEGAL_NAME.match(header):
+raise ValueError('Invalid header name {!r}'.format(header))
+
 values = list(values)
 for i, one_value in enumerate(values):
 if hasattr(one_value, 'encode'):
-values[i] = one_value.encode('latin-1')
+encoded_value = one_value.encode('latin-1')
 elif isinstance(one_value, int):
-values[i] = str(one_value).encode('ascii')
-value = b'\r\n\t'.join(values)
+encoded_value = str(one_value).encode('ascii')
+else:
+encoded_value = one_value
+
+# Newly defined header fields SHOULD limit their field values to
+# US-ASCII octets. A recipient SHOULD treat other octets in field
+# content (obs-text) as opaque data.
+if not _HEADER_LEGAL_VALUE.match(encoded_value):
+raise ValueError(
+'Invalid header value {}'.format(encoded_value))
+
+values[i] = encoded_value
+
+# http://tools.ietf.org/html/rfc7230#section-3.2.4 states that line
+# folding is obsolete, unless message/http MIME type is used and rules
+# are conformed to. otherwise, spaces should be used. it might be a
+# good idea to put validation for this rule in sometime in the future.
+# as it currently stands, there's no way to determine the MIME type of
+# the message at this point.
+value = b' '.join(values)
 header = header + b': ' + value
 self._output(header)
 
diff -r d12c7938c4b0 Lib/test/test_httplib.py
--- a/Lib/test/test_httplib.py  Mon Feb 16 13:33:32 2015 +0200
+++ b/Lib/test/test_httplib.py  Tue Feb 17 07:55:09 2015 -0800
@@ -171,6 +171,17 @@
 

[issue22928] HTTP header injection in urrlib2/urllib/httplib/http.client

[Demian Brecht]

Demian Brecht added the comment:

 Maybe join them with tabs rather than spaces then, since it was previously 
 \r\n\t. This way it is even closer to before.

After thinking about this a little more, I think I'd prefer to keep
spaces rather than tabs. The reason being is that, in my mind, now that
continuations have been made obsolete it's more natural to do something
like this:

putheader('Authorization', 'Bearer', 'my_token')

than

putheader('Authorization', 'Bearer my_token')

I realize it's a semantic change from previous behavior, but it seems to
me to be preferable given the latest RFCs. I'd think that at some point
in the future, we'd want to remove \x20 from the valid header value
range to entirely conform to the spec. This is the first step in
allowing for graceful deprecation.

--
Added file: http://bugs.python.org/file38154/issue22928_1.patch

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue22928
___diff -r d12c7938c4b0 Doc/library/http.client.rst
--- a/Doc/library/http.client.rst   Mon Feb 16 13:33:32 2015 +0200
+++ b/Doc/library/http.client.rst   Mon Feb 16 09:42:20 2015 -0800
@@ -292,10 +292,10 @@
 
 .. method:: HTTPConnection.putheader(header, argument[, ...])
 
-   Send an :rfc:`822`\ -style header to the server.  It sends a line to the 
server
-   consisting of the header, a colon and a space, and the first argument.  If 
more
-   arguments are given, continuation lines are sent, each consisting of a tab 
and
-   an argument.
+   Send an :rfc:`7230`\ -style header to the server.  It sends a line to the 
server
+   consisting of the header, a colon and a space, and the first argument. If
+   more arguments are given, they are appended to the header value, each
+   prepended with a single space.
 
 
 .. method:: HTTPConnection.endheaders(message_body=None)
diff -r d12c7938c4b0 Lib/http/client.py
--- a/Lib/http/client.pyMon Feb 16 13:33:32 2015 +0200
+++ b/Lib/http/client.pyMon Feb 16 09:42:20 2015 -0800
@@ -71,6 +71,7 @@
 import http
 import io
 import os
+import re
 import socket
 import collections
 from urllib.parse import urlsplit
@@ -87,6 +88,7 @@
 
 _UNKNOWN = 'UNKNOWN'
 
+
 # connection states
 _CS_IDLE = 'Idle'
 _CS_REQ_STARTED = 'Request-started'
@@ -107,6 +109,35 @@
 _MAXLINE = 65536
 _MAXHEADERS = 100
 
+# Header name/value ABNF (http://tools.ietf.org/html/rfc7230#section-3.2)
+#
+# VCHAR  = %x21-7E
+# obs-text   = %x80-FF
+# header-field   = field-name : OWS field-value OWS
+# field-name = token
+# field-value= *( field-content / obs-fold )
+# field-content  = field-vchar [ 1*( SP / HTAB ) field-vchar ]
+# field-vchar= VCHAR / obs-text
+#
+# obs-fold   = CRLF 1*( SP / HTAB )
+#; obsolete line folding
+#; see Section 3.2.4
+
+# token  = 1*tchar
+#
+# tchar  = ! / # / $ / % /  / ' / *
+#/ + / - / . / ^ / _ / ` / | / ~
+#/ DIGIT / ALPHA
+#; any VCHAR, except delimiters
+#
+# VCHAR defined in http://tools.ietf.org/html/rfc5234#appendix-B.1
+
+_HEADER_LEGAL_NAME = re.compile(b'^[!#$%\'*+-.^_`|~a-zA-z0-9]+$')
+# 0x20 (sp) is included in the valid character range for backwards
+# compatibility, where header values with spaces (i.e. auth headers) are passed
+# through to putheader as single values
+_HEADER_LEGAL_VALUE = re.compile(b'^\s*[\x20-\x7E]*\s*$')
+
 
 class HTTPMessage(email.message.Message):
 # XXX The only usage of this method is in
@@ -1000,13 +1031,32 @@
 
 if hasattr(header, 'encode'):
 header = header.encode('ascii')
+
+if not _HEADER_LEGAL_NAME.match(header):
+raise ValueError('Invalid header name {}'.format(header))
+
 values = list(values)
 for i, one_value in enumerate(values):
 if hasattr(one_value, 'encode'):
-values[i] = one_value.encode('latin-1')
+encoded_value = one_value.encode('latin-1')
 elif isinstance(one_value, int):
-values[i] = str(one_value).encode('ascii')
-value = b'\r\n\t'.join(values)
+encoded_value = str(one_value).encode('ascii')
+else:
+encoded_value = one_value
+
+if not _HEADER_LEGAL_VALUE.match(encoded_value):
+raise ValueError(
+'Invalid header value {}'.format(encoded_value))
+
+values[i] = encoded_value
+
+# http://tools.ietf.org/html/rfc7230#section-3.2.4 states that line
+# folding is obsolete, unless message/http MIME type is used and rules
+# are conformed to. otherwise, spaces should be used. it might be a
+# good idea to put validation for this rule in sometime in the future.
+# as it currently stands, there's no way to determine the MIME type of
+# the message at this point.
+value = b' 

[issue22928] HTTP header injection in urrlib2/urllib/httplib/http.client

[Demian Brecht]

Changes by Demian Brecht demianbre...@gmail.com:


--
versions:  -Python 2.7, Python 3.2, Python 3.3, Python 3.4

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue22928
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue22928] HTTP header injection in urrlib2/urllib/httplib/http.client

[Martin Panter]

Martin Panter added the comment:

But it is not natural to do things like this (based on headers sent by Firefox):

putheader(User-Agent, Mozilla/5.0, (X11;, Linux, x86_64;, rv:25.0), 
Gecko/20100101, Firefox/25.0)
putheader(Accept-Encoding, gzip,, deflate)

A way to properly encode different kinds of header values would be nice, but I 
don’t think the current putheader() API is it.

Also, I still think it would be good to allow non-ASCII characters in header 
values. At least when an already-encoded byte string is supplied. For instance, 
RTSP is based on HTTP but uses UTF-8 as a default encoding, rather than HTTP’s 
Latin-1. Otherwise, retaining the one_value.encode('latin-1') call is confusing 
when later on it rejects non-ASCII-encoded characters.

--

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue22928
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue22928] HTTP header injection in urrlib2/urllib/httplib/http.client

[Demian Brecht]

Demian Brecht added the comment:

 But it is not natural to do things like this (based on headers sent by 
 Firefox)

Good point.

 Otherwise, retaining the one_value.encode('latin-1') call is confusing when 
 later on it rejects non-ASCII-encoded characters.

I’m a little torn on this one given one of the SHOULD clauses in RFC 7230 about 
recipients treating headers with non-ASCII characters as opaque data. However, 
I’ve read a number of occasions where users are using latin-1 in practice (and 
it /is/ only a SHOULD clause), so I think it’s likely better to err on the side 
of caution and allow for the latin-1 charset at least.

As for utf-8 though, I think that once we start getting into the realm of other 
application protocols, that’s something that should have to be extended by the 
client implementation and not something that should be changed in the base HTTP 
implementation.

The odd part of the API though now is the fact that it’s variadic. I really 
have no strong opinion on whether elements should be tab or space delimited and 
the RFC doesn’t seem to lean either way. I think I’m still leaning towards 
space delimiting to give users the ability to write in either form 
(putheader(‘Authorization’, ‘Bearer’, ‘token’) or putheader(‘Authorization’, 
‘Bearer token’)). As another minor argument for it, it’s also likely a little 
nicer for logging. I think that optimally, the API would be a single value as 
you’d suggested, but I’d be concerned about the extent of backwards 
compatibility issues if that were to be done.

I’ll try to get some time tomorrow to make those changes, so it still leaves 
time for further debate :)

--

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue22928
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue22928] HTTP header injection in urrlib2/urllib/httplib/http.client

[Demian Brecht]

Demian Brecht added the comment:

Here's a patch addressing the potential vulnerability as reported. The patch 
should also bring the implementation up to date with the most recent standards 
around header names and values.

 There could be potential for breaking compatibility if people are 
 intentionally sending values with folded lines (obsoleted by the new HTTP 
 RFC).

I think I'm okay with this given line folding seems to have been implemented by 
passing multiple value parameters (folding was automatically taken care of by 
the library).

I don't think that this should be merged into anything pre 3.5 as safeguarding 
/should/ be accounted for by the developer, so I don't think I'd regard this as 
a high risk security issue. I'm definitely open to debate on that though.

--
Added file: http://bugs.python.org/file38133/issue22928.patch

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue22928
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue22928] HTTP header injection in urrlib2/urllib/httplib/http.client

[Martin Panter]

Martin Panter added the comment:

If we’re in the realm of 3.5 only changes, it might make sense to remove the 
multi-argument mode of putheader() altogether, and document it only generates a 
single line. (Currently still says it generates multiple lines.)

--

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue22928
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue22928] HTTP header injection in urrlib2/urllib/httplib/http.client

[Demian Brecht]

Demian Brecht added the comment:

I think that keeping the public API as-is is the better way to go, at
least for the shorter term given it won't require users to have to make
code changes. Thanks for the catch on the docs though, will update that.

--

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue22928
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue22928] HTTP header injection in urrlib2/urllib/httplib/http.client

[Martin Panter]

Martin Panter added the comment:

Good point. Maybe join them with tabs rather than spaces then, since it was 
previously \r\n\t. This way it is even closer to before.

--

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue22928
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue22928] HTTP header injection in urrlib2/urllib/httplib/http.client

[Martin Panter]

Martin Panter added the comment:

There could be potential for breaking compatibility if people are intentionally 
sending values with folded lines (obsoleted by the new HTTP RFC).

Perhaps the same error should be raised for values that cannot be encoded in 
Latin-1? Also, maybe most control characters should trigger an error, not just 
CR and LF. Apart from line folding, the HTTP RFC only allows visible ASCII 
characters, spaces and tabs, and obsolete non-ASCII chars = 0x80.

--
nosy: +vadmium

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue22928
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue22928] HTTP header injection in urrlib2/urllib/httplib/http.client

[Demian Brecht]

Changes by Demian Brecht demianbre...@gmail.com:


--
nosy: +demian.brecht

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue22928
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue22928] HTTP header injection in urrlib2/urllib/httplib/http.client

[R. David Murray]

Changes by R. David Murray rdmur...@bitdance.com:


--
nosy: +orsenthil, r.david.murray

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue22928
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue22928] HTTP header injection in urrlib2/urllib/httplib/http.client

[Guido Vranken]

New submission from Guido Vranken:

Proof of concept:

# Script for Python 2
import urllib2
opener = urllib2.build_opener()
opener.addheaders = [('User-agent', 'Mozilla/5.0' + chr(0x0A) + Location: 
header injection)]
response = opener.open(http://localhost:;)

# Data sent is:

GET / HTTP/1.1
Accept-Encoding: identity
Host: localhost:
Connection: close
User-Agent: Mozilla/5.0
Location: header injection



# End of script

# Python 3
from urllib.request import urlopen, build_opener
opener = build_opener()
opener.addheaders = [('User-agent', 'Mozilla/5.0' + chr(0x0A) + Location: 
header injection)]
opener.open(http://localhost:;)

# Data sent is:

GET / HTTP/1.1
Accept-Encoding: identity
Host: localhost:
Connection: close
User-Agent: Mozilla/5.0
Location: header injection



# End of script

It is the responsibility of the developer leveraging Python and its HTTP client 
libraries to ensure that their (web) application acts in accordance to official 
HTTP specifications and that no threats to security will arise from their code.
However, newlines inside headers are arguably a special case of breaking the 
conformity with RFC's in regard to the allowed character set. No illegal 
character used inside a HTTP header is likely to have a compromising side 
effect on back-end clients and servers and the integrity of their 
communication, as a result of the leniency of most web servers. However, a 
newline character (0x0A) embedded in a HTTP header invariably has the semantic 
consequence of denoting the start of an additional header line. To put it 
differently, not sanitizing headers in complete accordance to RFC's could be 
seen as as virtue in that it gives the programmer a maximum amount of freedom, 
without having to trade it for any likely or severe security ramifications, so 
that they may use illegal characters in testing environments and environments 
that are outlined by an expliticly less strict interpretation of the HTTP 
protocol. Newlines are special in that they enable anyone who is able to 
influence the header
  content, to, in effect, perform additional invocations to add_header().

In issue 17322 ( http://bugs.python.org/issue17322 ) there is some discussion 
as to the general compliance to RFC's by the HTTP client libraries. I'd like to 
opt to begin with prohibiting newline characters to be present in HTTP headers. 
Although this issue is not a hard vulnerability such as a buffer overflow, it 
does translate to a potentially equal level of severity when considered from 
the perspective of a web-enabled application, for which purpose the HTTP 
libraries are typically used for. Lack of input validation on the application 
developer's end will faciliate header injections, for example if user-supplied 
data will end up as cookie content verbatim.
Adding this proposed additional layer of validation inside Python minimizes the 
likelihood of a successful header injection while functionality is not notably 
affected.

I'm inclined to add this validation to putheader() in the 'http' module rather 
than in urllib, as this will secure all invocations to 'http' regardless of 
intermediate libraries such as urllib.

Included is a patch for the latest checkout of the default branch that will 
cause CannotSendHeader() to be raised if a newline character is detected in 
either a header name or its value. Aside from detecting \n, it also breaks on 
\r as their respective implications can be similar. Feel free to adjust, 
rewrite and transpose this to other branches where you feel this is appropriate.


Guido Vranken
Intelworks

--
components: Library (Lib)
files: disable_http_header_injection.patch
keywords: patch
messages: 231590
nosy: Guido
priority: normal
severity: normal
status: open
title: HTTP header injection in urrlib2/urllib/httplib/http.client
type: security
versions: Python 2.7, Python 3.2, Python 3.3, Python 3.4, Python 3.5, Python 3.6
Added file: http://bugs.python.org/file37264/disable_http_header_injection.patch

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue22928
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com