| 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. |
| |
| Fixes CVE-2020-10108 & CVE-2020-10109 - HTTP request splitting |
| https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2020-10108 |
| https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2020-10109 |
| |
| Upstream: |
| https://github.com/twisted/twisted/commit/4a7d22e490bb8ff836892cc99a1f54b85ccb0281 |
| |
| Signed-off-by: Matthew Weber <matthew.weber@rockwellcollins.com> |
| |
| |
| --- |
| 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): |