bus-proxy: beef up policy enforcement

- actually return permission errors to clients

- use the right ucreds field

- fix error paths when we cannot keep track of locally acquired names
  due to OOM

- avoid unnecessary global variables

- log when the policy denies access

- enforce correct policy rule order

- always request all the metadata its we need to make decisions

1 files changed, 120 insertions, 53 deletions

diff --git a/src/bus-proxyd/bus-policy.c b/src/bus-proxyd/bus-policy.c
index d543bf9af..3cfe9befb 100644
--- a/src/bus-proxyd/bus-policy.c
+++ b/src/bus-proxyd/bus-policy.c
@@ -593,14 +593,29 @@ static int file_load(Policy *p, const char *path) {
 }
 
 enum {
+        DENY,
         ALLOW,
         DUNNO,
-        DENY,
 };
 
+static const char *verdict_to_string(int v) {
+        switch (v) {
+
+        case DENY:
+                return "DENY";
+        case ALLOW:
+                return "ALLOW";
+        case DUNNO:
+                return "DUNNO";
+        }
+
+        return NULL;
+}
+
 struct policy_check_filter {
         PolicyItemClass class;
-        const struct ucred *ucred;
+        uid_t uid;
+        gid_t gid;
         int message_type;
         const char *name;
         const char *interface;
@@ -656,17 +671,15 @@ static int check_policy_item(PolicyItem *i, const struct policy_check_filter *fi
                 break;
 
         case POLICY_ITEM_USER:
-                assert(filter->ucred);
-
-                if ((streq_ptr(i->name, "*") || (i->uid_valid && i->uid == filter->ucred->uid)))
-                        return is_permissive(i);
+                if (filter->uid != (uid_t) -1)
+                        if ((streq_ptr(i->name, "*") || (i->uid_valid && i->uid == filter->uid)))
+                                return is_permissive(i);
                 break;
 
         case POLICY_ITEM_GROUP:
-                assert(filter->ucred);
-
-                if ((streq_ptr(i->name, "*") || (i->gid_valid && i->gid == filter->ucred->gid)))
-                        return is_permissive(i);
+                if (filter->gid != (gid_t) -1)
+                        if ((streq_ptr(i->name, "*") || (i->gid_valid && i->gid == filter->gid)))
+                                return is_permissive(i);
                 break;
 
         case POLICY_ITEM_IGNORE:
@@ -680,29 +693,31 @@ static int check_policy_item(PolicyItem *i, const struct policy_check_filter *fi
 static int check_policy_items(PolicyItem *items, const struct policy_check_filter *filter) {
 
         PolicyItem *i;
-        int r, ret = DUNNO;
+        int verdict = DUNNO;
 
         assert(filter);
 
         /* Check all policies in a set - a broader one might be followed by a more specific one,
          * and the order of rules in policy definitions matters */
         LIST_FOREACH(items, i, items) {
+                int v;
+
                 if (i->class != filter->class &&
                     !(i->class == POLICY_ITEM_OWN_PREFIX && filter->class == POLICY_ITEM_OWN))
                         continue;
 
-                r = check_policy_item(i, filter);
-                if (r != DUNNO)
-                        ret = r;
+                v = check_policy_item(i, filter);
+                if (v != DUNNO)
+                        verdict = v;
         }
 
-        return ret;
+        return verdict;
 }
 
 static int policy_check(Policy *p, const struct policy_check_filter *filter) {
 
         PolicyItem *items;
-        int r;
+        int verdict, v;
 
         assert(p);
         assert(filter);
@@ -712,68 +727,96 @@ static int policy_check(Policy *p, const struct policy_check_filter *filter) {
         /*
          * The policy check is implemented by the following logic:
          *
-         * 1. Check mandatory items. If the message matches any of these, it is decisive.
-         * 2. See if the passed ucred match against the user/group hashmaps. A matching entry is also decisive.
-         * 3. Consult the defaults if non of the above matched with a more specific rule.
-         * 4. If the message isn't caught be the defaults either, reject it.
+         *  1. Check default items
+         *  2. Check group items
+         *  3. Check user items
+         *  4. Check mandatory items
+         *
+         *  Later rules override earlier rules.
          */
 
-        r = check_policy_items(p->mandatory_items, filter);
-        if (r != DUNNO)
-                return r;
+        verdict = check_policy_items(p->default_items, filter);
 
-        if (filter->ucred) {
-                items = hashmap_get(p->user_items, UINT32_TO_PTR(filter->ucred->uid));
+        if (filter->gid != (gid_t) -1) {
+                items = hashmap_get(p->group_items, UINT32_TO_PTR(filter->gid));
                 if (items) {
-                        r = check_policy_items(items, filter);
-                        if (r != DUNNO)
-                                return r;
+                        v = check_policy_items(items, filter);
+                        if (v != DUNNO)
+                                verdict = v;
                 }
+        }
 
-                items = hashmap_get(p->group_items, UINT32_TO_PTR(filter->ucred->gid));
+        if (filter->uid != (uid_t) -1) {
+                items = hashmap_get(p->user_items, UINT32_TO_PTR(filter->uid));
                 if (items) {
-                        r = check_policy_items(items, filter);
-                        if (r != DUNNO)
-                                return r;
+                        v = check_policy_items(items, filter);
+                        if (v != DUNNO)
+                                verdict = v;
                 }
         }
 
-        return check_policy_items(p->default_items, filter);
+        v = check_policy_items(p->mandatory_items, filter);
+        if (v != DUNNO)
+                verdict = v;
+
+        return verdict;
 }
 
-bool policy_check_own(Policy *p, const struct ucred *ucred, const char *name) {
+bool policy_check_own(Policy *p, uid_t uid, gid_t gid, const char *name) {
 
         struct policy_check_filter filter = {
                 .class = POLICY_ITEM_OWN,
-                .ucred = ucred,
+                .uid   = uid,
+                .gid   = gid,
                 .name  = name,
         };
 
-        return policy_check(p, &filter) == ALLOW;
+        int verdict;
+
+        assert(p);
+        assert(name);
+
+        verdict = policy_check(p, &filter);
+
+        log_full(LOG_AUTH | (verdict != ALLOW ? LOG_WARNING : LOG_DEBUG),
+                 "Ownership permission check for uid=" UID_FMT " gid=" GID_FMT" name=%s: %s",
+                 uid, gid, strna(name), strna(verdict_to_string(verdict)));
+
+        return verdict == ALLOW;
 }
 
-bool policy_check_hello(Policy *p, const struct ucred *ucred) {
+bool policy_check_hello(Policy *p, uid_t uid, gid_t gid) {
 
         struct policy_check_filter filter = {
-                .ucred  = ucred,
+                .uid = uid,
+                .gid = gid,
         };
-        int user, group;
+        int verdict;
+
+        assert(p);
 
         filter.class = POLICY_ITEM_USER;
-        user = policy_check(p, &filter);
-        if (user == DENY)
-                return false;
+        verdict = policy_check(p, &filter);
+
+        if (verdict != DENY) {
+                int v;
 
-        filter.class = POLICY_ITEM_GROUP;
-        group = policy_check(p, &filter);
-        if (group == DENY)
-                return false;
+                filter.class = POLICY_ITEM_GROUP;
+                v = policy_check(p, &filter);
+                if (v != DUNNO)
+                        verdict = v;
+        }
 
-        return !(user == DUNNO && group == DUNNO);
+        log_full(LOG_AUTH | (verdict != ALLOW ? LOG_WARNING : LOG_DEBUG),
+                 "Hello permission check for uid=" UID_FMT " gid=" GID_FMT": %s",
+                 uid, gid, strna(verdict_to_string(verdict)));
+
+        return verdict == ALLOW;
 }
 
 bool policy_check_recv(Policy *p,
-                       const struct ucred *ucred,
+                       uid_t uid,
+                       gid_t gid,
                        int message_type,
                        const char *name,
                        const char *path,
@@ -782,7 +825,8 @@ bool policy_check_recv(Policy *p,
 
         struct policy_check_filter filter = {
                 .class        = POLICY_ITEM_RECV,
-                .ucred        = ucred,
+                .uid          = uid,
+                .gid          = gid,
                 .message_type = message_type,
                 .name         = name,
                 .interface    = interface,
@@ -790,11 +834,22 @@ bool policy_check_recv(Policy *p,
                 .member       = member,
         };
 
-        return policy_check(p, &filter) == ALLOW;
+        int verdict;
+
+        assert(p);
+
+        verdict = policy_check(p, &filter);
+
+        log_full(LOG_AUTH | (verdict != ALLOW ? LOG_WARNING : LOG_DEBUG),
+                 "Recieve permission check for uid=" UID_FMT " gid=" GID_FMT" message=%s name=%s interface=%s path=%s member=%s: %s",
+                 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;
 }
 
 bool policy_check_send(Policy *p,
-                       const struct ucred *ucred,
+                       uid_t uid,
+                       gid_t gid,
                        int message_type,
                        const char *name,
                        const char *path,
@@ -803,7 +858,8 @@ bool policy_check_send(Policy *p,
 
         struct policy_check_filter filter = {
                 .class        = POLICY_ITEM_SEND,
-                .ucred        = ucred,
+                .uid          = uid,
+                .gid          = gid,
                 .message_type = message_type,
                 .name         = name,
                 .interface    = interface,
@@ -811,7 +867,17 @@ bool policy_check_send(Policy *p,
                 .member       = member,
         };
 
-        return policy_check(p, &filter) == ALLOW;
+        int verdict;
+
+        assert(p);
+
+        verdict = policy_check(p, &filter);
+
+        log_full(LOG_AUTH | (verdict != ALLOW ? LOG_WARNING : LOG_DEBUG),
+                 "Send permission check for uid=" UID_FMT " gid=" GID_FMT" message=%s name=%s interface=%s path=%s member=%s: %s",
+                 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;
 }
 
 int policy_load(Policy *p, char **files) {
@@ -939,6 +1005,7 @@ static void dump_items(PolicyItem *items, const char *prefix) {
                         printf("%sGroup: %s (%d)\n",
                                prefix, strna(group), i->gid);
                 }
+                printf("%s-\n", prefix);
         }
 }