8000 bpo-38576: Disallow control characters in hostnames in http.client (G… · python/cpython@ff69c9d · GitHub
[go: up one dir, main page]
Skip to content

Commit ff69c9d

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 a927e91 commit ff69c9d

File tree

4 files changed

+57
-3
lines changed

4 files changed

+57
-3
lines changed

‎Lib/http/client.py

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

829829
(self.host, self.port) = self._get_hostport(host, port)
830830

831+
self._validate_host(self.host)
832+
831833
# This is stored as an instance variable to allow unit
832834
# tests to replace it with a suitable mockup
833835
self._create_connection = socket.create_connection
@@ -1183,6 +1185,14 @@ def _validate_path(self, url):
11831185
raise InvalidURL(f"URL can't contain control characters. {url!r} "
11841186
f"(found at least {match.group()!r})")
11851187

1188+
def _validate_host(self, host):
1189+
"""Validate a host so it doesn't contain control characters."""
1190+
# Prevent CVE-2019-18348.
1191+
match = _contains_disallowed_url_pchar_re.search(host)
1192+
if match:
1193+
raise InvalidURL(f"URL can't contain control characters. {host!r} "
1194+
f"(found at least {match.group()!r})")
1195+
11861196
def putheader(self, header, *values):
11871197
"""Send a request header line to the server.
11881198

‎Lib/test/test_httplib.py

Lines changed: 12 additions & 1 deletion
< 8000 td data-grid-cell-id="diff-6202bc3817db8ca8da16747c2447a99b5cb9037cc39f05a29f4dd152d155dac2-1171-1182-0" data-selected="false" role="gridcell" style="background-color:var(--bgColor-default);text-align:center" tabindex="-1" valign="top" class="focusable-grid-cell diff-line-number position-relative diff-line-number-neutral left-side">1171
Original file line numberDiff line numberDiff line change
@@ -1155,7 +1155,7 @@ def run_server():
11551155
thread.join()
11561156
self.assertEqual(result, b"proxied data\n")
11571157

1158-
def test_putrequest_override_validation(self):
1158+
def test_putrequest_override_domain_validation(self):
11591159
"""
11601160
It should be possible to override the default validation
11611161
behavior in putrequest (bpo-38216).
@@ -1168,6 +1168,17 @@ def _validate_path(self, url):
11681168
conn.sock = FakeSocket('')
11691169
conn.putrequest('GET', '/\x00')
11701170

1171+
def test_putrequest_override_host_validation(self):
1172+
class UnsafeHTTPConnection(client.HTTPConnection):
1173+
def _validate_host(self, url):
1174+
pass
1175+
1176+
conn = UnsafeHTTPConnection('example.com\r\n')
1177+
conn.sock = FakeSocket('')
1178+
# set skip_host so a ValueError is not raised upon adding the
1179+
# invalid URL as the value of the "Host:" header
1180+
conn.putrequest('GET', '/', skip_host=1)
1181+
1182
def test_putrequest_override_encoding(self):
11721183
"""
11731184
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
@@ -361,7 +361,7 @@ def test_willclose(self):
361361
self.unfakehttp()
362362

363363
@unittest.skipUnless(ssl, "ssl module required")
364-
def test_url_with_control_char_rejected(self):
364+
def test_url_path_with_control_char_rejected(self):
365365
for char_no in list(range(0, 0x21)) + [0x7f]:
366366
char = chr(char_no)
367367
schemeless_url = f"//localhost:7777/test{char}/"
@@ -388,7 +388,7 @@ def test_url_with_control_char_rejected(self):
388388
self.unfakehttp()
389389

390390
@unittest.skipUnless(ssl, "ssl module required")
391-
def test_url_with_newline_header_injection_rejected(self):
391+
def test_url_path_with_newline_header_injection_rejected(self):
392392
self.fakehttp(b"HTTP/1.1 200 OK\r\n\r\nHello.")
393393
host = "localhost:7777?a=1 HTTP/1.1\r\nX-injected: header\r\nTEST: 123"
394394
schemeless_url = "//" + host + ":8080/test/?test=a"
@@ -413,6 +413,38 @@ def test_url_with_newline_header_injection_rejected(self):
413413
finally:
414414
self.unfakehttp()
415415

416+
@unittest.skipUnless(ssl, "ssl module required")
417+
def test_url_host_with_control_char_rejected(self):
418+
for char_no in list(range(0, 0x21)) + [0x7f]:
419+
char = chr(char_no)
420+
schemeless_url = f"//localhost{char}/test/"
421+
self.fakehttp(b"HTTP/1.1 200 OK\r\n\r\nHello.")
422+
try:
423+
escaped_char_repr = repr(char).replace('\\', r'\\')
424+
InvalidURL = http.client.InvalidURL
425+
with self.assertRaisesRegex(
426+
InvalidURL, f"contain control.*{escaped_char_repr}"):
427+
urlopen(f"http:{schemeless_url}")
428+
with self.assertRaisesRegex(InvalidURL, f"contain control.*{escaped_char_repr}"):
429+
urlopen(f"https:{schemeless_url}")
430+
finally:
431+
self.unfakehttp()
432+
433+
@unittest.skipUnless(ssl, "ssl module required")
434+
def test_url_host_with_newline_header_injection_rejected(self):
435+
self.fakehttp(b"HTTP/1.1 200 OK\r\n\r\nHello.")
436+
host = "localhost\r\nX-injected: header\r\n"
437+
schemeless_url = "//" + host + ":8080/test/?test=a"
438+
try:
439+
InvalidURL = http.client.InvalidURL
440+
with self.assertRaisesRegex(
441+
InvalidURL, r"contain control.*\\r"):
442+
urlopen(f"http:{schemeless_url}")
443+
with self.assertRaisesRegex(InvalidURL, r"contain control.*\\n"):
444+
urlopen(f"https:{schemeless_url}")
445+
finally:
446+
self.unfakehttp()
447+
416448
def test_read_0_9(self):
417449
# "0.9" response accepted (but not "simple responses" without
418450
# 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)
0