]> git.ipfire.org Git - thirdparty/libvirt.git/commitdiff
event: filter global events by domain:getattr ACL [CVE-2014-0028]
authorEric Blake <eblake@redhat.com>
Tue, 14 Jan 2014 17:29:34 +0000 (10:29 -0700)
committerEric Blake <eblake@redhat.com>
Wed, 15 Jan 2014 21:56:09 +0000 (14:56 -0700)
Ever since ACL filtering was added in commit 7639736 (v1.1.1), a
user could still use event registration to obtain access to a
domain that they could not normally access via virDomainLookup*
or virConnectListAllDomains and friends.  We already have the
framework in the RPC generator for creating the filter, and
previous cleanup patches got us to the point that we can now
wire the filter through the entire object event stack.

Furthermore, whether or not domain:getattr is honored, use of
global events is a form of obtaining a list of networks, which
is covered by connect:search_domains added in a93cd08 (v1.1.0).
Ideally, we'd have a way to enforce connect:search_domains when
doing global registrations while omitting that check on a
per-domain registration.  But this patch just unconditionally
requires connect:search_domains, even when no list could be
obtained, based on the following observations:
1. Administrators are unlikely to grant domain:getattr for one
or all domains while still denying connect:search_domains - a
user that is able to manage domains will want to be able to
manage them efficiently, but efficient management includes being
able to list the domains they can access.  The idea of denying
connect:search_domains while still granting access to individual
domains is therefore not adding any real security, but just
serves as a layer of obscurity to annoy the end user.
2. In the current implementation, domain events are filtered
on the client; the server has no idea if a domain filter was
requested, and must therefore assume that all domain event
requests are global.  Even if we fix the RPC protocol to
allow for server-side filtering for newer client/server combos,
making the connect:serach_domains ACL check conditional on
whether the domain argument was NULL won't benefit older clients.
Therefore, we choose to document that connect:search_domains
is a pre-requisite to any domain event management.

Network events need the same treatment, with the obvious
change of using connect:search_networks and network:getattr.

* src/access/viraccessperm.h
(VIR_ACCESS_PERM_CONNECT_SEARCH_DOMAINS)
(VIR_ACCESS_PERM_CONNECT_SEARCH_NETWORKS): Document additional
effect of the permission.
* src/conf/domain_event.h (virDomainEventStateRegister)
(virDomainEventStateRegisterID): Add new parameter.
* src/conf/network_event.h (virNetworkEventStateRegisterID):
Likewise.
* src/conf/object_event_private.h (virObjectEventStateRegisterID):
Likewise.
* src/conf/object_event.c (_virObjectEventCallback): Track a filter.
(virObjectEventDispatchMatchCallback): Use filter.
(virObjectEventCallbackListAddID): Register filter.
* src/conf/domain_event.c (virDomainEventFilter): New function.
(virDomainEventStateRegister, virDomainEventStateRegisterID):
Adjust callers.
* src/conf/network_event.c (virNetworkEventFilter): New function.
(virNetworkEventStateRegisterID): Adjust caller.
* src/remote/remote_protocol.x
(REMOTE_PROC_CONNECT_DOMAIN_EVENT_REGISTER)
(REMOTE_PROC_CONNECT_DOMAIN_EVENT_REGISTER_ANY)
(REMOTE_PROC_CONNECT_NETWORK_EVENT_REGISTER_ANY): Generate a
filter, and require connect:search_domains instead of weaker
connect:read.
* src/test/test_driver.c (testConnectDomainEventRegister)
(testConnectDomainEventRegisterAny)
(testConnectNetworkEventRegisterAny): Update callers.
* src/remote/remote_driver.c (remoteConnectDomainEventRegister)
(remoteConnectDomainEventRegisterAny): Likewise.
* src/xen/xen_driver.c (xenUnifiedConnectDomainEventRegister)
(xenUnifiedConnectDomainEventRegisterAny): Likewise.
* src/vbox/vbox_tmpl.c (vboxDomainGetXMLDesc): Likewise.
* src/libxl/libxl_driver.c (libxlConnectDomainEventRegister)
(libxlConnectDomainEventRegisterAny): Likewise.
* src/qemu/qemu_driver.c (qemuConnectDomainEventRegister)
(qemuConnectDomainEventRegisterAny): Likewise.
* src/uml/uml_driver.c (umlConnectDomainEventRegister)
(umlConnectDomainEventRegisterAny): Likewise.
* src/network/bridge_driver.c
(networkConnectNetworkEventRegisterAny): Likewise.
* src/lxc/lxc_driver.c (lxcConnectDomainEventRegister)
(lxcConnectDomainEventRegisterAny): Likewise.

Signed-off-by: Eric Blake <eblake@redhat.com>
(cherry picked from commit f9f56340539d609cdc2e9d4ab812b9f146c3f100)

Conflicts:
src/conf/object_event.c - not backporting event refactoring
src/conf/object_event_private.h - likewise
src/conf/network_event.c - not backporting network events
src/conf/network_event.h - likewise
src/network/bridge_driver.c - likewise
src/access/viraccessperm.h - likewise
src/remote/remote_protocol.x - likewise
src/conf/domain_event.c - includes code that upstream has in object_event
src/conf/domain_event.h - context
src/libxl/libxl_driver.c - context
src/lxc/lxc_driver.c - context
src/remote/remote_driver.c - context, not backporting network events
src/test/test_driver.c - context, not backporting network events
src/uml/uml_driver.c - context
src/xen/xen_driver.c - context

12 files changed:
src/access/viraccessperm.h
src/conf/domain_event.c
src/conf/domain_event.h
src/libxl/libxl_driver.c
src/lxc/lxc_driver.c
src/qemu/qemu_driver.c
src/remote/remote_driver.c
src/remote/remote_protocol.x
src/test/test_driver.c
src/uml/uml_driver.c
src/vbox/vbox_tmpl.c
src/xen/xen_driver.c

index 2f76c95cee14495eb2739c2a8096e0a15614307a..64db81fc3bb6e9f29cdaaada1dfaeab6d0644fc5 100644 (file)
@@ -1,7 +1,7 @@
 /*
  * viraccessperm.h: access control permissions
  *
- * Copyright (C) 2012-2013 Red Hat, Inc.
+ * Copyright (C) 2012-2014 Red Hat, Inc.
  *
  * This library is free software; you can redistribute it and/or
  * modify it under the terms of the GNU Lesser General Public
@@ -47,7 +47,7 @@ typedef enum {
 
     /**
      * @desc: List domains
-     * @message: Listing domains requires authorization
+     * @message: Listing domains or using domain events requires authorization
      * @anonymous: 1
      */
     VIR_ACCESS_PERM_CONNECT_SEARCH_DOMAINS,
index de55d087967d0fe0ac126041efd2129fb2f0cd42..1736aa0a0341ae48ecea3a9ecfe2d03e3e9e9053 100644 (file)
 
 #define VIR_FROM_THIS VIR_FROM_NONE
 
+/**
+ * virObjectEventCallbackFilter:
+ * @conn: the connection pointer
+ * @event: the event about to be dispatched
+ * @opaque: opaque data registered with the filter
+ *
+ * Callback to do final filtering for a reason not tracked directly by
+ * virObjectEventStateRegisterID().  Return false if @event must not
+ * be sent to @conn.
+ */
+typedef bool (*virObjectEventCallbackFilter)(virConnectPtr conn,
+                                             virDomainEventPtr event,
+                                             void *opaque);
+
 struct _virDomainMeta {
     int id;
     char *name;
@@ -68,6 +82,8 @@ struct _virDomainEventCallback {
     int eventID;
     virConnectPtr conn;
     virDomainMetaPtr dom;
+    virObjectEventCallbackFilter filter;
+    void *filter_opaque;
     virConnectDomainEventGenericCallback cb;
     void *opaque;
     virFreeCallback freecb;
@@ -344,6 +360,9 @@ virDomainEventCallbackListPurgeMarked(virDomainEventCallbackListPtr cbList)
  * virDomainEventCallbackListAddID:
  * @conn: pointer to the connection
  * @cbList: the list
+ * @dom: optional domain to filter on
+ * @filter optional last-ditch filter callback
+ * @filter_opaque: opaque data to pass to @filter
  * @eventID: the event ID
  * @callback: the callback to add
  * @opaque: opaque data tio pass to callback
@@ -355,6 +374,8 @@ static int
 virDomainEventCallbackListAddID(virConnectPtr conn,
                                 virDomainEventCallbackListPtr cbList,
                                 virDomainPtr dom,
+                                virObjectEventCallbackFilter filter,
+                                void *filter_opaque,
                                 int eventID,
                                 virConnectDomainEventGenericCallback callback,
                                 void *opaque,
@@ -401,6 +422,8 @@ virDomainEventCallbackListAddID(virConnectPtr conn,
         memcpy(event->dom->uuid, dom->uuid, VIR_UUID_BUFLEN);
         event->dom->id = dom->id;
     }
+    event->filter = filter;
+    event->filter_opaque = filter_opaque;
 
     /* Make space on list */
     if (VIR_REALLOC_N(cbList->callbacks, cbList->count + 1) < 0)
@@ -440,6 +463,8 @@ error:
  * virDomainEventCallbackListAdd:
  * @conn: pointer to the connection
  * @cbList: the list
+ * @filter optional last-ditch filter callback
+ * @filter_opaque: opaque data to pass to @filter
  * @callback: the callback to add
  * @opaque: opaque data tio pass to callback
  *
@@ -448,11 +473,14 @@ error:
 static int
 virDomainEventCallbackListAdd(virConnectPtr conn,
                               virDomainEventCallbackListPtr cbList,
+                              virObjectEventCallbackFilter filter,
+                              void *filter_opaque,
                               virConnectDomainEventCallback callback,
                               void *opaque,
                               virFreeCallback freecb)
 {
     return virDomainEventCallbackListAddID(conn, cbList, NULL,
+                                           filter, filter_opaque,
                                            VIR_DOMAIN_EVENT_ID_LIFECYCLE,
                                            VIR_DOMAIN_EVENT_CALLBACK(callback),
                                            opaque, freecb, NULL);
@@ -680,6 +708,32 @@ static virDomainEventPtr virDomainEventNewInternal(int eventID,
     return event;
 }
 
+
+/**
+ * virDomainEventFilter:
+ * @conn: pointer to the connection
+ * @event: the event to check
+ * @opaque: opaque data holding ACL filter to use
+ *
+ * Internal function to run ACL filtering before dispatching an event
+ */
+static bool
+virDomainEventFilter(virConnectPtr conn, virDomainEventPtr event,
+                     void *opaque)
+{
+    virDomainDef dom;
+    virDomainObjListFilter filter = opaque;
+
+    /* For now, we just create a virDomainDef with enough contents to
+     * satisfy what viraccessdriverpolkit.c references.  This is a bit
+     * fragile, but I don't know of anything better.  */
+    dom.name = event->dom.name;
+    memcpy(dom.uuid, event->dom.uuid, VIR_UUID_BUFLEN);
+
+    return (filter)(conn, &dom);
+}
+
+
 virDomainEventPtr virDomainEventNew(int id, const char *name,
                                     const unsigned char *uuid,
                                     int type, int detail)
@@ -1381,6 +1435,9 @@ static int virDomainEventDispatchMatchCallback(virDomainEventPtr event,
     if (cb->eventID != event->eventID)
         return 0;
 
+    if (cb->filter && !(cb->filter)(cb->conn, event, cb->filter_opaque))
+        return 0;
+
     if (cb->dom) {
         /* Deliberately ignoring 'id' for matching, since that
          * will cause problems when a domain switches between
@@ -1510,6 +1567,7 @@ virDomainEventStateFlush(virDomainEventStatePtr state)
  * virDomainEventStateRegister:
  * @conn: connection to associate with callback
  * @state: domain event state
+ * @filter: optional ACL filter to limit which events can be sent
  * @callback: function to remove from event
  * @opaque: data blob to pass to callback
  * @freecb: callback to free @opaque
@@ -1522,6 +1580,7 @@ virDomainEventStateFlush(virDomainEventStatePtr state)
 int
 virDomainEventStateRegister(virConnectPtr conn,
                             virDomainEventStatePtr state,
+                            virDomainObjListFilter filter,
                             virConnectDomainEventCallback callback,
                             void *opaque,
                             virFreeCallback freecb)
@@ -1542,7 +1601,8 @@ virDomainEventStateRegister(virConnectPtr conn,
     }
 
     ret = virDomainEventCallbackListAdd(conn, state->callbacks,
-                                        callback, opaque, freecb);
+                                        filter ? virDomainEventFilter : NULL,
+                                        filter, callback, opaque, freecb);
 
     if (ret == -1 &&
         state->callbacks->count == 0 &&
@@ -1561,6 +1621,7 @@ cleanup:
  * virDomainEventStateRegisterID:
  * @conn: connection to associate with callback
  * @state: domain event state
+ * @filter: optional ACL filter to limit which events can be sent
  * @eventID: ID of the event type to register for
  * @cb: function to remove from event
  * @opaque: data blob to pass to callback
@@ -1575,6 +1636,7 @@ cleanup:
 int
 virDomainEventStateRegisterID(virConnectPtr conn,
                               virDomainEventStatePtr state,
+                              virDomainObjListFilter filter,
                               virDomainPtr dom,
                               int eventID,
                               virConnectDomainEventGenericCallback cb,
@@ -1597,8 +1659,9 @@ virDomainEventStateRegisterID(virConnectPtr conn,
         goto cleanup;
     }
 
-    ret = virDomainEventCallbackListAddID(conn, state->callbacks,
-                                          dom, eventID, cb, opaque, freecb,
+    ret = virDomainEventCallbackListAddID(conn, state->callbacks, dom,
+                                          filter ? virDomainEventFilter : NULL,
+                                          filter, eventID, cb, opaque, freecb,
                                           callbackID);
 
     if (ret == -1 &&
index f6b957d596be525d794fe5ab3efc9d9e6fb12f9f..9dc65e22d31ee42ec63a5d25351da7f9af11d71f 100644 (file)
@@ -1,7 +1,7 @@
 /*
  * domain_event.h: domain event queue processing helpers
  *
- * Copyright (C) 2012 Red Hat, Inc.
+ * Copyright (C) 2012-2014 Red Hat, Inc.
  * Copyright (C) 2008 VirtualIron
  *
  * This library is free software; you can redistribute it and/or
@@ -149,19 +149,21 @@ virDomainEventStateQueue(virDomainEventStatePtr state,
     ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
 int virDomainEventStateRegister(virConnectPtr conn,
                                 virDomainEventStatePtr state,
+                                virDomainObjListFilter filter,
                                 virConnectDomainEventCallback callback,
                                 void *opaque,
                                 virFreeCallback freecb)
-    ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3);
+    ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(4);
 int virDomainEventStateRegisterID(virConnectPtr conn,
                                   virDomainEventStatePtr state,
+                                  virDomainObjListFilter filter,
                                   virDomainPtr dom,
                                   int eventID,
                                   virConnectDomainEventGenericCallback cb,
                                   void *opaque,
                                   virFreeCallback freecb,
                                   int *callbackID)
-    ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(5);
+    ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(6);
 int
 virDomainEventStateDeregister(virConnectPtr conn,
                               virDomainEventStatePtr state,
index 0a67ad24f5980e3cb2755ae4887e14d3a41ae0f4..431dfa3d89e79f73c9d5ed488a5edec9b93c150f 100644 (file)
@@ -4203,6 +4203,7 @@ libxlConnectDomainEventRegister(virConnectPtr conn,
     libxlDriverLock(driver);
     ret = virDomainEventStateRegister(conn,
                                       driver->domainEventState,
+                                      virConnectDomainEventRegisterCheckACL,
                                       callback, opaque, freecb);
     libxlDriverUnlock(driver);
 
@@ -4880,6 +4881,7 @@ libxlConnectDomainEventRegisterAny(virConnectPtr conn, virDomainPtr dom, int eve
     libxlDriverLock(driver);
     if (virDomainEventStateRegisterID(conn,
                                       driver->domainEventState,
+                                      virConnectDomainEventRegisterAnyCheckACL,
                                       dom, eventID, callback, opaque,
                                       freecb, &ret) < 0)
         ret = -1;
index ece8230a7f613f1218925f74c1fba2624f29e947..b458474abc6aa05ce5677476a5e994aa12159753 100644 (file)
@@ -1295,6 +1295,7 @@ lxcConnectDomainEventRegister(virConnectPtr conn,
 
     ret = virDomainEventStateRegister(conn,
                                       driver->domainEventState,
+                                      virConnectDomainEventRegisterCheckACL,
                                       callback, opaque, freecb);
 
     return ret;
@@ -1335,6 +1336,7 @@ lxcConnectDomainEventRegisterAny(virConnectPtr conn,
 
     if (virDomainEventStateRegisterID(conn,
                                       driver->domainEventState,
+                                      virConnectDomainEventRegisterAnyCheckACL,
                                       dom, eventID,
                                       callback, opaque, freecb, &ret) < 0)
         ret = -1;
index 0170179c2ea3c3c33cae09b7c3c92070ba543ba0..c1d686886931f0b17c09889ab39fb4373ca08734 100644 (file)
@@ -9877,6 +9877,7 @@ qemuConnectDomainEventRegister(virConnectPtr conn,
 
     if (virDomainEventStateRegister(conn,
                                     driver->domainEventState,
+                                    virConnectDomainEventRegisterCheckACL,
                                     callback, opaque, freecb) < 0)
         goto cleanup;
 
@@ -9925,6 +9926,7 @@ qemuConnectDomainEventRegisterAny(virConnectPtr conn,
 
     if (virDomainEventStateRegisterID(conn,
                                       driver->domainEventState,
+                                      virConnectDomainEventRegisterAnyCheckACL,
                                       dom, eventID,
                                       callback, opaque, freecb, &ret) < 0)
         ret = -1;
index b3e86e1d5d0d727848e0bc8a4a53b91e84a2a86e..18e5273da826eb48fa2e81e972fe10eb28c7a263 100644 (file)
@@ -4309,7 +4309,7 @@ static int remoteConnectDomainEventRegister(virConnectPtr conn,
 
     remoteDriverLock(priv);
 
-    if ((count = virDomainEventStateRegister(conn, priv->domainEventState,
+    if ((count = virDomainEventStateRegister(conn, priv->domainEventState, NULL,
                                              callback, opaque, freecb)) < 0) {
          virReportError(VIR_ERR_RPC, "%s", _("adding cb to list"));
          goto done;
@@ -5095,7 +5095,7 @@ static int remoteConnectDomainEventRegisterAny(virConnectPtr conn,
     remoteDriverLock(priv);
 
     if ((count = virDomainEventStateRegisterID(conn,
-                                               priv->domainEventState,
+                                               priv->domainEventState, NULL,
                                                dom, eventID,
                                                callback, opaque, freecb,
                                                &callbackID)) < 0) {
index 24c3f6c0d790c3f983acd8aa8fc92f06b1be9838..e64ae40178d0fe386024674c856bc3293140ad8f 100644 (file)
@@ -1952,7 +1952,7 @@ struct remote_node_device_destroy_args {
 
 /*
  * Events Register/Deregister:
- * It would seem rpcgen does not like both args, and ret
+ * It would seem rpcgen does not like both args and ret
  * to be null. It will not generate the prototype otherwise.
  * Pass back a redundant boolean to force prototype generation.
  */
@@ -3606,7 +3606,8 @@ enum remote_procedure {
     /**
      * @generate: none
      * @priority: high
-     * @acl: connect:read
+     * @acl: connect:search_domains
+     * @aclfilter: domain:getattr
      */
     REMOTE_PROC_CONNECT_DOMAIN_EVENT_REGISTER = 105,
 
@@ -4038,7 +4039,8 @@ enum remote_procedure {
     /**
      * @generate: none
      * @priority: high
-     * @acl: connect:read
+     * @acl: connect:search_domains
+     * @aclfilter: domain:getattr
      */
     REMOTE_PROC_CONNECT_DOMAIN_EVENT_REGISTER_ANY = 167,
 
index c2256188dfeece01fbd7cdab9aefca2f7247ba16..c8b1282dc92a9bf662070eb65a8bc271160b6bb6 100644 (file)
@@ -5628,7 +5628,7 @@ testConnectDomainEventRegister(virConnectPtr conn,
 
     testDriverLock(driver);
     ret = virDomainEventStateRegister(conn,
-                                      driver->domainEventState,
+                                      driver->domainEventState, NULL,
                                       callback, opaque, freecb);
     testDriverUnlock(driver);
 
@@ -5666,7 +5666,7 @@ testConnectDomainEventRegisterAny(virConnectPtr conn,
 
     testDriverLock(driver);
     if (virDomainEventStateRegisterID(conn,
-                                      driver->domainEventState,
+                                      driver->domainEventState, NULL,
                                       dom, eventID,
                                       callback, opaque, freecb, &ret) < 0)
         ret = -1;
index 9ca352f46251f5a5675fdebf22b36a7520464cfc..6e6761bb9f98a01916b4b13499d1052ef8f45eb2 100644 (file)
@@ -2618,6 +2618,7 @@ umlConnectDomainEventRegister(virConnectPtr conn,
     umlDriverLock(driver);
     ret = virDomainEventStateRegister(conn,
                                       driver->domainEventState,
+                                      virConnectDomainEventRegisterCheckACL,
                                       callback, opaque, freecb);
     umlDriverUnlock(driver);
 
@@ -2660,6 +2661,7 @@ umlConnectDomainEventRegisterAny(virConnectPtr conn,
     umlDriverLock(driver);
     if (virDomainEventStateRegisterID(conn,
                                       driver->domainEventState,
+                                      virConnectDomainEventRegisterAnyCheckACL,
                                       dom, eventID,
                                       callback, opaque, freecb, &ret) < 0)
         ret = -1;
index 5b1704841768e378d020805db74e3975cf5ef6c5..27f4197e1172e563d3acac54bbd35ffc6a645a94 100644 (file)
@@ -7265,7 +7265,7 @@ static int vboxConnectDomainEventRegister(virConnectPtr conn,
              * later you can iterate over them
              */
 
-            ret = virDomainEventStateRegister(conn, data->domainEvents,
+            ret = virDomainEventStateRegister(conn, data->domainEvents, NULL,
                                               callback, opaque, freecb);
             VIR_DEBUG("virDomainEventStateRegister (ret = %d) (conn: %p, "
                       "callback: %p, opaque: %p, "
@@ -7357,7 +7357,7 @@ static int vboxConnectDomainEventRegisterAny(virConnectPtr conn,
              * later you can iterate over them
              */
 
-            if (virDomainEventStateRegisterID(conn, data->domainEvents,
+            if (virDomainEventStateRegisterID(conn, data->domainEvents, NULL,
                                               dom, eventID,
                                               callback, opaque, freecb, &ret) < 0)
                 ret = -1;
index cb64de65c29c6b7e66242e7a7074f849a95725b5..4356280a9e89fd06dea3629f21bf03b935da5173 100644 (file)
@@ -2306,6 +2306,7 @@ xenUnifiedConnectDomainEventRegister(virConnectPtr conn,
     }
 
     ret = virDomainEventStateRegister(conn, priv->domainEvents,
+                                      virConnectDomainEventRegisterCheckACL,
                                       callback, opaque, freefunc);
 
     xenUnifiedUnlock(priv);
@@ -2363,6 +2364,7 @@ xenUnifiedConnectDomainEventRegisterAny(virConnectPtr conn,
     }
 
     if (virDomainEventStateRegisterID(conn, priv->domainEvents,
+                                      virConnectDomainEventRegisterAnyCheckACL,
                                       dom, eventID,
                                       callback, opaque, freefunc, &ret) < 0)
         ret = -1;