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):
