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.

diff --git a/src/bus-proxyd/bus-xml-policy.c b/src/bus-proxyd/bus-xml-policy.c
index 0c60b6b6eb0f8783918f0d5979f11d25b1db6419..f7f3388ba95289d5e7d78283e2460f1f58ef215f 100644 (file)
--- 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 9716772e68ac6bb4b71385f548b89b4aa6517363..f2ec1bbea40a9c43917b06be87758d6d9955ef12 100644 (file)
--- 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 ab164474fa86a301abfe1ac9e7aeaca096f3c0c9..f7daec68fd7feae920343fd1436699137c9272cc 100644 (file)
--- 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 cd1b4b2e60a2efac92b17a5562d4431ce5251c98..4b07747e1ed71af9c7d5138eb44cbbf227e9c2aa 100644 (file)
--- 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);