diff mbox

[3/3] qemu-bridge-helper: Take ACL file gid into account

Message ID f8c84196122adc32f9067680e54a9de32a571756.1496132443.git.mprivozn@redhat.com (mailing list archive)
State New, archived
Headers show

Commit Message

Michal Privoznik May 30, 2017, 8:23 a.m. UTC
There's a problem with the way we currently parse ACL files. The
problem is, the bridge helper has usually SUID flag set and thus
runs as root in which case all the ACL files are parsed
(/etc/qemu/bridge.conf and all other files it includes).
Therefore, if there's say bob.conf owned by root:bob and I run
the bridge helper, it doesn't really matter whether I'm in the
bob group or not. Everything that is allowed to bob group is
allowed to me too.

The way this problem is fixed is whenever an ACL file is parsed,
the group owner of the file is stored among with the rules so
that it can be compared when evaluating.

There is one exception though. If an ACL file is owned by root
group the rules within apply to all groups. This is because
that's the usual setup currently (the bridge.conf is usually
owned by root:root) and anybody from root group can plug the
interface themselves anyway.

This idea of groups was introduced in bdef79a2994d6f0383 but got
broken by ditros setting SUID onto the binary.

Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
---
 qemu-bridge-helper.c | 44 ++++++++++++++++++++++++++++++--------------
 1 file changed, 30 insertions(+), 14 deletions(-)

Comments

Daniel P. Berrangé July 11, 2017, 2:08 p.m. UTC | #1
On Tue, May 30, 2017 at 10:23:35AM +0200, Michal Privoznik wrote:
> There's a problem with the way we currently parse ACL files. The
> problem is, the bridge helper has usually SUID flag set and thus
> runs as root in which case all the ACL files are parsed
> (/etc/qemu/bridge.conf and all other files it includes).
> Therefore, if there's say bob.conf owned by root:bob and I run
> the bridge helper, it doesn't really matter whether I'm in the
> bob group or not. Everything that is allowed to bob group is
> allowed to me too.

Ok so the ACL feature relies on the qemu-bridge-helper not having
privileges to read the files included from the master bridge.conf
file.

The easy way for this to work is if the qemu-bridge-helper binary
is given a filesystem capability eg

   setcap cap_net_admin=ep qemu-bridge-helper

in this mode, the bridge helper always runs with uid=500, gid=500,
so does not have permission to read included files which are group
owned by a different user.

If using SUID mode, the bridge helper would be given a custom
group and SUID privs

   chown root.qemu qemu-bridge-helper
   chmod ug+s qemu-bridge-helper

when it runs, it initially has effective uid=0 and effective gid=qemu

It then uses libcapng to drop privileges and change user ID, so that
it ends up with only cap_net_admin, and effective uid=500 and
effective gid=500 again.

So once again, it will be unable to read the included files which
are group owned by a different user.

IOW, I think everything is working normally.

The only scenario in which you can run SUID, and permissions do
not work would be if you compiled QEMU with libcapng support
disabled.

It would be wise to change the bridge helper to refuse to read
any included files with uid != 0 or gid != 0, if libcapng
support is disabled, so make it clear that permission checking
is inoperative.

Or better yet, just make libcapng mandatory when using the
bridge helper.

> The way this problem is fixed is whenever an ACL file is parsed,
> the group owner of the file is stored among with the rules so
> that it can be compared when evaluating.
> 
> There is one exception though. If an ACL file is owned by root
> group the rules within apply to all groups. This is because
> that's the usual setup currently (the bridge.conf is usually
> owned by root:root) and anybody from root group can plug the
> interface themselves anyway.
> 
> This idea of groups was introduced in bdef79a2994d6f0383 but got
> broken by ditros setting SUID onto the binary.

I don't believe it did, unless distros left out libcapng support
when building it.


Regards,
Daniel
diff mbox

Patch

diff --git a/qemu-bridge-helper.c b/qemu-bridge-helper.c
index a7f9bf06cc..eab9ad5096 100644
--- a/qemu-bridge-helper.c
+++ b/qemu-bridge-helper.c
@@ -48,6 +48,7 @@  enum {
 
 typedef struct ACLRule {
     int type;
+    gid_t group;
     char iface[IFNAMSIZ];
     QSIMPLEQ_ENTRY(ACLRule) entry;
 } ACLRule;
@@ -65,8 +66,13 @@  static int parse_acl_file(const char *filename, ACLList *acl_list)
     FILE *f;
     char line[4096];
     ACLRule *acl_rule;
+    struct stat stbuf;
     int ret = -1;
 
+    if (stat(filename, &stbuf) < 0) {
+        return -1;
+    }
+
     f = fopen(filename, "r");
     if (f == NULL) {
         return -1;
@@ -117,6 +123,7 @@  static int parse_acl_file(const char *filename, ACLList *acl_list)
                 acl_rule->type = ACL_DENY;
                 snprintf(acl_rule->iface, IFNAMSIZ, "%s", arg);
             }
+            acl_rule->group = stbuf.st_gid;
             QSIMPLEQ_INSERT_TAIL(acl_list, acl_rule, entry);
         } else if (strcmp(cmd, "allow") == 0) {
             acl_rule = g_malloc(sizeof(*acl_rule));
@@ -126,6 +133,7 @@  static int parse_acl_file(const char *filename, ACLList *acl_list)
                 acl_rule->type = ACL_ALLOW;
                 snprintf(acl_rule->iface, IFNAMSIZ, "%s", arg);
             }
+            acl_rule->group = stbuf.st_gid;
             QSIMPLEQ_INSERT_TAIL(acl_list, acl_rule, entry);
         } else if (strcmp(cmd, "include") == 0) {
             /* ignore errors */
@@ -229,6 +237,7 @@  int main(int argc, char **argv)
     ACLRule *acl_rule;
     ACLList acl_list;
     int access_allowed, access_denied;
+    gid_t group;
     int rv, ret = EXIT_FAILURE;
 
 #ifdef CONFIG_LIBCAP
@@ -275,24 +284,31 @@  int main(int argc, char **argv)
      */
     access_allowed = 0;
     access_denied = 0;
+    group = getgid();
     QSIMPLEQ_FOREACH(acl_rule, &acl_list, entry) {
-        switch (acl_rule->type) {
-        case ACL_ALLOW_ALL:
-            access_allowed = 1;
-            break;
-        case ACL_ALLOW:
-            if (strcmp(bridge, acl_rule->iface) == 0) {
+        /* If the acl policy file is owned by root group it
+         * applies to all groups. Otherwise it applies to that
+         * specific group. */
+        if (!acl_rule->group ||
+            (acl_rule->group && acl_rule->group == group)) {
+            switch (acl_rule->type) {
+            case ACL_ALLOW_ALL:
                 access_allowed = 1;
-            }
-            break;
-        case ACL_DENY_ALL:
-            access_denied = 1;
-            break;
-        case ACL_DENY:
-            if (strcmp(bridge, acl_rule->iface) == 0) {
+                break;
+            case ACL_ALLOW:
+                if (strcmp(bridge, acl_rule->iface) == 0) {
+                    access_allowed = 1;
+                }
+                break;
+            case ACL_DENY_ALL:
                 access_denied = 1;
+                break;
+            case ACL_DENY:
+                if (strcmp(bridge, acl_rule->iface) == 0) {
+                    access_denied = 1;
+                }
+                break;
             }
-            break;
         }
     }