From c7273f3054eeed482c796a07313b1ccb37852db1 Mon Sep 17 00:00:00 2001
From: Tim Burke <tim.burke@gmail.com>
Date: Tue, 14 Jun 2016 16:35:08 -0700
Subject: [PATCH] Add more validation for pre-signed URL expiration
Apparently v2 signatures allow any timestamp that fits in a signed 32-bit
integer, while v4 signatures can be valid for at most a week. See also:
http://docs.aws.amazon.com/AmazonS3/latest/API/sigv4-query-string-auth.html
Pick up a couple minor cleanups while we're at it.
Change-Id: I4ebaa67607bac35a9df266ed624c97ad2c112c0d
---
swift3/request.py | 31 +++++++++---
swift3/response.py | 4 ++
swift3/test/functional/test_presigned.py | 61 +++++++++++++++++++++++-
3 files changed, 89 insertions(+), 7 deletions(-)
diff --git a/swift3/request.py b/swift3/request.py
index bcee9865..24576ecf 100644
--- a/swift3/request.py
+++ b/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']
- if int(self.timestamp) + int(expires) < S3Timestamp.now():
+ err = None
+ 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()
diff --git a/swift3/response.py b/swift3/response.py
index 5482ef8b..8f9cf0fa 100644
--- a/swift3/response.py
+++ b/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.'
diff --git a/swift3/test/functional/test_presigned.py b/swift3/test/functional/test_presigned.py
index 77628bf3..49643887 100644
--- a/swift3/test/functional/test_presigned.py
+++ b/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'