This upstream patch addresses CVE-2016-0737 which is filed under

Launchpad bug 1466549. The issue is addressed in Swift 2.3.1 as well as

2.5.1 or later.

From 036c2f348d24c01c7a4deba3e44889c45270b46d Mon Sep 17 00:00:00 2001

From: Samuel Merritt <[email protected]>

Date: Thu, 18 Jun 2015 12:58:03 -0700

Subject: Get better at closing WSGI iterables.

PEP 333 (WSGI) says: "If the iterable returned by the application has

a close() method, the server or gateway must call that method upon

completion of the current request[.]"

There's a bunch of places where we weren't doing that; some of them

matter more than others. Calling .close() can prevent a connection

leak in some cases. In others, it just provides a certain pedantic

smugness. Either way, we should do what WSGI requires.

Noteworthy goofs include:

  * If a client is downloading a large object and disconnects halfway

    through, a proxy -> obj connection may be leaked. In this case,

    the WSGI iterable is a SegmentedIterable, which lacked a close()

    method. Thus, when the WSGI server noticed the client disconnect,

    it had no way of telling the SegmentedIterable about it, and so

    the underlying iterable for the segment's data didn't get

    closed.

    Here, it seems likely (though unproven) that the object server

    would time out and kill the connection, or that a

    ChunkWriteTimeout would fire down in the proxy server, so the

    leaked connection would eventually go away. However, a flurry of

    client disconnects could leave a big pile of useless connections.

  * If a conditional request receives a 304 or 412, the underlying

    app_iter is not closed. This mostly affects conditional requests

    for large objects.

The leaked connections were noticed by this patch's co-author, who

made the changes to SegmentedIterable. Those changes helped, but did

not completely fix, the issue. The rest of the patch is an attempt to

plug the rest of the holes.

Co-Authored-By: Romain LE DISEZ <[email protected]>

Closes-Bug: #1466549

Change-Id: I168e147aae7c1728e7e3fdabb7fba6f2d747d937

(cherry picked from commit 12d8a53fffea6e4bed8ba3d502ce625f5c6710b9

with fixed import conflicts)

---

 swift/common/middleware/dlo.py          |  8 ++++++--

 swift/common/middleware/slo.py          | 10 ++++++----

 swift/common/request_helpers.py         | 35 ++++++++++++---------------------

 swift/common/swob.py                    |  9 ++++++++-

 swift/common/utils.py                   | 22 +++++++++++++++++++++

 swift/proxy/controllers/obj.py          |  4 ++--

 test/unit/common/middleware/helpers.py  | 32 +++++++++++++++++++++++++++++-

 test/unit/common/middleware/test_dlo.py | 10 ++++++++--

 test/unit/common/middleware/test_slo.py | 13 ++++++++----

 9 files changed, 105 insertions(+), 38 deletions(-)

diff --git a/swift/common/middleware/dlo.py b/swift/common/middleware/dlo.py

index d2761ac..9330ccb 100644

--- a/swift/common/middleware/dlo.py

+++ b/swift/common/middleware/dlo.py

@@ -22,7 +22,8 @@ from swift.common.http import is_success

 from swift.common.swob import Request, Response, \

     HTTPRequestedRangeNotSatisfiable, HTTPBadRequest, HTTPConflict

 from swift.common.utils import get_logger, json, \

-    RateLimitedIterator, read_conf_dir, quote

+    RateLimitedIterator, read_conf_dir, quote, close_if_possible, \

+    closing_if_possible

 from swift.common.request_helpers import SegmentedIterable

 from swift.common.wsgi import WSGIContext, make_subrequest

 from urllib import unquote

@@ -48,7 +49,8 @@ class GetContext(WSGIContext):

         con_resp = con_req.get_response(self.dlo.app)

         if not is_success(con_resp.status_int):

             return con_resp, None

-        return None, json.loads(''.join(con_resp.app_iter))

+        with closing_if_possible(con_resp.app_iter):

+            return None, json.loads(''.join(con_resp.app_iter))

     def _segment_listing_iterator(self, req, version, account, container,

                                   prefix, segments, first_byte=None,

@@ -107,6 +109,7 @@ class GetContext(WSGIContext):

                 # we've already started sending the response body to the

                 # client, so all we can do is raise an exception to make the

                 # WSGI server close the connection early

+                close_if_possible(error_response.app_iter)

                 raise ListingIterError(

                     "Got status %d listing container /%s/%s" %

                     (error_response.status_int, account, container))

@@ -233,6 +236,7 @@ class GetContext(WSGIContext):

         # make sure this response is for a dynamic large object manifest

         for header, value in self._response_headers:

             if (header.lower() == 'x-object-manifest'):

+                close_if_possible(resp_iter)

                 response = self.get_or_head_response(req, value)

                 return response(req.environ, start_response)

         else:

diff --git a/swift/common/middleware/slo.py b/swift/common/middleware/slo.py

index e8f1707..c7a97a6 100644

--- a/swift/common/middleware/slo.py

+++ b/swift/common/middleware/slo.py

@@ -147,9 +147,9 @@ from swift.common.swob import Request, HTTPBadRequest, HTTPServerError, \

     Response

 from swift.common.utils import json, get_logger, config_true_value, \

     get_valid_utf8_str, override_bytes_from_content_type, split_path, \

-    register_swift_info, RateLimitedIterator, quote

-from swift.common.request_helpers import SegmentedIterable, \

-    closing_if_possible, close_if_possible

+    register_swift_info, RateLimitedIterator, quote, close_if_possible, \

+    closing_if_possible

+from swift.common.request_helpers import SegmentedIterable

 from swift.common.constraints import check_utf8, MAX_BUFFERED_SLO_SEGMENTS

 from swift.common.http import HTTP_NOT_FOUND, HTTP_UNAUTHORIZED, is_success

 from swift.common.wsgi import WSGIContext, make_subrequest

@@ -227,6 +227,7 @@ class SloGetContext(WSGIContext):

         sub_resp = sub_req.get_response(self.slo.app)

         if not is_success(sub_resp.status_int):

+            close_if_possible(sub_resp.app_iter)

             raise ListingIterError(

                 'ERROR: while fetching %s, GET of submanifest %s '

                 'failed with status %d' % (req.path, sub_req.path,

@@ -400,7 +401,8 @@ class SloGetContext(WSGIContext):

         return response(req.environ, start_response)

     def get_or_head_response(self, req, resp_headers, resp_iter):

-        resp_body = ''.join(resp_iter)

+        with closing_if_possible(resp_iter):

+            resp_body = ''.join(resp_iter)

         try:

             segments = json.loads(resp_body)

         except ValueError:

diff --git a/swift/common/request_helpers.py b/swift/common/request_helpers.py

index 14b9fd8..8aa8457 100644

--- a/swift/common/request_helpers.py

+++ b/swift/common/request_helpers.py

@@ -23,7 +23,6 @@ from swob in here without creating circular imports.

 import hashlib

 import itertools

 import time

-from contextlib import contextmanager

 from urllib import unquote

 from swift import gettext_ as _

 from swift.common.storage_policy import POLICIES

@@ -32,7 +31,8 @@ from swift.common.exceptions import ListingIterError, SegmentError

 from swift.common.http import is_success

 from swift.common.swob import (HTTPBadRequest, HTTPNotAcceptable,

                                HTTPServiceUnavailable)

-from swift.common.utils import split_path, validate_device_partition

+from swift.common.utils import split_path, validate_device_partition, \

+    close_if_possible

 from swift.common.wsgi import make_subrequest

@@ -249,26 +249,6 @@ def copy_header_subset(from_r, to_r, condition):

             to_r.headers[k] = v

-def close_if_possible(maybe_closable):

-    close_method = getattr(maybe_closable, 'close', None)

-    if callable(close_method):

-        return close_method()

-

-

-@contextmanager

-def closing_if_possible(maybe_closable):

-    """

-    Like contextlib.closing(), but doesn't crash if the object lacks a close()

-    method.

-

-    PEP 333 (WSGI) says: "If the iterable returned by the application has a

-    close() method, the server or gateway must call that method upon

-    completion of the current request[.]" This function makes that easier.

-    """

-    yield maybe_closable

-    close_if_possible(maybe_closable)

-

-

 class SegmentedIterable(object):

     """

     Iterable that returns the object contents for a large object.

@@ -304,6 +284,7 @@ class SegmentedIterable(object):

         self.peeked_chunk = None

         self.app_iter = self._internal_iter()

         self.validated_first_segment = False

+        self.current_resp = None

     def _internal_iter(self):

         start_time = time.time()

@@ -360,6 +341,8 @@ class SegmentedIterable(object):

                          'r_size': seg_resp.content_length,

                          's_etag': seg_etag,

                          's_size': seg_size})

+                else:

+                    self.current_resp = seg_resp

                 seg_hash = hashlib.md5()

                 for chunk in seg_resp.app_iter:

@@ -431,3 +414,11 @@ class SegmentedIterable(object):

             return itertools.chain([pc], self.app_iter)

         else:

             return self.app_iter

+

+    def close(self):

+        """

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

diff --git a/swift/common/swob.py b/swift/common/swob.py

index c2e3afb..a1bd9f6 100644

--- a/swift/common/swob.py

+++ b/swift/common/swob.py

@@ -49,7 +49,8 @@ import random

 import functools

 import inspect

-from swift.common.utils import reiterate, split_path, Timestamp, pairs

+from swift.common.utils import reiterate, split_path, Timestamp, pairs, \

+    close_if_possible

 from swift.common.exceptions import InvalidTimestamp

@@ -1203,12 +1204,14 @@ class Response(object):

                     etag in self.request.if_none_match:

                 self.status = 304

                 self.content_length = 0

+                close_if_possible(app_iter)

                 return ['']

             if etag and self.request.if_match and \

                etag not in self.request.if_match:

                 self.status = 412

                 self.content_length = 0

+                close_if_possible(app_iter)

                 return ['']

             if self.status_int == 404 and self.request.if_match \

@@ -1219,18 +1222,21 @@ class Response(object):

                 # Failed) response. [RFC 2616 section 14.24]

                 self.status = 412

                 self.content_length = 0

+                close_if_possible(app_iter)

                 return ['']

             if self.last_modified and self.request.if_modified_since \

                and self.last_modified <= self.request.if_modified_since:

                 self.status = 304

                 self.content_length = 0

+                close_if_possible(app_iter)

                 return ['']

             if self.last_modified and self.request.if_unmodified_since \

                and self.last_modified > self.request.if_unmodified_since:

                 self.status = 412

                 self.content_length = 0

+                close_if_possible(app_iter)

                 return ['']

         if self.request and self.request.method == 'HEAD':

@@ -1244,6 +1250,7 @@ class Response(object):

             if ranges == []:

                 self.status = 416

                 self.content_length = 0

+                close_if_possible(app_iter)

                 return ['']

             elif ranges:

                 range_size = len(ranges)

diff --git a/swift/common/utils.py b/swift/common/utils.py

index 19dcfd3..85c3824 100644

--- a/swift/common/utils.py

+++ b/swift/common/utils.py

@@ -3143,6 +3143,28 @@ def ismount_raw(path):

     return False

+def close_if_possible(maybe_closable):

+    close_method = getattr(maybe_closable, 'close', None)

+    if callable(close_method):

+        return close_method()

+

+

+@contextmanager

+def closing_if_possible(maybe_closable):

+    """

+    Like contextlib.closing(), but doesn't crash if the object lacks a close()

+    method.

+

+    PEP 333 (WSGI) says: "If the iterable returned by the application has a

+    close() method, the server or gateway must call that method upon

+    completion of the current request[.]" This function makes that easier.

+    """

+    try:

+        yield maybe_closable

+    finally:

+        close_if_possible(maybe_closable)

+

+

 _rfc_token = r'[^()<>@,;:\"/\[\]?={}\x00-\x20\x7f]+'

 _rfc_extension_pattern = re.compile(

     r'(?:\s*;\s*(' + _rfc_token + r")\s*(?:=\s*(" + _rfc_token +

diff --git a/swift/proxy/controllers/obj.py b/swift/proxy/controllers/obj.py

index a83242b..784dfef 100644

--- a/swift/proxy/controllers/obj.py

+++ b/swift/proxy/controllers/obj.py

@@ -43,7 +43,7 @@ from swift.common.utils import (

     clean_content_type, config_true_value, ContextPool, csv_append,

     GreenAsyncPile, GreenthreadSafeIterator, json, Timestamp,

     normalize_delete_at_timestamp, public, get_expirer_container,

-    quorum_size)

+    quorum_size, close_if_possible)

 from swift.common.bufferedhttp import http_connect

 from swift.common.constraints import check_metadata, check_object_creation, \

     check_copy_from_header, check_destination_header, \

@@ -68,7 +68,7 @@ from swift.common.swob import HTTPAccepted, HTTPBadRequest, HTTPNotFound, \

     HTTPServerError, HTTPServiceUnavailable, Request, HeaderKeyDict, \

     HTTPClientDisconnect, HTTPUnprocessableEntity, Response, HTTPException

 from swift.common.request_helpers import is_sys_or_user_meta, is_sys_meta, \

-    remove_items, copy_header_subset, close_if_possible

+    remove_items, copy_header_subset

 def copy_headers_into(from_r, to_r):

diff --git a/test/unit/common/middleware/helpers.py b/test/unit/common/middleware/helpers.py

index 68a4bfe..7c1b455 100644

--- a/test/unit/common/middleware/helpers.py

+++ b/test/unit/common/middleware/helpers.py

@@ -15,6 +15,7 @@

 # This stuff can't live in test/unit/__init__.py due to its swob dependency.

+from collections import defaultdict

 from copy import deepcopy

 from hashlib import md5

 from swift.common import swob

@@ -23,6 +24,20 @@ from swift.common.utils import split_path

 from test.unit import FakeLogger, FakeRing

+class LeakTrackingIter(object):

+    def __init__(self, inner_iter, fake_swift, path):

+        self.inner_iter = inner_iter

+        self.fake_swift = fake_swift

+        self.path = path

+

+    def __iter__(self):

+        for x in self.inner_iter:

+            yield x

+

+    def close(self):

+        self.fake_swift.mark_closed(self.path)

+

+

 class FakeSwift(object):

     """

     A good-enough fake Swift proxy server to use in testing middleware.

@@ -30,6 +45,7 @@ class FakeSwift(object):

     def __init__(self):

         self._calls = []

+        self._unclosed_req_paths = defaultdict(int)

         self.req_method_paths = []

         self.swift_sources = []

         self.uploaded = {}

@@ -105,7 +121,21 @@ class FakeSwift(object):

         req = swob.Request(env)

         resp = resp_class(req=req, headers=headers, body=body,

                           conditional_response=True)

-        return resp(env, start_response)

+        wsgi_iter = resp(env, start_response)

+        self.mark_opened(path)

+        return LeakTrackingIter(wsgi_iter, self, path)

+

+    def mark_opened(self, path):

+        self._unclosed_req_paths[path] += 1

+

+    def mark_closed(self, path):

+        self._unclosed_req_paths[path] -= 1

+

+    @property

+    def unclosed_requests(self):

+        return {path: count

+                for path, count in self._unclosed_req_paths.items()

+                if count > 0}

     @property

     def calls(self):

diff --git a/test/unit/common/middleware/test_dlo.py b/test/unit/common/middleware/test_dlo.py

index 16237eb..119e4ab 100644

--- a/test/unit/common/middleware/test_dlo.py

+++ b/test/unit/common/middleware/test_dlo.py

@@ -26,6 +26,7 @@ import unittest

 from swift.common import exceptions, swob

 from swift.common.middleware import dlo

+from swift.common.utils import closing_if_possible

 from test.unit.common.middleware.helpers import FakeSwift

@@ -54,8 +55,10 @@ class DloTestCase(unittest.TestCase):

         body = ''

         caught_exc = None

         try:

-            for chunk in body_iter:

-                body += chunk

+            # appease the close-checker

+            with closing_if_possible(body_iter):

+                for chunk in body_iter:

+                    body += chunk

         except Exception as exc:

             if expect_exception:

                 caught_exc = exc

@@ -279,6 +282,9 @@ class TestDloHeadManifest(DloTestCase):

 class TestDloGetManifest(DloTestCase):

+    def tearDown(self):

+        self.assertEqual(self.app.unclosed_requests, {})

+

     def test_get_manifest(self):

         expected_etag = '"%s"' % md5hex(

             md5hex("aaaaa") + md5hex("bbbbb") + md5hex("ccccc") +

diff --git a/test/unit/common/middleware/test_slo.py b/test/unit/common/middleware/test_slo.py

index 4160d91..4d483c8 100644

--- a/test/unit/common/middleware/test_slo.py

+++ b/test/unit/common/middleware/test_slo.py

@@ -24,7 +24,7 @@ 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 json

+from swift.common.utils import json, closing_if_possible

 from test.unit.common.middleware.helpers import FakeSwift

@@ -74,8 +74,10 @@ class SloTestCase(unittest.TestCase):

         body = ''

         caught_exc = None

         try:

-            for chunk in body_iter:

-                body += chunk

+            # appease the close-checker

+            with closing_if_possible(body_iter):

+                for chunk in body_iter:

+                    body += chunk

         except Exception as exc:

             if expect_exception:

                 caught_exc = exc

@@ -222,7 +224,7 @@ class TestSloPutManifest(SloTestCase):

             '/?multipart-manifest=put',

             environ={'REQUEST_METHOD': 'PUT'}, body=test_json_data)

         self.assertEquals(

-            self.slo.handle_multipart_put(req, fake_start_response),

+            list(self.slo.handle_multipart_put(req, fake_start_response)),

             ['passed'])

     def test_handle_multipart_put_success(self):

@@ -844,6 +846,9 @@ class TestSloGetManifest(SloTestCase):

                           'X-Object-Meta-Fish': 'Bass'},

             "[not {json (at ++++all")

+    def tearDown(self):

+        self.assertEqual(self.app.unclosed_requests, {})

+

     def test_get_manifest_passthrough(self):

         req = Request.blank(

             '/v1/AUTH_test/gettest/manifest-bc?multipart-manifest=get',

-- 

cgit v0.11.2