From bf9346d88de2aeb06da3b2cde62ffa6200936367 Mon Sep 17 00:00:00 2001 From: Tim Burke Date: Thu, 15 Aug 2019 14:33:06 -0700 Subject: [PATCH] Fix some request-smuggling vectors on py3 A Python 3 bug causes us to abort header parsing in some cases. We mostly worked around that in the related change, but that was *after* eventlet used the parsed headers to determine things like message framing. As a result, a client sending a malformed request (for example, sending both Content-Length *and* Transfer-Encoding: chunked headers) might have that request parsed properly and authorized by a proxy-server running Python 2, but the proxy-to-backend request could get misparsed if the backend is running Python 3. As a result, the single client request could be interpretted as multiple requests by an object server, only the first of which was properly authorized at the proxy. Now, after we find and parse additional headers that weren't parsed by Python, fix up eventlet's wsgi.input to reflect the message framing we expect given the complete set of headers. As an added precaution, if the client included Transfer-Encoding: chunked *and* a Content-Length, ensure that the Content-Length is not forwarded to the backend. Change-Id: I70c125df70b2a703de44662adc66f740cc79c7a9 Related-Change: I0f03c211f35a9a49e047a5718a9907b515ca88d7 Closes-Bug: 1840507 --- swift/common/wsgi.py | 19 +++++++ swift/proxy/server.py | 7 +++ test/unit/proxy/controllers/test_obj.py | 1 + test/unit/proxy/test_server.py | 74 ++++++++++++++++++++++++- 4 files changed, 98 insertions(+), 3 deletions(-) diff --git a/swift/common/wsgi.py b/swift/common/wsgi.py index 66f2c3e77f..8c8156d0c8 100644 --- a/swift/common/wsgi.py +++ b/swift/common/wsgi.py @@ -480,6 +480,11 @@ class SwiftHttpProtocol(wsgi.HttpProtocol): break header, value = line.split(':', 1) value = value.strip(' \t\n\r') + # NB: Eventlet looks at the headers obj to figure out + # whether the client said the connection should close; + # see https://github.com/eventlet/eventlet/blob/v0.25.0/ + # eventlet/wsgi.py#L504 + self.headers.add_header(header, value) headers_raw.append((header, value)) wsgi_key = 'HTTP_' + header.replace('-', '_').encode( 'latin1').upper().decode('latin1') @@ -488,6 +493,20 @@ class SwiftHttpProtocol(wsgi.HttpProtocol): wsgi_key = wsgi_key[5:] environ[wsgi_key] = value environ['headers_raw'] = tuple(headers_raw) + # Since we parsed some more headers, check to see if they + # change how our wsgi.input should behave + te = environ.get('HTTP_TRANSFER_ENCODING', '').lower() + if te.rsplit(',', 1)[-1].strip() == 'chunked': + environ['wsgi.input'].chunked_input = True + else: + length = environ.get('CONTENT_LENGTH') + if length: + length = int(length) + environ['wsgi.input'].content_length = length + if environ.get('HTTP_EXPECT', '').lower() == '100-continue': + environ['wsgi.input'].wfile = self.wfile + environ['wsgi.input'].wfile_line = \ + b'HTTP/1.1 100 Continue\r\n' return environ diff --git a/swift/proxy/server.py b/swift/proxy/server.py index ed2320e28b..42227b954a 100644 --- a/swift/proxy/server.py +++ b/swift/proxy/server.py @@ -457,6 +457,13 @@ class Application(object): if 'x-storage-token' in req.headers and \ 'x-auth-token' not in req.headers: req.headers['x-auth-token'] = req.headers['x-storage-token'] + te = req.headers.get('transfer-encoding', '').lower() + if te.rsplit(',', 1)[-1].strip() == 'chunked' and \ + 'content-length' in req.headers: + # RFC says if both are present, transfer-encoding wins. + # Definitely *don't* forward on the header the backend + # ought to ignore; that offers request-smuggling vectors. + del req.headers['content-length'] return req def handle_request(self, req): diff --git a/test/unit/proxy/controllers/test_obj.py b/test/unit/proxy/controllers/test_obj.py index 3a73b7b98b..17756968fe 100644 --- a/test/unit/proxy/controllers/test_obj.py +++ b/test/unit/proxy/controllers/test_obj.py @@ -1104,6 +1104,7 @@ class TestReplicatedObjController(CommonObjectControllerMixin, body = unchunk_body(body) self.assertEqual('100-continue', headers['Expect']) self.assertEqual('chunked', headers['Transfer-Encoding']) + self.assertNotIn('Content-Length', headers) else: self.assertNotIn('Transfer-Encoding', headers) if body or not test_body: diff --git a/test/unit/proxy/test_server.py b/test/unit/proxy/test_server.py index bcef252b7f..b99e75745a 100644 --- a/test/unit/proxy/test_server.py +++ b/test/unit/proxy/test_server.py @@ -3171,14 +3171,68 @@ class TestReplicatedObjectController( prolis = _test_sockets[0] sock = connect_tcp(('localhost', prolis.getsockname()[1])) fd = sock.makefile('rwb') - fd.write(b'PUT /v1/a/c/o.chunked HTTP/1.1\r\n' + with mock.patch('swift.obj.diskfile.fallocate') as mock_fallocate: + fd.write(b'PUT /v1/a/c/o.chunked HTTP/1.1\r\n' + b'Host: localhost\r\n' + b'Connection: keep-alive\r\n' + b'X-Storage-Token: t\r\n' + b'Content-Type: application/octet-stream\r\n' + b'Content-Length: 33\r\n' + b'Transfer-Encoding: chunked\r\n\r\n' + b'2\r\n' + b'oh\r\n' + b'4\r\n' + b' say\r\n' + b'4\r\n' + b' can\r\n' + b'4\r\n' + b' you\r\n' + b'4\r\n' + b' see\r\n' + b'3\r\n' + b' by\r\n' + b'4\r\n' + b' the\r\n' + b'8\r\n' + b' dawns\'\n\r\n' + b'0\r\n\r\n') + fd.flush() + headers = readuntil2crlfs(fd) + exp = b'HTTP/1.1 201' + self.assertEqual(headers[:len(exp)], exp) + self.assertFalse(mock_fallocate.mock_calls) + + fd.write(b'GET /v1/a/c/o.chunked HTTP/1.1\r\n' b'Host: localhost\r\n' b'Connection: close\r\n' b'X-Storage-Token: t\r\n' + b'\r\n') + fd.flush() + headers = readuntil2crlfs(fd) + exp = b'HTTP/1.1 200' + self.assertEqual(headers[:len(exp)], exp) + self.assertIn(b'Content-Length: 33', headers.split(b'\r\n')) + self.assertEqual(b"oh say can you see by the dawns'\n", fd.read(33)) + + @unpatch_policies + def test_PUT_message_length_using_both_with_crazy_meta(self): + prolis = _test_sockets[0] + sock = connect_tcp(('localhost', prolis.getsockname()[1])) + fd = sock.makefile('rwb') + fd.write(b'PUT /v1/a/c/o.chunked HTTP/1.1\r\n' + b'Host: localhost\r\n' + b'X-Storage-Token: t\r\n' b'Content-Type: application/octet-stream\r\n' b'Content-Length: 33\r\n' - b'Transfer-Encoding: chunked\r\n\r\n' - b'2\r\n' + b'X-Object-Meta-\xf0\x9f\x8c\xb4: \xf0\x9f\x91\x8d\r\n' + b'Expect: 100-continue\r\n' + b'Transfer-Encoding: chunked\r\n\r\n') + fd.flush() + headers = readuntil2crlfs(fd) + exp = b'HTTP/1.1 100 Continue' + self.assertEqual(headers[:len(exp)], exp) + # Since we got our 100 Continue, now we can send the body + fd.write(b'2\r\n' b'oh\r\n' b'4\r\n' b' say\r\n' @@ -3200,6 +3254,20 @@ class TestReplicatedObjectController( exp = b'HTTP/1.1 201' self.assertEqual(headers[:len(exp)], exp) + fd.write(b'GET /v1/a/c/o.chunked HTTP/1.1\r\n' + b'Host: localhost\r\n' + b'Connection: close\r\n' + b'X-Storage-Token: t\r\n' + b'\r\n') + fd.flush() + headers = readuntil2crlfs(fd) + exp = b'HTTP/1.1 200' + self.assertEqual(headers[:len(exp)], exp) + self.assertIn(b'Content-Length: 33', headers.split(b'\r\n')) + self.assertIn(b'X-Object-Meta-\xf0\x9f\x8c\xb4: \xf0\x9f\x91\x8d', + headers.split(b'\r\n')) + self.assertEqual(b"oh say can you see by the dawns'\n", fd.read(33)) + @unpatch_policies def test_PUT_bad_message_length(self): prolis = _test_sockets[0]