23127835 problem in SERVICE/GLANCE
authorDrew Fisher <drew.fisher@oracle.com>
Mon, 25 Apr 2016 08:26:37 -0700
changeset 5842 292637cfef88
parent 5841 97e8c4dc6a82
child 5843 9cefd39b551b
23127835 problem in SERVICE/GLANCE
components/openstack/glance/patches/08-CVE-2016-0757.patch
--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
+++ b/components/openstack/glance/patches/08-CVE-2016-0757.patch	Mon Apr 25 08:26:37 2016 -0700
@@ -0,0 +1,338 @@
+This fix will be included in future 2015.1.3 (kilo) and 11.0.2
+(liberty) releases.
+
+From c5c731c7153d6d46c27260474d2811d504dfac5c Mon Sep 17 00:00:00 2001
+From: Erno Kuvaja <[email protected]>
+Date: Tue, 19 Jan 2016 13:37:05 +0000
+Subject: [PATCH] Prevent user to remove last location of the image
+
+If the last location of the image is removed, image transitions back to queued.
+This allows user to upload new data into the existing image record. By
+preventing removal of the last location we prevent the image transition back to
+queued.
+
+This change also prevents doing the same operation via replacing the locations
+with empty list.
+
+SecurityImpact
+DocImpact
+APIImpact
+
+Conflicts:
+	glance/tests/unit/v2/test_images_resource.py
+
+Conflicts:
+	glance/api/v2/images.py
+
+Change-Id: Ieb03aaba887492819f9c58aa67f7acfcea81720e
+Closes-Bug: #1525915
+(cherry picked from commit e9e45baa9aaf58e69964419b6b4fb2048d115a0c)
+---
+ glance/api/v2/images.py                            |  19 +++-
+ glance/tests/functional/v2/test_images.py          |  14 ---
+ glance/tests/unit/v2/test_images_resource.py       | 122 ++++-----------------
+ ...oving-last-image-location-d5ee3e00efe14f34.yaml |  10 ++
+ 4 files changed, 44 insertions(+), 121 deletions(-)
+ create mode 100644 releasenotes/notes/Prevent-removing-last-image-location-d5ee3e00efe14f34.yaml
+
+diff --git a/glance/api/v2/images.py b/glance/api/v2/images.py
+index 22c8e70..90435ac 100644
+--- a/glance/api/v2/images.py
++++ b/glance/api/v2/images.py
+@@ -168,7 +168,10 @@ class ImagesController(object):
+         path = change['path']
+         path_root = path[0]
+         value = change['value']
+-        if path_root == 'locations':
++        if path_root == 'locations' and value == []:
++            msg = _("Cannot set locations to empty list.")
++            raise webob.exc.HTTPForbidden(message=msg)
++        elif path_root == 'locations' and value != []:
+             self._do_replace_locations(image, value)
+         else:
+             if hasattr(image, path_root):
+@@ -201,7 +204,10 @@ class ImagesController(object):
+         path = change['path']
+         path_root = path[0]
+         if path_root == 'locations':
+-            self._do_remove_locations(image, path[1])
++            try:
++                self._do_remove_locations(image, path[1])
++            except exception.Forbidden as e:
++                raise webob.exc.HTTPForbidden(e.msg)
+         else:
+             if hasattr(image, path_root):
+                 msg = _("Property %s may not be removed.")
+@@ -285,6 +291,11 @@ class ImagesController(object):
+                 explanation=utils.exception_to_str(ve))
+ 
+     def _do_remove_locations(self, image, path_pos):
++        if len(image.locations) == 1:
++            LOG.debug("User forbidden to remove last location of image %s",
++                      image.image_id)
++            msg = _("Cannot remove last location in the image.")
++            raise exception.Forbidden(message=msg)
+         pos = self._get_locations_op_pos(path_pos,
+                                          len(image.locations), False)
+         if pos is None:
+@@ -294,11 +305,11 @@ class ImagesController(object):
+             # NOTE(zhiyan): this actually deletes the location
+             # from the backend store.
+             image.locations.pop(pos)
++        # TODO(jokke): Fix this, we should catch what store throws and
++        # provide definitely something else than IternalServerError to user.
+         except Exception as e:
+             raise webob.exc.HTTPInternalServerError(
+                 explanation=utils.exception_to_str(e))
+-        if len(image.locations) == 0 and image.status == 'active':
+-            image.status = 'queued'
+ 
+ 
+ class RequestDeserializer(wsgi.JSONRequestDeserializer):
+diff --git a/glance/tests/functional/v2/test_images.py b/glance/tests/functional/v2/test_images.py
+index 35d3ab2..c27c27f 100644
+--- a/glance/tests/functional/v2/test_images.py
++++ b/glance/tests/functional/v2/test_images.py
+@@ -489,20 +489,6 @@ class TestImages(functional.FunctionalTest):
+         response = requests.patch(path, headers=headers, data=data)
+         self.assertEqual(200, response.status_code, response.text)
+ 
+-        # Remove all locations of the image then the image size shouldn't be
+-        # able to access
+-        path = self._url('/v2/images/%s' % image2_id)
+-        media_type = 'application/openstack-images-v2.1-json-patch'
+-        headers = self._headers({'content-type': media_type})
+-        doc = [{'op': 'replace', 'path': '/locations', 'value': []}]
+-        data = jsonutils.dumps(doc)
+-        response = requests.patch(path, headers=headers, data=data)
+-        self.assertEqual(200, response.status_code, response.text)
+-        image = jsonutils.loads(response.text)
+-        self.assertIsNone(image['size'])
+-        self.assertIsNone(image['virtual_size'])
+-        self.assertEqual('queued', image['status'])
+-
+         # Deletion should work. Deleting image-1
+         path = self._url('/v2/images/%s' % image_id)
+         response = requests.delete(path, headers=self._headers())
+diff --git a/glance/tests/unit/v2/test_images_resource.py b/glance/tests/unit/v2/test_images_resource.py
+index be4e60a..9cc1f25 100644
+--- a/glance/tests/unit/v2/test_images_resource.py
++++ b/glance/tests/unit/v2/test_images_resource.py
+@@ -1323,26 +1323,6 @@ class TestImagesController(base.IsolatedUnitTest):
+         self.assertRaises(webob.exc.HTTPConflict, self.controller.update,
+                           another_request, created_image.image_id, changes)
+ 
+-    def test_update_replace_locations(self):
+-        self.stubs.Set(store, 'get_size_from_backend',
+-                       unit_test_utils.fake_get_size_from_backend)
+-        request = unit_test_utils.get_fake_request()
+-        changes = [{'op': 'replace', 'path': ['locations'], 'value': []}]
+-        output = self.controller.update(request, UUID1, changes)
+-        self.assertEqual(UUID1, output.image_id)
+-        self.assertEqual(0, len(output.locations))
+-        self.assertEqual('queued', output.status)
+-        self.assertIsNone(output.size)
+-
+-        new_location = {'url': '%s/fake_location' % BASE_URI, 'metadata': {}}
+-        changes = [{'op': 'replace', 'path': ['locations'],
+-                    'value': [new_location]}]
+-        output = self.controller.update(request, UUID1, changes)
+-        self.assertEqual(UUID1, output.image_id)
+-        self.assertEqual(1, len(output.locations))
+-        self.assertEqual(new_location, output.locations[0])
+-        self.assertEqual('active', output.status)
+-
+     def test_update_replace_locations_non_empty(self):
+         new_location = {'url': '%s/fake_location' % BASE_URI, 'metadata': {}}
+         request = unit_test_utils.get_fake_request()
+@@ -1354,35 +1334,9 @@ class TestImagesController(base.IsolatedUnitTest):
+     def test_update_replace_locations_invalid(self):
+         request = unit_test_utils.get_fake_request()
+         changes = [{'op': 'replace', 'path': ['locations'], 'value': []}]
+-        output = self.controller.update(request, UUID1, changes)
+-        self.assertEqual(UUID1, output.image_id)
+-        self.assertEqual(0, len(output.locations))
+-        self.assertEqual('queued', output.status)
+-
+-        request = unit_test_utils.get_fake_request()
+-        changes = [{'op': 'replace', 'path': ['locations'],
+-                    'value': [{'url': 'unknow://foo', 'metadata': {}}]}]
+-        self.assertRaises(webob.exc.HTTPBadRequest, self.controller.update,
++        self.assertRaises(webob.exc.HTTPForbidden, self.controller.update,
+                           request, UUID1, changes)
+ 
+-    def test_update_replace_locations_status_exception(self):
+-        self.stubs.Set(store, 'get_size_from_backend',
+-                       unit_test_utils.fake_get_size_from_backend)
+-        request = unit_test_utils.get_fake_request()
+-        changes = [{'op': 'replace', 'path': ['locations'], 'value': []}]
+-        output = self.controller.update(request, UUID2, changes)
+-        self.assertEqual(UUID2, output.image_id)
+-        self.assertEqual(0, len(output.locations))
+-        self.assertEqual('queued', output.status)
+-
+-        self.db.image_update(None, UUID2, {'disk_format': None})
+-
+-        new_location = {'url': '%s/fake_location' % BASE_URI, 'metadata': {}}
+-        changes = [{'op': 'replace', 'path': ['locations'],
+-                    'value': [new_location]}]
+-        self.assertRaises(webob.exc.HTTPBadRequest, self.controller.update,
+-                          request, UUID2, changes)
+-
+     def test_update_add_property(self):
+         request = unit_test_utils.get_fake_request()
+ 
+@@ -1506,24 +1460,6 @@ class TestImagesController(base.IsolatedUnitTest):
+         self.assertRaises(webob.exc.HTTPBadRequest, self.controller.update,
+                           request, UUID1, changes)
+ 
+-    def test_update_add_locations_status_exception(self):
+-        self.stubs.Set(store, 'get_size_from_backend',
+-                       unit_test_utils.fake_get_size_from_backend)
+-        request = unit_test_utils.get_fake_request()
+-        changes = [{'op': 'replace', 'path': ['locations'], 'value': []}]
+-        output = self.controller.update(request, UUID2, changes)
+-        self.assertEqual(UUID2, output.image_id)
+-        self.assertEqual(0, len(output.locations))
+-        self.assertEqual('queued', output.status)
+-
+-        self.db.image_update(None, UUID2, {'disk_format': None})
+-
+-        new_location = {'url': '%s/fake_location' % BASE_URI, 'metadata': {}}
+-        changes = [{'op': 'add', 'path': ['locations', '-'],
+-                    'value': new_location}]
+-        self.assertRaises(webob.exc.HTTPBadRequest, self.controller.update,
+-                          request, UUID2, changes)
+-
+     def test_update_add_duplicate_locations(self):
+         new_location = {'url': '%s/fake_location' % BASE_URI, 'metadata': {}}
+         request = unit_test_utils.get_fake_request()
+@@ -1537,23 +1473,6 @@ class TestImagesController(base.IsolatedUnitTest):
+         self.assertRaises(webob.exc.HTTPBadRequest, self.controller.update,
+                           request, UUID1, changes)
+ 
+-    def test_update_replace_duplicate_locations(self):
+-        self.stubs.Set(store, 'get_size_from_backend',
+-                       unit_test_utils.fake_get_size_from_backend)
+-        request = unit_test_utils.get_fake_request()
+-        changes = [{'op': 'replace', 'path': ['locations'], 'value': []}]
+-        output = self.controller.update(request, UUID1, changes)
+-        self.assertEqual(UUID1, output.image_id)
+-        self.assertEqual(0, len(output.locations))
+-        self.assertEqual('queued', output.status)
+-
+-        new_location = {'url': '%s/fake_location' % BASE_URI, 'metadata': {}}
+-        changes = [{'op': 'replace', 'path': ['locations'],
+-                    'value': [new_location, new_location]}]
+-
+-        self.assertRaises(webob.exc.HTTPBadRequest, self.controller.update,
+-                          request, UUID1, changes)
+-
+     def test_update_add_too_many_locations(self):
+         self.config(image_location_quota=1)
+         request = unit_test_utils.get_fake_request()
+@@ -1654,9 +1573,12 @@ class TestImagesController(base.IsolatedUnitTest):
+             {'op': 'add', 'path': ['locations', '-'],
+              'value': {'url': '%s/fake_location_1' % BASE_URI,
+                        'metadata': {}}},
++            {'op': 'add', 'path': ['locations', '-'],
++             'value': {'url': '%s/fake_location_2' % BASE_URI,
++                       'metadata': {}}},
+         ]
+         self.controller.update(request, UUID1, changes)
+-        self.config(image_location_quota=1)
++        self.config(image_location_quota=2)
+ 
+         # We must remove two properties to avoid being
+         # over the limit of 1 property
+@@ -1669,8 +1591,8 @@ class TestImagesController(base.IsolatedUnitTest):
+         ]
+         output = self.controller.update(request, UUID1, changes)
+         self.assertEqual(UUID1, output.image_id)
+-        self.assertEqual(1, len(output.locations))
+-        self.assertIn('fake_location_3', output.locations[0]['url'])
++        self.assertEqual(2, len(output.locations))
++        self.assertIn('fake_location_3', output.locations[1]['url'])
+         self.assertNotEqual(output.created_at, output.updated_at)
+ 
+     def test_update_remove_base_property(self):
+@@ -1711,24 +1633,23 @@ class TestImagesController(base.IsolatedUnitTest):
+                        unit_test_utils.fake_get_size_from_backend)
+ 
+         request = unit_test_utils.get_fake_request()
+-        changes = [{'op': 'remove', 'path': ['locations', '0']}]
+-        output = self.controller.update(request, UUID1, changes)
+-        self.assertEqual(output.image_id, UUID1)
+-        self.assertEqual(0, len(output.locations))
+-        self.assertEqual('queued', output.status)
+-        self.assertIsNone(output.size)
+-
+         new_location = {'url': '%s/fake_location' % BASE_URI, 'metadata': {}}
+         changes = [{'op': 'add', 'path': ['locations', '-'],
+                     'value': new_location}]
++        self.controller.update(request, UUID1, changes)
++        changes = [{'op': 'remove', 'path': ['locations', '0']}]
+         output = self.controller.update(request, UUID1, changes)
+         self.assertEqual(UUID1, output.image_id)
+         self.assertEqual(1, len(output.locations))
+-        self.assertEqual(new_location, output.locations[0])
+         self.assertEqual('active', output.status)
+ 
+     def test_update_remove_location_invalid_pos(self):
+         request = unit_test_utils.get_fake_request()
++        changes = [
++            {'op': 'add', 'path': ['locations', '-'],
++             'value': {'url': '%s/fake_location' % BASE_URI,
++                       'metadata': {}}}]
++        self.controller.update(request, UUID1, changes)
+         changes = [{'op': 'remove', 'path': ['locations', None]}]
+         self.assertRaises(webob.exc.HTTPBadRequest, self.controller.update,
+                           request, UUID1, changes)
+@@ -1750,6 +1671,11 @@ class TestImagesController(base.IsolatedUnitTest):
+                        fake_delete_image_location_from_backend)
+ 
+         request = unit_test_utils.get_fake_request()
++        changes = [
++            {'op': 'add', 'path': ['locations', '-'],
++             'value': {'url': '%s/fake_location' % BASE_URI,
++                       'metadata': {}}}]
++        self.controller.update(request, UUID1, changes)
+         changes = [{'op': 'remove', 'path': ['locations', '0']}]
+         self.assertRaises(webob.exc.HTTPInternalServerError,
+                           self.controller.update, request, UUID1, changes)
+@@ -2036,16 +1962,6 @@ class TestImagesControllerPolicies(base.IsolatedUnitTest):
+         self.assertRaises(webob.exc.HTTPForbidden, self.controller.update,
+                           request, UUID1, changes)
+ 
+-        self.stubs.Set(self.store_utils, 'delete_image_location_from_backend',
+-                       fake_delete_image_location_from_backend)
+-
+-        changes = [{'op': 'replace', 'path': ['locations'], 'value': []}]
+-        self.controller.update(request, UUID1, changes)
+-        changes = [{'op': 'replace', 'path': ['locations'],
+-                    'value': [new_location]}]
+-        self.assertRaises(webob.exc.HTTPForbidden, self.controller.update,
+-                          request, UUID1, changes)
+-
+     def test_update_delete_image_location_unauthorized(self):
+         rules = {"delete_image_location": False}
+         self.policy.set_rules(rules)
+diff --git a/releasenotes/notes/Prevent-removing-last-image-location-d5ee3e00efe14f34.yaml b/releasenotes/notes/Prevent-removing-last-image-location-d5ee3e00efe14f34.yaml
+new file mode 100644
+index 0000000..344e6e5
+--- /dev/null
++++ b/releasenotes/notes/Prevent-removing-last-image-location-d5ee3e00efe14f34.yaml
+@@ -0,0 +1,10 @@
++---
++security:
++  - Fixing bug 1525915; image might be transitioning
++    from active to queued by regular user by removing
++    last location of image (or replacing locations
++    with empty list). This allows user to re-upload
++    data to the image breaking Glance's promise of
++    image data immutability. From now on, last
++    location cannot be removed and locations cannot
++    be replaced with empty list.
+-- 
+1.9.1
+