Skip to content

Commit bf9346d

Browse files
tipabutipabu
committed
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
1 parent 2abdd2d commit bf9346d

File tree

4 files changed

+98
-3
lines changed

4 files changed

+98
-3
lines changed

‎swift/common/wsgi.py‎

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -480,6 +480,11 @@ def get_environ(self, *args, **kwargs):
480480
break
481481
header, value = line.split(':', 1)
482482
value = value.strip(' \t\n\r')
483+
# NB: Eventlet looks at the headers obj to figure out
484+
# whether the client said the connection should close;
485+
# see https://github.com/eventlet/eventlet/blob/v0.25.0/
486+
# eventlet/wsgi.py#L504
487+
self.headers.add_header(header, value)
483488
headers_raw.append((header, value))
484489
wsgi_key = 'HTTP_' + header.replace('-', '_').encode(
485490
'latin1').upper().decode('latin1')
@@ -488,6 +493,20 @@ def get_environ(self, *args, **kwargs):
488493
wsgi_key = wsgi_key[5:]
489494
environ[wsgi_key] = value
490495
environ['headers_raw'] = tuple(headers_raw)
496+
# Since we parsed some more headers, check to see if they
497+
# change how our wsgi.input should behave
498+
te = environ.get('HTTP_TRANSFER_ENCODING', '').lower()
499+
if te.rsplit(',', 1)[-1].strip() == 'chunked':
500+
environ['wsgi.input'].chunked_input = True
501+
else:
502+
length = environ.get('CONTENT_LENGTH')
503+
if length:
504+
length = int(length)
505+
environ['wsgi.input'].content_length = length
506+
if environ.get('HTTP_EXPECT', '').lower() == '100-continue':
507+
environ['wsgi.input'].wfile = self.wfile
508+
environ['wsgi.input'].wfile_line = \
509+
b'HTTP/1.1 100 Continue\r\n'
491510
return environ
492511

493512

‎swift/proxy/server.py‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -457,6 +457,13 @@ def update_request(self, req):
457457
if 'x-storage-token' in req.headers and \
458458
'x-auth-token' not in req.headers:
459459
req.headers['x-auth-token'] = req.headers['x-storage-token']
460+
te = req.headers.get('transfer-encoding', '').lower()
461+
if te.rsplit(',', 1)[-1].strip() == 'chunked' and \
462+
'content-length' in req.headers:
463+
# RFC says if both are present, transfer-encoding wins.
464+
# Definitely *don't* forward on the header the backend
465+
# ought to ignore; that offers request-smuggling vectors.
466+
del req.headers['content-length']
460467
return req
461468

462469
def handle_request(self, req):

‎test/unit/proxy/controllers/test_obj.py‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1104,6 +1104,7 @@ def capture_headers(ip, port, device, part, method, path, headers,
11041104
body = unchunk_body(body)
11051105
self.assertEqual('100-continue', headers['Expect'])
11061106
self.assertEqual('chunked', headers['Transfer-Encoding'])
1107+
self.assertNotIn('Content-Length', headers)
11071108
else:
11081109
self.assertNotIn('Transfer-Encoding', headers)
11091110
if body or not test_body:

‎test/unit/proxy/test_server.py‎

Lines changed: 71 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -3171,14 +3171,68 @@ def test_PUT_message_length_using_both(self):
31713171
prolis = _test_sockets[0]
31723172
sock = connect_tcp(('localhost', prolis.getsockname()[1]))
31733173
fd = sock.makefile('rwb')
3174-
fd.write(b'PUT /v1/a/c/o.chunked HTTP/1.1\r\n'
3174+
with mock.patch('swift.obj.diskfile.fallocate') as mock_fallocate:
3175+
fd.write(b'PUT /v1/a/c/o.chunked HTTP/1.1\r\n'
3176+
b'Host: localhost\r\n'
3177+
b'Connection: keep-alive\r\n'
3178+
b'X-Storage-Token: t\r\n'
3179+
b'Content-Type: application/octet-stream\r\n'
3180+
b'Content-Length: 33\r\n'
3181+
b'Transfer-Encoding: chunked\r\n\r\n'
3182+
b'2\r\n'
3183+
b'oh\r\n'
3184+
b'4\r\n'
3185+
b' say\r\n'
3186+
b'4\r\n'
3187+
b' can\r\n'
3188+
b'4\r\n'
3189+
b' you\r\n'
3190+
b'4\r\n'
3191+
b' see\r\n'
3192+
b'3\r\n'
3193+
b' by\r\n'
3194+
b'4\r\n'
3195+
b' the\r\n'
3196+
b'8\r\n'
3197+
b' dawns\'\n\r\n'
3198+
b'0\r\n\r\n')
3199+
fd.flush()
3200+
headers = readuntil2crlfs(fd)
3201+
exp = b'HTTP/1.1 201'
3202+
self.assertEqual(headers[:len(exp)], exp)
3203+
self.assertFalse(mock_fallocate.mock_calls)
3204+
3205+
fd.write(b'GET /v1/a/c/o.chunked HTTP/1.1\r\n'
31753206
b'Host: localhost\r\n'
31763207
b'Connection: close\r\n'
31773208
b'X-Storage-Token: t\r\n'
3209+
b'\r\n')
3210+
fd.flush()
3211+
headers = readuntil2crlfs(fd)
3212+
exp = b'HTTP/1.1 200'
3213+
self.assertEqual(headers[:len(exp)], exp)
3214+
self.assertIn(b'Content-Length: 33', headers.split(b'\r\n'))
3215+
self.assertEqual(b"oh say can you see by the dawns'\n", fd.read(33))
3216+
3217+
@unpatch_policies
3218+
def test_PUT_message_length_using_both_with_crazy_meta(self):
3219+
prolis = _test_sockets[0]
3220+
sock = connect_tcp(('localhost', prolis.getsockname()[1]))
3221+
fd = sock.makefile('rwb')
3222+
fd.write(b'PUT /v1/a/c/o.chunked HTTP/1.1\r\n'
3223+
b'Host: localhost\r\n'
3224+
b'X-Storage-Token: t\r\n'
31783225
b'Content-Type: application/octet-stream\r\n'
31793226
b'Content-Length: 33\r\n'
3180-
b'Transfer-Encoding: chunked\r\n\r\n'
3181-
b'2\r\n'
3227+
b'X-Object-Meta-\xf0\x9f\x8c\xb4: \xf0\x9f\x91\x8d\r\n'
3228+
b'Expect: 100-continue\r\n'
3229+
b'Transfer-Encoding: chunked\r\n\r\n')
3230+
fd.flush()
3231+
headers = readuntil2crlfs(fd)
3232+
exp = b'HTTP/1.1 100 Continue'
3233+
self.assertEqual(headers[:len(exp)], exp)
3234+
# Since we got our 100 Continue, now we can send the body
3235+
fd.write(b'2\r\n'
31823236
b'oh\r\n'
31833237
b'4\r\n'
31843238
b' say\r\n'
@@ -3200,6 +3254,20 @@ def test_PUT_message_length_using_both(self):
32003254
exp = b'HTTP/1.1 201'
32013255
self.assertEqual(headers[:len(exp)], exp)
32023256

3257+
fd.write(b'GET /v1/a/c/o.chunked HTTP/1.1\r\n'
3258+
b'Host: localhost\r\n'
3259+
b'Connection: close\r\n'
3260+
b'X-Storage-Token: t\r\n'
3261+
b'\r\n')
3262+
fd.flush()
3263+
headers = readuntil2crlfs(fd)
3264+
exp = b'HTTP/1.1 200'
3265+
self.assertEqual(headers[:len(exp)], exp)
3266+
self.assertIn(b'Content-Length: 33', headers.split(b'\r\n'))
3267+
self.assertIn(b'X-Object-Meta-\xf0\x9f\x8c\xb4: \xf0\x9f\x91\x8d',
3268+
headers.split(b'\r\n'))
3269+
self.assertEqual(b"oh say can you see by the dawns'\n", fd.read(33))
3270+
32033271
@unpatch_policies
32043272
def test_PUT_bad_message_length(self):
32053273
prolis = _test_sockets[0]

0 commit comments

Comments
 (0)