openstack
/
swift
Code Issues Proposed changes

Verify client input for v4 signatures 

This is a combination of 2 commits.

==========

Previously, we would use the X-Amz-Content-SHA256 value when calculating
signatures, but wouldn't actually check the content that was sent. This
would allow a malicious third party that managed to capture the headers
for an object upload to overwrite that with arbitrary content provided
they could do so within the 5-minute clock-skew window.

Now, we wrap the wsgi.input that's sent on to the proxy-server app to
hash content as it's read and raise an error if there's a mismatch. Note
that clients using presigned-urls to upload have no defense against a
similar replay attack.

Notwithstanding the above security consideration, this *also* provides
better assurances that the client's payload was received correctly. Note
that this *does not* attempt to send an etag in footers, however, so the
proxy-to-object-server connection is not guarded against bit-flips.

In the future, Swift will hopefully grow a way to perform SHA256
verification on the object-server. This would offer two main benefits:

  - End-to-end message integrity checking.
  - Move CPU load of calculating the hash from the proxy (which is
    somewhat CPU-bound) to the object-server (which tends to have CPU to
    spare).

Closes-Bug: 1765834
(cherry picked from commit 3a8f5dbf9c)

----------

s3api: Allow clients to upload with UNSIGNED-PAYLOAD

(Some versions of?) awscli/boto3 will do v4 signatures but send a
Content-MD5 for end-to-end validation. Since a X-Amz-Content-SHA256
is still required to calculate signatures, it uses UNSIGNED-PAYLOAD
similar to how signatures work for pre-signed URLs.

Look for UNSIGNED-PAYLOAD and skip SHA256 validation if set.

(cherry picked from commit 82e446a8a0)
(cherry picked from commit 6ed165cf3f)

==========

Change-Id: I61eb12455c37376be4d739eee55a5f439216f0e9
This commit is contained in:
Tim Burke 2018-12-11 15:29:35 -08:00 committed by Tim Burke
parent c6e2fbb417
commit 423f96293b
3 changed files with 211 additions and 8 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

60
swift/common/middleware/s3api/s3request.py
View File

@ -24,7 +24,7 @@ import six
from six.moves.urllib.parse import quote, unquote, parse_qsl
import string
from swift.common.utils import split_path
from swift.common.utils import split_path, close_if_possible
from swift.common import swob
from swift.common.http import HTTP_OK, HTTP_CREATED, HTTP_ACCEPTED, \
HTTP_NO_CONTENT, HTTP_UNAUTHORIZED, HTTP_FORBIDDEN, HTTP_NOT_FOUND, \
@ -110,6 +110,34 @@ def _header_acl_property(resource):
doc='Get and set the %s acl property' % resource)
class HashingInput(object):
"""
wsgi.input wrapper to verify the hash of the input as it's read.
"""
def __init__(self, reader, content_length, hasher, expected_hex_hash):
self._input = reader
self._to_read = content_length
self._hasher = hasher()
self._expected = expected_hex_hash
def read(self, size=None):
chunk = self._input.read(size)
self._hasher.update(chunk)
self._to_read -= len(chunk)
if self._to_read < 0 or (size > len(chunk) and self._to_read) or (
self._to_read == 0 and
self._hasher.hexdigest() != self._expected):
self.close()
# Since we don't return the last chunk, the PUT never completes
raise swob.HTTPUnprocessableEntity(
'The X-Amz-Content-SHA56 you specified did not match '
'what we received.')
return chunk
def close(self):
close_if_possible(self._input)
class SigV4Mixin(object):
"""
A request class mixin to provide S3 signature v4 functionality
@ -358,6 +386,21 @@ class SigV4Mixin(object):
raise InvalidRequest(msg)
else:
hashed_payload = self.headers['X-Amz-Content-SHA256']
if hashed_payload != 'UNSIGNED-PAYLOAD':
if self.content_length == 0:
if hashed_payload != sha256().hexdigest():
raise BadDigest(
'The X-Amz-Content-SHA56 you specified did not '
'match what we received.')
elif self.content_length:
self.environ['wsgi.input'] = HashingInput(
self.environ['wsgi.input'],
self.content_length,
sha256,
hashed_payload)
# else, length not provided -- Swift will kick out a
# 411 Length Required which will get translated back
# to a S3-style response in S3Request._swift_error_codes
cr.append(hashed_payload)
return '\n'.join(cr).encode('utf-8')
@ -1189,12 +1232,15 @@ class S3Request(swob.Request):
sw_req = self.to_swift_req(method, container, obj, headers=headers,
body=body, query=query)
sw_resp = sw_req.get_response(app)
# reuse account and tokens
_, self.account, _ = split_path(sw_resp.environ['PATH_INFO'],
2, 3, True)
self.account = utf8encode(self.account)
try:
sw_resp = sw_req.get_response(app)
except swob.HTTPException as err:
sw_resp = err
else:
# reuse account and tokens
_, self.account, _ = split_path(sw_resp.environ['PATH_INFO'],
2, 3, True)
self.account = utf8encode(self.account)
resp = S3Response.from_swift_resp(sw_resp)
status = resp.status_int # pylint: disable-msg=E1101

82
test/unit/common/middleware/s3api/test_obj.py
View File

@ -485,6 +485,88 @@ class TestS3ApiObj(S3ApiTestCase):
# Check that s3api converts a Content-MD5 header into an etag.
self.assertEqual(headers['etag'], etag)
@s3acl
def test_object_PUT_v4(self):
body_sha = hashlib.sha256(self.object_body).hexdigest()
req = Request.blank(
'/bucket/object',
environ={'REQUEST_METHOD': 'PUT'},
headers={
'Authorization':
'AWS4-HMAC-SHA256 '
'Credential=test:tester/%s/us-east-1/s3/aws4_request, '
'SignedHeaders=host;x-amz-date, '
'Signature=hmac' % (
self.get_v4_amz_date_header().split('T', 1)[0]),
'x-amz-date': self.get_v4_amz_date_header(),
'x-amz-storage-class': 'STANDARD',
'x-amz-content-sha256': body_sha,
'Date': self.get_date_header()},
body=self.object_body)
req.date = datetime.now()
req.content_type = 'text/plain'
status, headers, body = self.call_s3api(req)
self.assertEqual(status.split()[0], '200')
# Check that s3api returns an etag header.
self.assertEqual(headers['etag'],
'"%s"' % self.response_headers['etag'])
_, _, headers = self.swift.calls_with_headers[-1]
# No way to determine ETag to send
self.assertNotIn('etag', headers)
@s3acl
def test_object_PUT_v4_bad_hash(self):
req = Request.blank(
'/bucket/object',
environ={'REQUEST_METHOD': 'PUT'},
headers={
'Authorization':
'AWS4-HMAC-SHA256 '
'Credential=test:tester/%s/us-east-1/s3/aws4_request, '
'SignedHeaders=host;x-amz-date, '
'Signature=hmac' % (
self.get_v4_amz_date_header().split('T', 1)[0]),
'x-amz-date': self.get_v4_amz_date_header(),
'x-amz-storage-class': 'STANDARD',
'x-amz-content-sha256': 'not the hash',
'Date': self.get_date_header()},
body=self.object_body)
req.date = datetime.now()
req.content_type = 'text/plain'
status, headers, body = self.call_s3api(req)
self.assertEqual(status.split()[0], '400')
self.assertEqual(self._get_error_code(body), 'BadDigest')
@s3acl
def test_object_PUT_v4_unsigned_payload(self):
req = Request.blank(
'/bucket/object',
environ={'REQUEST_METHOD': 'PUT'},
headers={
'Authorization':
'AWS4-HMAC-SHA256 '
'Credential=test:tester/%s/us-east-1/s3/aws4_request, '
'SignedHeaders=host;x-amz-date, '
'Signature=hmac' % (
self.get_v4_amz_date_header().split('T', 1)[0]),
'x-amz-date': self.get_v4_amz_date_header(),
'x-amz-storage-class': 'STANDARD',
'x-amz-content-sha256': 'UNSIGNED-PAYLOAD',
'Date': self.get_date_header()},
body=self.object_body)
req.date = datetime.now()
req.content_type = 'text/plain'
status, headers, body = self.call_s3api(req)
self.assertEqual(status.split()[0], '200')
# Check that s3api returns an etag header.
self.assertEqual(headers['etag'],
'"%s"' % self.response_headers['etag'])
_, _, headers = self.swift.calls_with_headers[-1]
# No way to determine ETag to send
self.assertNotIn('etag', headers)
def test_object_PUT_headers(self):
content_md5 = self.etag.decode('hex').encode('base64').strip()

77
test/unit/common/middleware/s3api/test_s3request.py
View File

@ -13,9 +13,12 @@
# See the License for the specific language governing permissions and
# limitations under the License.
import hashlib
from mock import patch, MagicMock
import unittest
from six import BytesIO
from swift.common import swob
from swift.common.swob import Request, HTTPNoContent
from swift.common.middleware.s3api.utils import mktime
@ -24,7 +27,7 @@ from swift.common.middleware.s3api.subresource import ACL, User, Owner, \
Grant, encode_acl
from test.unit.common.middleware.s3api.test_s3api import S3ApiTestCase
from swift.common.middleware.s3api.s3request import S3Request, \
S3AclRequest, SigV4Request, SIGV4_X_AMZ_DATE_FORMAT
S3AclRequest, SigV4Request, SIGV4_X_AMZ_DATE_FORMAT, HashingInput
from swift.common.middleware.s3api.s3response import InvalidArgument, \
NoSuchBucket, InternalError, \
AccessDenied, SignatureDoesNotMatch, RequestTimeTooSkewed
@ -761,5 +764,77 @@ class TestRequest(S3ApiTestCase):
self.assertTrue(sigv2_req.check_signature(
'wJalrXUtnFEMI/K7MDENG/bPxRfiCYEXAMPLEKEY'))
class TestHashingInput(S3ApiTestCase):
def test_good(self):
raw = b'123456789'
wrapped = HashingInput(BytesIO(raw), 9, hashlib.md5,
hashlib.md5(raw).hexdigest())
self.assertEqual(b'1234', wrapped.read(4))
self.assertEqual(b'56', wrapped.read(2))
# trying to read past the end gets us whatever's left
self.assertEqual(b'789', wrapped.read(4))
# can continue trying to read -- but it'll be empty
self.assertEqual(b'', wrapped.read(2))
self.assertFalse(wrapped._input.closed)
wrapped.close()
self.assertTrue(wrapped._input.closed)
def test_empty(self):
wrapped = HashingInput(BytesIO(b''), 0, hashlib.sha256,
hashlib.sha256(b'').hexdigest())
self.assertEqual(b'', wrapped.read(4))
self.assertEqual(b'', wrapped.read(2))
self.assertFalse(wrapped._input.closed)
wrapped.close()
self.assertTrue(wrapped._input.closed)
def test_too_long(self):
raw = b'123456789'
wrapped = HashingInput(BytesIO(raw), 8, hashlib.md5,
hashlib.md5(raw).hexdigest())
self.assertEqual(b'1234', wrapped.read(4))
self.assertEqual(b'56', wrapped.read(2))
# even though the hash matches, there was more data than we expected
with self.assertRaises(swob.Response) as raised:
wrapped.read(3)
self.assertEqual(raised.exception.status, '422 Unprocessable Entity')
# the error causes us to close the input
self.assertTrue(wrapped._input.closed)
def test_too_short(self):
raw = b'123456789'
wrapped = HashingInput(BytesIO(raw), 10, hashlib.md5,
hashlib.md5(raw).hexdigest())
self.assertEqual(b'1234', wrapped.read(4))
self.assertEqual(b'56', wrapped.read(2))
# even though the hash matches, there was more data than we expected
with self.assertRaises(swob.Response) as raised:
wrapped.read(4)
self.assertEqual(raised.exception.status, '422 Unprocessable Entity')
self.assertTrue(wrapped._input.closed)
def test_bad_hash(self):
raw = b'123456789'
wrapped = HashingInput(BytesIO(raw), 9, hashlib.sha256,
hashlib.md5(raw).hexdigest())
self.assertEqual(b'1234', wrapped.read(4))
self.assertEqual(b'5678', wrapped.read(4))
with self.assertRaises(swob.Response) as raised:
wrapped.read(4)
self.assertEqual(raised.exception.status, '422 Unprocessable Entity')
self.assertTrue(wrapped._input.closed)
def test_empty_bad_hash(self):
wrapped = HashingInput(BytesIO(b''), 0, hashlib.sha256, 'nope')
with self.assertRaises(swob.Response) as raised:
wrapped.read(3)
self.assertEqual(raised.exception.status, '422 Unprocessable Entity')
# the error causes us to close the input
self.assertTrue(wrapped._input.closed)
if __name__ == '__main__':
unittest.main()