Merge "Add more validation for pre-signed URL expiration"

swift3/request.py

@@ -46,7 +46,7 @@ from swift3.response import AccessDenied, InvalidArgument, InvalidDigest, \
     InternalError, NoSuchBucket, NoSuchKey, PreconditionFailed, InvalidRange, \
     MissingContentLength, InvalidStorageClass, S3NotImplemented, InvalidURI, \
     MalformedXML, InvalidRequest, RequestTimeout, InvalidBucketName, \
-    BadDigest, AuthorizationHeaderMalformed
+    BadDigest, AuthorizationHeaderMalformed, AuthorizationQueryParametersError
 from swift3.exception import NotS3Request, BadSwiftRequest
 from swift3.utils import utf8encode, LOGGER, check_path_header, S3Timestamp, \
     mktime
@@ -95,8 +95,6 @@ def _header_acl_property(resource):
 class SigV4Mixin(object):
     """
     A request class mixin to provide S3 signature v4 functionality
-
-    :param req_cls: a Request class (Request or S3AclRequest or child classes)
     """
 
     @property
@@ -139,8 +137,24 @@ class SigV4Mixin(object):
         """
         :param now: a S3Timestamp instance
         """
-        expires = self.params['X-Amz-Expires']
+        err = None
-        if int(self.timestamp) + int(expires) < S3Timestamp.now():
+        try:
+            expires = int(self.params['X-Amz-Expires'])
+        except ValueError:
+            err = 'X-Amz-Expires should be a number'
+        else:
+            if expires < 0:
+                err = 'X-Amz-Expires must be non-negative'
+            elif expires >= 2 ** 63:
+                err = 'X-Amz-Expires should be a number'
+            elif expires > 604800:
+                err = ('X-Amz-Expires must be less than a week (in seconds); '
+                       'that is, the given X-Amz-Expires must be less than '
+                       '604800 seconds')
+        if err:
+            raise AuthorizationQueryParametersError(err)
+
+        if int(self.timestamp) + expires < S3Timestamp.now():
             raise AccessDenied('Request has expired')
 
     def _parse_query_authentication(self):
@@ -288,7 +302,7 @@ class SigV4Mixin(object):
 
         # 5. Add signed headers into canonical request like
         # content-type;host;x-amz-date
-        cr.append(';'.join(sorted(n for n in headers_to_sign)))
+        cr.append(';'.join(sorted(headers_to_sign)))
 
         # 6. Add payload string at the tail
         if 'X-Amz-Credential' in self.params:
@@ -503,6 +517,11 @@ class Request(swob.Request):
         if S3Timestamp.now() > ex:
             raise AccessDenied('Request has expired')
 
+        if ex >= 2 ** 31:
+            raise AccessDenied(
+                'Invalid date (should be seconds since epoch): %s' %
+                self.params['Expires'])
+
     def _validate_dates(self):
         if self._is_query_auth:
             self._validate_expire_param()

swift3/response.py

@@ -230,6 +230,10 @@ class AuthorizationHeaderMalformed(ErrorResponse):
            'and Signature.'
 
 
+class AuthorizationQueryParametersError(ErrorResponse):
+    _status = '400 Bad Request'
+
+
 class BadDigest(ErrorResponse):
     _status = '400 Bad Request'
     _msg = 'The Content-MD5 you specified did not match what we received.'

swift3/test/functional/test_presigned.py

@@ -21,7 +21,7 @@ import requests
 from swift3.etree import fromstring
 from swift3.cfg import CONF
 from swift3.test.functional import Swift3FunctionalTestCase
-from swift3.test.functional.utils import get_error_code
+from swift3.test.functional.utils import get_error_code, get_error_msg
 
 
 class TestSwift3PresignedUrls(Swift3FunctionalTestCase):
@@ -96,6 +96,65 @@ class TestSwift3PresignedUrls(Swift3FunctionalTestCase):
         self.assertEqual(resp.status_code, 204,
                          'Got %d %s' % (resp.status_code, resp.content))
 
+    def test_expiration_limits(self):
+        if os.environ.get('S3_USE_SIGV4'):
+            self._test_expiration_limits_v4()
+        else:
+            self._test_expiration_limits_v2()
+
+    def _test_expiration_limits_v2(self):
+        bucket = 'test-bucket'
+
+        # Expiration date is too far in the future
+        url, headers = self.conn.generate_url_and_headers(
+            'GET', bucket, expires_in=2 ** 32)
+        resp = requests.get(url, headers=headers)
+        self.assertEqual(resp.status_code, 403,
+                         'Got %d %s' % (resp.status_code, resp.content))
+        self.assertEqual(get_error_code(resp.content),
+                         'AccessDenied')
+        self.assertIn('Invalid date (should be seconds since epoch)',
+                      get_error_msg(resp.content))
+
+    def _test_expiration_limits_v4(self):
+        bucket = 'test-bucket'
+
+        # Expiration is negative
+        url, headers = self.conn.generate_url_and_headers(
+            'GET', bucket, expires_in=-1)
+        resp = requests.get(url, headers=headers)
+        self.assertEqual(resp.status_code, 400,
+                         'Got %d %s' % (resp.status_code, resp.content))
+        self.assertEqual(get_error_code(resp.content),
+                         'AuthorizationQueryParametersError')
+        self.assertIn('X-Amz-Expires must be non-negative',
+                      get_error_msg(resp.content))
+
+        # Expiration date is too far in the future
+        for exp in (7 * 24 * 60 * 60 + 1,
+                    2 ** 63 - 1):
+            url, headers = self.conn.generate_url_and_headers(
+                'GET', bucket, expires_in=exp)
+            resp = requests.get(url, headers=headers)
+            self.assertEqual(resp.status_code, 400,
+                             'Got %d %s' % (resp.status_code, resp.content))
+            self.assertEqual(get_error_code(resp.content),
+                             'AuthorizationQueryParametersError')
+            self.assertIn('X-Amz-Expires must be less than 604800 seconds',
+                          get_error_msg(resp.content))
+
+        # Expiration date is *way* too far in the future, or isn't a number
+        for exp in (2 ** 63, 'foo'):
+            url, headers = self.conn.generate_url_and_headers(
+                'GET', bucket, expires_in=2 ** 63)
+            resp = requests.get(url, headers=headers)
+            self.assertEqual(resp.status_code, 400,
+                             'Got %d %s' % (resp.status_code, resp.content))
+            self.assertEqual(get_error_code(resp.content),
+                             'AuthorizationQueryParametersError')
+            self.assertEqual('X-Amz-Expires should be a number',
+                             get_error_msg(resp.content))
+
     def test_object(self):
         bucket = 'test-bucket'
         obj = 'object'