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

[Gabe Westmaas]

Given a public, non-protected image, a non-admin user can issue a delete against that image which may delete the image from the backend storage repository. The client will get a 403 unauthorized response, but the backend delete method is called prior to checking for those permissions on the glance registry.

[Mark Washenberger (markwash)]

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

[Mark Washenberger (markwash)]

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'):

[Mark Washenberger (markwash)]

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)

[Thierry Carrez (ttx)]

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

[Thierry Carrez (ttx)]

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 ?

[Mark Washenberger (markwash)]

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.

[Mark Washenberger (markwash)]

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.

[Brian Waldon (bcwaldon)]

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

[Thierry Carrez (ttx)]

Adding markmc to cover stable release(s)

[Thierry Carrez (ttx)]

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 ?

[Thierry Carrez (ttx)]

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

[Eoghan Glynn (eglynn)]

+2 on Mark's fix for Folsom.

[Iccha Sethi (iccha-sethi)]

The patch and tests look good for Folsom.

[Stuart McLaren (stuart-mclaren)]

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.

[Stuart McLaren (stuart-mclaren)]

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.

[Thierry Carrez (ttx)]

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

[Stuart McLaren (stuart-mclaren)]

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.

[Thierry Carrez (ttx)]

@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 ;)

[Stuart McLaren (stuart-mclaren)]

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.

[Thierry Carrez (ttx)]

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.

[Thierry Carrez (ttx)]

Anyone up for posting an Essex patch ?

[Stuart McLaren (stuart-mclaren)]

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

[Brian Waldon (bcwaldon)]

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.

[Brian Waldon (bcwaldon)]

The attached patch fixes the vulnerability for me

[Brian Waldon (bcwaldon)]

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.

[Thierry Carrez (ttx)]

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.

[Brian Waldon (bcwaldon)]

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.

[Thierry Carrez (ttx)]

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.

[Brian Waldon (bcwaldon)]

+1

[Thierry Carrez (ttx)]

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

[Stuart McLaren (stuart-mclaren)]

+1 For the Essex patch.

[Russell Bryant (russellb)]

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.

[Brian Waldon (bcwaldon)]

November 7th works for me.

[Thierry Carrez (ttx)]

Sent to downstream stakeholders

[Thierry Carrez (ttx)]

CVE-2012-4573

[Thierry Carrez (ttx)]

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

[OpenStack Infra (hudson-openstack)]

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

[OpenStack Infra (hudson-openstack)]

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

[OpenStack Infra (hudson-openstack)]

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

[Russell Bryant (russellb)]

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). :-)

[Russell Bryant (russellb)]

Patches are public now, so opening the bug.

[OpenStack Infra (hudson-openstack)]

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

[OpenStack Infra (hudson-openstack)]

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

[Jamie Strandboge (jdstrand)]

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.

[Russell Bryant (russellb)]

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.

[Russell Bryant (russellb)]

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

[Thierry Carrez (ttx)]

@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.

[Jamie Strandboge (jdstrand)]

@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.

[OpenStack Infra (hudson-openstack)]

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

[Thierry Carrez (ttx)]

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

[Jamie Strandboge (jdstrand)]

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

[Clint Byrum (clint-fewbar)]

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!

[Brian Murray (brian-murray)]

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!

[Yolanda Robla (yolanda.robla)]

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.

[Yolanda Robla (yolanda.robla)]

Test coverage log.

[Scott Kitterman (kitterman)]

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.