[OSSA 2016-006] Normal user can change image status if show_multiple_locations has been set to true (CVE-2016-0757)

Since this report concerns a possible security risk, an incomplete security advisory task has been added while the core security reviewers for the affected project or projects confirm the bug and discuss the scope of any vulnerability along with potential solutions.

I can confirm this bug and the security risks related to this. A non-admin user can modify public images and inject locations pointing to dangerous images.

We have 2 ways to "fix" this bug:

1) Recommend people to not use this config option on public endpoints until we refactor the import process and remove/improve this functionality

2) Actually fix this bug and backport patches accordingly.

My preference is #2 and we have 3 things to do here, IMHO:

1) Whenever an image is left without locations, it should be moved to 'deleted' as it'd be left without data and that's like deleting the image.

2) Forbid location changes for non-owners. I shouldn't be able to modify locations for an image I don't own as that allows for things like the one reported in this bug. Even if #1 is done, we'll still need this because I user could add a second location and then delete the old one to walk around #1.

3) Change the default policy for `*_image_location` to be admin only.

I'd like to get opinions from folks with access to this bug before we move forward with the fix.

Note that (like another recent bug) this behaviour is publically documented.

From:

http://developer.openstack.org/api-ref-image-v2.html

"After you remove all locations from the image and with correct permissions, you can use API calls to view the image status as queued."

Given this behavior is described in published documentation, is there any reason to keep the bug private?

> Given this behavior is described in published documentation, is there any reason to keep the bug private?

I think some of this behaviour, eg:
 A non-admin user can modify public images and inject locations pointing to dangerous images.

Is not publically documented. I'd hold off on marking public until we get a chance to digest the various possible behaviours.

> 1) Whenever an image is left without locations, it should be moved to 'deleted'

What about forbidding removal of the last location? It may be a bit surprising if your image disappears without doing an explicit image delete.

(If your image has only one location and you want to remove that location then delete the image.)

It looks like the checksum didn't change after the upload of the new bytes:

 | checksum | eb9139e4942121f22bbc2afc0400b2a4 |

I'm presuming this is just because the same image was used and if the bytes were different the new checksum/size will be registered.

> What about forbidding removal of the last location? It may be a bit surprising if your image disappears without doing an explicit image delete.

TBH, this was my other option but I found this behavior surprising as well but, perhaps, it'd be easier for users to understand and it'd prevent users from shooting themselves on the foot by, erroneously, removing the last location.

> It looks like the checksum didn't change after the upload of the new bytes:

You, sir, have a sharp eye. I'd assume the same.

As Stuart mentioned, the risks related to this behavior are not documented and I'd prefer to keep to keep this as a private bug until we have a better solution for it.

Has anyone arrived at a better solution, or is it time to bring this to a wider (public) audience so more developers have an opportunity to come up with something, and so operators can at least be aware of the risks here?

Sorry for getting back to this so late.

As it stands right now, I'd probably go with Stuart's proposal and prevent the deletion of the last image location. We'll work on a patch for this. Stuart, any chance you can take this on? Otherwise, I'll find someone (or do it myself, I guess)

@Erno

Any chance you could take this on?

I should have the fix for this. Will need to do some testing still and finish writing the tests before I upload the patch to the bug.

This bug can be also triggered by replacing the locations with empty list.

Patch for master

Patch for stable/liberty

Patch for stable/kilo

Master patch looks good to me. Stuart, any chance you can give this a look asap?

How about getting rid of [1], which is really the root-cause?

[1] https://github.com/openstack/glance/blob/master/glance/api/v2/images.py#L271-L272

And, this https://github.com/openstack/glance/blob/master/glance/api/v2/images.py#L313-L314 ?

Hemanth,

first one was good catch, didn't realize that we do it in that many places. If you look the patches uploaded the second one was removed in them already.

I tested Erno's patch and it does fixes the issue, AFAICT. I think it'd be ok to move with this one and hold off a proper refactor once the CVE is fixed.

Hermanth mentioned on IRC that this could be introducing a backwards incompatible change. I'm personally not super worried about this as it is a *bug* and an ugly one. Don't think there's a good way to fix this in a backwards compatible way.

Thoughts?

It indeed is backwards incompatible. On the same time this is one of those things that has been fundamentally wrong and only way to plug that security hole is to break that wrong behavior.

+1 to postponing the refactor.

A couple of question while drafting the impact description:

* In the bug description, the non admin user is able to remove location after the image is set to public, but can the user does that on images the user didn't upload ?
* The backward incompatible part is about user being able to previously remove the last location (or set an empty list of locations) ? Would this prevent a working workflow some user might have build around that feature ?

Backward incompatible changes are usually not handled by an OSSA tasks but rather triage as a class B type of bug (according to VMT taxonomy: https://security.openstack.org/vmt-process.html#incident-report-taxonomy ).

Trying to remove location of an image not owned by the user results in
500 Internal Server Error: You are not permitted to modify locations for this image. (HTTP 500)

And considering an active image without location already break the "contract", then this is less of a backward incompatible change and more of a real fix. Though I added a "note" about that behavior change to warn about regression after the patch is applied.

Here is the impact description draft #1:

Title: Glance image status manipulation through locations
Reporter: Erno Kuvaja (HPE)
Products: Glance
Affects: <=2015.1.2, >=11.0.0 <= 11.0.1

Description:
Erno Kuvaja from HPE reported a vulnerability in Glance. By removing the last location of an image, an authenticated user can change the image status to queue. This breaks the immutability promise glance and has similar way as described in OSSA 2015-019 as the image gets transitioned from active to queued and new image data can be uploaded. Only setup with show_multiple_locations set to true (not default) are affected.

Pre-OSSA Note:
The proposed fix prevents to remove the last location of an image so that an active image is always available. This action was previously incorrectly allowed.

A few typo fixed:

Title: Glance image status manipulation through locations
Reporter: Erno Kuvaja (HPE)
Products: Glance
Affects: <=2015.1.2, >=11.0.0 <= 11.0.1

Description:
Erno Kuvaja from HPE reported a vulnerability in Glance. By removing the last location of an image, an authenticated user can change the image status to queue. This breaks the immutability promise as described in OSSA 2015-019 as the image gets transitioned from active to queued and new image data can be uploaded. Only setup with show_multiple_locations set to true (not default) are affected.

Pre-OSSA Note:
The proposed fix prevents the removal of the last location of an image so that an active image is always available. This action was previously incorrectly allowed.

Hi Tristan,

Re #23:
* User can only perform these actions on the image it owns. (Regardless the original creator and visibility settings.)
* Yes the backwards incompatible part is that our API has previously allowed removing all locations from image (which has been documented feature) but this transition back to queued opens the vulnerability on the process. This fix might break some users who are relying the false assumption that it would be ok to replace the data of existing image in the special case that the multiple locations has been configured.

Thank Erno for the details. Since this only breaks use-cases that actually abuse glance immutability I assume this is really a fix that stable branch needs.

Here is another impact description draft, hopefully more clear and simple:

Title: Glance image status manipulation through locations removal
Reporter: Erno Kuvaja (HPE)
Products: Glance
Affects: <=2015.1.2, >=11.0.0 <= 11.0.1

Description:
Erno Kuvaja from HPE reported a vulnerability in Glance. By removing the last location of an image, an authenticated user may change the image status back to queued and may be able to upload new image data resulting in a broken Glance's immutability promise. A malicious tenant may exploit this flaw to silently replace image data it owns, regardless of the original creator or the visibility settings. Only setups with show_multiple_locations enabled (not default) are affected.

Pre-OSSA Note:
The proposed fix prevents the removal of the last location of an image so that an active image is always available. This action was previously incorrectly allowed and the fix might break some users who are relying on the false assumption that it would be ok to replace the data of existing image in the special case that the multiple locations has been configured.

Thanks Tristan,

That looks good to me at least.

Erno, since you mentioned in comment #26 that this was a documented feature, can you also indicate where the corresponding documentation is which states that and provide some rough approximation of how you would correct it after these patches merge? That too might help make this behavior change's significance or insignificance more obvious.

As for Tristan's impact description and behavior change note in comment #27, these seem fine.

Jeremy,

Here at least:
http://developer.openstack.org/api-ref-image-v2.html#updateImage-v2

Proposed disclosure date:
2016-02-03, 1500 UTC

Data sounds good to me! I'd gratefully appreciate a ping on IRC as a reminder (Erno and I can take care of the rest)

Related fix proposed to branch: master
Review: https://review.openstack.org/275739

Reviewed: https://review.openstack.org/275736
Committed: https://git.openstack.org/cgit/openstack/glance/commit/?id=6deee085e8f8045633745ba1aabc1779ac5b2a89
Submitter: Jenkins
Branch: stable/liberty

commit 6deee085e8f8045633745ba1aabc1779ac5b2a89
Author: Erno Kuvaja <email address hidden>
Date: Tue Jan 19 13:37:05 2016 +0000

    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

    Change-Id: Ieb03aaba887492819f9c58aa67f7acfcea81720e
    Closes-Bug: #1525915
    (cherry picked from commit e9e45baa9aaf58e69964419b6b4fb2048d115a0c)

I can't reproduce:
http://paste.openstack.org/show/485919/

I noticed that you used "../openrc" to change to non-admin.
but actually it did nothing. As you used "../openrc admin admin" first, it was still admin role.

and I found the code:
https://github.com/openstack/glance/blob/master/glance/api/authorization.py#L250

is it really a security bug?

read again, Sorry for my comment before.

Wangxiyuan, this only happen for images already owned by the user, and when the setup has show_multiple_locations enabled (not default).

Reviewed: https://review.openstack.org/275739
Committed: https://git.openstack.org/cgit/openstack/ossa/commit/?id=2666088c83658eb802e732eee9dd718b373cbf9a
Submitter: Jenkins
Branch: master

commit 2666088c83658eb802e732eee9dd718b373cbf9a
Author: Tristan Cacqueray <email address hidden>
Date: Wed Feb 3 10:00:06 2016 -0500

    Add OSSA 2016-006 (CVE-2016-0757)

    Change-Id: I7fce1a3f54310bc8afa0a39d0f06ce6a4c2cba6c
    Related-Bug: #1525915

Reviewed: https://review.openstack.org/275737
Committed: https://git.openstack.org/cgit/openstack/glance/commit/?id=6179e1e98808548f1c12a2b66784cac3c1e5ac0f
Submitter: Jenkins
Branch: master

commit 6179e1e98808548f1c12a2b66784cac3c1e5ac0f
Author: Erno Kuvaja <email address hidden>
Date: Tue Jan 19 13:37:05 2016 +0000

    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

    Change-Id: Ieb03aaba887492819f9c58aa67f7acfcea81720e
    Closes-Bug: #1525915

Reviewed: https://review.openstack.org/275735
Committed: https://git.openstack.org/cgit/openstack/glance/commit/?id=c5c731c7153d6d46c27260474d2811d504dfac5c
Submitter: Jenkins
Branch: stable/kilo

commit c5c731c7153d6d46c27260474d2811d504dfac5c
Author: Erno Kuvaja <email address hidden>
Date: Tue Jan 19 13:37:05 2016 +0000

    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)

This issue was fixed in the openstack/glance 12.0.0.0b3 development milestone.

This issue was fixed in the openstack/glance 2015.1.4 release.

This issue was fixed in the openstack/glance 2015.1.4 release.

This issue was fixed in the openstack/glance 11.0.2 release.