Re: [openstack-dev] [nova] Unvalidated user input passed to functions
On 15 May 2015 at 14:13, Daniel P. Berrange 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 __ OpenStack Development Mailing List (not for usage questions) Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
Re: [openstack-dev] [nova] Unvalidated user input passed to functions
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. > > 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. > 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. > Does python have anything like perl's taint mode? If so, it might be > worth investigating its use. I don't believe so. Regards, Daniel -- |: http://berrange.com -o-http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :| __ OpenStack Development Mailing List (not for usage questions) Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
Re: [openstack-dev] [nova] Unvalidated user input passed to functions
On 05/15/2015 05:41 AM, 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. > > 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? > > 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. Libvirt validates that the base and top arguments to blockcommit make sense (in part because it may have to rewrite the string passed in to a different but equivalent file name for qemu to do the right thing, since qemu does strcmp rather than inode matching). Qemu also has the ability to set an arbitrary backing file string into the metadata; if this arbitrary string is under user control, then it is up to the user to validate that the string is correct to avoid breaking the chain (and doing something nasty like setting /etc/passwd as the new backing file the next time the chain is parsed from qcow2 files). But I don't think libvirt exposes the arbitrary backing name to the user, but rather computes a relative backing string itself, so that also doesn't seem to be a problem. And yes, you are protected by requiring admin context - anyone that can cause libvirt to start a new domain and write arbitrary XML already has effective root permissions on the host, because they can design the XML to hand any file of their choosing to the guest. Security is only at risk when there is elevation - if a guest could do things to cause the host to hand away privileged files, rather than only the host changing XML or backing file strings, is when we have to start worrying. The host changing strings is not elevation, just the user shooting themselves in the foot. But you are also right that it might be nice to validate strings prior to handing them to libvirt - while libvirt is able to validate that strings make sense within the chains that libvirt is aware of, it cannot know if there are additional restrictions that should be in place at the upper level (such as whether a user is entitled to access the storage locations referenced in the strings, according to nova rules). -- Eric Blake eblake redhat com+1-919-301-3266 Libvirt virtualization library http://libvirt.org signature.asc Description: OpenPGP digital signature __ OpenStack Development Mailing List (not for usage questions) Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
[openstack-dev] [nova] Unvalidated user input passed to functions
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. 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? 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. Does python have anything like perl's taint mode? If so, it might be worth investigating its use. Matt -- Matthew Booth Red Hat Engineering, Virtualisation Team Phone: +442070094448 (UK) GPG ID: D33C3490 GPG FPR: 3733 612D 2D05 5458 8A8A 1600 3441 EA19 D33C 3490 __ OpenStack Development Mailing List (not for usage questions) Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
