Open Stack

On 15 May 2015 at 14:13, Daniel P. Berrange <berrange at redhat.com> wrote:
> On Fri, May 15, 2015 at 12:41:20PM +0100, Matthew Booth wrote:
>> I was looking at the migrations api, and I noticed that the api passes
>> the request query unchecked to get_migrations, where it ultimately ends
>> up in a db query. I was curious and spent a couple of hours checking
>> this morning. There are a few instances of this.

In general, I want to encourage the use of private security bugs to
start discussions on these kinds of topics:
https://security.openstack.org/vmt-process.html#reception

>> I didn't find any security bugs, however I feel that this extremely bad
>> practise, and is likely to result in a security bug eventually. For
>> example, note that os-assisted-volume-snapshots:delete does not validate
>> delete_info before passing it to volume_snapshot_delete. I looked at
>> this quite carefully, and I think we are only protected from a host
>> compromise because:
>>
>> 1. The api requires admin context
>> 2. libvirt's security policy
>>
>> I could be wrong on that, though, so perhaps somebody else could check?
>
> Item 1 is pretty much the "protection" here. In general this is a problem
> with the design of os-assisted-volume-snapshots:delete API - the very
> fact that it is intended to allow arbitrary file paths to be specified
> by the user makes it effectively impossible to validate - any path has
> to be considered valid :-( This means it should never be allowed for
> anyone except trusted cloud admin.
>
> The majority of our APIs though are better designed and do not allow the
> API user to supply file paths and similarly sensitive parameters that
> refer to host resources. Usually the user only provides unique identifiers
> (UUIDs) and high level requirements (ie MAC addresses, disk sizes) and
> not file paths or similar.

The v2.1 API introduces the concept of strong validation for all API calls:
http://specs.openstack.org/openstack/nova-specs/specs/kilo/implemented/v2-on-v3-api.html#rest-api-impact

Basically, I except v2.1 to white list all input, eventually.

But seems like only create has had the proper validation added at this point:
https://github.com/openstack/nova/blob/master/nova/api/openstack/compute/plugins/v3/assisted_volume_snapshots.py#L46
https://github.com/openstack/nova/blob/master/nova/api/openstack/compute/schemas/v3/assisted_volume_snapshots.py#L15

Adding the extra validation is most likely to involve a new
micro-version (and thus needs a spec), but thats still TBD.

>> Passing unvalidated input to a function isn't necessarily bad, for
>> example if it is only used for filtering, but it should be clearly
>> marked as such so it isn't used in an unsafe manner. This marking should
>> follow the data as far as it goes through any number of function calls.
>> libvirt's _volume_snapshot_delete function is a long way from the
>> originating api call, and it is not at all obvious that the commit_base
>> and commit_top arguments to virt_dom.blockCommit() are unvalidated.
>
> I think the most important thing is really not to design more APIs like
> os-assisted-volume-snapshots which are inherantly dangerous due to the
> parameters they are design to allow :-( For those few we do have, we
> should definitely vet it as carefully as possible.

Is this not the API thats meant to be only be called by Cinder?
https://blueprints.launchpad.net/nova/+spec/qemu-assisted-snapshots
http://specs.openstack.org/openstack/nova-specs/specs/juno/implemented/libvirt-volume-snap-network-disk.html

If so, maybe the default policy should change to reflect that?
Restrict it to "cinder", and not "admins"? This being more important
in v2.1 where you can't disable any extensions (eventually).

Is there a way we can evolve that to API be a safer interface? Is
anyone wiling to implement that?

Thanks,
John