Skip to content

Commit 08480dc

Browse files
gpsheadmiss-islington
authored andcommitted
[3.14] gh-150743: Limit trailer lines and interim responses read by http.client (GH-150749)
http.client read chunked-response trailer lines and skipped interim (1xx) responses in unbounded loops, so a server streaming either forever would hang the client even with a socket timeout set (data keeps arriving, so the timeout never fires). Trailer lines are now limited to max_response_headers (100 by default) and interim responses to 100; HTTPException is raised past either limit. Follow-up to gh-88188 for CVE-2021-3737, which bounded header lines within an interim response but not these two sibling loops. (cherry picked from commit 84badb7) Co-authored-by: Gregory P. Smith <68491+gpshead@users.noreply.github.com> --- This issue was reported to us via [GHSA-w4q2-g22w-6fr4](https://github.com/python/cpython/security/advisories/GHSA-w4q2-g22w-6fr4)
1 parent 556aa09 commit 08480dc

3 files changed

Lines changed: 84 additions & 1 deletion

File tree

Lib/http/client.py

Lines changed: 20 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -111,6 +111,13 @@
111111
_MAXLINE = 65536
112112
_MAXHEADERS = 100
113113

114+
# maximal number of interim (1xx) responses tolerated before the final
115+
# response. Real servers send at most a few; without a bound, a server
116+
# streaming "100 Continue" responses would hang getresponse() forever.
117+
# A socket timeout cannot detect that as data keeps arriving within every
118+
# timeout window.
119+
_MAXINTERIMRESPONSES = 100
120+
114121
# Data larger than this will be read in chunks, to prevent extreme
115122
# overallocation.
116123
_MIN_READ_BUF_SIZE = 1 << 20
@@ -319,7 +326,7 @@ def begin(self):
319326
return
320327

321328
# read until we get a non-100 response
322-
while True:
329+
for _ in range(_MAXINTERIMRESPONSES):
323330
version, status, reason = self._read_status()
324331
if status != CONTINUE:
325332
break
@@ -328,6 +335,9 @@ def begin(self):
328335
if self.debuglevel > 0:
329336
print("headers:", skipped_headers)
330337
del skipped_headers
338+
else:
339+
raise HTTPException(
340+
f"got more than {_MAXINTERIMRESPONSES} interim responses")
331341

332342
self.code = self.status = status
333343
self.reason = reason.strip()
@@ -545,6 +555,7 @@ def _read_next_chunk_size(self):
545555
def _read_and_discard_trailer(self):
546556
# read and discard trailer up to the CRLF terminator
547557
### note: we shouldn't have any trailers!
558+
trailers_read = 0
548559
while True:
549560
line = self.fp.readline(_MAXLINE + 1)
550561
if len(line) > _MAXLINE:
@@ -555,6 +566,14 @@ def _read_and_discard_trailer(self):
555566
break
556567
if line in (b'\r\n', b'\n', b''):
557568
break
569+
# Bound the trailer count just as response headers are bounded.
570+
# A server streaming trailer lines forever would otherwise hang
571+
# the client; a socket timeout cannot detect that as data keeps
572+
# arriving within every timeout window.
573+
trailers_read += 1
574+
if trailers_read > _MAXHEADERS:
575+
raise HTTPException(
576+
f"got more than {_MAXHEADERS} trailers")
558577

559578
def _get_chunk_left(self):
560579
# return self.chunk_left, reading a new chunk if necessary.

Lib/test/test_httplib.py

Lines changed: 58 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1093,6 +1093,35 @@ def test_overflowing_header_limit_after_100(self):
10931093
self.assertIn('got more than ', str(cm.exception))
10941094
self.assertIn('headers', str(cm.exception))
10951095

1096+
def test_too_many_interim_responses(self):
1097+
# A server streaming "100 Continue" responses forever must not
1098+
# hang getresponse().
1099+
body = (
1100+
'HTTP/1.1 100 Continue\r\n\r\n'
1101+
* (client._MAXINTERIMRESPONSES + 1)
1102+
)
1103+
resp = client.HTTPResponse(FakeSocket(body))
1104+
with self.assertRaises(client.HTTPException) as cm:
1105+
resp.begin()
1106+
self.assertIn('got more than ', str(cm.exception))
1107+
self.assertIn('interim responses', str(cm.exception))
1108+
1109+
def test_multiple_interim_responses(self):
1110+
# A reasonable number of interim responses before the final
1111+
# response is skipped as before.
1112+
body = (
1113+
'HTTP/1.1 100 Continue\r\n\r\n' * 3 +
1114+
'HTTP/1.1 200 OK\r\n'
1115+
'Content-Length: 5\r\n'
1116+
'\r\n'
1117+
'hello'
1118+
)
1119+
resp = client.HTTPResponse(FakeSocket(body), method="GET")
1120+
resp.begin()
1121+
self.assertEqual(resp.status, 200)
1122+
self.assertEqual(resp.read(), b'hello')
1123+
resp.close()
1124+
10961125
def test_overflowing_chunked_line(self):
10971126
body = (
10981127
'HTTP/1.1 200 OK\r\n'
@@ -1164,6 +1193,35 @@ def test_chunked_trailers(self):
11641193
self.assertEqual(sock.file.read(), b"") #we read to the end
11651194
resp.close()
11661195

1196+
def test_chunked_too_many_trailers(self):
1197+
"""A response streaming endless trailer lines must raise, not hang"""
1198+
too_many_trailers = "".join(
1199+
f"X-Trailer{i}: {i}\r\n" for i in range(client._MAXHEADERS + 1)
1200+
)
1201+
# An unbounded read() reaches the trailers via the final 0 chunk.
1202+
sock = FakeSocket(
1203+
chunked_start + last_chunk + too_many_trailers + chunked_end)
1204+
resp = client.HTTPResponse(sock, method="GET")
1205+
resp.begin()
1206+
with self.assertRaisesRegex(
1207+
client.HTTPException,
1208+
f"got more than {client._MAXHEADERS} trailers",
1209+
):
1210+
resp.read()
1211+
resp.close()
1212+
1213+
# A bounded read(amt) larger than the body hits the same limit.
1214+
sock = FakeSocket(
1215+
chunked_start + last_chunk + too_many_trailers + chunked_end)
1216+
resp = client.HTTPResponse(sock, method="GET")
1217+
resp.begin()
1218+
with self.assertRaisesRegex(
1219+
client.HTTPException,
1220+
f"got more than {client._MAXHEADERS} trailers",
1221+
):
1222+
resp.read(len(chunked_expected) + 1)
1223+
resp.close()
1224+
11671225
def test_chunked_sync(self):
11681226
"""Check that we don't read past the end of the chunked-encoding stream"""
11691227
expected = chunked_expected
Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
:mod:`http.client` now limits the number of chunked-response trailer lines
2+
it will read to 100, and the number of interim (1xx) responses it will
3+
skip to 100. A malicious or broken server could previously stream trailer
4+
lines or ``100 Continue`` responses forever, hanging the client even when
5+
a socket timeout was in use.
6+
Reported by ``@YLChen-007`` via GHSA-w4q2-g22w-6fr4.

0 commit comments

Comments
 (0)