Skip to content

Commit

Permalink
Fix several request smuggling attacks.
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
markrwilliams committed Mar 4, 2020
1 parent b4602e5 commit 4a7d22e
Show file tree
Hide file tree
Showing 3 changed files with 187 additions and 15 deletions.
64 changes: 49 additions & 15 deletions src/twisted/web/http.py
Original file line number Diff line number Diff line change
Expand Up @@ -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):
"""
Expand All @@ -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:
Expand Down
1 change: 1 addition & 0 deletions src/twisted/web/newsfragments/9770.bugfix
Original file line number Diff line number Diff line change
@@ -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.
137 changes: 137 additions & 0 deletions src/twisted/web/test/test_http.py
Original file line number Diff line number Diff line change
Expand Up @@ -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):
Expand Down

0 comments on commit 4a7d22e

Please sign in to comment.