From bb8071a4cae5ab3fe321481dd3d73662ffb26052 Mon Sep 17 00:00:00 2001
From: Victor Stinner <victor.stinner@gmail.com>
Date: Tue, 21 May 2019 15:12:33 +0200
Subject: [PATCH] bpo-30458: Disallow control chars in http URLs (GH-12755)
 (GH-13154) (GH-13315)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Disallow control chars in http URLs in urllib2.urlopen.  This
addresses a potential security problem for applications that do not
sanity check their URLs where http request headers could be injected.

Disable https related urllib tests on a build without ssl (GH-13032)
These tests require an SSL enabled build. Skip these tests when
python is built without SSL to fix test failures.

Use httplib.InvalidURL instead of ValueError as the new error case's
exception. (GH-13044)

Backport Co-Authored-By: Miro Hrončok <miro@hroncok.cz>

(cherry picked from commit 7e200e0763f5b71c199aaf98bd5588f291585619)

Notes on backport to Python 2.7:

* test_urllib tests urllib.urlopen() which quotes the URL and so is
  not vulerable to HTTP Header Injection.
* Add tests to test_urllib2 on urllib2.urlopen().
* Reject non-ASCII characters: range 0x80-0xff.
---
 Lib/httplib.py                                                     |   19 +++
 Lib/test/test_urllib.py                                            |   25 ++++
 Lib/test/test_urllib2.py                                           |   51 +++++++++-
 Lib/test/test_xmlrpc.py                                            |   13 ++
 Misc/NEWS.d/next/Security/2019-04-10-08-53-30.bpo-30458.51E-DA.rst |    1 
 5 files changed, 106 insertions(+), 3 deletions(-)
 create mode 100644 Misc/NEWS.d/next/Security/2019-04-10-08-53-30.bpo-30458.51E-DA.rst

--- a/Lib/httplib.py
+++ b/Lib/httplib.py
@@ -242,7 +242,16 @@ _MAXHEADERS = 100
 # definitions to allow for backwards compatibility
 _is_legal_header_name = re.compile(r'\A[^:\s][^:\r\n]*\Z').match
 _is_illegal_header_value = re.compile(r'\n(?![ \t])|\r(?![ \t\n])').search
+
+# These characters are not allowed within HTTP URL paths.
+#  See https://tools.ietf.org/html/rfc3986#section-3.3 and the
+#  https://tools.ietf.org/html/rfc3986#appendix-A pchar definition.
+# Prevents CVE-2019-9740.  Includes control characters such as \r\n.
+# Restrict non-ASCII characters above \x7f (0x80-0xff).
 _contains_disallowed_url_pchar_re = re.compile('[\x00-\x20\x7f-\xff]')
+# Arguably only these _should_ allowed:
+#  _is_allowed_url_pchars_re = re.compile(r"^[/!$&'()*+,;=:@%a-zA-Z0-9._~-]+$")
+# We are more lenient for assumed real world compatibility purposes.
 
 # These characters are not allowed within HTTP method names
 # to prevent http header injection.
@@ -880,9 +889,15 @@ class HTTPConnection:
         self._method = method
         if not url:
             url = '/'
-        str = '%s %s %s' % (method, url, self._http_vsn_str)
+        # Prevent CVE-2019-9740.
+        match = _contains_disallowed_url_pchar_re.search(url)
+        if match:
+            raise InvalidURL("URL can't contain control characters. %r "
+                             "(found at least %r)"
+                             % (url, match.group()))
+        hdr = '%s %s %s' % (method, url, self._http_vsn_str)
 
-        self._output(str)
+        self._output(hdr)
 
         if self._http_vsn == 11:
             # Issue some standard headers for better HTTP/1.1 compliance
--- a/Lib/test/test_urllib.py
+++ b/Lib/test/test_urllib.py
@@ -162,6 +162,31 @@ class urlopen_HttpTests(unittest.TestCas
         finally:
             self.unfakehttp()
 
+    def test_url_with_control_char_rejected(self):
+        for char_no in range(0, 0x21) + range(0x7f, 0x100):
+            char = chr(char_no)
+            schemeless_url = "//localhost:7777/test%s/" % char
+            self.fakehttp(b"HTTP/1.1 200 OK\r\n\r\nHello.")
+            try:
+                # urllib quotes the URL so there is no injection.
+                resp = urllib.urlopen("http:" + schemeless_url)
+                self.assert_(char not in resp.geturl())
+            finally:
+                self.unfakehttp()
+
+    def test_url_with_newline_header_injection_rejected(self):
+        self.fakehttp(b"HTTP/1.1 200 OK\r\n\r\nHello.")
+        host = "localhost:7777?a=1 HTTP/1.1\r\nX-injected: header\r\nTEST: 123"
+        schemeless_url = "//" + host + ":8080/test/?test=a"
+        try:
+            # urllib quotes the URL so there is no injection.
+            resp = urllib.urlopen("http:" + schemeless_url)
+            self.assert_(' ' not in resp.geturl())
+            self.assert_('\r' not in resp.geturl())
+            self.assert_('\n' not in resp.geturl())
+        finally:
+            self.unfakehttp()
+
     def test_read_bogus(self):
         # urlopen() should raise IOError for many error codes.
         self.fakehttp('''HTTP/1.1 401 Authentication Required
--- a/Lib/test/test_urllib2.py
+++ b/Lib/test/test_urllib2.py
@@ -8,6 +8,9 @@ import StringIO
 import urllib2
 from urllib2 import Request, OpenerDirector
 
+from test.test_urllib import FakeHTTPMixin
+
+
 # XXX
 # Request
 # CacheFTPHandler (hard to write)
@@ -1170,7 +1173,7 @@ class HandlerTests(unittest.TestCase):
         self.assertEqual(len(http_handler.requests), 1)
         self.assertFalse(http_handler.requests[0].has_header(auth_header))
 
-class MiscTests(unittest.TestCase):
+class MiscTests(unittest.TestCase, FakeHTTPMixin):
 
     def test_build_opener(self):
         class MyHTTPHandler(urllib2.HTTPHandler): pass
@@ -1261,6 +1264,52 @@ class RequestTests(unittest.TestCase):
         self.assertEqual("www.python.org", self.get.get_origin_req_host())
         self.assertEqual("www.perl.org", self.get.get_host())
 
+    @unittest.skipUnless(ssl, "ssl module required")
+    def test_url_with_control_char_rejected(self):
+        for char_no in range(0, 0x21) + range(0x7f, 0x100):
+            char = chr(char_no)
+            schemeless_url = "//localhost:7777/test%s/" % char
+            self.fakehttp(b"HTTP/1.1 200 OK\r\n\r\nHello.")
+            try:
+                # We explicitly test urllib.request.urlopen() instead of the top
+                # level 'def urlopen()' function defined in this... (quite ugly)
+                # test suite.  They use different url opening codepaths.  Plain
+                # urlopen uses FancyURLOpener which goes via a codepath that
+                # calls urllib.parse.quote() on the URL which makes all of the
+                # above attempts at injection within the url _path_ safe.
+                escaped_char_repr = repr(char).replace('\\', r'\\')
+                InvalidURL = httplib.InvalidURL
+                self.assertRaises(
+                    InvalidURL,
+                    urllib2.urlopen, "http:" + schemeless_url)
+                self.assertRaises(
+                    InvalidURL,
+                    urllib2.urlopen, "https:" + schemeless_url)
+            finally:
+                self.unfakehttp()
+
+    @unittest.skipUnless(ssl, "ssl module required")
+    def test_url_with_newline_header_injection_rejected(self):
+        self.fakehttp(b"HTTP/1.1 200 OK\r\n\r\nHello.")
+        host = "localhost:7777?a=1 HTTP/1.1\r\nX-injected: header\r\nTEST: 123"
+        schemeless_url = "//" + host + ":8080/test/?test=a"
+        try:
+            # We explicitly test urllib2.urlopen() instead of the top
+            # level 'def urlopen()' function defined in this... (quite ugly)
+            # test suite.  They use different url opening codepaths.  Plain
+            # urlopen uses FancyURLOpener which goes via a codepath that
+            # calls urllib.parse.quote() on the URL which makes all of the
+            # above attempts at injection within the url _path_ safe.
+            InvalidURL = httplib.InvalidURL
+            self.assertRaises(
+                InvalidURL,
+                urllib2.urlopen, "http:" + schemeless_url)
+            self.assertRaises(InvalidURL,
+                urllib2.urlopen, "https:" + schemeless_url)
+        finally:
+            self.unfakehttp()
+
+
 
 def test_main(verbose=None):
     from test import test_urllib2
--- a/Lib/test/test_xmlrpc.py
+++ b/Lib/test/test_xmlrpc.py
@@ -492,6 +492,19 @@ class SimpleServerTestCase(unittest.Test
         # This avoids waiting for the socket timeout.
         self.test_simple1()
 
+    def test_partial_post(self):
+        # Check that a partial POST doesn't make the server loop: issue #14001.
+        conn = httplib.HTTPConnection('localhost', PORT)
+        conn.send('POST /RPC2 HTTP/1.0\r\n'
+                  'Content-Length: 100\r\n\r\n'
+                  'bye HTTP/1.1\r\n'
+                  'Host: %s:%s\r\n'
+                  'Accept-Encoding: identity\r\n'
+                  'Content-Length: 0\r\n\r\n'
+                  % ('localhost', PORT))
+        conn.close()
+
+
 # This is a contrived way to make a failure occur on the server side
 # in order to test the _send_traceback_header flag on the server
 class FailingMessageClass(mimetools.Message):
--- /dev/null
+++ b/Misc/NEWS.d/next/Security/2019-04-10-08-53-30.bpo-30458.51E-DA.rst
@@ -0,0 +1 @@
+Address CVE-2019-9740 by disallowing URL paths with embedded whitespace or control characters through into the underlying http client request.  Such potentially malicious header injection URLs now cause an httplib.InvalidURL exception to be raised.
