[OSSA-2012-017] Non-admin users can cause public glance images to be deleted
Affects | Status | Importance | Assigned to | Milestone | |
---|---|---|---|---|---|
| Glance |
Critical
|
Russell Bryant | ||
| Essex |
Critical
|
Brian Waldon | ||
| Folsom |
Critical
|
Mark Washenberger | ||
| Grizzly |
Critical
|
Russell Bryant | ||
| OpenStack Security Advisory |
Undecided
|
Russell Bryant | ||
| glance (Ubuntu) |
Undecided
|
Unassigned | ||
| Quantal |
Undecided
|
Unassigned |
Bug Description
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.
Related branches
- Openstack Ubuntu Testers: Pending requested 2012-11-30
-
Diff: 86 lines (+55/-2)2 files modifieddebian/changelog (+53/-1)
debian/control (+2/-1)
- Ubuntu Server Developers: Pending requested 2012-03-16
-
Diff: 31 lines (+10/-3)2 files modifieddebian/changelog (+6/-0)
debian/glance-common.postinst (+4/-3)
CVE References
Changed in glance: | |
importance: | Undecided → Critical |
assignee: | nobody → Mark Washenberger (markwash) |
status: | New → In Progress |
milestone: | none → grizzly-1 |
Mark Washenberger (markwash) wrote : | #1 |
Mark Washenberger (markwash) wrote : | #2 |
Here is a patchset that should fix things up.
commit a18ecf4a27e2bc4
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: I2cd06d151b1c9a
diff --git a/glance/
index 097dc05..9a64b4a 100644
--- a/glance/
+++ b/glance/
@@ -812,22 +812,28 @@ class Controller(
- status = 'deleted'
+ if image['location'] and CONF.delayed_
+ status = 'pending_delete'
+ else:
+ status = 'deleted'
+
try:
+ # Delete the image from the registry first, since we rely on it
+ # for authorization checks.
+ # See https:/
+ registry.
+ registry.
+
# 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:/
if image['location']:
if CONF.delayed_
- status = 'pending_delete'
-
- registry.
- registry.
except exception.NotFound, e:
msg = ("Failed to find image to delete: %(e)s" % locals())
for line in msg.split('\n'):
Mark Washenberger (markwash) wrote : | #3 |
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 cce2af5a48de737
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: I8964da5d074aab
diff --git a/glance/
index fecea11..92dcf90 100644
--- a/glance/
+++ b/glance/
@@ -60,7 +60,13 @@ class FakeRegistryCon
def getresponse(self):
mapper = routes.Mapper()
- api = context.
+ 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.FakeAuthM
+ else:
+ api = context.
webob_res = self.req.
return utils.FakeHTTPR
diff --git a/glance/
index ce09aab..1a18bde 100644
--- a/glance/
+++ b/glance/
@@ -2930,6 +2930,26 @@ class TestGlanceAPI(
res = req.get_
+ def test_delete_
+ # Verify we can get the image data
+ req = webob.Request.
+ req.method = 'GET'
+ req.headers[
+ res = req.get_
+ self.assertEqua
+ self.assertEqua
+
+ # Verify we cannot delete the image
+ req.method = 'DELETE'
+ res = req.get_
+ self.assertEqua
+
+ # Verify the image data is still there
+ req.method = 'GET'
+ res = req.get_
+ self.assertEqua
+ self.assertEqua
+
def test_delete_
"""Delete an image in a queued state
diff --git a/glance/
index 8054732..9971bf5 100644
--- a/glance/
+++ b/glance/
@@ -369,6 +369,7 @@ class FakeAuthMiddlew
+ 'auth_tok': auth_tok,
}
Thierry Carrez (ttx) wrote : | #4 |
Which versions are affected by this bug ? Folsom I presume, what about Essex/Diablo ?
Thierry Carrez (ttx) wrote : | #5 |
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) wrote : | #6 |
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) wrote : | #7 |
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 fba60c3a588ccba
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.
Mark Washenberger (markwash) wrote : | #8 |
Mark Washenberger (markwash) wrote : | #9 |
Brian Waldon (bcwaldon) wrote : | #10 |
I've verified that this fix works on stable/folsom
Thierry Carrez (ttx) wrote : | #11 |
Adding markmc to cover stable release(s)
Thierry Carrez (ttx) wrote : | #12 |
Essex has:
https:/
So unless req.context.
Diablo has:
https:/
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) wrote : | #13 |
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) wrote : | #14 |
+2 on Mark's fix for Folsom.
Iccha Sethi (iccha-sethi) wrote : | #15 |
The patch and tests look good for Folsom.
Stuart McLaren (stuart-mclaren) wrote : | #16 |
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) wrote : | #17 |
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) wrote : | #18 |
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) wrote : | #19 |
Hi Thierry, which stable/diablo code path is vunerable?
I think this bit is ok:
def schedule_
"""
Given a uri and a time, schedule the deletion of an image.
"""
use_delay = config.
if not use_delay:
try:
return delete_
except (UnsupportedBac
msg = _("Failed to delete image from store (%(uri)s).") % locals()
registry.
Thierry Carrez (ttx) wrote : | #20 |
@Stuart: I fear that in the latter scenario we'll have executed delete_
Stuart McLaren (stuart-mclaren) wrote : | #21 |
Hmm, the formatting for my post got lost -- sorry.
In "schedule_
Am I reading this wrong/is there another path that takes us to 'delete_
Thanks.
Thierry Carrez (ttx) wrote : | #22 |
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.
no longer affects: | glance/diablo |
Thierry Carrez (ttx) wrote : | #23 |
Anyone up for posting an Essex patch ?
Stuart McLaren (stuart-mclaren) wrote : | #24 |
Thierry, ok thanks. Always good to have a second pair of eyes.
Brian Waldon (bcwaldon) wrote : | #25 |
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) wrote : | #26 |
The attached patch fixes the vulnerability for me
Brian Waldon (bcwaldon) wrote : | #27 |
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) wrote : | #28 |
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) wrote : | #29 |
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) wrote : | #30 |
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) wrote : | #31 |
+1
Thierry Carrez (ttx) wrote : | #32 |
Another glance-core review needed on Brian's Essex patch !
Stuart McLaren (stuart-mclaren) wrote : | #33 |
+1 For the Essex patch.
Russell Bryant (russellb) wrote : | #34 |
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) wrote : | #35 |
November 7th works for me.
Thierry Carrez (ttx) wrote : | #36 |
Sent to downstream stakeholders
Thierry Carrez (ttx) wrote : | #37 |
CVE-2012-4573
Thierry Carrez (ttx) wrote : | #38 |
Adding canonical-security so that they can coordinate the Ubuntu fix within this bug
Fix proposed to branch: stable/essex
Review: https:/
Fix proposed to branch: stable/folsom
Review: https:/
Changed in glance: | |
assignee: | Mark Washenberger (markwash) → Russell Bryant (russellb) |
Fix proposed to branch: master
Review: https:/
Russell Bryant (russellb) wrote : Re: Non-admin users can cause public glance images to be deleted from the backend storage repository | #42 |
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) wrote : | #43 |
Patches are public now, so opening the bug.
information type: | Private Security → Public Security |
Reviewed: https:/
Committed: http://
Submitter: Jenkins
Branch: master
commit 6ab0992e5472ae3
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: I1a06b7c7421524
Changed in glance: | |
status: | In Progress → Fix Committed |
Reviewed: https:/
Committed: http://
Submitter: Jenkins
Branch: stable/folsom
commit 90bcdc5a89e350a
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: I1a06b7c7421524
Jamie Strandboge (jdstrand) wrote : Re: Non-admin users can cause public glance images to be deleted from the backend storage repository | #46 |
I've been watching the Folsom patch iterations and have applied 90bcdc5a89e350a
Russell Bryant (russellb) wrote : | #47 |
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) wrote : | #48 |
advisory for reference: https:/
Thierry Carrez (ttx) wrote : | #49 |
@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) wrote : | #50 |
@Thierry: no, I was using that final commit. After some investigation I discovered I also needed to patch the testsuite with 7841cc93050fa48
Reviewed: https:/
Committed: http://
Submitter: Jenkins
Branch: stable/essex
commit efd7e75b1f419a5
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: Icf2f117a094c71
Thierry Carrez (ttx) wrote : Re: Non-admin users can cause public glance images to be deleted from the backend storage repository | #52 |
Errata published at: https:/
Changed in glance: | |
status: | Fix Committed → Fix Released |
Changed in glance (Ubuntu): | |
status: | New → Fix Released |
Changed in glance (Ubuntu Quantal): | |
status: | New → Confirmed |
Jamie Strandboge (jdstrand) wrote : | #53 |
This was fixed in 12.10 in http://
Changed in glance (Ubuntu Quantal): | |
status: | Confirmed → Fix Released |
Hello Gabe, or anyone else affected,
Accepted glance into quantal-proposed. The package will build now and be available at http://
Please help us by testing this new package. See https:/
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-
Further information regarding the verification process can be found at https:/
tags: | added: verification-needed |
no longer affects: | glance/grizzly |
Brian Murray (brian-murray) wrote : | #55 |
Hello Gabe, or anyone else affected,
Accepted glance into precise-proposed. The package will build now and be available at http://
Please help us by testing this new package. See https:/
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-
Further information regarding the verification process can be found at https:/
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:/
Stable review: https:/
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) wrote : Re: Non-admin users can cause public glance images to be deleted from the backend storage repository | #57 |
Test coverage log.
tags: |
added: verification-done removed: verification-needed |
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.
summary: |
- Non-admin users can cause public glance images to be deleted from the - backend storage repository + [OSSA-2012-017] Non-admin users can cause public glance images to be + deleted from the backend storage repository |
Changed in ossa: | |
assignee: | nobody → Russell Bryant (russellb) |
status: | New → Fix Released |
summary: |
[OSSA-2012-017] Non-admin users can cause public glance images to be - deleted from the backend storage repository + deleted |
This bug would affect any image that a user can view but isn't allowed to delete, including shared images.