On Tue, Jan 28, 2014 at 05:14:40PM -0700, Eric Blake wrote: > Commit f9f56340 for CVE-2014-0028 almost had the right idea - we > need to check the ACL rules to filter which events to send. But > it overlooked one thing: the event dispatch queue is running in > the main loop thread, and therefore does not normally have a > current virIdentityPtr. But filter checks can be based on current > identity, so when libvirtd.conf contains access_drivers=["polkit"], > we ended up rejecting access for EVERY event due to failure to > look up the current identity, even if it should have been allowed. > > Furthermore, even for events that are triggered by API calls, it > is important to remember that the point of events is that they can > be copied across multiple connections, which may have separate > identities and permissions. So even if events were dispatched > from a context where we have an identity, we must change to the > correct identity of the connection that will be receiving the > event, rather than basing a decision on the context that triggered > the event, when deciding whether to filter an event to a > particular connection. > > If there were an easy way to get from virConnectPtr to the > appropriate virIdentityPtr, then object_event.c could adjust the > identity prior to checking whether to dispatch an event. But > setting up that back-reference is a bit invasive. Instead, it > is easier to delay the filtering check until lower down the > stack, at the point where we have direct access to the RPC > client object that owns an identity. As such, this patch ends > up reverting a large portion of the framework of commit f9f56340. > We also have to teach 'make check' to special-case the fact that > the event registration filtering is done at the point of dispatch, > rather than the point of registration. Note that even though we > don't actually use virConnectDomainEventRegisterCheckACL (because > the RegisterAny variant is sufficient), we still generate the > function for the purposes of documenting that the filtering > takes place. > > Also note that I did not entirely delete the notion of a filter > from object_event.c; I still plan on using that for my upcoming > patch series for qemu monitor events in libvirt-qemu.so. In > other words, while this patch changes ACL filtering to live in > remote.c and therefore we have no current client of the filtering > in object_event.c, the notion of filtering in object_event.c is > still useful down the road. > > * src/check-aclrules.pl: Exempt event registration from having to > pass checkACL filter down call stack. > * daemon/remote.c (remoteRelayDomainEventCheckACL) > (remoteRelayNetworkEventCheckACL): New functions. > (remoteRelay*Event*): Use new functions. > * src/conf/domain_event.h (virDomainEventStateRegister) > (virDomainEventStateRegisterID): Drop unused parameter. > * src/conf/network_event.h (virNetworkEventStateRegisterID): > Likewise. > * src/conf/domain_event.c (virDomainEventFilter): Delete unused > function. > * src/conf/network_event.c (virNetworkEventFilter): Likewise. > * src/libxl/libxl_driver.c: Adjust caller. > * src/lxc/lxc_driver.c: Likewise. > * src/network/bridge_driver.c: Likewise. > * src/qemu/qemu_driver.c: Likewise. > * src/remote/remote_driver.c: Likewise. > * src/test/test_driver.c: Likewise. > * src/uml/uml_driver.c: Likewise. > * src/vbox/vbox_tmpl.c: Likewise. > * src/xen/xen_driver.c: Likewise. > > Signed-off-by: Eric Blake <eblake@xxxxxxxxxx> > (cherry picked from commit TBD) > > Conflicts: > daemon/remote.c - not backporting network events > src/conf/network_event.c - likewise > src/conf/network_event.h - likewise > src/network/bridge_driver.c - likewise > src/conf/domain_event.c - revert back to pre-CVE state > src/conf/domain_event.h - likewise > src/libxl/libxl_driver.c - likewise > src/lxc/lxc_driver.c - likewise > src/remote/remote_driver.c - likewise > src/test/test_driver.c - likewise > src/uml/uml_driver.c - likewise > src/xen/xen_driver.c - likewise > --- > > This is the backport to the various stable branches, assuming the > main patch is accepted for the master branch. I'm also going to > send an alternative representation that might make review easier. ACK 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 :| -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list