From 4a7d22e490bb8ff836892cc99a1f54b85ccb0281 Mon Sep 17 00:00:00 2001 From: Mark Williams <mrw@enotuniq.org> Date: Sun, 16 Feb 2020 19:00:10 -0800 Subject: [PATCH] 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. --- src/twisted/web/http.py | 64 +++++++--- src/twisted/web/newsfragments/9770.bugfix | 1 + src/twisted/web/test/test_http.py | 137 ++++++++++++++++++++++ 3 files changed, 187 insertions(+), 15 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 f0fb05b4d69..06d830fe30f 100644 --- a/src/twisted/web/http.py +++ b/src/twisted/web/http.py @@ -2171,6 +2171,51 @@ def _finishRequestBody(self, data): 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): """ @@ -2196,21 +2241,10 @@ def headerReceived(self, line): 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 00000000000..4f1be97de8a --- /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 0a0db09b750..578cb500cda 100644 --- a/src/twisted/web/test/test_http.py +++ b/src/twisted/web/test/test_http.py @@ -2252,6 +2252,143 @@ def process(self): 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):