From f62266fdd5feeec45d2037411fa0c83964e751a7 Mon Sep 17 00:00:00 2001 From: Tom Most Date: Mon, 3 Jun 2019 22:03:22 -0700 Subject: [PATCH 2/4] Fix CVE-2020-10108 and CVE-2020-10109 Refactor to reduce duplication (cherry picked from commit d2f6dd9b3766509f40c980aac67ca8475da67c6f) Fix several request smuggling attacks. 1. Requests with multiple Content-Length headers were allowed (thanks to Jake Miller from Bishop Fox and ZeddYu Lu) and now fail with a 400; 2. Requests with a Content-Length header and a Transfer-Encoding header honored the first header (thanks to Jake Miller from Bishop Fox) and now fail with a 400; 3. Requests whose Transfer-Encoding header had a value other than "chunked" and "identity" (thanks to ZeddYu Lu) were allowed and now fail with a 400. (cherry picked from commit 4a7d22e490bb8ff836892cc99a1f54b85ccb0281) --- src/twisted/web/http.py | 64 +++++-- src/twisted/web/newsfragments/9770.bugfix | 1 + src/twisted/web/test/test_http.py | 215 +++++++++++++++++----- 3 files changed, 223 insertions(+), 57 deletions(-) create mode 100644 src/twisted/web/newsfragments/9770.bugfix diff --git a/src/twisted/web/http.py b/src/twisted/web/http.py index fe88d33739..71188a8d50 100644 --- a/src/twisted/web/http.py +++ b/src/twisted/web/http.py @@ -2170,6 +2170,51 @@ class HTTPChannel(basic.LineReceiver, policies.TimeoutMixin): self.allContentReceived() self._dataBuffer.append(data) + def _maybeChooseTransferDecoder(self, header, data): + """ + If the provided header is C{content-length} or + C{transfer-encoding}, choose the appropriate decoder if any. + + Returns L{True} if the request can proceed and L{False} if not. + """ + + def fail(): + self._respondToBadRequestAndDisconnect() + self.length = None + + # Can this header determine the length? + if header == b'content-length': + try: + length = int(data) + except ValueError: + fail() + return False + newTransferDecoder = _IdentityTransferDecoder( + length, self.requests[-1].handleContentChunk, self._finishRequestBody) + elif header == b'transfer-encoding': + # XXX Rather poorly tested code block, apparently only exercised by + # test_chunkedEncoding + if data.lower() == b'chunked': + length = None + newTransferDecoder = _ChunkedTransferDecoder( + self.requests[-1].handleContentChunk, self._finishRequestBody) + elif data.lower() == b'identity': + return True + else: + fail() + return False + else: + # It's not a length related header, so exit + return True + + if self._transferDecoder is not None: + fail() + return False + else: + self.length = length + self._transferDecoder = newTransferDecoder + return True + def headerReceived(self, line): """ @@ -2191,21 +2236,10 @@ class HTTPChannel(basic.LineReceiver, policies.TimeoutMixin): header = header.lower() data = data.strip() - if header == b'content-length': - try: - self.length = int(data) - except ValueError: - self._respondToBadRequestAndDisconnect() - self.length = None - return False - self._transferDecoder = _IdentityTransferDecoder( - self.length, self.requests[-1].handleContentChunk, self._finishRequestBody) - elif header == b'transfer-encoding' and data.lower() == b'chunked': - # XXX Rather poorly tested code block, apparently only exercised by - # test_chunkedEncoding - self.length = None - self._transferDecoder = _ChunkedTransferDecoder( - self.requests[-1].handleContentChunk, self._finishRequestBody) + + if not self._maybeChooseTransferDecoder(header, data): + return False + reqHeaders = self.requests[-1].requestHeaders values = reqHeaders.getRawHeaders(header) if values is not None: diff --git a/src/twisted/web/newsfragments/9770.bugfix b/src/twisted/web/newsfragments/9770.bugfix new file mode 100644 index 0000000000..4f1be97de8 --- /dev/null +++ b/src/twisted/web/newsfragments/9770.bugfix @@ -0,0 +1 @@ +Fix several request smuggling attacks: requests with multiple Content-Length headers were allowed (thanks to Jake Miller from Bishop Fox and ZeddYu Lu) and now fail with a 400; requests with a Content-Length header and a Transfer-Encoding header honored the first header (thanks to Jake Miller from Bishop Fox) and now fail with a 400; requests whose Transfer-Encoding header had a value other than "chunked" and "identity" (thanks to ZeddYu Lu) were allowed and now fail a 400. \ No newline at end of file diff --git a/src/twisted/web/test/test_http.py b/src/twisted/web/test/test_http.py index 6001d1e40d..8f2c1bac21 100644 --- a/src/twisted/web/test/test_http.py +++ b/src/twisted/web/test/test_http.py @@ -1441,7 +1441,8 @@ class ParsingTests(unittest.TestCase): """ Execute a web request based on plain text content. - @param httpRequest: Content for the request which is processed. + @param httpRequest: Content for the request which is processed. Each + L{"\n"} will be replaced with L{"\r\n"}. @type httpRequest: C{bytes} @param requestFactory: 2-argument callable returning a Request. @@ -1480,6 +1481,32 @@ class ParsingTests(unittest.TestCase): return channel + def assertRequestRejected(self, requestLines): + """ + Execute a HTTP request and assert that it is rejected with a 400 Bad + Response and disconnection. + + @param requestLines: Plain text lines of the request. These lines will + be joined with newlines to form the HTTP request that is processed. + @type requestLines: C{list} of C{bytes} + """ + httpRequest = b"\n".join(requestLines) + processed = [] + + class MyRequest(http.Request): + def process(self): + processed.append(self) + self.finish() + + channel = self.runRequest(httpRequest, MyRequest, success=False) + self.assertEqual( + channel.transport.value(), + b"HTTP/1.1 400 Bad Request\r\n\r\n", + ) + self.assertTrue(channel.transport.disconnecting) + self.assertEqual(processed, []) + + def test_invalidNonAsciiMethod(self): """ When client sends invalid HTTP method containing @@ -1603,45 +1630,24 @@ class ParsingTests(unittest.TestCase): def test_tooManyHeaders(self): """ - L{HTTPChannel} enforces a limit of C{HTTPChannel.maxHeaders} on the + C{HTTPChannel} enforces a limit of C{HTTPChannel.maxHeaders} on the number of headers received per request. """ - processed = [] - class MyRequest(http.Request): - def process(self): - processed.append(self) - requestLines = [b"GET / HTTP/1.0"] for i in range(http.HTTPChannel.maxHeaders + 2): requestLines.append(networkString("%s: foo" % (i,))) requestLines.extend([b"", b""]) - channel = self.runRequest(b"\n".join(requestLines), MyRequest, 0) - self.assertEqual(processed, []) - self.assertEqual( - channel.transport.value(), - b"HTTP/1.1 400 Bad Request\r\n\r\n") + self.assertRequestRejected(requestLines) def test_invalidContentLengthHeader(self): """ - If a Content-Length header with a non-integer value is received, a 400 - (Bad Request) response is sent to the client and the connection is - closed. + If a I{Content-Length} header with a non-integer value is received, + a 400 (Bad Request) response is sent to the client and the connection + is closed. """ - processed = [] - class MyRequest(http.Request): - def process(self): - processed.append(self) - self.finish() - - requestLines = [b"GET / HTTP/1.0", b"Content-Length: x", b"", b""] - channel = self.runRequest(b"\n".join(requestLines), MyRequest, 0) - self.assertEqual( - channel.transport.value(), - b"HTTP/1.1 400 Bad Request\r\n\r\n") - self.assertTrue(channel.transport.disconnecting) - self.assertEqual(processed, []) + self.assertRequestRejected([b"GET / HTTP/1.0", b"Content-Length: x", b"", b""]) def test_invalidHeaderNoColon(self): @@ -1649,24 +1655,12 @@ class ParsingTests(unittest.TestCase): If a header without colon is received a 400 (Bad Request) response is sent to the client and the connection is closed. """ - processed = [] - class MyRequest(http.Request): - def process(self): - processed.append(self) - self.finish() - - requestLines = [b"GET / HTTP/1.0", b"HeaderName ", b"", b""] - channel = self.runRequest(b"\n".join(requestLines), MyRequest, 0) - self.assertEqual( - channel.transport.value(), - b"HTTP/1.1 400 Bad Request\r\n\r\n") - self.assertTrue(channel.transport.disconnecting) - self.assertEqual(processed, []) + self.assertRequestRejected([b"GET / HTTP/1.0", b"HeaderName ", b"", b""]) def test_headerLimitPerRequest(self): """ - L{HTTPChannel} enforces the limit of C{HTTPChannel.maxHeaders} per + C{HTTPChannel} enforces the limit of C{HTTPChannel.maxHeaders} per request so that headers received in an earlier request do not count towards the limit when processing a later request. """ @@ -2152,6 +2146,143 @@ Hello, self.flushLoggedErrors(AttributeError) + def assertDisconnectingBadRequest(self, request): + """ + Assert that the given request bytes fail with a 400 bad + request without calling L{Request.process}. + + @param request: A raw HTTP request + @type request: L{bytes} + """ + class FailedRequest(http.Request): + processed = False + def process(self): + FailedRequest.processed = True + + channel = self.runRequest(request, FailedRequest, success=False) + self.assertFalse(FailedRequest.processed, "Request.process called") + self.assertEqual( + channel.transport.value(), + b"HTTP/1.1 400 Bad Request\r\n\r\n") + self.assertTrue(channel.transport.disconnecting) + + + def test_duplicateContentLengths(self): + """ + A request which includes multiple C{content-length} headers + fails with a 400 response without calling L{Request.process}. + """ + self.assertRequestRejected([ + b'GET /a HTTP/1.1', + b'Content-Length: 56', + b'Content-Length: 0', + b'Host: host.invalid', + b'', + b'', + ]) + + + def test_duplicateContentLengthsWithPipelinedRequests(self): + """ + Two pipelined requests, the first of which includes multiple + C{content-length} headers, trigger a 400 response without + calling L{Request.process}. + """ + self.assertRequestRejected([ + b'GET /a HTTP/1.1', + b'Content-Length: 56', + b'Content-Length: 0', + b'Host: host.invalid', + b'', + b'', + b'GET /a HTTP/1.1', + b'Host: host.invalid', + b'', + b'', + ]) + + + def test_contentLengthAndTransferEncoding(self): + """ + A request that includes both C{content-length} and + C{transfer-encoding} headers fails with a 400 response without + calling L{Request.process}. + """ + self.assertRequestRejected([ + b'GET /a HTTP/1.1', + b'Transfer-Encoding: chunked', + b'Content-Length: 0', + b'Host: host.invalid', + b'', + b'', + ]) + + + def test_contentLengthAndTransferEncodingWithPipelinedRequests(self): + """ + Two pipelined requests, the first of which includes both + C{content-length} and C{transfer-encoding} headers, triggers a + 400 response without calling L{Request.process}. + """ + self.assertRequestRejected([ + b'GET /a HTTP/1.1', + b'Transfer-Encoding: chunked', + b'Content-Length: 0', + b'Host: host.invalid', + b'', + b'', + b'GET /a HTTP/1.1', + b'Host: host.invalid', + b'', + b'', + ]) + + + def test_unknownTransferEncoding(self): + """ + A request whose C{transfer-encoding} header includes a value + other than C{chunked} or C{identity} fails with a 400 response + without calling L{Request.process}. + """ + self.assertRequestRejected([ + b'GET /a HTTP/1.1', + b'Transfer-Encoding: unknown', + b'Host: host.invalid', + b'', + b'', + ]) + + + def test_transferEncodingIdentity(self): + """ + A request with a valid C{content-length} and a + C{transfer-encoding} whose value is C{identity} succeeds. + """ + body = [] + + class SuccessfulRequest(http.Request): + processed = False + def process(self): + body.append(self.content.read()) + self.setHeader(b'content-length', b'0') + self.finish() + + request = b'''\ +GET / HTTP/1.1 +Host: host.invalid +Content-Length: 2 +Transfer-Encoding: identity + +ok +''' + channel = self.runRequest(request, SuccessfulRequest, False) + self.assertEqual(body, [b'ok']) + self.assertEqual( + channel.transport.value(), + b'HTTP/1.1 200 OK\r\nContent-Length: 0\r\n\r\n', + ) + + class QueryArgumentsTests(unittest.TestCase): def testParseqs(self): -- 2.39.2