Skip to content

Commit

Permalink
Better scoping for tempurls, especially container tempurls
Browse files Browse the repository at this point in the history
It used to be that a GET of a tempurl referencing a large object would
let you download that large object regardless of where its segments
lived. However, this led to some violated user expectations around
container tempurls.

(Note on shorthand: all tempurls reference objects. However, "account
tempurl" and "container tempurl" are shorthand meaning tempurls
generated using a key on the account or container, respectively.)

Let's say an application is given tempurl keys to a particular
container, and it does all its work therein using those keys. The user
expects that, if the application is compromised, then the attacker
only gains access to the "compromised-container". However, with the old
behavior, the attacker could read data from *any* container like so:

1) Choose a "victim-container" to download

2) Create PUT and GET tempurl for any object name within the
   "compromised-container". The object doesn't need to exist;
   we'll create it.

3) Using the PUT tempurl, upload a DLO manifest with
   "X-Object-Manifest: /victim-container/"

4) Using the GET tempurl, download the object created in step 3. The
   result will be the concatenation of all objects in the
   "victim-container".

Step 3 need not be for all objects in the "victim-container"; for
example, a value "X-Object-Manifest: /victim-container/abc" would only
be the concatenation of all objects whose names begin with "abc". By
probing for object names in this way, individual objects may be found
and extracted.

A similar bug would exist for manifests referencing other accounts
except that neither the X-Object-Manifest (DLO) nor the JSON manifest
document (SLO) have a way of specifying a different account.

This change makes it so that a container tempurl only grants access to
objects within its container, *including* large-object segments. This
breaks backward compatibility for container tempurls that may have
pointed to cross container *LO's, but (a) there are security
implications, and (b) container tempurls are a relatively new feature.

This works by having the tempurl middleware install an authorization
callback ('swift.authorize' in the WSGI environment) that limits the
scope of any requests to the account or container from which the key
came.

This requires swift.authorize to persist for both the manifest request
and all segment requests; this is done by having the proxy server
restore it to the WSGI environment prior to returning from __call__.

[CVE-2015-5223]

Co-Authored-By: Clay Gerrard <[email protected]>
Co-Authored-By: Alistair Coles <[email protected]>
Co-Authored-By: Christian Schwede <[email protected]>
Co-Authored-By: Matthew Oliver <[email protected]>

Change-Id: Ie6d52f7a07e87f6fec21ed8b0ec1d84be8b2b11c
Closes-Bug: 1449212
  • Loading branch information
smerritt authored and notmyname committed Aug 26, 2015
1 parent 10b2939 commit d4409c0
Show file tree
Hide file tree
Showing 4 changed files with 333 additions and 68 deletions.
105 changes: 80 additions & 25 deletions swift/common/middleware/tempurl.py
Original file line number Diff line number Diff line change
Expand Up @@ -152,6 +152,10 @@
DEFAULT_OUTGOING_ALLOW_HEADERS = 'x-object-meta-public-*'


CONTAINER_SCOPE = 'container'
ACCOUNT_SCOPE = 'account'


def get_tempurl_keys_from_metadata(meta):
"""
Extracts the tempurl keys from metadata.
Expand All @@ -172,6 +176,38 @@ def disposition_format(filename):
quote(filename, safe=' /'), quote(filename))


def authorize_same_account(account_to_match):

def auth_callback_same_account(req):
try:
_ver, acc, _rest = req.split_path(2, 3, True)
except ValueError:
return HTTPUnauthorized(request=req)

if acc == account_to_match:
return None
else:
return HTTPUnauthorized(request=req)

return auth_callback_same_account


def authorize_same_container(account_to_match, container_to_match):

def auth_callback_same_container(req):
try:
_ver, acc, con, _rest = req.split_path(3, 4, True)
except ValueError:
return HTTPUnauthorized(request=req)

if acc == account_to_match and con == container_to_match:
return None
else:
return HTTPUnauthorized(request=req)

return auth_callback_same_container


class TempURL(object):
"""
WSGI Middleware to grant temporary URLs specific access to Swift
Expand Down Expand Up @@ -304,10 +340,10 @@ def __call__(self, env, start_response):
return self.app(env, start_response)
if not temp_url_sig or not temp_url_expires:
return self._invalid(env, start_response)
account = self._get_account(env)
account, container = self._get_account_and_container(env)
if not account:
return self._invalid(env, start_response)
keys = self._get_keys(env, account)
keys = self._get_keys(env)
if not keys:
return self._invalid(env, start_response)
if env['REQUEST_METHOD'] == 'HEAD':
Expand All @@ -322,11 +358,16 @@ def __call__(self, env, start_response):
else:
hmac_vals = self._get_hmacs(env, temp_url_expires, keys)

# While it's true that any() will short-circuit, this doesn't affect
# the timing-attack resistance since the only way this will
# short-circuit is when a valid signature is passed in.
is_valid_hmac = any(streq_const_time(temp_url_sig, hmac)
for hmac in hmac_vals)
is_valid_hmac = False
hmac_scope = None
for hmac, scope in hmac_vals:
# While it's true that we short-circuit, this doesn't affect the
# timing-attack resistance since the only way this will
# short-circuit is when a valid signature is passed in.
if streq_const_time(temp_url_sig, hmac):
is_valid_hmac = True
hmac_scope = scope
break
if not is_valid_hmac:
return self._invalid(env, start_response)
# disallowed headers prevent accidently allowing upload of a pointer
Expand All @@ -337,7 +378,12 @@ def __call__(self, env, start_response):
if resp:
return resp
self._clean_incoming_headers(env)
env['swift.authorize'] = lambda req: None

if hmac_scope == ACCOUNT_SCOPE:
env['swift.authorize'] = authorize_same_account(account)
else:
env['swift.authorize'] = authorize_same_container(account,
container)
env['swift.authorize_override'] = True
env['REMOTE_USER'] = '.wsgi.tempurl'
qs = {'temp_url_sig': temp_url_sig,
Expand Down Expand Up @@ -378,22 +424,23 @@ def _start_response(status, headers, exc_info=None):

return self.app(env, _start_response)

def _get_account(self, env):
def _get_account_and_container(self, env):
"""
Returns just the account for the request, if it's an object
request and one of the configured methods; otherwise, None is
Returns just the account and container for the request, if it's an
object request and one of the configured methods; otherwise, None is
returned.
:param env: The WSGI environment for the request.
:returns: Account str or None.
:returns: (Account str, container str) or (None, None).
"""
if env['REQUEST_METHOD'] in self.methods:
try:
ver, acc, cont, obj = split_path(env['PATH_INFO'], 4, 4, True)
except ValueError:
return None
return (None, None)
if ver == 'v1' and obj.strip('/'):
return acc
return (acc, cont)
return (None, None)

def _get_temp_url_info(self, env):
"""
Expand Down Expand Up @@ -423,18 +470,23 @@ def _get_temp_url_info(self, env):
inline = True
return temp_url_sig, temp_url_expires, filename, inline

def _get_keys(self, env, account):
def _get_keys(self, env):
"""
Returns the X-[Account|Container]-Meta-Temp-URL-Key[-2] header values
for the account or container, or an empty list if none are set.
for the account or container, or an empty list if none are set. Each
value comes as a 2-tuple (key, scope), where scope is either
CONTAINER_SCOPE or ACCOUNT_SCOPE.
Returns 0-4 elements depending on how many keys are set in the
account's or container's metadata.
:param env: The WSGI environment for the request.
:param account: Account str.
:returns: [X-Account-Meta-Temp-URL-Key str value if set,
X-Account-Meta-Temp-URL-Key-2 str value if set]
:returns: [
(X-Account-Meta-Temp-URL-Key str value, ACCOUNT_SCOPE) if set,
(X-Account-Meta-Temp-URL-Key-2 str value, ACCOUNT_SCOPE if set,
(X-Container-Meta-Temp-URL-Key str value, CONTAINER_SCOPE) if set,
(X-Container-Meta-Temp-URL-Key-2 str value, CONTAINER_SCOPE if set,
]
"""
account_info = get_account_info(env, self.app, swift_source='TU')
account_keys = get_tempurl_keys_from_metadata(account_info['meta'])
Expand All @@ -443,25 +495,28 @@ def _get_keys(self, env, account):
container_keys = get_tempurl_keys_from_metadata(
container_info.get('meta', []))

return account_keys + container_keys
return ([(ak, ACCOUNT_SCOPE) for ak in account_keys] +
[(ck, CONTAINER_SCOPE) for ck in container_keys])

def _get_hmacs(self, env, expires, keys, request_method=None):
def _get_hmacs(self, env, expires, scoped_keys, request_method=None):
"""
:param env: The WSGI environment for the request.
:param expires: Unix timestamp as an int for when the URL
expires.
:param keys: Key strings, from the X-Account-Meta-Temp-URL-Key[-2] of
the account.
:param scoped_keys: (key, scope) tuples like _get_keys() returns
:param request_method: Optional override of the request in
the WSGI env. For example, if a HEAD
does not match, you may wish to
override with GET to still allow the
HEAD.
:returns: a list of (hmac, scope) 2-tuples
"""
if not request_method:
request_method = env['REQUEST_METHOD']
return [get_hmac(
request_method, env['PATH_INFO'], expires, key) for key in keys]
return [
(get_hmac(request_method, env['PATH_INFO'], expires, key), scope)
for (key, scope) in scoped_keys]

def _invalid(self, env, start_response):
"""
Expand Down
11 changes: 9 additions & 2 deletions swift/proxy/server.py
Original file line number Diff line number Diff line change
Expand Up @@ -381,6 +381,7 @@ def handle_request(self, req):
allowed_methods = getattr(controller, 'allowed_methods', set())
return HTTPMethodNotAllowed(
request=req, headers={'Allow': ', '.join(allowed_methods)})
old_authorize = None
if 'swift.authorize' in req.environ:
# We call authorize before the handler, always. If authorized,
# we remove the swift.authorize hook so isn't ever called
Expand All @@ -391,7 +392,7 @@ def handle_request(self, req):
if not resp and not req.headers.get('X-Copy-From-Account') \
and not req.headers.get('Destination-Account'):
# No resp means authorized, no delayed recheck required.
del req.environ['swift.authorize']
old_authorize = req.environ['swift.authorize']
else:
# Response indicates denial, but we might delay the denial
# and recheck later. If not delayed, return the error now.
Expand All @@ -401,7 +402,13 @@ def handle_request(self, req):
# gets mutated during handling. This way logging can display the
# method the client actually sent.
req.environ['swift.orig_req_method'] = req.method
return handler(req)
try:
if old_authorize:
req.environ.pop('swift.authorize', None)
return handler(req)
finally:
if old_authorize:
req.environ['swift.authorize'] = old_authorize
except HTTPException as error_response:
return error_response
except (Exception, Timeout):
Expand Down
114 changes: 114 additions & 0 deletions test/functional/tests.py
Original file line number Diff line number Diff line change
Expand Up @@ -3090,6 +3090,59 @@ def test_GET_with_key_2(self):
contents = self.env.obj.read(parms=parms, cfg={'no_auth_token': True})
self.assertEqual(contents, "obj contents")

def test_GET_DLO_inside_container(self):
seg1 = self.env.container.file(
"get-dlo-inside-seg1" + Utils.create_name())
seg2 = self.env.container.file(
"get-dlo-inside-seg2" + Utils.create_name())
seg1.write("one fish two fish ")
seg2.write("red fish blue fish")

manifest = self.env.container.file("manifest" + Utils.create_name())
manifest.write(
'',
hdrs={"X-Object-Manifest": "%s/get-dlo-inside-seg" %
(self.env.container.name,)})

expires = int(time.time()) + 86400
sig = self.tempurl_sig(
'GET', expires, self.env.conn.make_path(manifest.path),
self.env.tempurl_key)
parms = {'temp_url_sig': sig,
'temp_url_expires': str(expires)}

contents = manifest.read(parms=parms, cfg={'no_auth_token': True})
self.assertEqual(contents, "one fish two fish red fish blue fish")

def test_GET_DLO_outside_container(self):
seg1 = self.env.container.file(
"get-dlo-outside-seg1" + Utils.create_name())
seg2 = self.env.container.file(
"get-dlo-outside-seg2" + Utils.create_name())
seg1.write("one fish two fish ")
seg2.write("red fish blue fish")

container2 = self.env.account.container(Utils.create_name())
container2.create()

manifest = container2.file("manifest" + Utils.create_name())
manifest.write(
'',
hdrs={"X-Object-Manifest": "%s/get-dlo-outside-seg" %
(self.env.container.name,)})

expires = int(time.time()) + 86400
sig = self.tempurl_sig(
'GET', expires, self.env.conn.make_path(manifest.path),
self.env.tempurl_key)
parms = {'temp_url_sig': sig,
'temp_url_expires': str(expires)}

# cross container tempurl works fine for account tempurl key
contents = manifest.read(parms=parms, cfg={'no_auth_token': True})
self.assertEqual(contents, "one fish two fish red fish blue fish")
self.assert_status([200])

def test_PUT(self):
new_obj = self.env.container.file(Utils.create_name())

Expand Down Expand Up @@ -3422,6 +3475,67 @@ def test_tempurl_keys_hidden_from_acl_readonly(self):
'Container TempURL key-2 found, should not be visible '
'to readonly ACLs')

def test_GET_DLO_inside_container(self):
seg1 = self.env.container.file(
"get-dlo-inside-seg1" + Utils.create_name())
seg2 = self.env.container.file(
"get-dlo-inside-seg2" + Utils.create_name())
seg1.write("one fish two fish ")
seg2.write("red fish blue fish")

manifest = self.env.container.file("manifest" + Utils.create_name())
manifest.write(
'',
hdrs={"X-Object-Manifest": "%s/get-dlo-inside-seg" %
(self.env.container.name,)})

expires = int(time.time()) + 86400
sig = self.tempurl_sig(
'GET', expires, self.env.conn.make_path(manifest.path),
self.env.tempurl_key)
parms = {'temp_url_sig': sig,
'temp_url_expires': str(expires)}

contents = manifest.read(parms=parms, cfg={'no_auth_token': True})
self.assertEqual(contents, "one fish two fish red fish blue fish")

def test_GET_DLO_outside_container(self):
container2 = self.env.account.container(Utils.create_name())
container2.create()
seg1 = container2.file(
"get-dlo-outside-seg1" + Utils.create_name())
seg2 = container2.file(
"get-dlo-outside-seg2" + Utils.create_name())
seg1.write("one fish two fish ")
seg2.write("red fish blue fish")

manifest = self.env.container.file("manifest" + Utils.create_name())
manifest.write(
'',
hdrs={"X-Object-Manifest": "%s/get-dlo-outside-seg" %
(container2.name,)})

expires = int(time.time()) + 86400
sig = self.tempurl_sig(
'GET', expires, self.env.conn.make_path(manifest.path),
self.env.tempurl_key)
parms = {'temp_url_sig': sig,
'temp_url_expires': str(expires)}

# cross container tempurl does not work for container tempurl key
try:
manifest.read(parms=parms, cfg={'no_auth_token': True})
except ResponseError as e:
self.assertEqual(e.status, 401)
else:
self.fail('request did not error')
try:
manifest.info(parms=parms, cfg={'no_auth_token': True})
except ResponseError as e:
self.assertEqual(e.status, 401)
else:
self.fail('request did not error')


class TestContainerTempurlUTF8(Base2, TestContainerTempurl):
set_up = False
Expand Down
Loading

0 comments on commit d4409c0

Please sign in to comment.