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: [email protected]?subject:unsubscribe http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
