sys-cluster/swift: fixing CVE-2016-0737, CVE-2016-0738

Package-Manager: portage-2.2.26
This commit is contained in:
Matthew Thode
2016-01-20 11:15:58 -06:00
parent 4a71ab54e4
commit 17caf32b3b
3 changed files with 294 additions and 854 deletions

View File

@@ -1,854 +0,0 @@
From 668b22a4a92ce7f842a247c38dcf5010338716dd Mon Sep 17 00:00:00 2001
From: Clay Gerrard <clay.gerrard@gmail.com>
Date: Thu, 23 Jul 2015 22:36:21 -0700
Subject: [PATCH 1/2] Disallow unsafe tempurl operations to point to
unauthorized data
Do not allow PUT tempurls to create pointers to other data. Specifically
disallow the creation of DLO object manifests by returning an error if a
non-safe tempurl request includes an X-Object-Manifest header regardless of
the value of the header.
This prevents discoverability attacks which can use any PUT tempurl to probe
for private data by creating a DLO object manifest and then using the PUT
tempurl to head the object which would 404 if the prefix does not match any
object data or form a valid DLO HEAD response if it does.
This also prevents a tricky and potentially unexpected consequence of PUT
tempurls which would make it unsafe to allow a user to download objects
created by tempurl (even if they just created them) because the result of
reading the object created via tempurl may not be the data which was uploaded.
Co-Authored-By: Kota Tsuyuzaki <tsuyuzaki.kota@lab.ntt.co.jp>
Change-Id: I91161dfb0f089c3990aca1b4255b520299ef73c8
---
swift/common/middleware/tempurl.py | 31 ++++++++++++++++++++++++-
test/functional/tests.py | 36 +++++++++++++++++++++++++++++
test/unit/common/middleware/test_tempurl.py | 19 +++++++++++++++
3 files changed, 85 insertions(+), 1 deletion(-)
diff --git a/swift/common/middleware/tempurl.py b/swift/common/middleware/tempurl.py
index 3dd1448..659121e 100644
--- a/swift/common/middleware/tempurl.py
+++ b/swift/common/middleware/tempurl.py
@@ -122,11 +122,13 @@ from urllib import urlencode
from urlparse import parse_qs
from swift.proxy.controllers.base import get_account_info, get_container_info
-from swift.common.swob import HeaderKeyDict, HTTPUnauthorized
+from swift.common.swob import HeaderKeyDict, HTTPUnauthorized, HTTPBadRequest
from swift.common.utils import split_path, get_valid_utf8_str, \
register_swift_info, get_hmac, streq_const_time, quote
+DISALLOWED_INCOMING_HEADERS = 'x-object-manifest'
+
#: Default headers to remove from incoming requests. Simply a whitespace
#: delimited list of header names and names can optionally end with '*' to
#: indicate a prefix match. DEFAULT_INCOMING_ALLOW_HEADERS is a list of
@@ -230,6 +232,10 @@ class TempURL(object):
#: The methods allowed with Temp URLs.
self.methods = methods
+ self.disallowed_headers = set(
+ 'HTTP_' + h.upper().replace('-', '_')
+ for h in DISALLOWED_INCOMING_HEADERS.split())
+
headers = DEFAULT_INCOMING_REMOVE_HEADERS
if 'incoming_remove_headers' in conf:
headers = conf['incoming_remove_headers']
@@ -323,6 +329,13 @@ class TempURL(object):
for hmac in hmac_vals)
if not is_valid_hmac:
return self._invalid(env, start_response)
+ # disallowed headers prevent accidently allowing upload of a pointer
+ # to data that the PUT tempurl would not otherwise allow access for.
+ # It should be safe to provide a GET tempurl for data that an
+ # untrusted client just uploaded with a PUT tempurl.
+ resp = self._clean_disallowed_headers(env, start_response)
+ if resp:
+ return resp
self._clean_incoming_headers(env)
env['swift.authorize'] = lambda req: None
env['swift.authorize_override'] = True
@@ -465,6 +478,22 @@ class TempURL(object):
body = '401 Unauthorized: Temp URL invalid\n'
return HTTPUnauthorized(body=body)(env, start_response)
+ def _clean_disallowed_headers(self, env, start_response):
+ """
+ Validate the absense of disallowed headers for "unsafe" operations.
+
+ :returns: None for safe operations or swob.HTTPBadResponse if the
+ request includes disallowed headers.
+ """
+ if env['REQUEST_METHOD'] in ('GET', 'HEAD', 'OPTIONS'):
+ return
+ for h in env:
+ if h in self.disallowed_headers:
+ return HTTPBadRequest(
+ body='The header %r is not allowed in this tempurl' %
+ h[len('HTTP_'):].title().replace('_', '-'))(
+ env, start_response)
+
def _clean_incoming_headers(self, env):
"""
Removes any headers from the WSGI environment as per the
diff --git a/test/functional/tests.py b/test/functional/tests.py
index 95f168e..800d070 100644
--- a/test/functional/tests.py
+++ b/test/functional/tests.py
@@ -2732,6 +2732,42 @@ class TestTempurl(Base):
self.assert_(new_obj.info(parms=put_parms,
cfg={'no_auth_token': True}))
+ def test_PUT_manifest_access(self):
+ new_obj = self.env.container.file(Utils.create_name())
+
+ # give out a signature which allows a PUT to new_obj
+ expires = int(time.time()) + 86400
+ sig = self.tempurl_sig(
+ 'PUT', expires, self.env.conn.make_path(new_obj.path),
+ self.env.tempurl_key)
+ put_parms = {'temp_url_sig': sig,
+ 'temp_url_expires': str(expires)}
+
+ # try to create manifest pointing to some random container
+ try:
+ new_obj.write('', {
+ 'x-object-manifest': '%s/foo' % 'some_random_container'
+ }, parms=put_parms, cfg={'no_auth_token': True})
+ except ResponseError as e:
+ self.assertEqual(e.status, 400)
+ else:
+ self.fail('request did not error')
+
+ # create some other container
+ other_container = self.env.account.container(Utils.create_name())
+ if not other_container.create():
+ raise ResponseError(self.conn.response)
+
+ # try to create manifest pointing to new container
+ try:
+ new_obj.write('', {
+ 'x-object-manifest': '%s/foo' % other_container
+ }, parms=put_parms, cfg={'no_auth_token': True})
+ except ResponseError as e:
+ self.assertEqual(e.status, 400)
+ else:
+ self.fail('request did not error')
+
def test_HEAD(self):
expires = int(time.time()) + 86400
sig = self.tempurl_sig(
diff --git a/test/unit/common/middleware/test_tempurl.py b/test/unit/common/middleware/test_tempurl.py
index e665563..ba42ace 100644
--- a/test/unit/common/middleware/test_tempurl.py
+++ b/test/unit/common/middleware/test_tempurl.py
@@ -649,6 +649,25 @@ class TestTempURL(unittest.TestCase):
self.assertTrue('Temp URL invalid' in resp.body)
self.assertTrue('Www-Authenticate' in resp.headers)
+ def test_disallowed_header_object_manifest(self):
+ self.tempurl = tempurl.filter_factory({})(self.auth)
+ method = 'PUT'
+ expires = int(time() + 86400)
+ path = '/v1/a/c/o'
+ key = 'abc'
+ hmac_body = '%s\n%s\n%s' % (method, expires, path)
+ sig = hmac.new(key, hmac_body, sha1).hexdigest()
+ req = self._make_request(
+ path, method='PUT', keys=[key],
+ headers={'x-object-manifest': 'private/secret'},
+ environ={'QUERY_STRING': 'temp_url_sig=%s&temp_url_expires=%s' % (
+ sig, expires)})
+ resp = req.get_response(self.tempurl)
+ self.assertEquals(resp.status_int, 400)
+ self.assertTrue('header' in resp.body)
+ self.assertTrue('not allowed' in resp.body)
+ self.assertTrue('X-Object-Manifest' in resp.body)
+
def test_removed_incoming_header(self):
self.tempurl = tempurl.filter_factory({
'incoming_remove_headers': 'x-remove-this'})(self.auth)
--
2.4.6
From fdd96d85dab7655649c75d5c6f6df5639c742daf Mon Sep 17 00:00:00 2001
From: Samuel Merritt <sam@swiftstack.com>
Date: Tue, 11 Aug 2015 09:10:13 -0500
Subject: [PATCH 2/2] Better scoping for tempurls, especially container
tempurls
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__.
Co-Authored-By: Clay Gerrard <clayg@swiftstack.com>
Co-Authored-By: Alistair Coles <alistair.coles@hp.com>
Co-Authored-By: Christian Schwede <cschwede@redhat.com>
Co-Authored-By: Matthew Oliver <matt@oliver.net.au>
Change-Id: I11078af178cb9acdd9039388282fcd0db165ba7a
---
swift/common/middleware/tempurl.py | 105 +++++++++++++----
swift/proxy/server.py | 11 +-
test/functional/tests.py | 114 +++++++++++++++++++
test/unit/common/middleware/test_tempurl.py | 171 +++++++++++++++++++++-------
4 files changed, 333 insertions(+), 68 deletions(-)
diff --git a/swift/common/middleware/tempurl.py b/swift/common/middleware/tempurl.py
index 659121e..fb8bb01 100644
--- a/swift/common/middleware/tempurl.py
+++ b/swift/common/middleware/tempurl.py
@@ -152,6 +152,10 @@ DEFAULT_OUTGOING_REMOVE_HEADERS = 'x-object-meta-*'
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.
@@ -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
@@ -304,10 +340,10 @@ class TempURL(object):
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':
@@ -322,11 +358,16 @@ class TempURL(object):
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
@@ -337,7 +378,12 @@ class TempURL(object):
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,
@@ -378,22 +424,23 @@ class TempURL(object):
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):
"""
@@ -423,18 +470,23 @@ class TempURL(object):
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'])
@@ -443,25 +495,28 @@ class TempURL(object):
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):
"""
diff --git a/swift/proxy/server.py b/swift/proxy/server.py
index b631542..8ff4956 100644
--- a/swift/proxy/server.py
+++ b/swift/proxy/server.py
@@ -378,6 +378,7 @@ class Application(object):
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
@@ -388,7 +389,7 @@ class Application(object):
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.
@@ -398,7 +399,13 @@ class Application(object):
# 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):
diff --git a/test/functional/tests.py b/test/functional/tests.py
index 800d070..1c342f0 100644
--- a/test/functional/tests.py
+++ b/test/functional/tests.py
@@ -2714,6 +2714,59 @@ class TestTempurl(Base):
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())
@@ -3042,6 +3095,67 @@ class TestContainerTempurl(Base):
'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
diff --git a/test/unit/common/middleware/test_tempurl.py b/test/unit/common/middleware/test_tempurl.py
index ba42ace..f153147 100644
--- a/test/unit/common/middleware/test_tempurl.py
+++ b/test/unit/common/middleware/test_tempurl.py
@@ -29,6 +29,7 @@
# limitations under the License.
import hmac
+import itertools
import unittest
from hashlib import sha1
from time import time
@@ -44,10 +45,13 @@ class FakeApp(object):
self.calls = 0
self.status_headers_body_iter = status_headers_body_iter
if not self.status_headers_body_iter:
- self.status_headers_body_iter = iter([('404 Not Found', {
- 'x-test-header-one-a': 'value1',
- 'x-test-header-two-a': 'value2',
- 'x-test-header-two-b': 'value3'}, '')])
+ self.status_headers_body_iter = iter(
+ itertools.repeat((
+ '404 Not Found', {
+ 'x-test-header-one-a': 'value1',
+ 'x-test-header-two-a': 'value2',
+ 'x-test-header-two-b': 'value3'},
+ '')))
self.request = None
def __call__(self, env, start_response):
@@ -69,16 +73,18 @@ class TestTempURL(unittest.TestCase):
self.auth = tempauth.filter_factory({'reseller_prefix': ''})(self.app)
self.tempurl = tempurl.filter_factory({})(self.auth)
- def _make_request(self, path, environ=None, keys=(), **kwargs):
+ def _make_request(self, path, environ=None, keys=(), container_keys=None,
+ **kwargs):
if environ is None:
environ = {}
_junk, account, _junk, _junk = utils.split_path(path, 2, 4)
- self._fake_cache_environ(environ, account, keys)
+ self._fake_cache_environ(environ, account, keys,
+ container_keys=container_keys)
req = Request.blank(path, environ=environ, **kwargs)
return req
- def _fake_cache_environ(self, environ, account, keys):
+ def _fake_cache_environ(self, environ, account, keys, container_keys=None):
"""
Fake out the caching layer for get_account_info(). Injects account data
into environ such that keys are the tempurl keys, if set.
@@ -96,8 +102,13 @@ class TestTempURL(unittest.TestCase):
'bytes': '0',
'meta': meta}
+ meta = {}
+ for i, key in enumerate(container_keys or []):
+ meta_name = 'Temp-URL-key' + (("-%d" % (i + 1) if i else ""))
+ meta[meta_name] = key
+
container_cache_key = 'swift.container/' + account + '/c'
- environ.setdefault(container_cache_key, {'meta': {}})
+ environ.setdefault(container_cache_key, {'meta': meta})
def test_passthrough(self):
resp = self._make_request('/v1/a/c/o').get_response(self.tempurl)
@@ -581,6 +592,81 @@ class TestTempURL(unittest.TestCase):
self.assertTrue('Temp URL invalid' in resp.body)
self.assertTrue('Www-Authenticate' in resp.headers)
+ def test_authorize_limits_scope(self):
+ req_other_object = Request.blank("/v1/a/c/o2")
+ req_other_container = Request.blank("/v1/a/c2/o2")
+ req_other_account = Request.blank("/v1/a2/c2/o2")
+
+ key_kwargs = {
+ 'keys': ['account-key', 'shared-key'],
+ 'container_keys': ['container-key', 'shared-key'],
+ }
+
+ # A request with the account key limits the pre-authed scope to the
+ # account level.
+ method = 'GET'
+ expires = int(time() + 86400)
+ path = '/v1/a/c/o'
+
+ hmac_body = '%s\n%s\n%s' % (method, expires, path)
+ sig = hmac.new('account-key', hmac_body, sha1).hexdigest()
+ qs = '?temp_url_sig=%s&temp_url_expires=%s' % (sig, expires)
+
+ # make request will setup the environ cache for us
+ req = self._make_request(path + qs, **key_kwargs)
+ resp = req.get_response(self.tempurl)
+ self.assertEquals(resp.status_int, 404) # sanity check
+
+ authorize = req.environ['swift.authorize']
+ # Requests for other objects happen if, for example, you're
+ # downloading a large object or creating a large-object manifest.
+ oo_resp = authorize(req_other_object)
+ self.assertEqual(oo_resp, None)
+ oc_resp = authorize(req_other_container)
+ self.assertEqual(oc_resp, None)
+ oa_resp = authorize(req_other_account)
+ self.assertEqual(oa_resp.status_int, 401)
+
+ # A request with the container key limits the pre-authed scope to
+ # the container level; a different container in the same account is
+ # out of scope and thus forbidden.
+ hmac_body = '%s\n%s\n%s' % (method, expires, path)
+ sig = hmac.new('container-key', hmac_body, sha1).hexdigest()
+ qs = '?temp_url_sig=%s&temp_url_expires=%s' % (sig, expires)
+
+ req = self._make_request(path + qs, **key_kwargs)
+ resp = req.get_response(self.tempurl)
+ self.assertEquals(resp.status_int, 404) # sanity check
+
+ authorize = req.environ['swift.authorize']
+ oo_resp = authorize(req_other_object)
+ self.assertEqual(oo_resp, None)
+ oc_resp = authorize(req_other_container)
+ self.assertEqual(oc_resp.status_int, 401)
+ oa_resp = authorize(req_other_account)
+ self.assertEqual(oa_resp.status_int, 401)
+
+ # If account and container share a key (users set these, so this can
+ # happen by accident, stupidity, *or* malice!), limit the scope to
+ # account level. This prevents someone from shrinking the scope of
+ # account-level tempurls by reusing one of the account's keys on a
+ # container.
+ hmac_body = '%s\n%s\n%s' % (method, expires, path)
+ sig = hmac.new('shared-key', hmac_body, sha1).hexdigest()
+ qs = '?temp_url_sig=%s&temp_url_expires=%s' % (sig, expires)
+
+ req = self._make_request(path + qs, **key_kwargs)
+ resp = req.get_response(self.tempurl)
+ self.assertEquals(resp.status_int, 404) # sanity check
+
+ authorize = req.environ['swift.authorize']
+ oo_resp = authorize(req_other_object)
+ self.assertEqual(oo_resp, None)
+ oc_resp = authorize(req_other_container)
+ self.assertEqual(oc_resp, None)
+ oa_resp = authorize(req_other_account)
+ self.assertEqual(oa_resp.status_int, 401)
+
def test_changed_path_invalid(self):
method = 'GET'
expires = int(time() + 86400)
@@ -828,35 +914,38 @@ class TestTempURL(unittest.TestCase):
self.assertTrue('x-conflict-header-test' in resp.headers)
self.assertEqual(resp.headers['x-conflict-header-test'], 'value')
- def test_get_account(self):
- self.assertEquals(self.tempurl._get_account({
- 'REQUEST_METHOD': 'HEAD', 'PATH_INFO': '/v1/a/c/o'}), 'a')
- self.assertEquals(self.tempurl._get_account({
- 'REQUEST_METHOD': 'GET', 'PATH_INFO': '/v1/a/c/o'}), 'a')
- self.assertEquals(self.tempurl._get_account({
- 'REQUEST_METHOD': 'PUT', 'PATH_INFO': '/v1/a/c/o'}), 'a')
- self.assertEquals(self.tempurl._get_account({
- 'REQUEST_METHOD': 'POST', 'PATH_INFO': '/v1/a/c/o'}), 'a')
- self.assertEquals(self.tempurl._get_account({
- 'REQUEST_METHOD': 'DELETE', 'PATH_INFO': '/v1/a/c/o'}), 'a')
- self.assertEquals(self.tempurl._get_account({
- 'REQUEST_METHOD': 'UNKNOWN', 'PATH_INFO': '/v1/a/c/o'}), None)
- self.assertEquals(self.tempurl._get_account({
- 'REQUEST_METHOD': 'GET', 'PATH_INFO': '/v1/a/c/'}), None)
- self.assertEquals(self.tempurl._get_account({
- 'REQUEST_METHOD': 'GET', 'PATH_INFO': '/v1/a/c//////'}), None)
- self.assertEquals(self.tempurl._get_account({
- 'REQUEST_METHOD': 'GET', 'PATH_INFO': '/v1/a/c///o///'}), 'a')
- self.assertEquals(self.tempurl._get_account({
- 'REQUEST_METHOD': 'GET', 'PATH_INFO': '/v1/a/c'}), None)
- self.assertEquals(self.tempurl._get_account({
- 'REQUEST_METHOD': 'GET', 'PATH_INFO': '/v1/a//o'}), None)
- self.assertEquals(self.tempurl._get_account({
- 'REQUEST_METHOD': 'GET', 'PATH_INFO': '/v1//c/o'}), None)
- self.assertEquals(self.tempurl._get_account({
- 'REQUEST_METHOD': 'GET', 'PATH_INFO': '//a/c/o'}), None)
- self.assertEquals(self.tempurl._get_account({
- 'REQUEST_METHOD': 'GET', 'PATH_INFO': '/v2/a/c/o'}), None)
+ def test_get_account_and_container(self):
+ self.assertEquals(self.tempurl._get_account_and_container({
+ 'REQUEST_METHOD': 'HEAD', 'PATH_INFO': '/v1/a/c/o'}), ('a', 'c'))
+ self.assertEquals(self.tempurl._get_account_and_container({
+ 'REQUEST_METHOD': 'GET', 'PATH_INFO': '/v1/a/c/o'}), ('a', 'c'))
+ self.assertEquals(self.tempurl._get_account_and_container({
+ 'REQUEST_METHOD': 'PUT', 'PATH_INFO': '/v1/a/c/o'}), ('a', 'c'))
+ self.assertEquals(self.tempurl._get_account_and_container({
+ 'REQUEST_METHOD': 'POST', 'PATH_INFO': '/v1/a/c/o'}), ('a', 'c'))
+ self.assertEquals(self.tempurl._get_account_and_container({
+ 'REQUEST_METHOD': 'DELETE', 'PATH_INFO': '/v1/a/c/o'}), ('a', 'c'))
+ self.assertEquals(self.tempurl._get_account_and_container({
+ 'REQUEST_METHOD': 'UNKNOWN', 'PATH_INFO': '/v1/a/c/o'}),
+ (None, None))
+ self.assertEquals(self.tempurl._get_account_and_container({
+ 'REQUEST_METHOD': 'GET', 'PATH_INFO': '/v1/a/c/'}), (None, None))
+ self.assertEquals(self.tempurl._get_account_and_container({
+ 'REQUEST_METHOD': 'GET', 'PATH_INFO': '/v1/a/c//////'}),
+ (None, None))
+ self.assertEquals(self.tempurl._get_account_and_container({
+ 'REQUEST_METHOD': 'GET', 'PATH_INFO': '/v1/a/c///o///'}),
+ ('a', 'c'))
+ self.assertEquals(self.tempurl._get_account_and_container({
+ 'REQUEST_METHOD': 'GET', 'PATH_INFO': '/v1/a/c'}), (None, None))
+ self.assertEquals(self.tempurl._get_account_and_container({
+ 'REQUEST_METHOD': 'GET', 'PATH_INFO': '/v1/a//o'}), (None, None))
+ self.assertEquals(self.tempurl._get_account_and_container({
+ 'REQUEST_METHOD': 'GET', 'PATH_INFO': '/v1//c/o'}), (None, None))
+ self.assertEquals(self.tempurl._get_account_and_container({
+ 'REQUEST_METHOD': 'GET', 'PATH_INFO': '//a/c/o'}), (None, None))
+ self.assertEquals(self.tempurl._get_account_and_container({
+ 'REQUEST_METHOD': 'GET', 'PATH_INFO': '/v2/a/c/o'}), (None, None))
def test_get_temp_url_info(self):
s = 'f5d5051bddf5df7e27c628818738334f'
@@ -908,13 +997,13 @@ class TestTempURL(unittest.TestCase):
self.assertEquals(
self.tempurl._get_hmacs(
{'REQUEST_METHOD': 'GET', 'PATH_INFO': '/v1/a/c/o'},
- 1, ['abc']),
- ['026d7f7cc25256450423c7ad03fc9f5ffc1dab6d'])
+ 1, [('abc', 'account')]),
+ [('026d7f7cc25256450423c7ad03fc9f5ffc1dab6d', 'account')])
self.assertEquals(
self.tempurl._get_hmacs(
{'REQUEST_METHOD': 'HEAD', 'PATH_INFO': '/v1/a/c/o'},
- 1, ['abc'], request_method='GET'),
- ['026d7f7cc25256450423c7ad03fc9f5ffc1dab6d'])
+ 1, [('abc', 'account')], request_method='GET'),
+ [('026d7f7cc25256450423c7ad03fc9f5ffc1dab6d', 'account')])
def test_invalid(self):
--
2.4.6

View File

@@ -0,0 +1,167 @@
From fc1376b356894f39dfab1e71c1fcb87a943461aa Mon Sep 17 00:00:00 2001
From: Samuel Merritt <sam@swiftstack.com>
Date: Tue, 8 Dec 2015 16:36:05 -0800
Subject: [PATCH] Fix memory/socket leak in proxy on truncated SLO/DLO GET
When a client disconnected while consuming an SLO or DLO GET response,
the proxy would leak a socket. This could be observed via strace as a
socket that had shutdown() called on it, but was never closed. It
could also be observed by counting entries in /proc/<pid>/fd, where
<pid> is the pid of a proxy server worker process.
This is due to a memory leak in SegmentedIterable. A SegmentedIterable
has an 'app_iter' attribute, which is a generator. That generator
references 'self' (the SegmentedIterable object). This creates a
cyclic reference: the generator refers to the SegmentedIterable, and
the SegmentedIterable refers to the generator.
Python can normally handle cyclic garbage; reference counting won't
reclaim it, but the garbage collector will. However, objects with
finalizers will stop the garbage collector from collecting them* and
the cycle of which they are part.
For most objects, "has finalizer" is synonymous with "has a __del__
method". However, a generator has a finalizer once it's started
running and before it finishes: basically, while it has stack frames
associated with it**.
When a client disconnects mid-stream, we get a memory leak. We have
our SegmentedIterable object (call it "si"), and its associated
generator. si.app_iter is the generator, and the generator closes over
si, so we have a cycle; and the generator has started but not yet
finished, so the generator needs finalization; hence, the garbage
collector won't ever clean it up.
The socket leak comes in because the generator *also* refers to the
request's WSGI environment, which contains wsgi.input, which
ultimately refers to a _socket object from the standard
library. Python's _socket objects only close their underlying file
descriptor when their reference counts fall to 0***.
This commit makes SegmentedIterable.close() call
self.app_iter.close(), thereby unwinding its generator's stack and
making it eligible for garbage collection.
* in Python < 3.4, at least. See PEP 442.
** see PyGen_NeedsFinalizing() in Objects/genobject.c and also
has_finalizer() in Modules/gcmodule.c in Python.
*** see sock_dealloc() in Modules/socketmodule.c in Python. See
sock_close() in the same file for the other half of the sad story.
Change-Id: I74ea49eaa7d5c372cdc2399148d5495d3007dbd0
Co-Authored-By: Kota Tsuyuzaki <tsuyuzaki.kota@lab.ntt.co.jp>
---
swift/common/request_helpers.py | 6 ++-
test/unit/common/middleware/test_slo.py | 65 ++++++++++++++++++++++++++++++++-
2 files changed, 68 insertions(+), 3 deletions(-)
diff --git a/swift/common/request_helpers.py b/swift/common/request_helpers.py
index a533087..922240a 100644
--- a/swift/common/request_helpers.py
+++ b/swift/common/request_helpers.py
@@ -435,6 +435,9 @@ class SegmentedIterable(object):
self.logger.exception(_('ERROR: An error occurred '
'while retrieving segments'))
raise
+ finally:
+ if self.current_resp:
+ close_if_possible(self.current_resp.app_iter)
def app_iter_range(self, *a, **kw):
"""
@@ -477,5 +480,4 @@ class SegmentedIterable(object):
Called when the client disconnect. Ensure that the connection to the
backend server is closed.
"""
- if self.current_resp:
- close_if_possible(self.current_resp.app_iter)
+ close_if_possible(self.app_iter)
diff --git a/test/unit/common/middleware/test_slo.py b/test/unit/common/middleware/test_slo.py
index b131240..ee0ce0f 100644
--- a/test/unit/common/middleware/test_slo.py
+++ b/test/unit/common/middleware/test_slo.py
@@ -26,7 +26,8 @@ from swift.common import swob, utils
from swift.common.exceptions import ListingIterError, SegmentError
from swift.common.middleware import slo
from swift.common.swob import Request, Response, HTTPException
-from swift.common.utils import quote, json, closing_if_possible
+from swift.common.utils import quote, json, closing_if_possible, \
+ close_if_possible
from test.unit.common.middleware.helpers import FakeSwift
@@ -1765,6 +1766,68 @@ class TestSloGetManifest(SloTestCase):
self.assertEqual(headers['X-Object-Meta-Fish'], 'Bass')
self.assertEqual(body, '')
+ def test_generator_closure(self):
+ # Test that the SLO WSGI iterable closes its internal .app_iter when
+ # it receives a close() message.
+ #
+ # This is sufficient to fix a memory leak. The memory leak arises
+ # due to cyclic references involving a running generator; a running
+ # generator sometimes preventes the GC from collecting it in the
+ # same way that an object with a defined __del__ does.
+ #
+ # There are other ways to break the cycle and fix the memory leak as
+ # well; calling .close() on the generator is sufficient, but not
+ # necessary. However, having this test is better than nothing for
+ # preventing regressions.
+ leaks = [0]
+
+ class LeakTracker(object):
+ def __init__(self, inner_iter):
+ leaks[0] += 1
+ self.inner_iter = iter(inner_iter)
+
+ def __iter__(self):
+ return self
+
+ def next(self):
+ return next(self.inner_iter)
+
+ def close(self):
+ leaks[0] -= 1
+ close_if_possible(self.inner_iter)
+
+ class LeakTrackingSegmentedIterable(slo.SegmentedIterable):
+ def _internal_iter(self, *a, **kw):
+ it = super(
+ LeakTrackingSegmentedIterable, self)._internal_iter(
+ *a, **kw)
+ return LeakTracker(it)
+
+ status = [None]
+ headers = [None]
+
+ def start_response(s, h, ei=None):
+ status[0] = s
+ headers[0] = h
+
+ req = Request.blank(
+ '/v1/AUTH_test/gettest/manifest-abcd',
+ environ={'REQUEST_METHOD': 'GET',
+ 'HTTP_ACCEPT': 'application/json'})
+
+ # can't self.call_slo() here since we don't want to consume the
+ # whole body
+ with patch.object(slo, 'SegmentedIterable',
+ LeakTrackingSegmentedIterable):
+ app_resp = self.slo(req.environ, start_response)
+ self.assertEqual(status[0], '200 OK') # sanity check
+ body_iter = iter(app_resp)
+ chunk = next(body_iter)
+ self.assertEqual(chunk, 'aaaaa') # sanity check
+
+ app_resp.close()
+ self.assertEqual(0, leaks[0])
+
def test_head_manifest_is_efficient(self):
req = Request.blank(
'/v1/AUTH_test/gettest/manifest-abcd',
--
2.7.0

View File

@@ -0,0 +1,127 @@
# Copyright 1999-2016 Gentoo Foundation
# Distributed under the terms of the GNU General Public License v2
# $Id$
EAPI=5
PYTHON_COMPAT=( python2_7 )
inherit distutils-r1 eutils linux-info user
DESCRIPTION="A highly available, distributed, and eventually consistent object/blob store"
HOMEPAGE="https://launchpad.net/swift"
SRC_URI="https://launchpad.net/${PN}/liberty/${PV}/+download/${P}.tar.gz"
LICENSE="Apache-2.0"
SLOT="0"
KEYWORDS="~amd64 ~x86"
IUSE="proxy account container object test +memcached"
REQUIRED_USE="|| ( proxy account container object )"
CDEPEND="
>=dev-python/pbr-0.8.0[${PYTHON_USEDEP}]
<dev-python/pbr-2.0[${PYTHON_USEDEP}]"
DEPEND="
dev-python/setuptools[${PYTHON_USEDEP}]
${CDEPEND}
test? (
${RDEPEND}
dev-python/coverage[${PYTHON_USEDEP}]
dev-python/nose[${PYTHON_USEDEP}]
dev-python/nosexcover[${PYTHON_USEDEP}]
dev-python/nosehtmloutput[${PYTHON_USEDEP}]
dev-python/oslo-sphinx[${PYTHON_USEDEP}]
>=dev-python/sphinx-1.1.2[${PYTHON_USEDEP}]
<dev-python/sphinx-1.2[${PYTHON_USEDEP}]
>=dev-python/mock-1.0[${PYTHON_USEDEP}]
dev-python/python-swiftclient[${PYTHON_USEDEP}]
>=dev-python/python-keystoneclient-1.3.0[${PYTHON_USEDEP}]
>=dev-python/bandit-0.10.1[${PYTHON_USEDEP}]
)"
RDEPEND="
${CDEPEND}
virtual/python-dnspython[${PYTHON_USEDEP}]
>=dev-python/eventlet-0.16.1[${PYTHON_USEDEP}]
!~dev-python/eventlet-0.17.0[${PYTHON_USEDEP}]
>=dev-python/greenlet-0.3.1[${PYTHON_USEDEP}]
>=dev-python/netifaces-0.5[${PYTHON_USEDEP}]
!~dev-python/netifaces-0.10.0[${PYTHON_USEDEP}]
!~dev-python/netifaces-0.10.1[${PYTHON_USEDEP}]
>=dev-python/pastedeploy-1.3.3[${PYTHON_USEDEP}]
>=dev-python/simplejson-2.0.9[${PYTHON_USEDEP}]
>=dev-python/six-1.9.0[${PYTHON_USEDEP}]
dev-python/pyxattr[${PYTHON_USEDEP}]
~dev-python/PyECLib-1.0.7[${PYTHON_USEDEP}]
memcached? ( net-misc/memcached )
net-misc/rsync[xattr]"
CONFIG_CHECK="~EXT3_FS_XATTR ~SQUASHFS_XATTR ~CIFS_XATTR ~JFFS2_FS_XATTR
~TMPFS_XATTR ~UBIFS_FS_XATTR ~EXT2_FS_XATTR ~REISERFS_FS_XATTR ~EXT4_FS_XATTR
~ZFS"
PATCHES=(
"${FILESDIR}/cve-2016-0738-stable-liberty.patch"
)
pkg_setup() {
enewuser swift
enewgroup swift
}
src_prepare() {
sed -i 's/xattr/pyxattr/g' swift.egg-info/requires.txt || die
sed -i 's/xattr/pyxattr/g' requirements.txt || die
sed -i '/^hacking/d' test-requirements.txt || die
distutils-r1_python_prepare_all
}
src_test () {
# https://bugs.launchpad.net/swift/+bug/1249727
find . \( -name test_wsgi.py -o -name test_locale.py -o -name test_utils.py \) -delete || die
SKIP_PIP_INSTALL=1 PBR_VERSION=0.6.0 sh .unittests || die
}
python_install() {
distutils-r1_python_install
keepdir /etc/swift
insinto /etc/swift
newins "etc/swift.conf-sample" "swift.conf"
newins "etc/rsyncd.conf-sample" "rsyncd.conf"
newins "etc/mime.types-sample" "mime.types-sample"
newins "etc/memcache.conf-sample" "memcache.conf-sample"
newins "etc/drive-audit.conf-sample" "drive-audit.conf-sample"
newins "etc/dispersion.conf-sample" "dispersion.conf-sample"
if use proxy; then
newinitd "${FILESDIR}/swift-proxy.initd" "swift-proxy"
newins "etc/proxy-server.conf-sample" "proxy-server.conf"
if use memcached; then
sed -i '/depend/a\
need memcached' "${D}/etc/init.d/swift-proxy"
fi
fi
if use account; then
newinitd "${FILESDIR}/swift-account.initd" "swift-account"
newins "etc/account-server.conf-sample" "account-server.conf"
fi
if use container; then
newinitd "${FILESDIR}/swift-container.initd" "swift-container"
newins "etc/container-server.conf-sample" "container-server.conf"
fi
if use object; then
newinitd "${FILESDIR}/swift-object.initd" "swift-object"
newins "etc/object-server.conf-sample" "object-server.conf"
newins "etc/object-expirer.conf-sample" "object-expirer.conf"
fi
fowners swift:swift "/etc/swift" || die "fowners failed"
}
pkg_postinst() {
elog "Openstack swift will default to using insecure http unless a"
elog "certificate is created in /etc/swift/cert.crt and the associated key"
elog "in /etc/swift/cert.key. These can be created with the following:"
elog " * cd /etc/swift"
elog " * openssl req -new -x509 -nodes -out cert.crt -keyout cert.key"
}