394 lines
14 KiB
Diff
394 lines
14 KiB
Diff
From f62266fdd5feeec45d2037411fa0c83964e751a7 Mon Sep 17 00:00:00 2001
|
|
From: Tom Most <twm@freecog.net>
|
|
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
|
|
|
