python · vstinner · May 21, 2019 · May 7, 2019 · May 20, 2019 · gpshead
diff --git a/Lib/httplib.py b/Lib/httplib.py
@@ -247,6 +247,16 @@
 _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.
+
 # We always set the Content-Length header for these methods because some
 # servers will otherwise respond with a 411
 _METHODS_EXPECTING_BODY = {'PATCH', 'POST', 'PUT'}
@@ -927,6 +937,12 @@ def putrequest(self, method, url, skip_host=0, skip_accept_encoding=0):
         self._method = method
         if not url:
             url = '/'
+        # 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(hdr)

diff --git a/Lib/test/test_urllib.py b/Lib/test/test_urllib.py
@@ -9,6 +9,10 @@
 import sys
 import mimetools
 import tempfile
+try:
+    import ssl
+except ImportError:
+    ssl = None
 
 from test import test_support
 from base64 import b64encode
@@ -257,6 +261,33 @@ def test_url_fragment(self):
         finally:
             self.unfakehttp()
 
+    @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:
+                # urllib quotes the URL so there is no injection.
+                resp = urllib.urlopen("http:" + schemeless_url)
+                self.assertNotIn(char, resp.geturl())
+            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:
+            # urllib quotes the URL so there is no injection.
+            resp = urllib.urlopen("http:" + schemeless_url)
+            self.assertNotIn(' ', resp.geturl())
+            self.assertNotIn('\r', resp.geturl())
+            self.assertNotIn('\n', 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

diff --git a/Lib/test/test_urllib2.py b/Lib/test/test_urllib2.py
@@ -1,5 +1,5 @@
 import unittest
-from test import test_support
+from test import support
 from test import test_urllib
 
 import os
@@ -15,6 +15,9 @@
 except ImportError:
     ssl = None
 
+from test.test_urllib import FakeHTTPMixin
+
+
 # XXX
 # Request
 # CacheFTPHandler (hard to write)
@@ -683,7 +686,7 @@ def test_file(self):
         h = urllib2.FileHandler()
         o = h.parent = MockOpener()
 
-        TESTFN = test_support.TESTFN
+        TESTFN = support.TESTFN
         urlpath = sanepathname2url(os.path.abspath(TESTFN))
         towrite = "hello, world\n"
         urls = [
@@ -1154,7 +1157,7 @@ def test_basic_auth_with_unquoted_realm(self):
         opener.add_handler(auth_handler)
         opener.add_handler(http_handler)
         msg = "Basic Auth Realm was unquoted"
-        with test_support.check_warnings((msg, UserWarning)):
+        with support.check_warnings((msg, UserWarning)):
             self._test_basic_auth(opener, auth_handler, "Authorization",
                                   realm, http_handler, password_manager,
                                   "http://acme.example.com/protected",
@@ -1262,7 +1265,7 @@ def _test_basic_auth(self, opener, auth_handler, auth_header,
         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
@@ -1317,6 +1320,52 @@ def test_unsupported_algorithm(self):
             "Unsupported digest authentication algorithm 'invalid'"
         )
 
+    @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
+                with self.assertRaisesRegexp(
+                    InvalidURL, "contain control.*" + escaped_char_repr):
+                    urllib2.urlopen("http:" + schemeless_url)
+                with self.assertRaisesRegexp(
+                    InvalidURL, "contain control.*" + escaped_char_repr):
+                    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 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.
+            InvalidURL = httplib.InvalidURL
+            with self.assertRaisesRegexp(
+                InvalidURL, r"contain control.*\\r.*(found at least . .)"):
+                urllib2.urlopen("http:" + schemeless_url)
+            with self.assertRaisesRegexp(InvalidURL, r"contain control.*\\n"):
+                urllib2.urlopen("https:" + schemeless_url)
+        finally:
+            self.unfakehttp()
+
+
 
 class RequestTests(unittest.TestCase):
 
@@ -1412,14 +1461,14 @@ def test_HTTPError_interface_call(self):
 
 def test_main(verbose=None):
     from test import test_urllib2
-    test_support.run_doctest(test_urllib2, verbose)
-    test_support.run_doctest(urllib2, verbose)
+    support.run_doctest(test_urllib2, verbose)
+    support.run_doctest(urllib2, verbose)
     tests = (TrivialTests,
              OpenerDirectorTests,
              HandlerTests,
              MiscTests,
              RequestTests)
-    test_support.run_unittest(*tests)
+    support.run_unittest(*tests)
 
 if __name__ == "__main__":
     test_main(verbose=True)
diff --git a/Lib/test/test_xmlrpc.py b/Lib/test/test_xmlrpc.py
@@ -659,7 +659,13 @@ def test_dotted_attribute(self):
     def test_partial_post(self):
         # Check that a partial POST doesn't make the server loop: issue #14001.
         conn = httplib.HTTPConnection(ADDR, PORT)
-        conn.request('POST', '/RPC2 HTTP/1.
92DE
0\r\nContent-Length: 100\r\n\r\nbye')
+        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'
+                  % (ADDR, PORT))
         conn.close()
 
 class SimpleServerEncodingTestCase(BaseServerTestCase):

diff --git a/Misc/NEWS.d/next/Security/2019-04-10-08-53-30.bpo-30458.51E-DA.rst 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.
Original file line number	Diff line number	Diff line change
		@@ -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.