Verify client input for v4 signatures

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).

Change-Id: I61eb12455c37376be4d739eee55a5f439216f0e9
Closes-Bug: 1765834

swift/common/middleware/s3api/s3request.py

@@ -24,7 +24,8 @@ import six
 from six.moves.urllib.parse import quote, unquote, parse_qsl
 import string
 
-from swift.common.utils import split_path, json, get_swift_info
+from swift.common.utils import split_path, json, get_swift_info, \
+    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 +111,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
@@ -401,6 +430,20 @@ class SigV4Mixin(object):
             raise InvalidRequest(msg)
         else:
             hashed_payload = self.headers['X-Amz-Content-SHA256']
+            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, 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')
 
@@ -1264,12 +1307,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

test/unit/common/middleware/s3api/test_obj.py

@@ -469,6 +469,60 @@ 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')
+        print(body)
+        self.assertEqual(self._get_error_code(body), 'BadDigest')
+
     def test_object_PUT_headers(self):
         content_md5 = self.etag.decode('hex').encode('base64').strip()
 

test/unit/common/middleware/s3api/test_s3request.py

@@ -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
@@ -802,5 +805,76 @@ class TestRequest(S3ApiTestCase):
             u'\u30c9\u30e9\u30b4\u30f3'))
 
 
+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()