bus-proxy: don't print error-messages if we check multiple dests

If we test the policy against multiple destination names, we really should
not print warnings if one of the names results in DENY. Instead, pass the
whole array of names to the policy and let it deal with it.

4 files changed, 162 insertions, 121 deletions

diff --git a/src/bus-proxyd/bus-xml-policy.c b/src/bus-proxyd/bus-xml-policy.c
index 0c60b6b6e..f7f3388ba 100644
--- a/src/bus-proxyd/bus-xml-policy.c
+++ b/src/bus-proxyd/bus-xml-policy.c
@@ -22,6 +22,7 @@
 #include "xml.h"
 #include "fileio.h"
 #include "strv.h"
+#include "set.h"
 #include "conf-files.h"
 #include "bus-internal.h"
 #include "bus-message.h"
@@ -865,15 +866,14 @@ bool policy_check_hello(Policy *p, uid_t uid, gid_t gid) {
         return verdict == ALLOW;
 }
 
-bool policy_check_recv(Policy *p,
-                       uid_t uid,
-                       gid_t gid,
-                       int message_type,
-                       const char *name,
-                       const char *path,
-                       const char *interface,
-                       const char *member,
-                       bool dbus_to_kernel) {
+bool policy_check_one_recv(Policy *p,
+                           uid_t uid,
+                           gid_t gid,
+                           int message_type,
+                           const char *name,
+                           const char *path,
+                           const char *interface,
+                           const char *member) {
 
         struct policy_check_filter filter = {
                 .class        = POLICY_ITEM_RECV,
@@ -886,30 +886,64 @@ bool policy_check_recv(Policy *p,
                 .member       = member,
         };
 
-        int verdict;
-
         assert(p);
 
-        verdict = policy_check(p, &filter);
-
-        log_full(LOG_AUTH | (verdict != ALLOW ? LOG_WARNING : LOG_DEBUG),
-                 "Receive permission check %s for uid=" UID_FMT " gid=" GID_FMT" message=%s name=%s path=%s interface=%s member=%s: %s",
-                 dbus_to_kernel ? "dbus-1 to kernel" : "kernel to dbus-1", uid, gid, bus_message_type_to_string(message_type), strna(name),
-                 strna(path), strna(interface), strna(member), strna(verdict_to_string(verdict)));
-
-        return verdict == ALLOW;
+        return policy_check(p, &filter) == ALLOW;
 }
 
-bool policy_check_send(Policy *p,
+bool policy_check_recv(Policy *p,
                        uid_t uid,
                        gid_t gid,
                        int message_type,
-                       const char *name,
+                       Set *names,
+                       char **namesv,
                        const char *path,
                        const char *interface,
                        const char *member,
                        bool dbus_to_kernel) {
 
+        char *n, **nv, *last = NULL;
+        bool allow = false;
+        Iterator i;
+
+        assert(p);
+
+        if (set_isempty(names) && strv_isempty(namesv)) {
+                allow = policy_check_one_recv(p, uid, gid, message_type, NULL, path, interface, member);
+        } else {
+                SET_FOREACH(n, names, i) {
+                        last = n;
+                        allow = policy_check_one_recv(p, uid, gid, message_type, n, path, interface, member);
+                        if (allow)
+                                break;
+                }
+                if (!allow) {
+                        STRV_FOREACH(nv, namesv) {
+                                last = *nv;
+                                allow = policy_check_one_recv(p, uid, gid, message_type, *nv, path, interface, member);
+                                if (allow)
+                                        break;
+                        }
+                }
+        }
+
+        log_full(LOG_AUTH | (!allow ? LOG_WARNING : LOG_DEBUG),
+                 "Receive permission check %s for uid=" UID_FMT " gid=" GID_FMT" message=%s name=%s path=%s interface=%s member=%s: %s",
+                 dbus_to_kernel ? "dbus-1 to kernel" : "kernel to dbus-1", uid, gid, bus_message_type_to_string(message_type), strna(last),
+                 strna(path), strna(interface), strna(member), allow ? "ALLOW" : "DENY");
+
+        return allow;
+}
+
+bool policy_check_one_send(Policy *p,
+                           uid_t uid,
+                           gid_t gid,
+                           int message_type,
+                           const char *name,
+                           const char *path,
+                           const char *interface,
+                           const char *member) {
+
         struct policy_check_filter filter = {
                 .class        = POLICY_ITEM_SEND,
                 .uid          = uid,
@@ -921,18 +955,57 @@ bool policy_check_send(Policy *p,
                 .member       = member,
         };
 
-        int verdict;
+        assert(p);
+
+        return policy_check(p, &filter) == ALLOW;
+}
+
+bool policy_check_send(Policy *p,
+                       uid_t uid,
+                       gid_t gid,
+                       int message_type,
+                       Set *names,
+                       char **namesv,
+                       const char *path,
+                       const char *interface,
+                       const char *member,
+                       bool dbus_to_kernel,
+                       char **out_used_name) {
+
+        char *n, **nv, *last = NULL;
+        bool allow = false;
+        Iterator i;
 
         assert(p);
 
-        verdict = policy_check(p, &filter);
+        if (set_isempty(names) && strv_isempty(namesv)) {
+                allow = policy_check_one_send(p, uid, gid, message_type, NULL, path, interface, member);
+        } else {
+                SET_FOREACH(n, names, i) {
+                        last = n;
+                        allow = policy_check_one_send(p, uid, gid, message_type, n, path, interface, member);
+                        if (allow)
+                                break;
+                }
+                if (!allow) {
+                        STRV_FOREACH(nv, namesv) {
+                                last = *nv;
+                                allow = policy_check_one_send(p, uid, gid, message_type, *nv, path, interface, member);
+                                if (allow)
+                                        break;
+                        }
+                }
+        }
 
-        log_full(LOG_AUTH | (verdict != ALLOW ? LOG_WARNING : LOG_DEBUG),
+        if (out_used_name)
+                *out_used_name = last;
+
+        log_full(LOG_AUTH | (!allow ? LOG_WARNING : LOG_DEBUG),
                  "Send permission check %s for uid=" UID_FMT " gid=" GID_FMT" message=%s name=%s path=%s interface=%s member=%s: %s",
-                 dbus_to_kernel ? "dbus-1 to kernel" : "kernel to dbus-1", uid, gid, bus_message_type_to_string(message_type), strna(name),
-                 strna(path), strna(interface), strna(member), strna(verdict_to_string(verdict)));
+                 dbus_to_kernel ? "dbus-1 to kernel" : "kernel to dbus-1", uid, gid, bus_message_type_to_string(message_type), strna(last),
+                 strna(path), strna(interface), strna(member), allow ? "ALLOW" : "DENY");
 
-        return verdict == ALLOW;
+        return allow;
 }
 
 int policy_load(Policy *p, char **files) {
diff --git a/src/bus-proxyd/bus-xml-policy.h b/src/bus-proxyd/bus-xml-policy.h
index 9716772e6..f2ec1bbea 100644
--- a/src/bus-proxyd/bus-xml-policy.h
+++ b/src/bus-proxyd/bus-xml-policy.h
@@ -26,6 +26,7 @@
 
 #include "list.h"
 #include "hashmap.h"
+#include "set.h"
 
 typedef enum PolicyItemType {
         _POLICY_ITEM_TYPE_UNSET = 0,
@@ -91,24 +92,43 @@ void policy_free(Policy *p);
 
 bool policy_check_own(Policy *p, uid_t uid, gid_t gid, const char *name);
 bool policy_check_hello(Policy *p, uid_t uid, gid_t gid);
+bool policy_check_one_recv(Policy *p,
+                           uid_t uid,
+                           gid_t gid,
+                           int message_type,
+                           const char *name,
+                           const char *path,
+                           const char *interface,
+                           const char *member);
 bool policy_check_recv(Policy *p,
                        uid_t uid,
                        gid_t gid,
                        int message_type,
-                       const char *name,
+                       Set *names,
+                       char **namesv,
                        const char *path,
                        const char *interface,
                        const char *member,
                        bool dbus_to_kernel);
+bool policy_check_one_send(Policy *p,
+                           uid_t uid,
+                           gid_t gid,
+                           int message_type,
+                           const char *name,
+                           const char *path,
+                           const char *interface,
+                           const char *member);
 bool policy_check_send(Policy *p,
                        uid_t uid,
                        gid_t gid,
                        int message_type,
-                       const char *name,
+                       Set *names,
+                       char **namesv,
                        const char *path,
                        const char *interface,
                        const char *member,
-                       bool dbus_to_kernel);
+                       bool dbus_to_kernel,
+                       char **out_used_name);
 
 void policy_dump(Policy *p);
 
diff --git a/src/bus-proxyd/proxy.c b/src/bus-proxyd/proxy.c
index ab164474f..f7daec68f 100644
--- a/src/bus-proxyd/proxy.c
+++ b/src/bus-proxyd/proxy.c
@@ -425,7 +425,6 @@ static int process_policy_unlocked(sd_bus *from, sd_bus *to, sd_bus_message *m,
                 uid_t sender_uid = UID_INVALID;
                 gid_t sender_gid = GID_INVALID;
                 char **sender_names = NULL;
-                bool granted = false;
 
                 /* Driver messages are always OK */
                 if (streq_ptr(m->sender, "org.freedesktop.DBus"))
@@ -456,34 +455,9 @@ static int process_policy_unlocked(sd_bus *from, sd_bus *to, sd_bus_message *m,
                 }
 
                 /* First check whether the sender can send the message to our name */
-                if (set_isempty(owned_names)) {
-                        if (policy_check_send(policy, sender_uid, sender_gid, m->header->type, NULL, m->path, m->interface, m->member, false))
-                                granted = true;
-                } else {
-                        Iterator i;
-                        char *n;
-
-                        SET_FOREACH(n, owned_names, i)
-                                if (policy_check_send(policy, sender_uid, sender_gid, m->header->type, n, m->path, m->interface, m->member, false)) {
-                                        granted = true;
-                                        break;
-                                }
-                }
-
-                if (granted) {
-                        /* Then check whether us (the recipient) can receive from the sender's name */
-                        if (strv_isempty(sender_names)) {
-                                if (policy_check_recv(policy, our_ucred->uid, our_ucred->gid, m->header->type, NULL, m->path, m->interface, m->member, false))
-                                        return 0;
-                        } else {
-                                char **n;
-
-                                STRV_FOREACH(n, sender_names) {
-                                        if (policy_check_recv(policy, our_ucred->uid, our_ucred->gid, m->header->type, *n, m->path, m->interface, m->member, false))
-                                                return 0;
-                                }
-                        }
-                }
+                if (policy_check_send(policy, sender_uid, sender_gid, m->header->type, owned_names, NULL, m->path, m->interface, m->member, false, NULL) &&
+                    policy_check_recv(policy, our_ucred->uid, our_ucred->gid, m->header->type, NULL, sender_names, m->path, m->interface, m->member, false))
+                        return 0;
 
                 /* Return an error back to the caller */
                 if (m->header->type == SD_BUS_MESSAGE_METHOD_CALL)
@@ -499,7 +473,7 @@ static int process_policy_unlocked(sd_bus *from, sd_bus *to, sd_bus_message *m,
                 gid_t destination_gid = GID_INVALID;
                 const char *destination_unique = NULL;
                 char **destination_names = NULL;
-                bool granted = false;
+                char *n;
 
                 /* Driver messages are always OK */
                 if (streq_ptr(m->destination, "org.freedesktop.DBus"))
@@ -525,42 +499,24 @@ static int process_policy_unlocked(sd_bus *from, sd_bus *to, sd_bus_message *m,
                 }
 
                 /* First check if we (the sender) can send to this name */
-                if (strv_isempty(destination_names)) {
-                        if (policy_check_send(policy, our_ucred->uid, our_ucred->gid, m->header->type, NULL, m->path, m->interface, m->member, true))
-                                granted = true;
-                } else {
-                        char **n;
-
-                        STRV_FOREACH(n, destination_names) {
-                                if (policy_check_send(policy, our_ucred->uid, our_ucred->gid, m->header->type, *n, m->path, m->interface, m->member, true)) {
-
-                                        /* If we made a receiver decision,
-                                           then remember which name's policy
-                                           we used, and to which unique ID it
-                                           mapped when we made the
-                                           decision. Then, let's pass this to
-                                           the kernel when sending the
-                                           message, so that it refuses the
-                                           operation should the name and
-                                           unique ID not map to each other
-                                           anymore. */
-
-                                        r = free_and_strdup(&m->destination_ptr, *n);
-                                        if (r < 0)
-                                                return r;
-
-                                        r = bus_kernel_parse_unique_name(destination_unique, &m->verify_destination_id);
-                                        if (r < 0)
-                                                break;
-
-                                        granted = true;
-                                        break;
-                                }
+                if (policy_check_send(policy, our_ucred->uid, our_ucred->gid, m->header->type, NULL, destination_names, m->path, m->interface, m->member, true, &n)) {
+                        if (n) {
+                                /* If we made a receiver decision, then remember which
+                                 * name's policy we used, and to which unique ID it
+                                 * mapped when we made the decision. Then, let's pass
+                                 * this to the kernel when sending the message, so that
+                                 * it refuses the operation should the name and unique
+                                 * ID not map to each other anymore. */
+
+                                r = free_and_strdup(&m->destination_ptr, n);
+                                if (r < 0)
+                                        return r;
+
+                                r = bus_kernel_parse_unique_name(destination_unique, &m->verify_destination_id);
+                                if (r < 0)
+                                        return r;
                         }
-                }
 
-                /* Then check if the recipient can receive from our name */
-                if (granted) {
                         if (sd_bus_message_is_signal(m, NULL, NULL)) {
                                 /* If we forward a signal from dbus-1 to kdbus,
                                  * we have no idea who the recipient is.
@@ -571,16 +527,8 @@ static int process_policy_unlocked(sd_bus *from, sd_bus *to, sd_bus_message *m,
                                  * receiver policies to the message. Therefore,
                                  * skip policy checks in this case. */
                                 return 0;
-                        } else if (set_isempty(owned_names)) {
-                                if (policy_check_recv(policy, destination_uid, destination_gid, m->header->type, NULL, m->path, m->interface, m->member, true))
-                                        return 0;
-                        } else {
-                                Iterator i;
-                                char *n;
-
-                                SET_FOREACH(n, owned_names, i)
-                                        if (policy_check_recv(policy, destination_uid, destination_gid, m->header->type, n, m->path, m->interface, m->member, true))
-                                                return 0;
+                        } else if (policy_check_recv(policy, destination_uid, destination_gid, m->header->type, owned_names, NULL, m->path, m->interface, m->member, true)) {
+                                return 0;
                         }
                 }
 
diff --git a/src/bus-proxyd/test-bus-xml-policy.c b/src/bus-proxyd/test-bus-xml-policy.c
index cd1b4b2e6..4b07747e1 100644
--- a/src/bus-proxyd/test-bus-xml-policy.c
+++ b/src/bus-proxyd/test-bus-xml-policy.c
@@ -105,8 +105,8 @@ int main(int argc, char *argv[]) {
         /* Signaltest */
         assert_se(test_policy_load(&p, "signals.conf") == 0);
 
-        assert_se(policy_check_send(&p, 0, 0, SD_BUS_MESSAGE_SIGNAL, "bli.bla.blubb", NULL, "/an/object/path", NULL, false) == true);
-        assert_se(policy_check_send(&p, 1, 0, SD_BUS_MESSAGE_SIGNAL, "bli.bla.blubb", NULL, "/an/object/path", NULL, false) == false);
+        assert_se(policy_check_one_send(&p, 0, 0, SD_BUS_MESSAGE_SIGNAL, "bli.bla.blubb", NULL, "/an/object/path", NULL) == true);
+        assert_se(policy_check_one_send(&p, 1, 0, SD_BUS_MESSAGE_SIGNAL, "bli.bla.blubb", NULL, "/an/object/path", NULL) == false);
 
         policy_free(&p);
 
@@ -114,12 +114,12 @@ int main(int argc, char *argv[]) {
         assert_se(test_policy_load(&p, "methods.conf") == 0);
         policy_dump(&p);
 
-        assert_se(policy_check_send(&p, 0, 0, SD_BUS_MESSAGE_METHOD_CALL, "org.test.test1", "/an/object/path", "bli.bla.blubb", "Member", false) == false);
-        assert_se(policy_check_send(&p, 0, 0, SD_BUS_MESSAGE_METHOD_CALL, "org.test.test1", "/an/object/path", "bli.bla.blubb", "Member", false) == false);
-        assert_se(policy_check_send(&p, 0, 0, SD_BUS_MESSAGE_METHOD_CALL, "org.test.test1", "/an/object/path", "org.test.int1", "Member", false) == true);
-        assert_se(policy_check_send(&p, 0, 0, SD_BUS_MESSAGE_METHOD_CALL, "org.test.test1", "/an/object/path", "org.test.int2", "Member", false) == true);
+        assert_se(policy_check_one_send(&p, 0, 0, SD_BUS_MESSAGE_METHOD_CALL, "org.test.test1", "/an/object/path", "bli.bla.blubb", "Member") == false);
+        assert_se(policy_check_one_send(&p, 0, 0, SD_BUS_MESSAGE_METHOD_CALL, "org.test.test1", "/an/object/path", "bli.bla.blubb", "Member") == false);
+        assert_se(policy_check_one_send(&p, 0, 0, SD_BUS_MESSAGE_METHOD_CALL, "org.test.test1", "/an/object/path", "org.test.int1", "Member") == true);
+        assert_se(policy_check_one_send(&p, 0, 0, SD_BUS_MESSAGE_METHOD_CALL, "org.test.test1", "/an/object/path", "org.test.int2", "Member") == true);
 
-        assert_se(policy_check_recv(&p, 0, 0, SD_BUS_MESSAGE_METHOD_CALL, "org.test.test3", "/an/object/path", "org.test.int3", "Member111", false) == true);
+        assert_se(policy_check_one_recv(&p, 0, 0, SD_BUS_MESSAGE_METHOD_CALL, "org.test.test3", "/an/object/path", "org.test.int3", "Member111") == true);
 
         policy_free(&p);
 
@@ -162,19 +162,19 @@ int main(int argc, char *argv[]) {
 
         assert_se(policy_check_own(&p, 0, 0, "org.foo.FooService") == true);
         assert_se(policy_check_own(&p, 0, 0, "org.foo.FooService2") == false);
-        assert_se(policy_check_send(&p, 0, 0, SD_BUS_MESSAGE_METHOD_CALL, "org.test.test1", "/an/object/path", "org.test.int2", "Member", false) == false);
-        assert_se(policy_check_send(&p, 0, 0, SD_BUS_MESSAGE_METHOD_CALL, "org.test.test1", "/an/object/path", "org.foo.FooBroadcastInterface", "Member", false) == true);
-        assert_se(policy_check_recv(&p, 0, 0, SD_BUS_MESSAGE_METHOD_CALL, "org.foo.FooService", "/an/object/path", "org.foo.FooBroadcastInterface", "Member", false) == true);
-        assert_se(policy_check_recv(&p, 0, 0, SD_BUS_MESSAGE_METHOD_CALL, "org.foo.FooService", "/an/object/path", "org.foo.FooBroadcastInterface2", "Member", false) == false);
-        assert_se(policy_check_recv(&p, 0, 0, SD_BUS_MESSAGE_METHOD_CALL, "org.foo.FooService2", "/an/object/path", "org.foo.FooBroadcastInterface", "Member", false) == false);
+        assert_se(policy_check_one_send(&p, 0, 0, SD_BUS_MESSAGE_METHOD_CALL, "org.test.test1", "/an/object/path", "org.test.int2", "Member") == false);
+        assert_se(policy_check_one_send(&p, 0, 0, SD_BUS_MESSAGE_METHOD_CALL, "org.test.test1", "/an/object/path", "org.foo.FooBroadcastInterface", "Member") == true);
+        assert_se(policy_check_one_recv(&p, 0, 0, SD_BUS_MESSAGE_METHOD_CALL, "org.foo.FooService", "/an/object/path", "org.foo.FooBroadcastInterface", "Member") == true);
+        assert_se(policy_check_one_recv(&p, 0, 0, SD_BUS_MESSAGE_METHOD_CALL, "org.foo.FooService", "/an/object/path", "org.foo.FooBroadcastInterface2", "Member") == false);
+        assert_se(policy_check_one_recv(&p, 0, 0, SD_BUS_MESSAGE_METHOD_CALL, "org.foo.FooService2", "/an/object/path", "org.foo.FooBroadcastInterface", "Member") == false);
 
         assert_se(policy_check_own(&p, 100, 0, "org.foo.FooService") == false);
         assert_se(policy_check_own(&p, 100, 0, "org.foo.FooService2") == false);
-        assert_se(policy_check_send(&p, 100, 0, SD_BUS_MESSAGE_METHOD_CALL, "org.test.test1", "/an/object/path", "org.test.int2", "Member", false) == false);
-        assert_se(policy_check_send(&p, 100, 0, SD_BUS_MESSAGE_METHOD_CALL, "org.test.test1", "/an/object/path", "org.foo.FooBroadcastInterface", "Member", false) == false);
-        assert_se(policy_check_recv(&p, 100, 0, SD_BUS_MESSAGE_METHOD_CALL, "org.foo.FooService", "/an/object/path", "org.foo.FooBroadcastInterface", "Member", false) == true);
-        assert_se(policy_check_recv(&p, 100, 0, SD_BUS_MESSAGE_METHOD_CALL, "org.foo.FooService", "/an/object/path", "org.foo.FooBroadcastInterface2", "Member", false) == false);
-        assert_se(policy_check_recv(&p, 100, 0, SD_BUS_MESSAGE_METHOD_CALL, "org.foo.FooService2", "/an/object/path", "org.foo.FooBroadcastInterface", "Member", false) == false);
+        assert_se(policy_check_one_send(&p, 100, 0, SD_BUS_MESSAGE_METHOD_CALL, "org.test.test1", "/an/object/path", "org.test.int2", "Member") == false);
+        assert_se(policy_check_one_send(&p, 100, 0, SD_BUS_MESSAGE_METHOD_CALL, "org.test.test1", "/an/object/path", "org.foo.FooBroadcastInterface", "Member") == false);
+        assert_se(policy_check_one_recv(&p, 100, 0, SD_BUS_MESSAGE_METHOD_CALL, "org.foo.FooService", "/an/object/path", "org.foo.FooBroadcastInterface", "Member") == true);
+        assert_se(policy_check_one_recv(&p, 100, 0, SD_BUS_MESSAGE_METHOD_CALL, "org.foo.FooService", "/an/object/path", "org.foo.FooBroadcastInterface2", "Member") == false);
+        assert_se(policy_check_one_recv(&p, 100, 0, SD_BUS_MESSAGE_METHOD_CALL, "org.foo.FooService2", "/an/object/path", "org.foo.FooBroadcastInterface", "Member") == false);
 
         policy_free(&p);