Admin rights shouldn't work cross-tenant

[Alexej Ababilov]

Consider user A is an `admin` in Y tenant and has no roles in Z tenant. Let G image be an image from Z tenant.

User A gets a token for Y tenant and sends a request: "delete G image". Currently, glance allows such malicious action.

That's because glance/common/context.py:RequestContext allows `admin` to mutate any image despite its owner. The A user will be considered as an `admin` by ContextMiddleware since Keystone reports its `admin` role for Y tenant because the token belongs to it.

The problem exists in this branch:
commit fc758a46e77de1746c796cd64228694521ee2b07
Author: Dan Prince <email address hidden>
Date: Thu Jun 7 22:23:48 2012 -0400

The same security hole is found in Keystone: an `admin` user is allowed to do anything, e.g. remove users and their roles for any tenant.

[Thierry Carrez (ttx)]

Adding PTL for input. I /think/ this is by design though... admins are not scoped to tenants ?

[Brian Waldon (bcwaldon)]

Yeah, this should be fixed. We're treating roles as being globally scoped, when they're actually scoped to a specific tenant like you describe.

[Thierry Carrez (ttx)]

@Joe: Your take on the keystone side of the vulnerability ?

[Joseph Heck (heckj)]

While I agree this should be fixed, it's not a security bug but how the initial version of authorization was implemented.

In the Diablo and Essex releases of OpenStack, Admin was effectively global and not per-tenant or per-service. That's the entire reason of adding in domains to Keystone, and behind the idea of unifying the role names (which are installation-global) to match up with local service policy.json files. (i.e. move to "nova-admin", "glance-admin", etc instead of a single "admin")

If you want a role that's a global admin, you can still use "admin" and create associated policy.json files that respect that identifier.

[Joseph Heck (heckj)]

(tl;dr - to be resolved with Keystone V3 API implementation)

[Thierry Carrez (ttx)]

Brian, Joe: so you both agree this is not a vulnerability, but by (admittedly weak) design ? And that it should definitely be strengthened in future revisions of the API ?

If yes, I'd suggest that we open this bug as a known and wanted security improvement, rather than keep it embargoed as an exploitable vulnerability.

Alessio: would that work for you ?

[Alexej Ababilov (aababilov)]

Sorry for the delay!
Would it be acceptable if admin can access any image in owner_is_tenant mode, otherwise image is modifiable only by users of its tenant.

Here is my patch (https://github.com/aababilov/glance/blob/07c7988d1f8c08aebed789a8b9b5d875cef2fb46/glance/db/simple/api.py), however I should fix unit tests as well:

def is_image_mutable(context, image):
    """Return True if the image is mutable in this context."""
    # Is admin and owner is user == image mutable
    if context.is_admin and not context.owner_is_tenant:
        return True

    # No owner == image not mutable
    if image['owner'] is None or context.owner is None:
        return False

    # Image only mutable by its owner
    return image['owner'] == context.owner

(and so on for another functions)

[Thierry Carrez (ttx)]

@Brian, Joe: please answer to comment 6 and 7

[Joseph Heck (heckj)]

I'm good for opening it up - it's how the roles are implemented and supported by the individual services. The keystone project has a blueprint that Liem (from HP) is working on now to gather up a recommended set of policy.json files and related roles to provide a layout with explicit per-service administration functions. (https://blueprints.launchpad.net/keystone/+spec/document-deployment-suggestions-policy)

For Alessio, I'd recommend making that a rule in policy.json rather than in the code itself, as that makes it configurable by policy rather than hard coded, but I'll defer to whatever Brian suggests here.

[Brian Waldon (bcwaldon)]

I agree with Joe here, this is a future improvement, not a bug. However, we do need to be clear with that the implications of assigning a user an admin-like role entails.

As for future work, we need to separate service-level admin vs tenant-level admin. Right now, we create something that looks like a tenant-level admin, when it has the ability to act as if it were service-level.