|
1 This upstream patch addresses CVE-2016-0737 which is filed under |
|
2 Launchpad bug 1466549. The issue is addressed in Swift 2.3.1 as well as |
|
3 2.5.1 or later. |
|
4 |
|
5 From 036c2f348d24c01c7a4deba3e44889c45270b46d Mon Sep 17 00:00:00 2001 |
|
6 From: Samuel Merritt <[email protected]> |
|
7 Date: Thu, 18 Jun 2015 12:58:03 -0700 |
|
8 Subject: Get better at closing WSGI iterables. |
|
9 |
|
10 PEP 333 (WSGI) says: "If the iterable returned by the application has |
|
11 a close() method, the server or gateway must call that method upon |
|
12 completion of the current request[.]" |
|
13 |
|
14 There's a bunch of places where we weren't doing that; some of them |
|
15 matter more than others. Calling .close() can prevent a connection |
|
16 leak in some cases. In others, it just provides a certain pedantic |
|
17 smugness. Either way, we should do what WSGI requires. |
|
18 |
|
19 Noteworthy goofs include: |
|
20 |
|
21 * If a client is downloading a large object and disconnects halfway |
|
22 through, a proxy -> obj connection may be leaked. In this case, |
|
23 the WSGI iterable is a SegmentedIterable, which lacked a close() |
|
24 method. Thus, when the WSGI server noticed the client disconnect, |
|
25 it had no way of telling the SegmentedIterable about it, and so |
|
26 the underlying iterable for the segment's data didn't get |
|
27 closed. |
|
28 |
|
29 Here, it seems likely (though unproven) that the object server |
|
30 would time out and kill the connection, or that a |
|
31 ChunkWriteTimeout would fire down in the proxy server, so the |
|
32 leaked connection would eventually go away. However, a flurry of |
|
33 client disconnects could leave a big pile of useless connections. |
|
34 |
|
35 * If a conditional request receives a 304 or 412, the underlying |
|
36 app_iter is not closed. This mostly affects conditional requests |
|
37 for large objects. |
|
38 |
|
39 The leaked connections were noticed by this patch's co-author, who |
|
40 made the changes to SegmentedIterable. Those changes helped, but did |
|
41 not completely fix, the issue. The rest of the patch is an attempt to |
|
42 plug the rest of the holes. |
|
43 |
|
44 Co-Authored-By: Romain LE DISEZ <[email protected]> |
|
45 |
|
46 Closes-Bug: #1466549 |
|
47 |
|
48 Change-Id: I168e147aae7c1728e7e3fdabb7fba6f2d747d937 |
|
49 (cherry picked from commit 12d8a53fffea6e4bed8ba3d502ce625f5c6710b9 |
|
50 with fixed import conflicts) |
|
51 --- |
|
52 swift/common/middleware/dlo.py | 8 ++++++-- |
|
53 swift/common/middleware/slo.py | 10 ++++++---- |
|
54 swift/common/request_helpers.py | 35 ++++++++++++--------------------- |
|
55 swift/common/swob.py | 9 ++++++++- |
|
56 swift/common/utils.py | 22 +++++++++++++++++++++ |
|
57 swift/proxy/controllers/obj.py | 4 ++-- |
|
58 test/unit/common/middleware/helpers.py | 32 +++++++++++++++++++++++++++++- |
|
59 test/unit/common/middleware/test_dlo.py | 10 ++++++++-- |
|
60 test/unit/common/middleware/test_slo.py | 13 ++++++++---- |
|
61 9 files changed, 105 insertions(+), 38 deletions(-) |
|
62 |
|
63 diff --git a/swift/common/middleware/dlo.py b/swift/common/middleware/dlo.py |
|
64 index d2761ac..9330ccb 100644 |
|
65 --- a/swift/common/middleware/dlo.py |
|
66 +++ b/swift/common/middleware/dlo.py |
|
67 @@ -22,7 +22,8 @@ from swift.common.http import is_success |
|
68 from swift.common.swob import Request, Response, \ |
|
69 HTTPRequestedRangeNotSatisfiable, HTTPBadRequest, HTTPConflict |
|
70 from swift.common.utils import get_logger, json, \ |
|
71 - RateLimitedIterator, read_conf_dir, quote |
|
72 + RateLimitedIterator, read_conf_dir, quote, close_if_possible, \ |
|
73 + closing_if_possible |
|
74 from swift.common.request_helpers import SegmentedIterable |
|
75 from swift.common.wsgi import WSGIContext, make_subrequest |
|
76 from urllib import unquote |
|
77 @@ -48,7 +49,8 @@ class GetContext(WSGIContext): |
|
78 con_resp = con_req.get_response(self.dlo.app) |
|
79 if not is_success(con_resp.status_int): |
|
80 return con_resp, None |
|
81 - return None, json.loads(''.join(con_resp.app_iter)) |
|
82 + with closing_if_possible(con_resp.app_iter): |
|
83 + return None, json.loads(''.join(con_resp.app_iter)) |
|
84 |
|
85 def _segment_listing_iterator(self, req, version, account, container, |
|
86 prefix, segments, first_byte=None, |
|
87 @@ -107,6 +109,7 @@ class GetContext(WSGIContext): |
|
88 # we've already started sending the response body to the |
|
89 # client, so all we can do is raise an exception to make the |
|
90 # WSGI server close the connection early |
|
91 + close_if_possible(error_response.app_iter) |
|
92 raise ListingIterError( |
|
93 "Got status %d listing container /%s/%s" % |
|
94 (error_response.status_int, account, container)) |
|
95 @@ -233,6 +236,7 @@ class GetContext(WSGIContext): |
|
96 # make sure this response is for a dynamic large object manifest |
|
97 for header, value in self._response_headers: |
|
98 if (header.lower() == 'x-object-manifest'): |
|
99 + close_if_possible(resp_iter) |
|
100 response = self.get_or_head_response(req, value) |
|
101 return response(req.environ, start_response) |
|
102 else: |
|
103 diff --git a/swift/common/middleware/slo.py b/swift/common/middleware/slo.py |
|
104 index e8f1707..c7a97a6 100644 |
|
105 --- a/swift/common/middleware/slo.py |
|
106 +++ b/swift/common/middleware/slo.py |
|
107 @@ -147,9 +147,9 @@ from swift.common.swob import Request, HTTPBadRequest, HTTPServerError, \ |
|
108 Response |
|
109 from swift.common.utils import json, get_logger, config_true_value, \ |
|
110 get_valid_utf8_str, override_bytes_from_content_type, split_path, \ |
|
111 - register_swift_info, RateLimitedIterator, quote |
|
112 -from swift.common.request_helpers import SegmentedIterable, \ |
|
113 - closing_if_possible, close_if_possible |
|
114 + register_swift_info, RateLimitedIterator, quote, close_if_possible, \ |
|
115 + closing_if_possible |
|
116 +from swift.common.request_helpers import SegmentedIterable |
|
117 from swift.common.constraints import check_utf8, MAX_BUFFERED_SLO_SEGMENTS |
|
118 from swift.common.http import HTTP_NOT_FOUND, HTTP_UNAUTHORIZED, is_success |
|
119 from swift.common.wsgi import WSGIContext, make_subrequest |
|
120 @@ -227,6 +227,7 @@ class SloGetContext(WSGIContext): |
|
121 sub_resp = sub_req.get_response(self.slo.app) |
|
122 |
|
123 if not is_success(sub_resp.status_int): |
|
124 + close_if_possible(sub_resp.app_iter) |
|
125 raise ListingIterError( |
|
126 'ERROR: while fetching %s, GET of submanifest %s ' |
|
127 'failed with status %d' % (req.path, sub_req.path, |
|
128 @@ -400,7 +401,8 @@ class SloGetContext(WSGIContext): |
|
129 return response(req.environ, start_response) |
|
130 |
|
131 def get_or_head_response(self, req, resp_headers, resp_iter): |
|
132 - resp_body = ''.join(resp_iter) |
|
133 + with closing_if_possible(resp_iter): |
|
134 + resp_body = ''.join(resp_iter) |
|
135 try: |
|
136 segments = json.loads(resp_body) |
|
137 except ValueError: |
|
138 diff --git a/swift/common/request_helpers.py b/swift/common/request_helpers.py |
|
139 index 14b9fd8..8aa8457 100644 |
|
140 --- a/swift/common/request_helpers.py |
|
141 +++ b/swift/common/request_helpers.py |
|
142 @@ -23,7 +23,6 @@ from swob in here without creating circular imports. |
|
143 import hashlib |
|
144 import itertools |
|
145 import time |
|
146 -from contextlib import contextmanager |
|
147 from urllib import unquote |
|
148 from swift import gettext_ as _ |
|
149 from swift.common.storage_policy import POLICIES |
|
150 @@ -32,7 +31,8 @@ from swift.common.exceptions import ListingIterError, SegmentError |
|
151 from swift.common.http import is_success |
|
152 from swift.common.swob import (HTTPBadRequest, HTTPNotAcceptable, |
|
153 HTTPServiceUnavailable) |
|
154 -from swift.common.utils import split_path, validate_device_partition |
|
155 +from swift.common.utils import split_path, validate_device_partition, \ |
|
156 + close_if_possible |
|
157 from swift.common.wsgi import make_subrequest |
|
158 |
|
159 |
|
160 @@ -249,26 +249,6 @@ def copy_header_subset(from_r, to_r, condition): |
|
161 to_r.headers[k] = v |
|
162 |
|
163 |
|
164 -def close_if_possible(maybe_closable): |
|
165 - close_method = getattr(maybe_closable, 'close', None) |
|
166 - if callable(close_method): |
|
167 - return close_method() |
|
168 - |
|
169 - |
|
170 -@contextmanager |
|
171 -def closing_if_possible(maybe_closable): |
|
172 - """ |
|
173 - Like contextlib.closing(), but doesn't crash if the object lacks a close() |
|
174 - method. |
|
175 - |
|
176 - PEP 333 (WSGI) says: "If the iterable returned by the application has a |
|
177 - close() method, the server or gateway must call that method upon |
|
178 - completion of the current request[.]" This function makes that easier. |
|
179 - """ |
|
180 - yield maybe_closable |
|
181 - close_if_possible(maybe_closable) |
|
182 - |
|
183 - |
|
184 class SegmentedIterable(object): |
|
185 """ |
|
186 Iterable that returns the object contents for a large object. |
|
187 @@ -304,6 +284,7 @@ class SegmentedIterable(object): |
|
188 self.peeked_chunk = None |
|
189 self.app_iter = self._internal_iter() |
|
190 self.validated_first_segment = False |
|
191 + self.current_resp = None |
|
192 |
|
193 def _internal_iter(self): |
|
194 start_time = time.time() |
|
195 @@ -360,6 +341,8 @@ class SegmentedIterable(object): |
|
196 'r_size': seg_resp.content_length, |
|
197 's_etag': seg_etag, |
|
198 's_size': seg_size}) |
|
199 + else: |
|
200 + self.current_resp = seg_resp |
|
201 |
|
202 seg_hash = hashlib.md5() |
|
203 for chunk in seg_resp.app_iter: |
|
204 @@ -431,3 +414,11 @@ class SegmentedIterable(object): |
|
205 return itertools.chain([pc], self.app_iter) |
|
206 else: |
|
207 return self.app_iter |
|
208 + |
|
209 + def close(self): |
|
210 + """ |
|
211 + Called when the client disconnect. Ensure that the connection to the |
|
212 + backend server is closed. |
|
213 + """ |
|
214 + if self.current_resp: |
|
215 + close_if_possible(self.current_resp.app_iter) |
|
216 diff --git a/swift/common/swob.py b/swift/common/swob.py |
|
217 index c2e3afb..a1bd9f6 100644 |
|
218 --- a/swift/common/swob.py |
|
219 +++ b/swift/common/swob.py |
|
220 @@ -49,7 +49,8 @@ import random |
|
221 import functools |
|
222 import inspect |
|
223 |
|
224 -from swift.common.utils import reiterate, split_path, Timestamp, pairs |
|
225 +from swift.common.utils import reiterate, split_path, Timestamp, pairs, \ |
|
226 + close_if_possible |
|
227 from swift.common.exceptions import InvalidTimestamp |
|
228 |
|
229 |
|
230 @@ -1203,12 +1204,14 @@ class Response(object): |
|
231 etag in self.request.if_none_match: |
|
232 self.status = 304 |
|
233 self.content_length = 0 |
|
234 + close_if_possible(app_iter) |
|
235 return [''] |
|
236 |
|
237 if etag and self.request.if_match and \ |
|
238 etag not in self.request.if_match: |
|
239 self.status = 412 |
|
240 self.content_length = 0 |
|
241 + close_if_possible(app_iter) |
|
242 return [''] |
|
243 |
|
244 if self.status_int == 404 and self.request.if_match \ |
|
245 @@ -1219,18 +1222,21 @@ class Response(object): |
|
246 # Failed) response. [RFC 2616 section 14.24] |
|
247 self.status = 412 |
|
248 self.content_length = 0 |
|
249 + close_if_possible(app_iter) |
|
250 return [''] |
|
251 |
|
252 if self.last_modified and self.request.if_modified_since \ |
|
253 and self.last_modified <= self.request.if_modified_since: |
|
254 self.status = 304 |
|
255 self.content_length = 0 |
|
256 + close_if_possible(app_iter) |
|
257 return [''] |
|
258 |
|
259 if self.last_modified and self.request.if_unmodified_since \ |
|
260 and self.last_modified > self.request.if_unmodified_since: |
|
261 self.status = 412 |
|
262 self.content_length = 0 |
|
263 + close_if_possible(app_iter) |
|
264 return [''] |
|
265 |
|
266 if self.request and self.request.method == 'HEAD': |
|
267 @@ -1244,6 +1250,7 @@ class Response(object): |
|
268 if ranges == []: |
|
269 self.status = 416 |
|
270 self.content_length = 0 |
|
271 + close_if_possible(app_iter) |
|
272 return [''] |
|
273 elif ranges: |
|
274 range_size = len(ranges) |
|
275 diff --git a/swift/common/utils.py b/swift/common/utils.py |
|
276 index 19dcfd3..85c3824 100644 |
|
277 --- a/swift/common/utils.py |
|
278 +++ b/swift/common/utils.py |
|
279 @@ -3143,6 +3143,28 @@ def ismount_raw(path): |
|
280 return False |
|
281 |
|
282 |
|
283 +def close_if_possible(maybe_closable): |
|
284 + close_method = getattr(maybe_closable, 'close', None) |
|
285 + if callable(close_method): |
|
286 + return close_method() |
|
287 + |
|
288 + |
|
289 +@contextmanager |
|
290 +def closing_if_possible(maybe_closable): |
|
291 + """ |
|
292 + Like contextlib.closing(), but doesn't crash if the object lacks a close() |
|
293 + method. |
|
294 + |
|
295 + PEP 333 (WSGI) says: "If the iterable returned by the application has a |
|
296 + close() method, the server or gateway must call that method upon |
|
297 + completion of the current request[.]" This function makes that easier. |
|
298 + """ |
|
299 + try: |
|
300 + yield maybe_closable |
|
301 + finally: |
|
302 + close_if_possible(maybe_closable) |
|
303 + |
|
304 + |
|
305 _rfc_token = r'[^()<>@,;:\"/\[\]?={}\x00-\x20\x7f]+' |
|
306 _rfc_extension_pattern = re.compile( |
|
307 r'(?:\s*;\s*(' + _rfc_token + r")\s*(?:=\s*(" + _rfc_token + |
|
308 diff --git a/swift/proxy/controllers/obj.py b/swift/proxy/controllers/obj.py |
|
309 index a83242b..784dfef 100644 |
|
310 --- a/swift/proxy/controllers/obj.py |
|
311 +++ b/swift/proxy/controllers/obj.py |
|
312 @@ -43,7 +43,7 @@ from swift.common.utils import ( |
|
313 clean_content_type, config_true_value, ContextPool, csv_append, |
|
314 GreenAsyncPile, GreenthreadSafeIterator, json, Timestamp, |
|
315 normalize_delete_at_timestamp, public, get_expirer_container, |
|
316 - quorum_size) |
|
317 + quorum_size, close_if_possible) |
|
318 from swift.common.bufferedhttp import http_connect |
|
319 from swift.common.constraints import check_metadata, check_object_creation, \ |
|
320 check_copy_from_header, check_destination_header, \ |
|
321 @@ -68,7 +68,7 @@ from swift.common.swob import HTTPAccepted, HTTPBadRequest, HTTPNotFound, \ |
|
322 HTTPServerError, HTTPServiceUnavailable, Request, HeaderKeyDict, \ |
|
323 HTTPClientDisconnect, HTTPUnprocessableEntity, Response, HTTPException |
|
324 from swift.common.request_helpers import is_sys_or_user_meta, is_sys_meta, \ |
|
325 - remove_items, copy_header_subset, close_if_possible |
|
326 + remove_items, copy_header_subset |
|
327 |
|
328 |
|
329 def copy_headers_into(from_r, to_r): |
|
330 diff --git a/test/unit/common/middleware/helpers.py b/test/unit/common/middleware/helpers.py |
|
331 index 68a4bfe..7c1b455 100644 |
|
332 --- a/test/unit/common/middleware/helpers.py |
|
333 +++ b/test/unit/common/middleware/helpers.py |
|
334 @@ -15,6 +15,7 @@ |
|
335 |
|
336 # This stuff can't live in test/unit/__init__.py due to its swob dependency. |
|
337 |
|
338 +from collections import defaultdict |
|
339 from copy import deepcopy |
|
340 from hashlib import md5 |
|
341 from swift.common import swob |
|
342 @@ -23,6 +24,20 @@ from swift.common.utils import split_path |
|
343 from test.unit import FakeLogger, FakeRing |
|
344 |
|
345 |
|
346 +class LeakTrackingIter(object): |
|
347 + def __init__(self, inner_iter, fake_swift, path): |
|
348 + self.inner_iter = inner_iter |
|
349 + self.fake_swift = fake_swift |
|
350 + self.path = path |
|
351 + |
|
352 + def __iter__(self): |
|
353 + for x in self.inner_iter: |
|
354 + yield x |
|
355 + |
|
356 + def close(self): |
|
357 + self.fake_swift.mark_closed(self.path) |
|
358 + |
|
359 + |
|
360 class FakeSwift(object): |
|
361 """ |
|
362 A good-enough fake Swift proxy server to use in testing middleware. |
|
363 @@ -30,6 +45,7 @@ class FakeSwift(object): |
|
364 |
|
365 def __init__(self): |
|
366 self._calls = [] |
|
367 + self._unclosed_req_paths = defaultdict(int) |
|
368 self.req_method_paths = [] |
|
369 self.swift_sources = [] |
|
370 self.uploaded = {} |
|
371 @@ -105,7 +121,21 @@ class FakeSwift(object): |
|
372 req = swob.Request(env) |
|
373 resp = resp_class(req=req, headers=headers, body=body, |
|
374 conditional_response=True) |
|
375 - return resp(env, start_response) |
|
376 + wsgi_iter = resp(env, start_response) |
|
377 + self.mark_opened(path) |
|
378 + return LeakTrackingIter(wsgi_iter, self, path) |
|
379 + |
|
380 + def mark_opened(self, path): |
|
381 + self._unclosed_req_paths[path] += 1 |
|
382 + |
|
383 + def mark_closed(self, path): |
|
384 + self._unclosed_req_paths[path] -= 1 |
|
385 + |
|
386 + @property |
|
387 + def unclosed_requests(self): |
|
388 + return {path: count |
|
389 + for path, count in self._unclosed_req_paths.items() |
|
390 + if count > 0} |
|
391 |
|
392 @property |
|
393 def calls(self): |
|
394 diff --git a/test/unit/common/middleware/test_dlo.py b/test/unit/common/middleware/test_dlo.py |
|
395 index 16237eb..119e4ab 100644 |
|
396 --- a/test/unit/common/middleware/test_dlo.py |
|
397 +++ b/test/unit/common/middleware/test_dlo.py |
|
398 @@ -26,6 +26,7 @@ import unittest |
|
399 |
|
400 from swift.common import exceptions, swob |
|
401 from swift.common.middleware import dlo |
|
402 +from swift.common.utils import closing_if_possible |
|
403 from test.unit.common.middleware.helpers import FakeSwift |
|
404 |
|
405 |
|
406 @@ -54,8 +55,10 @@ class DloTestCase(unittest.TestCase): |
|
407 body = '' |
|
408 caught_exc = None |
|
409 try: |
|
410 - for chunk in body_iter: |
|
411 - body += chunk |
|
412 + # appease the close-checker |
|
413 + with closing_if_possible(body_iter): |
|
414 + for chunk in body_iter: |
|
415 + body += chunk |
|
416 except Exception as exc: |
|
417 if expect_exception: |
|
418 caught_exc = exc |
|
419 @@ -279,6 +282,9 @@ class TestDloHeadManifest(DloTestCase): |
|
420 |
|
421 |
|
422 class TestDloGetManifest(DloTestCase): |
|
423 + def tearDown(self): |
|
424 + self.assertEqual(self.app.unclosed_requests, {}) |
|
425 + |
|
426 def test_get_manifest(self): |
|
427 expected_etag = '"%s"' % md5hex( |
|
428 md5hex("aaaaa") + md5hex("bbbbb") + md5hex("ccccc") + |
|
429 diff --git a/test/unit/common/middleware/test_slo.py b/test/unit/common/middleware/test_slo.py |
|
430 index 4160d91..4d483c8 100644 |
|
431 --- a/test/unit/common/middleware/test_slo.py |
|
432 +++ b/test/unit/common/middleware/test_slo.py |
|
433 @@ -24,7 +24,7 @@ from swift.common import swob, utils |
|
434 from swift.common.exceptions import ListingIterError, SegmentError |
|
435 from swift.common.middleware import slo |
|
436 from swift.common.swob import Request, Response, HTTPException |
|
437 -from swift.common.utils import json |
|
438 +from swift.common.utils import json, closing_if_possible |
|
439 from test.unit.common.middleware.helpers import FakeSwift |
|
440 |
|
441 |
|
442 @@ -74,8 +74,10 @@ class SloTestCase(unittest.TestCase): |
|
443 body = '' |
|
444 caught_exc = None |
|
445 try: |
|
446 - for chunk in body_iter: |
|
447 - body += chunk |
|
448 + # appease the close-checker |
|
449 + with closing_if_possible(body_iter): |
|
450 + for chunk in body_iter: |
|
451 + body += chunk |
|
452 except Exception as exc: |
|
453 if expect_exception: |
|
454 caught_exc = exc |
|
455 @@ -222,7 +224,7 @@ class TestSloPutManifest(SloTestCase): |
|
456 '/?multipart-manifest=put', |
|
457 environ={'REQUEST_METHOD': 'PUT'}, body=test_json_data) |
|
458 self.assertEquals( |
|
459 - self.slo.handle_multipart_put(req, fake_start_response), |
|
460 + list(self.slo.handle_multipart_put(req, fake_start_response)), |
|
461 ['passed']) |
|
462 |
|
463 def test_handle_multipart_put_success(self): |
|
464 @@ -844,6 +846,9 @@ class TestSloGetManifest(SloTestCase): |
|
465 'X-Object-Meta-Fish': 'Bass'}, |
|
466 "[not {json (at ++++all") |
|
467 |
|
468 + def tearDown(self): |
|
469 + self.assertEqual(self.app.unclosed_requests, {}) |
|
470 + |
|
471 def test_get_manifest_passthrough(self): |
|
472 req = Request.blank( |
|
473 '/v1/AUTH_test/gettest/manifest-bc?multipart-manifest=get', |
|
474 -- |
|
475 cgit v0.11.2 |
|
476 |