From e53f243a4d744912204cda9ae380879fca2e7ef1 Mon Sep 17 00:00:00 2001 From: Gen Xu Date: Wed, 5 May 2021 04:21:43 -0700 Subject: [PATCH 1/7] bpo-44022: Fix httplib client deny of service with total header size check after 100 --- Lib/http/client.py | 5 ++++- Lib/test/test_httplib.py | 8 ++++++++ 2 files changed, 12 insertions(+), 1 deletion(-) diff --git a/Lib/http/client.py b/Lib/http/client.py index 4b1f692844474f..e15bcf90f81cf2 100644 --- a/Lib/http/client.py +++ b/Lib/http/client.py @@ -309,9 +309,12 @@ def begin(self): if status != CONTINUE: break # skip the header from the 100 response + header_total_size = 0 while True: skip = self.fp.readline(_MAXLINE + 1) - if len(skip) > _MAXLINE: + line_length = len(skip) + header_total_size += line_length + if line_length > _MAXLINE or header_total_size > _MAXLINE: raise LineTooLong("header line") skip = skip.strip() if not skip: diff --git a/Lib/test/test_httplib.py b/Lib/test/test_httplib.py index db41e29a4b8394..2c01cc1fbf0337 100644 --- a/Lib/test/test_httplib.py +++ b/Lib/test/test_httplib.py @@ -1180,6 +1180,14 @@ def test_overflowing_header_line(self): resp = client.HTTPResponse(FakeSocket(body)) self.assertRaises(client.LineTooLong, resp.begin) + def test_overflowing_total_header_size_after_100(self): + body = ( + 'HTTP/1.1 100 OK\r\n' + 'r\n' * 32768 + ) + resp = client.HTTPResponse(FakeSocket(body)) + self.assertRaises(client.LineTooLong, resp.begin) + def test_overflowing_chunked_line(self): body = ( 'HTTP/1.1 200 OK\r\n' From 55206e1717004faac3ed4b110d647818b14b017d Mon Sep 17 00:00:00 2001 From: "blurb-it[bot]" <43283697+blurb-it[bot]@users.noreply.github.com> Date: Wed, 5 May 2021 17:37:05 +0000 Subject: [PATCH 2/7] =?UTF-8?q?=F0=9F=93=9C=F0=9F=A4=96=20Added=20by=20blu?= =?UTF-8?q?rb=5Fit.?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../next/Security/2021-05-05-17-37-04.bpo-44022.bS3XJ9.rst | 1 + 1 file changed, 1 insertion(+) create mode 100644 Misc/NEWS.d/next/Security/2021-05-05-17-37-04.bpo-44022.bS3XJ9.rst diff --git a/Misc/NEWS.d/next/Security/2021-05-05-17-37-04.bpo-44022.bS3XJ9.rst b/Misc/NEWS.d/next/Security/2021-05-05-17-37-04.bpo-44022.bS3XJ9.rst new file mode 100644 index 00000000000000..a3eab75ffdf6bb --- /dev/null +++ b/Misc/NEWS.d/next/Security/2021-05-05-17-37-04.bpo-44022.bS3XJ9.rst @@ -0,0 +1 @@ +Added total header size limit after receving http 100 to httplib client for avoiding malicious server packet causing client hangs. \ No newline at end of file From d1ac02b7c289af4c9718fb026af5ece8d95ed711 Mon Sep 17 00:00:00 2001 From: Gen Xu Date: Wed, 5 May 2021 14:01:44 -0700 Subject: [PATCH 3/7] Using provided header reads function to skip. --- Lib/http/client.py | 40 ++++++++++++++++++++-------------------- Lib/test/test_httplib.py | 4 ++-- 2 files changed, 22 insertions(+), 22 deletions(-) diff --git a/Lib/http/client.py b/Lib/http/client.py index e15bcf90f81cf2..7a970f5de7cb54 100644 --- a/Lib/http/client.py +++ b/Lib/http/client.py @@ -202,15 +202,11 @@ def getallmatchingheaders(self, name): lst.append(line) return lst -def parse_headers(fp, _class=HTTPMessage): - """Parses only RFC2822 headers from a file pointer. - - email Parser wants to see strings rather than bytes. - But a TextIOWrapper around self.rfile would buffer too many bytes - from the stream, bytes which we later need to read as bytes. - So we read the correct bytes here, as bytes, for email Parser - to parse. +def read_headers(fp): + """Reads headers into a list from a file pointer. + Length of line is limited by _MAXLINE, and number of + headers is limited by _MAXHEADERS. """ headers = [] while True: @@ -222,6 +218,19 @@ def parse_headers(fp, _class=HTTPMessage): raise HTTPException("got more than %d headers" % _MAXHEADERS) if line in (b'\r\n', b'\n', b''): break + return headers + +def parse_headers(fp, _class=HTTPMessage): + """Parses only RFC2822 headers from a file pointer. + + email Parser wants to see strings rather than bytes. + But a TextIOWrapper around self.rfile would buffer too many bytes + from the stream, bytes which we later need to read as bytes. + So we read the correct bytes here, as bytes, for email Parser + to parse. + + """ + headers = read_headers(fp) hstring = b''.join(headers).decode('iso-8859-1') return email.parser.Parser(_class=_class).parsestr(hstring) @@ -309,18 +318,9 @@ def begin(self): if status != CONTINUE: break # skip the header from the 100 response - header_total_size = 0 - while True: - skip = self.fp.readline(_MAXLINE + 1) - line_length = len(skip) - header_total_size += line_length - if line_length > _MAXLINE or header_total_size > _MAXLINE: - raise LineTooLong("header line") - skip = skip.strip() - if not skip: - break - if self.debuglevel > 0: - print("header:", skip) + skip = read_headers(self.fp) + if self.debuglevel > 0: + print("header:", skip) self.code = self.status = status self.reason = reason.strip() diff --git a/Lib/test/test_httplib.py b/Lib/test/test_httplib.py index 2c01cc1fbf0337..edb92fb82272a8 100644 --- a/Lib/test/test_httplib.py +++ b/Lib/test/test_httplib.py @@ -1180,13 +1180,13 @@ def test_overflowing_header_line(self): resp = client.HTTPResponse(FakeSocket(body)) self.assertRaises(client.LineTooLong, resp.begin) - def test_overflowing_total_header_size_after_100(self): + def test_overflowing_header_limit_after_100(self): body = ( 'HTTP/1.1 100 OK\r\n' 'r\n' * 32768 ) resp = client.HTTPResponse(FakeSocket(body)) - self.assertRaises(client.LineTooLong, resp.begin) + self.assertRaises(client.HTTPException, resp.begin) def test_overflowing_chunked_line(self): body = ( From 362c50d879dafa80cc452161e16edb9a58d16e87 Mon Sep 17 00:00:00 2001 From: Gen Xu Date: Wed, 5 May 2021 14:04:46 -0700 Subject: [PATCH 4/7] Fix test_all. --- Lib/test/test_httplib.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Lib/test/test_httplib.py b/Lib/test/test_httplib.py index edb92fb82272a8..e50fee30a1a667 100644 --- a/Lib/test/test_httplib.py +++ b/Lib/test/test_httplib.py @@ -1592,7 +1592,7 @@ def test_all(self): expected = {"responses"} # White-list documented dict() object # HTTPMessage, parse_headers(), and the HTTP status code constants are # intentionally omitted for simplicity - denylist = {"HTTPMessage", "parse_headers"} + denylist = {"HTTPMessage", "read_headers", "parse_headers"} for name in dir(client): if name.startswith("_") or name in denylist: continue From 17941e81cf3d189b849b2ac322ed27239727478c Mon Sep 17 00:00:00 2001 From: "Gregory P. Smith" Date: Wed, 5 May 2021 14:48:54 -0700 Subject: [PATCH 5/7] renamed the new function to _read_headers to keep it private. --- Lib/http/client.py | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/Lib/http/client.py b/Lib/http/client.py index 7a970f5de7cb54..08cf2ed9b3716b 100644 --- a/Lib/http/client.py +++ b/Lib/http/client.py @@ -202,8 +202,8 @@ def getallmatchingheaders(self, name): lst.append(line) return lst -def read_headers(fp): - """Reads headers into a list from a file pointer. +def _read_headers(fp): + """Reads potential header lines into a list from a file pointer. Length of line is limited by _MAXLINE, and number of headers is limited by _MAXHEADERS. @@ -230,7 +230,7 @@ def parse_headers(fp, _class=HTTPMessage): to parse. """ - headers = read_headers(fp) + headers = _read_headers(fp) hstring = b''.join(headers).decode('iso-8859-1') return email.parser.Parser(_class=_class).parsestr(hstring) @@ -318,9 +318,10 @@ def begin(self): if status != CONTINUE: break # skip the header from the 100 response - skip = read_headers(self.fp) + skipped_headers = _read_headers(self.fp) if self.debuglevel > 0: - print("header:", skip) + print("headers:", skipped_headers) + del skipped_headers self.code = self.status = status self.reason = reason.strip() From 5ce1a3178e1fc746fb4b4db7c942da1a45b55048 Mon Sep 17 00:00:00 2001 From: "Gregory P. Smith" Date: Wed, 5 May 2021 14:53:39 -0700 Subject: [PATCH 6/7] no need to exclude read_headers in test_all now. --- Lib/test/test_httplib.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Lib/test/test_httplib.py b/Lib/test/test_httplib.py index e50fee30a1a667..e9272569ecc531 100644 --- a/Lib/test/test_httplib.py +++ b/Lib/test/test_httplib.py @@ -1589,10 +1589,10 @@ def readline(self, limit): class OfflineTest(TestCase): def test_all(self): # Documented objects defined in the module should be in __all__ - expected = {"responses"} # White-list documented dict() object + expected = {"responses"} # Allowlist documented dict() object # HTTPMessage, parse_headers(), and the HTTP status code constants are # intentionally omitted for simplicity - denylist = {"HTTPMessage", "read_headers", "parse_headers"} + denylist = {"HTTPMessage", "parse_headers"} for name in dir(client): if name.startswith("_") or name in denylist: continue From 859cd6e609d9f9275f9e5437ff8ce21c3490e3dc Mon Sep 17 00:00:00 2001 From: "Gregory P. Smith" Date: Wed, 5 May 2021 14:55:37 -0700 Subject: [PATCH 7/7] reword and add ReST formatting. --- .../next/Security/2021-05-05-17-37-04.bpo-44022.bS3XJ9.rst | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/Misc/NEWS.d/next/Security/2021-05-05-17-37-04.bpo-44022.bS3XJ9.rst b/Misc/NEWS.d/next/Security/2021-05-05-17-37-04.bpo-44022.bS3XJ9.rst index a3eab75ffdf6bb..cf6b63e3961558 100644 --- a/Misc/NEWS.d/next/Security/2021-05-05-17-37-04.bpo-44022.bS3XJ9.rst +++ b/Misc/NEWS.d/next/Security/2021-05-05-17-37-04.bpo-44022.bS3XJ9.rst @@ -1 +1,2 @@ -Added total header size limit after receving http 100 to httplib client for avoiding malicious server packet causing client hangs. \ No newline at end of file +mod:`http.client` now avoids infinitely reading potential HTTP headers after a +``100 Continue`` status response from the server.