authority check for create volume API happens too late

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.

That's correct, we never used the decorator because it takes the volume object as the third argument. While we could've written a different decorator, what we used to do instead was call the check_policy method directly after creating the object in the db.

Obviously in hindsight this isn't much better than what's being done now. It shouldn't be too difficult to create a new check_policy wrapper to use specifically for create, or at least a slightly modified version of what we had back in Grizzly that doesn't require the volume ref.

I'll submit a proposal patch here shortly.

diff --git a/cinder/volume/api.py b/cinder/volume/api.py
index 11abff8..6134fd4 100644
--- a/cinder/volume/api.py
+++ b/cinder/volume/api.py
@@ -185,6 +185,7 @@ class API(base.Base):
                 safe = True
         return safe

+
     def create(self, context, size, name, description, snapshot=None,
                image_id=None, volume_type=None, metadata=None,
                availability_zone=None, source_volume=None,
@@ -192,6 +193,8 @@ class API(base.Base):
                source_replica=None, consistencygroup=None,
                cgsnapshot=None, multiattach=False):

+ check_policy(context, 'create_volume')
+
         # NOTE(jdg): we can have a create without size if we're
         # doing a create from snap or volume. Currently
         # the taskflow api will handle this and pull in the

Rather simple it turns out, don't know that we even need to remove the second check in this case. At least not as part of the advisory IMO.

Let me know if this seems acceptable to others.

'create' for the action name instead of 'create_volume'?
   check_policy(context, 'create')
So that we could use the existing rule in policy.json:
   "volume:create": ""

@Anna

Good catch, but actually volume_create --> volume:create in the policy file.

You can test this out pretty easily, just apply the patch, add "rule:admin_api" to the json file and try doing a create as a "normal" user.

Thanks.

What informations are disclosed in error messages ?

@Tristan
 It raises the Policy exception and gives a 403, here's the output when using the client:

ubuntu@fluffy:~$ source devstack/openrc demo demo
ubuntu@fluffy:~$ cinder create 1
ERROR: Policy doesn't allow volume:create to be performed. (HTTP 403) (Request-ID: req-48a3e026-c5f8-48fd-9cc7-dd823ce52e67)
ubuntu@fluffy:~$

If we prefer to mask that I can surely wrap that in a catch and expose a different exception, that will be a shortcoming in other places as well I think.

Is there a reason to consider this leak sensitive and keep this report as a private security bug ?

Seems like it mostly warrants an api hardening patch and it could be fixed as a normal bug.

@Tristan
We are disclosing information during error checking prior to user authorization being checked.

For example:
   https://github.com/openstack/cinder/blob/master/cinder/volume/api.py#L218
might tell user what volume types are supported by a consistency group;
   https://github.com/openstack/cinder/blob/master/cinder/volume/api.py#L230
might disclose information about source volume;
   https://github.com/openstack/cinder/blob/master/cinder/volume/api.py#L247
might disclose information about snapshot types;
   https://github.com/openstack/cinder/blob/master/cinder/volume/api.py#L255
might print some information about timing of availability zone cache updates.

A user could call create volume API to gain information (based on error messages) that otherwise the user would not have access to. Then this is security exposure.
But if this information can be obtained via some other normal means or this information is not exploitable, then this is not a security risk.

Personally I'm mixed on the sensitivity, however it seems fine to error on the side of caution. I'm not sure why the invalid volume-type exception message is really "secret" or security related, but I can understand if some public providers wish to keep that sort of information from leaking.

If you all are ok with the proposed patch it's simple enough to implement and get out the door. If there are secondary concerns regarding the patch in terms of the exception and response those can be addressed as public bugs without much debate IMO.

Oh... I almost forgot, the point WRT to the items Anna pointed out:

I'm not sure how/why you would have a user that can auth but not have the ability to create via policy. It might well be a use case, so they have an admin that is the only one allowed to create volumes, and then those are given out for use by tenants perhaps. Makes sense for things like internal services maybe (DB as a service etc).

That being said, I'm not sure why types would be considered confidential or sensitive info. Consistency groups.. well, that's just ugly leaky abstraction IMHO. The type info and CG info currently are public and available to all tenants so not sure that's a concern. We are working on private types, at which point this might matter, but not sure I see the security impact.

Just my opinions on this; easy to fix either way I think.

I agree with John's last comment: I'm not really seeing anything here sensitive or interesting enough for this to be considered a serious vulnerability instead of a hardening effort.

Thanks for the detailed explanation!

I didn't saw last comments when proposing series tasks, scratch those unless you can demonstrate a scenario where an attacker can gain advantage from this leak.

If not then, then it's class D type of bug report ( https://security.openstack.org/vmt-process.html#incident-report-taxonomy ).

Thank you all for additional information. I agree with your assessments.

@John - could you please give some details on "volume_create --> volume:create in the policy file" in comment #6?

check_policy(context, 'create_volume')

def check_policy(context, action, target_obj=None):
   ...
   _action = 'volume:%s' % action

Doesn't this map to 'volume:create_volume' and not to 'volume:create' that we have in the policy.json? No?

@Anna
So we don't actually have a "volume:create_volume" in the policy file. The check methods parse this out and map it to the rule associated with "volume:create" which is here in the policy file:
  https://github.com/openstack/cinder/blob/master/etc/cinder/policy.json#L8

So my patch works, because the checker will parse it out, but technically it should just probably be:
s/check_policy(context, 'create_volume')/check_policy(context, 'volume')/

Or even better explicitly include the key:
check_policy(context, 'volume:create')

@John... I'm not quite following. It looks to me like we need to call check_policy(context, 'create'), which https://github.com/openstack/cinder/blob/master/cinder/volume/api.py#L112 will then turn into "volume:create"

@Matthew please see my comment in #17.

Yes, what you have is correct, all of these iterations "work" however. Grab me on IRC via PM if you want to discuss more. Also load it up in devstack and you can try it out yourself.

Patch is still completely flexible IMO, just need an answer if we're opening this up and I should just submit it or not.

Thanks,
John

Should this bug go public? Seems like there is an agreement that this bug is not a security risk.

Yes. Opening now.

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

Reviewed: https://review.openstack.org/217899
Committed: https://git.openstack.org/cgit/openstack/cinder/commit/?id=7686777023b6703230850280be895c154ef8f07d
Submitter: Jenkins
Branch: master

commit 7686777023b6703230850280be895c154ef8f07d
Author: Anna Sortland <email address hidden>
Date: Thu Aug 27 15:36:11 2015 -0500

    Earlier authority check for create volume API

    create() API in cinder/volume/api.py does the authority check
    in cinder/volume/flows/api/create_volume.py.
    This creates potential for disclosing information during error checking
    prior to user authorization being checked.
    This fix will do authority check to create() itself, so that
    it is done before proceeding with the rest of code flow.

    Change-Id: I27dbdf5f3ae4e3d681cdbf77df10706721254ffc
    Closes-Bug: #1472031