[Python-checkins] bpo-38576: Disallow control characters in hostnames in http.client (GH-18995)
Miss Islington (bot)
webhook-mailer at python.org
Sat Mar 14 15:13:03 EDT 2020
https://github.com/python/cpython/commit/34f85af3229f86c004a954c3f261ceea1f5e9f95
commit: 34f85af3229f86c004a954c3f261ceea1f5e9f95
branch: 3.7
author: Miss Islington (bot) <31488909+miss-islington at users.noreply.github.com>
committer: GitHub <noreply at github.com>
date: 2020-03-14T12:12:58-07:00
summary:
bpo-38576: Disallow control characters in hostnames in http.client (GH-18995)
Add host validation for control characters for more CVE-2019-18348 protection.
(cherry picked from commit 9165addc22d05e776a54319a8531ebd0b2fe01ef)
Co-authored-by: Ashwin Ramaswami <aramaswamis at gmail.com>
files:
A Misc/NEWS.d/next/Security/2020-03-14-14-57-44.bpo-38576.OowwQn.rst
M Lib/http/client.py
M Lib/test/test_httplib.py
M Lib/test/test_urllib.py
diff --git a/Lib/http/client.py b/Lib/http/client.py
index f7a183c774032..09c57af865ee0 100644
--- a/Lib/http/client.py
+++ b/Lib/http/client.py
@@ -850,6 +850,8 @@ def __init__(self, host, port=None, timeout=socket._GLOBAL_DEFAULT_TIMEOUT,
(self.host, self.port) = self._get_hostport(host, port)
+ self._validate_host(self.host)
+
# This is stored as an instance variable to allow unit
# tests to replace it with a suitable mockup
self._create_connection = socket.create_connection
@@ -1205,6 +1207,14 @@ def _validate_path(self, url):
raise InvalidURL(f"URL can't contain control characters. {url!r} "
f"(found at least {match.group()!r})")
+ def _validate_host(self, host):
+ """Validate a host so it doesn't contain control characters."""
+ # Prevent CVE-2019-18348.
+ match = _contains_disallowed_url_pchar_re.search(host)
+ if match:
+ raise InvalidURL(f"URL can't contain control characters. {host!r} "
+ f"(found at least {match.group()!r})")
+
def putheader(self, header, *values):
"""Send a request header line to the server.
diff --git a/Lib/test/test_httplib.py b/Lib/test/test_httplib.py
index 9facb2e360179..891393ab869e6 100644
--- a/Lib/test/test_httplib.py
+++ b/Lib/test/test_httplib.py
@@ -1157,7 +1157,7 @@ def run_server():
thread.join()
self.assertEqual(result, b"proxied data\n")
- def test_putrequest_override_validation(self):
+ def test_putrequest_override_domain_validation(self):
"""
It should be possible to override the default validation
behavior in putrequest (bpo-38216).
@@ -1170,6 +1170,17 @@ def _validate_path(self, url):
conn.sock = FakeSocket('')
conn.putrequest('GET', '/\x00')
+ def test_putrequest_override_host_validation(self):
+ class UnsafeHTTPConnection(client.HTTPConnection):
+ def _validate_host(self, url):
+ pass
+
+ conn = UnsafeHTTPConnection('example.com\r\n')
+ conn.sock = FakeSocket('')
+ # set skip_host so a ValueError is not raised upon adding the
+ # invalid URL as the value of the "Host:" header
+ conn.putrequest('GET', '/', skip_host=1)
+
def test_putrequest_override_encoding(self):
"""
It should be possible to override the default encoding
diff --git a/Lib/test/test_urllib.py b/Lib/test/test_urllib.py
index 113c678dc1a6d..0305e7aa18849 100644
--- a/Lib/test/test_urllib.py
+++ b/Lib/test/test_urllib.py
@@ -353,7 +353,7 @@ def test_willclose(self):
self.unfakehttp()
@unittest.skipUnless(ssl, "ssl module required")
- def test_url_with_control_char_rejected(self):
+ def test_url_path_with_control_char_rejected(self):
for char_no in list(range(0, 0x21)) + [0x7f]:
char = chr(char_no)
schemeless_url = f"//localhost:7777/test{char}/"
@@ -380,7 +380,7 @@ def test_url_with_control_char_rejected(self):
self.unfakehttp()
@unittest.skipUnless(ssl, "ssl module required")
- def test_url_with_newline_header_injection_rejected(self):
+ def test_url_path_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"
@@ -405,6 +405,38 @@ def test_url_with_newline_header_injection_rejected(self):
finally:
self.unfakehttp()
+ @unittest.skipUnless(ssl, "ssl module required")
+ def test_url_host_with_control_char_rejected(self):
+ for char_no in list(range(0, 0x21)) + [0x7f]:
+ char = chr(char_no)
+ schemeless_url = f"//localhost{char}/test/"
+ self.fakehttp(b"HTTP/1.1 200 OK\r\n\r\nHello.")
+ try:
+ escaped_char_repr = repr(char).replace('\\', r'\\')
+ InvalidURL = http.client.InvalidURL
+ with self.assertRaisesRegex(
+ InvalidURL, f"contain control.*{escaped_char_repr}"):
+ urlopen(f"http:{schemeless_url}")
+ with self.assertRaisesRegex(InvalidURL, f"contain control.*{escaped_char_repr}"):
+ urlopen(f"https:{schemeless_url}")
+ finally:
+ self.unfakehttp()
+
+ @unittest.skipUnless(ssl, "ssl module required")
+ def test_url_host_with_newline_header_injection_rejected(self):
+ self.fakehttp(b"HTTP/1.1 200 OK\r\n\r\nHello.")
+ host = "localhost\r\nX-injected: header\r\n"
+ schemeless_url = "//" + host + ":8080/test/?test=a"
+ try:
+ InvalidURL = http.client.InvalidURL
+ with self.assertRaisesRegex(
+ InvalidURL, r"contain control.*\\r"):
+ urlopen(f"http:{schemeless_url}")
+ with self.assertRaisesRegex(InvalidURL, r"contain control.*\\n"):
+ urlopen(f"https:{schemeless_url}")
+ finally:
+ self.unfakehttp()
+
def test_read_0_9(self):
# "0.9" response accepted (but not "simple responses" without
# a status line)
diff --git a/Misc/NEWS.d/next/Security/2020-03-14-14-57-44.bpo-38576.OowwQn.rst b/Misc/NEWS.d/next/Security/2020-03-14-14-57-44.bpo-38576.OowwQn.rst
new file mode 100644
index 0000000000000..34b8af28988fa
--- /dev/null
+++ b/Misc/NEWS.d/next/Security/2020-03-14-14-57-44.bpo-38576.OowwQn.rst
@@ -0,0 +1 @@
+Disallow control characters in hostnames in http.client, addressing CVE-2019-18348. Such potentially malicious header injection URLs now cause a InvalidURL to be raised.
\ No newline at end of file
More information about the Python-checkins
mailing list