get_admin_context inadvertently elevates thread

[Samuel Matzek]

Cinder's cinder.context.get_admin_context is inadvertently elevating the thread to use an admin context and essentially discards the thread's user context for the remainder of the request.

This has security implications since any calls done after cinder.context.get_admin_context that obtain and use the thread's current context will be using an admin context instead of the user's context.

This has serviceability implications because every call to get_admin_context will switch the thread's context, which changes the request ID. This makes it very difficult or impossible to use the request ID in log entries to follow a request through a flow.

The root cause is that cinder.context.RequestContext class' __init__ is not passing overwrite=overwrite to the parent class as it should at [1].

I looked at Nova and it does not have this problem.
I looked at Neutron and its get_admin_context flow does not have this problem but the neutron.context.py get_admin_context_without_session method will have this same issue.
Those are the only other projects I checked.
I have not opened a bug against neutron or any other services since I am not sure on the procedures for security issues that hit multiple projects like this one.

Recreation code:
from cinder import context as cinder_context
from oslo_context import context

context.RequestContext()
print "Thread's context at start %s" % context.get_current().to_dict()
admin_cxt = context.get_admin_context()
print "Thread's context after oslo get_admin_context %s" % context.get_current().to_dict()
admin_cxt = cinder_context.get_admin_context()
print "Thread's context after cinder get_admin_context %s" % context.get_current().to_dict()

Produces output:
Thread's context at start {'domain': None, 'project_domain': None, 'auth_token': None, 'resource_uuid': None, 'is_admin': False, 'user': None, 'tenant': None, 'read_only': False, 'show_deleted': False, 'user_identity': '- - - - -', 'request_id': 'req-9e09cfa9-33de-4aee-ae19-caa2613b2fb2', 'user_domain': None}
Thread's context after oslo get_admin_context {'domain': None, 'project_domain': None, 'auth_token': None, 'resource_uuid': None, 'is_admin': False, 'user': None, 'tenant': None, 'read_only': False, 'show_deleted': False, 'user_identity': '- - - - -', 'request_id': 'req-9e09cfa9-33de-4aee-ae19-caa2613b2fb2', 'user_domain': None}
Thread's context after cinder get_admin_context {'domain': None, 'project_name': None, 'project_domain': None, 'timestamp': '2015-10-29T14:26:19.880000', 'auth_token': None, 'remote_address': None, 'quota_class': None, 'resource_uuid': None, 'is_admin': True, 'user': None, 'service_catalog': [], 'tenant': None, 'read_only': False, 'project_id': None, 'user_id': None, 'show_deleted': False, 'roles': ['admin'], 'user_identity': '- - - - -', 'read_deleted': 'no', 'request_id': 'req-585989b8-d431-4352-93f2-f313147fa715', 'user_domain': None}

[1] https://github.com/openstack/cinder/blob/master/cinder/context.py#L73

[Grant Murphy (gmurphy)]

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.

[Samuel Matzek (smatzek)]

The security implications of this may be N/A or minor for Cinder and Neutron. In attempting to determine the scope I looked at the calls to oslo_context.context.get_current(), which I believe is the only way to get context from the thread local.

I did not see any calls to this method in Cinder or Neutron which would lead me to believe neither project is going after the thread local context to use it and are all depending on the context passed through the call chain.

[Matthew Edmonds (edmondsw)]

It looks like Sahara would also be impacted here.

[Kevin Benton (kevinbenton)]

I am going to remove Neutron since we don't use the method to retrieve the context from thread local.

[Duncan Thomas (duncan-thomas)]

This is definitely a bug in cinder, and should be fixed, but it doesn't look like we rely on the context after the elevation, so I don't think there are security issues, just usability. i.e. Even if fixed, cinder would behave the same whether we passed override=True or False.

This is only from a first look though, and I'd like to get somebody else to confirm

[Tristan Cacqueray (tristan-cacqueray)]

Is Sahara still using the context after the elevation ? Or like for Cinder it does not cause a security issue ?

[Michael McCune (mimccune)]

I will look into this a little more, but I don't think that sahara continues to use the context after the request has ended. In most cases, the context is elevated to admin, an action is performed, and then the thread's lifetime is over.

[Tristan Cacqueray (tristan-cacqueray)]

I suspect this is a class D type of bug according to VMT taxonomy ( https://security.openstack.org/vmt-process.html#incident-report-taxonomy ).

If nobody complains, let's remove the ossa task and open this bug by the end of week.

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.openstack.org/247102
Committed: https://git.openstack.org/cgit/openstack/cinder/commit/?id=2cf1f17ee5dc4957efbbc1f1f8f3781d7b9408d5
Submitter: Jenkins
Branch: master

commit 2cf1f17ee5dc4957efbbc1f1f8f3781d7b9408d5
Author: Samuel Matzek <email address hidden>
Date: Wed Nov 18 13:40:42 2015 -0600

    Preserve request id in Cinder logs

    Several Cinder volume drivers make calls to get the admin context.
    When the admin context is retrieved the user context and its request
    ID is lost and all subsequent log entries have different request IDs.

    The fix is to pass the overwrite parameter in Cinder's RequestContext
    __init__ method to the parent oslo class.

    Partial-Bug: #1511406

    Change-Id: I8972b46f15518f22dc9bb340d7c1ba08be1fa2bc

[Sergey Reshetnyak (sreshetniak)]

This bug is > 60 days without activity. We are unsetting assignee and milestone and setting status to Incomplete in order to allow its expiry in 60 days.

If the bug is still valid, then update the bug status.

[Matthew Edmonds (edmondsw)]

This bug still appears to be valid for sahara, with get_admin_context not passing overwrite=False when it creates a new Context instance (default is True).

[Matthew Edmonds (edmondsw)]

Sahara fix proposed here: https://review.openstack.org/#/c/292557/

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.openstack.org/292557
Committed: https://git.openstack.org/cgit/openstack/sahara/commit/?id=eb5a6bcc06a9e7faff4030d278f418264bb0562e
Submitter: Jenkins
Branch: master

commit eb5a6bcc06a9e7faff4030d278f418264bb0562e
Author: Matthew Edmonds <email address hidden>
Date: Mon Mar 14 14:55:07 2016 -0400

    get_admin_context overwriting context

    When the admin context is retrieved, the user context and its request
    ID is lost. All subsequent log entries would have different request IDs
    and any further context checking will be using an admin's context
    instead of the user's context. I'm not aware of any further checking
    today, but better to fix this now and avoid that being an issue in the
    future.

    Change-Id: I1371ddb70736a49f9124f85352d6d195aec97c80
    Closes-Bug: #1511406

[Thierry Carrez (ttx)]

This issue was fixed in the openstack/sahara 4.0.0.0rc1 release candidate.