Open Stack

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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 604 bytes
Desc: OpenPGP digital signature
URL: <http://lists.openstack.org/pipermail/openstack-dev/attachments/20150515/1cbea891/attachment.pgp>