openstack/swift
Code Issues Proposed changes

Fix some request-smuggling vectors on py3

Browse Source 
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
This commit is contained in:
Tim Burke 2019-08-15 14:33:06 -07:00
parent 2abdd2d70d
commit bf9346d88d
4 changed files with 98 additions and 3 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

19
swift/common/wsgi.py
View File

@ -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

7
swift/proxy/server.py
View File

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

1
test/unit/proxy/controllers/test_obj.py
View File

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

74
test/unit/proxy/test_server.py
View File

@ -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]