context.elevated: copy.copy causes admin role leak

[Amir Sadoughi]

In neutron/context.py,

```
        context = copy.copy(self)
        context.is_admin = True

        if 'admin' not in [x.lower() for x in context.roles]:
            context.roles.append('admin')
```

copy.copy should be replaced by copy.deepcopy such that the list reference is not shared between objects. From my cursory search on github this also affects cinder, gantt, nova, neutron, and manila.

[Amir Sadoughi (amir-sadoughi)]

I wonder if there are features that rely on this bug since it's been in the code for a while.

[Tristan Cacqueray (tristan-cacqueray)]

Thanks for the report, the OSSA task is set to incomplete pending additional details from project cores.

Are the effect of this bug noticable using the CLI or something ? I wonder when does a original context get used after elevation

[Salvatore Orlando (salvatore-orlando)]

This was already privately reported by a community member to me (not the reporter).
I suggested filing a bug, and keeping it private until we analysed it. I'm not sure if this submission is related to that conversation.

The potential security issue here is the following:

1) operation starts in admin context (ctx)
2) operation elevates the context (ctx_elevated = ctx.elevated)
3) since the copy is shallow ctx_elevated.roles == ctx.roles -> the original context has now the admin role too!

when I was notified by it I have already looked if there was any place in the code where malicious users could exploit this bug to gain admin privileges on tenant operations, but I could not find any.

Also, a context lifespan is a single API operation, so this does not give leeway to attackers to persistently gain admin credentials.

I think this a bug which can potentially open up security issues if not fixed, but is not a security issue in itself.

[Thierry Carrez (ttx)]

Feels like this could be fixed publicly -- but maybe we should check with cinder, gantt, nova, neutron, and manila first. Adding coresec teams from there.

[Thierry Carrez (ttx)]

cinder, gantt, nova, neutron, and manila:
Please check that the current shallow copy, while annoying and opening up possibilities, is probably not exploitable right now and could be fixed in public.

[Andrew Laski (alaski)]

This fact on its own is not exploitable by a user within Nova. Salvatores analysis holds there as well.

I have not done exhaustive testing against each call that uses elevation at some point to say that there's no security consequence with this. But if there was an unintended elevation of privileges that exposed extra information or allowed undesired actions it is not made more exploitable with this knowledge. There is no user control over where/when this elevation happens.

So my opinion is that this could be addressed publicly but if specific instances of unintended elevations are found those could be addressed privately while this is being fixed.

[Jay Bryant (jsbryant)]

From a Cinder perspective I think that it is highly unlikely that this would be exploited or has a vulnerability. Given that it is a simple one line fix I think we could just put up the change without referencing this bug or explaining that there is a security issue and get it through. Don't have to wave a red flag about it. Just get it fixed.

[Thierry Carrez (ttx)]

This feels like although it's definitely a bug, this is currently not exploitable. I propose we make this bug public and issue fixes ASAP (we can even backport them), but no OSSA (class D).

Will open on Thursday unless someone complains.

[Jay Bryant (jsbryant)]

I have a patch for Cinder that I can push as soon as this is opened up.

[Thierry Carrez (ttx)]

Class D

[OpenStack Infra (hudson-openstack)]

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

[OpenStack Infra (hudson-openstack)]

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

[OpenStack Infra (hudson-openstack)]

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

[OpenStack Infra (hudson-openstack)]

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

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.openstack.org/136061
Committed: https://git.openstack.org/cgit/openstack/cinder/commit/?id=285cfaf0954d4c3e320b205c288240c1828476fe
Submitter: Jenkins
Branch: master

commit 285cfaf0954d4c3e320b205c288240c1828476fe
Author: Jay S. Bryant <email address hidden>
Date: Thu Nov 20 11:06:48 2014 -0600

    context.elevated() should use copy.deepcopy()

    Currently context.elevated is just doing a copy.copy(self).
    This needs to be changed to use copy.deepcopy so that the
    list reference is not shared between objects which leaves
    the possibility of an admin role leak.

    This fix changes context.elevated use copy.deepcopy.

    Change-Id: I349c53ccbe9e02ad2a3e84ae897424db9785a170
    Closes-bug: 1386932

[OpenStack Infra (hudson-openstack)]

Fix proposed to branch: stable/juno
Review: https://review.openstack.org/136391

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.openstack.org/136102
Committed: https://git.openstack.org/cgit/openstack/manila/commit/?id=d37290ce76f04013964f31d719d32b0f46c0b997
Submitter: Jenkins
Branch: master

commit d37290ce76f04013964f31d719d32b0f46c0b997
Author: Your Name <email address hidden>
Date: Thu Nov 20 21:01:19 2014 +0200

    Fix context.elevated

    Replace copy.copy() with copy.deepcopy() in 'elevated' method of RequestContext
    class to remove addition of admin role to original context that can be used by
    malicious users.

    Change-Id: Ie28acd9c6c9c75ab00f440b49996a1de7523158b
    Closes-bug: #1386932

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.openstack.org/136035
Committed: https://git.openstack.org/cgit/openstack/neutron/commit/?id=98fae47ad1b9b72e41d444ce6f96cf5f2a3b6f0c
Submitter: Jenkins
Branch: master

commit 98fae47ad1b9b72e41d444ce6f96cf5f2a3b6f0c
Author: Ann Kamyshnikova <email address hidden>
Date: Thu Nov 20 19:13:52 2014 +0300

    Fix context.elevated

    The current version of elevated method sets for the original context
    the admin role too. This change fix this.

    Added unittest.

    Closes-bug: #1386932

    Change-Id: Ife881112efa151e53bfa4b7af35643dcf2d1114f

[OpenStack Infra (hudson-openstack)]

Fix proposed to branch: stable/juno
Review: https://review.openstack.org/136956

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.openstack.org/136266
Committed: https://git.openstack.org/cgit/openstack/nova/commit/?id=06e2319806c618898071eba662d5bf9773be4d39
Submitter: Jenkins
Branch: master

commit 06e2319806c618898071eba662d5bf9773be4d39
Author: Matthew Gilliard <email address hidden>
Date: Fri Nov 21 08:55:56 2014 +0000

    Prevent admin role leak in context.elevated

    context.elevated was creating a copy of the current context then adding
    'admin' to the roles of that context. This should be a deepcopy, otherwise
    'admin' is added to the original context too.

    Change-Id: I8ab00c88a8e76a14fb9f4ae96dfdb5f018fc2d0f
    Closes-bug: 1386932

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.openstack.org/136956
Committed: https://git.openstack.org/cgit/openstack/neutron/commit/?id=d09ba29a5f16ad26fb01abfc9791c1ef7a845bc7
Submitter: Jenkins
Branch: stable/juno

commit d09ba29a5f16ad26fb01abfc9791c1ef7a845bc7
Author: Ann Kamyshnikova <email address hidden>
Date: Thu Nov 20 19:13:52 2014 +0300

    Fix context.elevated

    The current version of elevated method sets for the original context
    the admin role too. This change fix this.

    Added unittest.

    Closes-bug: #1386932

    Change-Id: Ife881112efa151e53bfa4b7af35643dcf2d1114f
    (cherry picked from commit 98fae47ad1b9b72e41d444ce6f96cf5f2a3b6f0c)

[OpenStack Infra (hudson-openstack)]

Change abandoned by Jay Bryant (<email address hidden>) on branch: stable/juno
Review: https://review.openstack.org/136391
Reason: This change is old and no longer appropriate for Juno.

[Alberto Murillo (powerbsd-o)]

Hi.

This change did cause an issue in nova-api when runing under uwsgi+nginx or apache+mod_wsgi.

https://bugs.launchpad.net/nova/+bug/1506958

Reverting the change does not sound like a good option.
any comments on the best way to fix it are more than welcome :)

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.openstack.org/260615
Committed: https://git.openstack.org/cgit/openstack/nova/commit/?id=82457f2462621b6a9c653dce2baf38d0623e25ee
Submitter: Jenkins
Branch: master

commit 82457f2462621b6a9c653dce2baf38d0623e25ee
Author: Marian Horban <email address hidden>
Date: Mon Dec 7 07:30:11 2015 -0500

    Replace copy.deepcopy of RequestContext with copy.copy

    Instance of RequestContext contains many objects and some of them like
    mutexes could not be copied. Also a deepcopy of the entire
    RequestContext wastes CPU time.

    To avoid problems with deepcopy and avoid performance overhead this
    patch changes deepcopy of RequestContext to shallow copy and makes
    deepcopy of only the 'roles' member because of security issue
    LP #1386932.

    Closes-Bug: #1506958
    Related-Bug: #1386932
    Change-Id: I1e2c00e95e1c4bcd0ec7bf075458789d6fb06e99

[OpenStack Infra (hudson-openstack)]

Related fix proposed to branch: stable/liberty
Review: https://review.openstack.org/288529

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.openstack.org/288529
Committed: https://git.openstack.org/cgit/openstack/nova/commit/?id=3c2d75d60a1d11726905a9a3f8eb745b4e4ad4cd
Submitter: Jenkins
Branch: stable/liberty

commit 3c2d75d60a1d11726905a9a3f8eb745b4e4ad4cd
Author: Marian Horban <email address hidden>
Date: Mon Dec 7 07:30:11 2015 -0500

    Replace copy.deepcopy of RequestContext with copy.copy

    Instance of RequestContext contains many objects and some of them like
    mutexes could not be copied. Also a deepcopy of the entire
    RequestContext wastes CPU time.

    To avoid problems with deepcopy and avoid performance overhead this
    patch changes deepcopy of RequestContext to shallow copy and makes
    deepcopy of only the 'roles' member because of security issue
    LP #1386932.

    Closes-Bug: #1506958
    Related-Bug: #1386932
    Change-Id: I1e2c00e95e1c4bcd0ec7bf075458789d6fb06e99
    (cherry picked from commit 82457f2462621b6a9c653dce2baf38d0623e25ee)