[OSSA-2012-017] Non-admin users can cause public glance images to be deleted

This bug would affect any image that a user can view but isn't allowed to delete, including shared images.

Here is a patchset that should fix things up.

commit a18ecf4a27e2bc461c6ea7a86c8927e1b5a9742f
Author: Mark Washenberger <email address hidden>
Date: Thu Oct 11 07:32:14 2012 +0000

    Delete from store after registry delete.

    Because we rely on the registry to determine authorization in the glance
    v1 api, we must attempt a registry delete before deleting an image from
    the image store.

    Fixes bug 1065187.

    Change-Id: I2cd06d151b1c9a2d0fd944f05c1d63fffe2f843a

diff --git a/glance/api/v1/images.py b/glance/api/v1/images.py
index 097dc05..9a64b4a 100644
--- a/glance/api/v1/images.py
+++ b/glance/api/v1/images.py
@@ -812,22 +812,28 @@ class Controller(controller.BaseController):
                                 request=req,
                                 content_type="text/plain")

- status = 'deleted'
+ if image['location'] and CONF.delayed_delete:
+ status = 'pending_delete'
+ else:
+ status = 'deleted'
+
         try:
+ # Delete the image from the registry first, since we rely on it
+ # for authorization checks.
+ # See https://bugs.launchpad.net/glance/+bug/1065187
+ registry.update_image_metadata(req.context, id, {'status': status})
+ registry.delete_image_metadata(req.context, id)
+
             # The image's location field may be None in the case
             # of a saving or queued image, therefore don't ask a backend
             # to delete the image if the backend doesn't yet store it.
             # See https://bugs.launchpad.net/glance/+bug/747799
             if image['location']:
                 if CONF.delayed_delete:
- status = 'pending_delete'
                     schedule_delayed_delete_from_backend(image['location'], id)
                 else:
                     safe_delete_from_backend(image['location'],
                                              req.context, id)
-
- registry.update_image_metadata(req.context, id, {'status': status})
- registry.delete_image_metadata(req.context, id)
         except exception.NotFound, e:
             msg = ("Failed to find image to delete: %(e)s" % locals())
             for line in msg.split('\n'):

Here is a patch that adds a test for this behavior. I did this separately so the fix could be backported alone, since this test depends on some more recent changes in master.

commit cce2af5a48de737869d53eeafdb0532eede136d4
Author: Mark Washenberger <email address hidden>
Date: Wed Oct 10 20:23:24 2012 +0000

    Add a test for bug 1065187.

    This is done separately from the bug fix to make it easier to apply the
    fix to older branches.

    Change-Id: I8964da5d074aabadbdcf8c6b7ef844b616e1aca4

diff --git a/glance/tests/stubs.py b/glance/tests/stubs.py
index fecea11..92dcf90 100644
--- a/glance/tests/stubs.py
+++ b/glance/tests/stubs.py
@@ -60,7 +60,13 @@ class FakeRegistryConnection(object):

     def getresponse(self):
         mapper = routes.Mapper()
- api = context.UnauthenticatedContextMiddleware(rserver.API(mapper))
+ server = rserver.API(mapper)
+ # NOTE(markwash): we need to pass through context auth information if
+ # we have it.
+ if 'X-Auth-Token' in self.req.headers:
+ api = utils.FakeAuthMiddleware(server)
+ else:
+ api = context.UnauthenticatedContextMiddleware(server)
         webob_res = self.req.get_response(api)

         return utils.FakeHTTPResponse(status=webob_res.status_int,
diff --git a/glance/tests/unit/v1/test_api.py b/glance/tests/unit/v1/test_api.py
index ce09aab..1a18bde 100644
--- a/glance/tests/unit/v1/test_api.py
+++ b/glance/tests/unit/v1/test_api.py
@@ -2930,6 +2930,26 @@ class TestGlanceAPI(base.IsolatedUnitTest):
         res = req.get_response(self.api)
         self.assertEquals(res.status_int, webob.exc.HTTPNotFound.code)

+ def test_delete_not_allowed(self):
+ # Verify we can get the image data
+ req = webob.Request.blank("/images/%s" % UUID2)
+ req.method = 'GET'
+ req.headers['X-Auth-Token'] = 'user:tenant:'
+ res = req.get_response(self.api)
+ self.assertEqual(res.status_int, 200)
+ self.assertEqual(len(res.body), 19)
+
+ # Verify we cannot delete the image
+ req.method = 'DELETE'
+ res = req.get_response(self.api)
+ self.assertEqual(res.status_int, 403)
+
+ # Verify the image data is still there
+ req.method = 'GET'
+ res = req.get_response(self.api)
+ self.assertEqual(res.status_int, 200)
+ self.assertEqual(len(res.body), 19)
+
     def test_delete_queued_image(self):
         """Delete an image in a queued state

diff --git a/glance/tests/utils.py b/glance/tests/utils.py
index 8054732..9971bf5 100644
--- a/glance/tests/utils.py
+++ b/glance/tests/utils.py
@@ -369,6 +369,7 @@ class FakeAuthMiddleware(wsgi.Middleware):
             'tenant': tenant,
             'roles': roles,
             'is_admin': self.is_admin,
+ 'auth_tok': auth_tok,
         }

         req.context = context.RequestContext(**kwargs)

Which versions are affected by this bug ? Folsom I presume, what about Essex/Diablo ?

Also two precisions on impact, so that we can draft the description correctly:

* you mention "non-protected images", could you elaborate on the class of images that are vulnerable ? Does it mean any image on non-read-only backends ? Or is there a way to "protect" images on read/write backends ?

* you mention "may delete the image", but from the looks of the code it looks like vulnerable ("non-protected") images would always get deleted by a delete request ?

I think I can help fill in the gaps.

Images have an attribute called "protected" which, if true, prevents them from being deleted. Since modifying protected to set it to False does have the appropriate authorization checks, protected images cannot be deleted from the backend storage in this way.

If a user submits a delete to an image they can see (but shouldn't be able to delete), they indeed *always* cause the image data to be deleted from the backend store unless the image has protected == True.

Also, from my assessment of the code, this particular bug potentially goes back a very long way to when the registry and api code were first integrated in commit fba60c3a588ccbae61e3ba8cf6558efc36d116a5 on December 13 2010.

However, it is also possible that authorization checking code was removed from glance.store at some point. I haven't been able to track that down yet.

I've verified that this fix works on stable/folsom

Adding markmc to cover stable release(s)

Essex has:
https://github.com/openstack/glance/blob/7fdccb1ada360b0ba8b69472477c5cedfa45bcda/glance/api/v1/images.py#L710
So unless req.context.read_only is set on non-admin requests, I guess it's vulnerable, please confirm.

Diablo has:
https://github.com/openstack/glance/blob/5b26c53c8e5143de37e270146d4ea9755c5c3f32/glance/api/v1/images.py#L598
which is very similar to Essex, so if Essex is vulnerable, Diablo will be.

Looks like a workaround for Folsom (apart from setting all images to protected) would be to refuse v1 API (enable_v1_api = False) as this seems to be v1-specific ?

OK, time to make progress on this... adding glance-core to the mix.

We need from Glance core:
* Formal pre-approval on the proposed patch (comment 8) -- currently it has one +1 from Brian
* confirmation that Diablo and Essex are vulnerable
* backported patches for Essex if vulnerable

Then the VMT will:
* Draft an impact description
* Pull stable/diablo team in if Diablo is affected
* Push the proposed fixes downstream

+2 on Mark's fix for Folsom.

The patch and tests look good for Folsom.

I've tried to reproduce on our (patched) diablo system and was unable to, with message "Attempted to modify image user did not own." . I then performed code analysis on the unpatched diablo/stable code branch -- see attached. I think Diablo is ok -- but would welcome a second pair of eyes.

I don't have a machine running Essex, so can't test, but looking at the code there seems -- at least in the case where delayed delete is enabled -- to be a problem.

Just looking at the code Essex (and Diablo) seem to be vulnerable in case you don't use delayed_delete. If you do, the code will return 403 but I'm not sure it won't still delete the image (after the delay) ?

Hi Thierry, which stable/diablo code path is vunerable?

I think this bit is ok:

def schedule_delete_from_backend(uri, options, context, image_id, **kwargs):
    """
    Given a uri and a time, schedule the deletion of an image.
    """
    use_delay = config.get_option(options, 'delayed_delete', type='bool',
                                  default=False)
    if not use_delay:
        registry.update_image_metadata(options, context, image_id,
                                       {'status': 'deleted'}) <<< if delayed delete is disabled we'll hit this -- which I think will fail with not authorized.
        try:
            return delete_from_backend(uri, **kwargs)
        except (UnsupportedBackend, exception.NotFound):
            msg = _("Failed to delete image from store (%(uri)s).") % locals()
            logger.error(msg)

    registry.update_image_metadata(options, context, image_id,
                                   {'status': 'pending_delete'}) <<< if delayed delete is enabled we'll hit this -- which I think will fail with not authorized.

@Stuart: I fear that in the latter scenario we'll have executed delete_from_backend() -- the 403 will just prevent you from also erasing metadata. But harm will have been done already... Or do I miss something ? I'm not exactly a Glance expert ;)

Hmm, the formatting for my post got lost -- sorry.

In "schedule_delete_from_backend" the "delete_from_backend" is indented so its part of the 'if not use_delay' conditional. My reading of this is that that whole if block will get skipped when delayed delete is switched on. We'll then hit the line where an attempt to set 'pending_delete' is made, and -- as far as I can tell -- get a not authorized at that point.

Am I reading this wrong/is there another path that takes us to 'delete_from_backend()'?

Thanks.

No, I think you're right, was mislead by indenting. Diablo sounds unaffected, and Essex affected. Another pair of eyes on that analysis couldn't hurt.

Anyone up for posting an Essex patch ?

Thierry, ok thanks. Always good to have a second pair of eyes.

I confirmed that this does affect Essex. An authenticated user can force a delete to be scheduled for an image he does not own if it is public or has been explicitly shared with him. The vulnerability does not apply in the case that delayed_delete is disabled.

The attached patch fixes the vulnerability for me

The two patches Mark has provided both have my approval to be applied to master.

The one patch that fixes the bug without any tests has my approval for stable/folsom.

I'd like to have somebody else verify my patch to stable/essex.

Proposed description:

Title: Authentication bypass for image deletion
Reporter: Gabe Westmaas (Rackspace)
Products: Glance
Affects: Essex, Folsom, Grizzly

Description:
Gabe Westmaas from Rackspace reported a vulnerability in Glance authentication of image deletion requests. Authenticated users may be able to delete arbitrary, non-protected images from Glance servers. Only setups using delayed deletes and enabling Glance v1 API are affected.

I'd reword the last sentence. My thinking is that the 'delayed_delete' bit only matters on Essex:

Folsom/Grizzly deployments that expose the v1 API are affected by this vulnerability. Essex deployments that use delayed_delete are also affected.

Good catch! New proposed description is:

Title: Authentication bypass for image deletion
Reporter: Gabe Westmaas (Rackspace)
Products: Glance
Affects: Essex, Folsom, Grizzly

Description:
Gabe Westmaas from Rackspace reported a vulnerability in Glance authentication of image deletion requests. Authenticated users may be able to delete arbitrary, non-protected images from Glance servers. Only Folsom/Grizzly deployments that expose the v1 API are affected by this vulnerability. Additionally, Essex deployments that use the delayed_delete option are also affected.

+1

Another glance-core review needed on Brian's Essex patch !

+1 For the Essex patch.

It looks like we're all set on approvals now. The next step is deciding on a release date and doing the advance notification.

How about Wednesday, November 7th?

Thierry, would you like to send out the advance notification on this? Let me know and I'll push it out if you'd like.

November 7th works for me.

Sent to downstream stakeholders

CVE-2012-4573

Adding canonical-security so that they can coordinate the Ubuntu fix within this bug

Fix proposed to branch: stable/essex
Review: https://review.openstack.org/15562

Fix proposed to branch: stable/folsom
Review: https://review.openstack.org/15563

Fix proposed to branch: master
Review: https://review.openstack.org/15564

In the future, try using "git format-patch" to export commits for posting to bugs. That makes it a whole lot easier to merge (using git am). :-)

Patches are public now, so opening the bug.

Reviewed: https://review.openstack.org/15564
Committed: http://github.com/openstack/glance/commit/6ab0992e5472ae3f9bef0d2ced41030655d9d2bc
Submitter: Jenkins
Branch: master

commit 6ab0992e5472ae3f9bef0d2ced41030655d9d2bc
Author: Mark Washenberger <email address hidden>
Date: Wed Nov 7 09:59:56 2012 -0500

    Delete from store after registry delete.

    Because we rely on the registry to determine authorization in the glance
    v1 api, we must attempt a registry delete before deleting an image from
    the image store.

    This patch includes the test for the bug, which was posted separately
    on the bug.

    Fixes bug 1065187.

    Change-Id: I1a06b7c7421524066c684539e2f3516c4ed2c475

Reviewed: https://review.openstack.org/15563
Committed: http://github.com/openstack/glance/commit/90bcdc5a89e350a358cf320a03f5afe99795f6f6
Submitter: Jenkins
Branch: stable/folsom

commit 90bcdc5a89e350a358cf320a03f5afe99795f6f6
Author: Mark Washenberger <email address hidden>
Date: Wed Nov 7 09:59:56 2012 -0500

    Delete from store after registry delete.

    Because we rely on the registry to determine authorization in the glance
    v1 api, we must attempt a registry delete before deleting an image from
    the image store.

    This patch includes the test for the bug, which was posted separately
    on the bug.

    Fixes bug 1065187.

    Change-Id: I1a06b7c7421524066c684539e2f3516c4ed2c475

I've been watching the Folsom patch iterations and have applied 90bcdc5a89e350a358cf320a03f5afe99795f6f6 based on patch set 7 from https://review.openstack.org/#/c/15563/, but I am still getting testsuite failures in the newly added test_delete_not_allowed() on Ubuntu 12.10 with 2012.2-0ubuntu2. Please see attached.

The advisory (OSSA 2012-017) has been posted.

Unfortunately, not all of the patches made it through jenkins today. The patches for master and stable/folsom did. The patch for stable/essex did not. First it was blocked by the sqlalchemy upgrade. Pinning the sqlalchemy version got merged into stable/essex. Now the patch is blocked by a failure in tempest. The advisory just includes a link to the gerrit review for stable/essex.

It would be great if the glance guys could help get this patch pushed through for stable/essex.

advisory for reference: https://lists.launchpad.net/openstack/msg18386.html

@jamie: did the patch merged to stable/folsom finally solve it for you ? Looks like the tests passed on the final merge...

FWIW, we are working on setting up a private Gerrit instance so that we can review and test the security patches more effectively. The current "attach patch, +1 and pray" approach really sucks and we know it.

@Thierry: no, I was using that final commit. After some investigation I discovered I also needed to patch the testsuite with 7841cc93050fa48a1fbd8847cec887f0360be9a3 ('FakeAuth not always admin'). Now the testsuite passes. Thanks.

Reviewed: https://review.openstack.org/15562
Committed: http://github.com/openstack/glance/commit/efd7e75b1f419a52c7103c7840e24af8e5deb29d
Submitter: Jenkins
Branch: stable/essex

commit efd7e75b1f419a52c7103c7840e24af8e5deb29d
Author: Brian Waldon <email address hidden>
Date: Wed Nov 7 10:06:43 2012 -0500

    Ensure image owned by user before delayed_deletion

    Fixes bug 1065187.

    Change-Id: Icf2f117a094c712bad645ef5f297e9f7da994c84

Errata published at: https://lists.launchpad.net/openstack/msg18466.html

This was fixed in 12.10 in http://www.ubuntu.com/usn/usn-1626-1.

Hello Gabe, or anyone else affected,

Accepted glance into quantal-proposed. The package will build now and be available at http://launchpad.net/ubuntu/+source/glance/2012.2.1-0ubuntu1 in a few hours, and then in the -proposed repository.

Please help us by testing this new package. See https://wiki.ubuntu.com/Testing/EnableProposed for documentation how to enable and use -proposed. Your feedback will aid us getting this update out to other Ubuntu users.

If this package fixes the bug for you, please add a comment to this bug, mentioning the version of the package you tested, and change the tag from verification-needed to verification-done. If it does not fix the bug for you, please add a comment stating that, and change the tag to verification-failed. In either case, details of your testing will help us make a better decision.

Further information regarding the verification process can be found at https://wiki.ubuntu.com/QATeam/PerformingSRUVerification . Thank you in advance!

Hello Gabe, or anyone else affected,

Accepted glance into precise-proposed. The package will build now and be available at http://launchpad.net/ubuntu/+source/glance/2012.1.3+stable-20130423-74b067df-0ubuntu1 in a few hours, and then in the -proposed repository.

Please help us by testing this new package. See https://wiki.ubuntu.com/Testing/EnableProposed for documentation how to enable and use -proposed. Your feedback will aid us getting this update out to other Ubuntu users.

If this package fixes the bug for you, please add a comment to this bug, mentioning the version of the package you tested, and change the tag from verification-needed to verification-done. If it does not fix the bug for you, please add a comment stating that, and change the tag to verification-failed. In either case, details of your testing will help us make a better decision.

Further information regarding the verification process can be found at https://wiki.ubuntu.com/QATeam/PerformingSRUVerification . Thank you in advance!

Please find the attached test log from the Ubuntu Server Team's CI infrastructure. As part of the verification process for this bug, Glance has been deployed and configured across multiple nodes using precise-proposed as an installation source. After successful bring-up and configuration of the cluster, a number of exercises and smoke tests have be invoked to ensure the updated package did not introduce any regressions. A number of test iterations were carried out to catch any possible transient errors.

Please Note the list of installed packages at the top and bottom of the report.

For records of upstream test coverage of this update, please see the Jenkins links in the comments of the relevant upstream code-review(s):

Trunk review: https://review.openstack.org/15564
Stable review: https://review.openstack.org/15562

As per the provisional Micro Release Exception granted to this package by the Technical Board, we hope this contributes toward verification of this update.

Test coverage log.

The verification of this Stable Release Update has completed successfully and the package has now been released to -updates. Subsequently, the Ubuntu Stable Release Updates Team is being unsubscribed and will not receive messages about this bug report. In the event that you encounter a regression using the package from -updates please report a new bug using ubuntu-bug and tag the bug report regression-update so we can easily find any regresssions.