Skip to content

Commit 34f85af

Browse files
miss-islingtonepicfaace
miss-islington
and
epicfaace
authored
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 9165add) Co-authored-by: Ashwin Ramaswami <aramaswamis@gmail.com>
1 parent 725cbce commit 34f85af

4 files changed

Lines changed: 57 additions & 3 deletions

File tree

‎Lib/http/client.py‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -850,6 +850,8 @@ def __init__(self, host, port=None, timeout=socket._GLOBAL_DEFAULT_TIMEOUT,
850850

851851
(self.host, self.port) = self._get_hostport(host, port)
852852

853+
self._validate_host(self.host)
854+
853855
# This is stored as an instance variable to allow unit
854856
# tests to replace it with a suitable mockup
855857
self._create_connection = socket.create_connection
@@ -1205,6 +1207,14 @@ def _validate_path(self, url):
12051207
raise InvalidURL(f"URL can't contain control characters. {url!r} "
12061208
f"(found at least {match.group()!r})")
12071209

1210+
def _validate_host(self, host):
1211+
"""Validate a host so it doesn't contain control characters."""
1212+
# Prevent CVE-2019-18348.
1213+
match = _contains_disallowed_url_pchar_re.search(host)
1214+
if match:
1215+
raise InvalidURL(f"URL can't contain control characters. {host!r} "
1216+
f"(found at least {match.group()!r})")
1217+
12081218
def putheader(self, header, *values):
12091219
"""Send a request header line to the server.
12101220

‎Lib/test/test_httplib.py‎

Lines changed: 12 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1157,7 +1157,7 @@ def run_server():
11571157
thread.join()
11581158
self.assertEqual(result, b"proxied data\n")
11591159

1160-
def test_putrequest_override_validation(self):
1160+
def test_putrequest_override_domain_validation(self):
11611161
"""
11621162
It should be possible to override the default validation
11631163
behavior in putrequest (bpo-38216).
@@ -1170,6 +1170,17 @@ def _validate_path(self, url):
11701170
conn.sock = FakeSocket('')
11711171
conn.putrequest('GET', '/\x00')
11721172

1173+
def test_putrequest_override_host_validation(self):
1174+
class UnsafeHTTPConnection(client.HTTPConnection):
1175+
def _validate_host(self, url):
1176+
pass
1177+
1178+
conn = UnsafeHTTPConnection('example.com\r\n')
1179+
conn.sock = FakeSocket('')
1180+
# set skip_host so a ValueError is not raised upon adding the
1181+
# invalid URL as the value of the "Host:" header
1182+
conn.putrequest('GET', '/', skip_host=1)
1183+
11731184
def test_putrequest_override_encoding(self):
11741185
"""
11751186
It should be possible to override the default encoding

‎Lib/test/test_urllib.py‎

Lines changed: 34 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -353,7 +353,7 @@ def test_willclose(self):
353353
self.unfakehttp()
354354

355355
@unittest.skipUnless(ssl, "ssl module required")
356-
def test_url_with_control_char_rejected(self):
356+
def test_url_path_with_control_char_rejected(self):
357357
for char_no in list(range(0, 0x21)) + [0x7f]:
358358
char = chr(char_no)
359359
schemeless_url = f"//localhost:7777/test{char}/"
@@ -380,7 +380,7 @@ def test_url_with_control_char_rejected(self):
380380
self.unfakehttp()
381381

382382
@unittest.skipUnless(ssl, "ssl module required")
383-
def test_url_with_newline_header_injection_rejected(self):
383+
def test_url_path_with_newline_header_injection_rejected(self):
384384
self.fakehttp(b"HTTP/1.1 200 OK\r\n\r\nHello.")
385385
host = "localhost:7777?a=1 HTTP/1.1\r\nX-injected: header\r\nTEST: 123"
386386
schemeless_url = "//" + host + ":8080/test/?test=a"
@@ -405,6 +405,38 @@ def test_url_with_newline_header_injection_rejected(self):
405405
finally:
406406
self.unfakehttp()
407407

408+
@unittest.skipUnless(ssl, "ssl module required")
409+
def test_url_host_with_control_char_rejected(self):
410+
for char_no in list(range(0, 0x21)) + [0x7f]:
411+
char = chr(char_no)
412+
schemeless_url = f"//localhost{char}/test/"
413+
self.fakehttp(b"HTTP/1.1 200 OK\r\n\r\nHello.")
414+
try:
415+
escaped_char_repr = repr(char).replace('\\', r'\\')
416+
InvalidURL = http.client.InvalidURL
417+
with self.assertRaisesRegex(
418+
InvalidURL, f"contain control.*{escaped_char_repr}"):
419+
urlopen(f"http:{schemeless_url}")
420+
with self.assertRaisesRegex(InvalidURL, f"contain control.*{escaped_char_repr}"):
421+
urlopen(f"https:{schemeless_url}")
422+
finally:
423+
self.unfakehttp()
424+
425+
@unittest.skipUnless(ssl, "ssl module required")
426+
def test_url_host_with_newline_header_injection_rejected(self):
427+
self.fakehttp(b"HTTP/1.1 200 OK\r\n\r\nHello.")
428+
host = "localhost\r\nX-injected: header\r\n"
429+
schemeless_url = "//" + host + ":8080/test/?test=a"
430+
try:
431+
InvalidURL = http.client.InvalidURL
432+
with self.assertRaisesRegex(
433+
InvalidURL, r"contain control.*\\r"):
434+
urlopen(f"http:{schemeless_url}")
435+
with self.assertRaisesRegex(InvalidURL, r"contain control.*\\n"):
436+
urlopen(f"https:{schemeless_url}")
437+
finally:
438+
self.unfakehttp()
439+
408440
def test_read_0_9(self):
409441
# "0.9" response accepted (but not "simple responses" without
410442
# a status line)

‎Misc/NEWS.d/next/Security/2020-03-14-14-57-44.bpo-38576.OowwQn.rst‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
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.

0 commit comments

Comments
 (0)