Fix qemu-bridge-helper with SUID

PATCH Applied Michal Privoznik 3 participants 10 messages May 30, 2017 – Jul 14, 2017

Share & tools

Michal Privoznik

author

2017-05-30 08:23 UTC #0 lore raw

cover Fix qemu-bridge-helper with SUID

For more description see patch 3. Long story short, if the bridge helper runs with SUID, the mechanism we rely on (DAC denying access to ACL files) does not work.

Michal Privoznik (3):
qemu-bridge-helper: Reverse return value setting logic
qemu-bridge-helper: Reverse return value setting logic in
parse_acl_file
qemu-bridge-helper: Take ACL file gid into account

 qemu-bridge-helper.c | 79 ++++++++++++++++++++++++++++------------------------
 1 file changed, 42 insertions(+), 37 deletions(-)

Signature · 1 line

2.13.0

Michal Privoznik

author

2017-05-30 08:23 UTC #1 lore raw

PATCH 1/3 qemu-bridge-helper: Reverse return value setting logic

Instead of initializing the return value @ret to a success value and then having to reset it to failure value in every error path, we can do the opposite. Initialize the value to failure value and then set it to success value only after we've succeeded in all we've attempted.

Signed-off-by Michal Privoznik <[email protected]>

---
 qemu-bridge-helper.c | 22 +++++-----------------
 1 file changed, 5 insertions(+), 17 deletions(-)

qemu-bridge-helper.c +5 ~~-17~~

index 5396fbfbb6..af6613ea18 100644
@@ -228,7 +228,7 @@ int main(int argc, char **argv)
     ACLRule *acl_rule;
     ACLList acl_list;
     int access_allowed, access_denied;
-    int ret = EXIT_SUCCESS;
+    int rv, ret = EXIT_FAILURE;
 
 #ifdef CONFIG_LIBCAP
     /* if we're run from an suid binary, immediately drop privileges preserving

@@ -265,7 +265,6 @@ int main(int argc, char **argv)
     if (parse_acl_file(DEFAULT_ACL_FILE, &acl_list) == -1) {
         fprintf(stderr, "failed to parse default acl file `%s'\n",
                 DEFAULT_ACL_FILE);
-        ret = EXIT_FAILURE;
         goto cleanup;
     }

@@ -298,7 +297,6 @@ int main(int argc, char **argv)
 
     if ((access_allowed == 0) || (access_denied == 1)) {
         fprintf(stderr, "access denied by acl file\n");
-        ret = EXIT_FAILURE;
         goto cleanup;
     }

@@ -306,7 +304,6 @@ int main(int argc, char **argv)
     ctlfd = socket(AF_INET, SOCK_STREAM, 0);
     if (ctlfd == -1) {
         fprintf(stderr, "failed to open control socket: %s\n", strerror(errno));
-        ret = EXIT_FAILURE;
         goto cleanup;
     }

@@ -314,7 +311,6 @@ int main(int argc, char **argv)
     fd = open("/dev/net/tun", O_RDWR);
     if (fd == -1) {
         fprintf(stderr, "failed to open /dev/net/tun: %s\n", strerror(errno));
-        ret = EXIT_FAILURE;
         goto cleanup;
     }

@@ -328,7 +324,6 @@ int main(int argc, char **argv)
 
     if (ioctl(fd, TUNSETIFF, &ifr) == -1) {
         fprintf(stderr, "failed to create tun device: %s\n", strerror(errno));
-        ret = EXIT_FAILURE;
         goto cleanup;
     }

@@ -340,7 +335,6 @@ int main(int argc, char **argv)
     if (ioctl(ctlfd, SIOCGIFMTU, &ifr) == -1) {
         fprintf(stderr, "failed to get mtu of bridge `%s': %s\n",
                 bridge, strerror(errno));
-        ret = EXIT_FAILURE;
         goto cleanup;
     }

@@ -353,7 +347,6 @@ int main(int argc, char **argv)
     if (ioctl(ctlfd, SIOCSIFMTU, &ifr) == -1) {
         fprintf(stderr, "failed to set mtu of device `%s' to %d: %s\n",
                 iface, mtu, strerror(errno));
-        ret = EXIT_FAILURE;
         goto cleanup;
     }

@@ -364,14 +357,12 @@ int main(int argc, char **argv)
     if (ioctl(ctlfd, SIOCGIFHWADDR, &ifr) < 0) {
         fprintf(stderr, "failed to get MAC address of device `%s': %s\n",
                 iface, strerror(errno));
-        ret = EXIT_FAILURE;
         goto cleanup;
     }
     ifr.ifr_hwaddr.sa_data[0] = 0xFE;
     if (ioctl(ctlfd, SIOCSIFHWADDR, &ifr) < 0) {
         fprintf(stderr, "failed to set MAC address of device `%s': %s\n",
                 iface, strerror(errno));
-        ret = EXIT_FAILURE;
         goto cleanup;
     }

@@ -384,15 +375,14 @@ int main(int argc, char **argv)
     ifargs[2] = 0;
     ifargs[3] = 0;
     ifr.ifr_data = (void *)ifargs;
-    ret = ioctl(ctlfd, SIOCDEVPRIVATE, &ifr);
+    rv = ioctl(ctlfd, SIOCDEVPRIVATE, &ifr);
 #else
     ifr.ifr_ifindex = ifindex;
-    ret = ioctl(ctlfd, SIOCBRADDIF, &ifr);
+    rv = ioctl(ctlfd, SIOCBRADDIF, &ifr);
 #endif
-    if (ret == -1) {
+    if (rv == -1) {
         fprintf(stderr, "failed to add interface `%s' to bridge `%s': %s\n",
                 iface, bridge, strerror(errno));
-        ret = EXIT_FAILURE;
         goto cleanup;
     }

@@ -401,7 +391,6 @@ int main(int argc, char **argv)
     if (ioctl(ctlfd, SIOCGIFFLAGS, &ifr) == -1) {
         fprintf(stderr, "failed to get interface flags for `%s': %s\n",
                 iface, strerror(errno));
-        ret = EXIT_FAILURE;
         goto cleanup;
     }

@@ -409,7 +398,6 @@ int main(int argc, char **argv)
     if (ioctl(ctlfd, SIOCSIFFLAGS, &ifr) == -1) {
         fprintf(stderr, "failed to bring up interface `%s': %s\n",
                 iface, strerror(errno));
-        ret = EXIT_FAILURE;
         goto cleanup;
     }

@@ -417,13 +405,13 @@ int main(int argc, char **argv)
     if (send_fd(unixfd, fd) == -1) {
         fprintf(stderr, "failed to write fd to unix socket: %s\n",
                 strerror(errno));
-        ret = EXIT_FAILURE;
         goto cleanup;
     }
 
     /* ... */
 
     /* profit! */
+    ret = EXIT_SUCCESS;
 
 cleanup:
     if (fd >= 0) {

Signature · 1 line

2.13.0

Michal Privoznik

author

2017-05-30 08:23 UTC #2 lore raw

PATCH 2/3 qemu-bridge-helper: Reverse return value setting logic in parse_acl_file

In reply to Michal Privoznik

Just like in the previous commit, it's better to have just a single point of exit from a function as we can have cleanup code just once instead of copying it all over the place.

Signed-off-by Michal Privoznik <[email protected]>

---
 qemu-bridge-helper.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

qemu-bridge-helper.c +7 -6

index af6613ea18..a7f9bf06cc 100644
@@ -65,6 +65,7 @@ static int parse_acl_file(const char *filename, ACLList *acl_list)
     FILE *f;
     char line[4096];
     ACLRule *acl_rule;
+    int ret = -1;
 
     f = fopen(filename, "r");
     if (f == NULL) {

@@ -92,9 +93,8 @@ static int parse_acl_file(const char *filename, ACLList *acl_list)
 
         if (arg == NULL) {
             fprintf(stderr, "Invalid config line:\n  %s\n", line);
-            fclose(f);
             errno = EINVAL;
-            return -1;
+            goto cleanup;
         }
 
         *arg = 0;

@@ -132,15 +132,16 @@ static int parse_acl_file(const char *filename, ACLList *acl_list)
             parse_acl_file(arg, acl_list);
         } else {
             fprintf(stderr, "Unknown command `%s'\n", cmd);
-            fclose(f);
             errno = EINVAL;
-            return -1;
+            goto cleanup;
         }
     }
 
-    fclose(f);
+    ret = 0;
 
-    return 0;
+ cleanup:
+    fclose(f);
+    return ret;
 }
 
 static bool has_vnet_hdr(int fd)

Signature · 1 line

2.13.0

Michal Privoznik

author

2017-05-30 08:23 UTC #3 lore raw

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

In reply to Michal Privoznik

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 <[email protected]>

---
 qemu-bridge-helper.c | 44 ++++++++++++++++++++++++++++++--------------
 1 file changed, 30 insertions(+), 14 deletions(-)

qemu-bridge-helper.c +30 ~~-14~~

index a7f9bf06cc..eab9ad5096 100644
@@ -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;
         }
     }

Signature · 1 line

2.13.0

Daniel P. Berrange

2017-07-11 14:08 UTC #4 lore raw

Re: [Qemu-devel] [PATCH 3/3] qemu-bridge-helper: Take ACL file gid into account

Quoted 8 lines · Michal Privoznik

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.

Quoted 4 lines

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.

Quoted 8 lines

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

Signature · 3 lines

|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Michal Privoznik

author

2017-06-22 15:58 UTC #5 lore raw

In reply to Michal Privoznik

Quoted 4 lines · Michal Privoznik

For more description see patch 3. Long story short, if the bridge helper runs with SUID, the mechanism we rely on (DAC denying access to ACL files) does not work.

Quoted 9 lines

Michal Privoznik (3):
qemu-bridge-helper: Reverse return value setting logic
qemu-bridge-helper: Reverse return value setting logic in
parse_acl_file
qemu-bridge-helper: Take ACL file gid into account
 qemu-bridge-helper.c | 79 ++++++++++++++++++++++++++++------------------------
 1 file changed, 42 insertions(+), 37 deletions(-)

ping?

Michal

Michal Privoznik

author

2017-07-11 13:10 UTC #6 lore raw

Quoted 11 lines · Michal Privoznik

Quoted 10 lines · Michal Privoznik
For more description see patch 3. Long story short, if the bridge helper runs with SUID, the mechanism we rely on (DAC denying access to ACL files) does not work.
Michal Privoznik (3):
qemu-bridge-helper: Reverse return value setting logic
qemu-bridge-helper: Reverse return value setting logic in
parse_acl_file
qemu-bridge-helper: Take ACL file gid into account

Quoted 6 lines

Quoted 3 lines

 qemu-bridge-helper.c | 79 ++++++++++++++++++++++++++++------------------------
 1 file changed, 42 insertions(+), 37 deletions(-)

ping?

ping^2?

Michal

Daniel P. Berrange

2017-07-11 14:55 UTC #7 lore raw

Quoted 12 lines · Michal Privoznik

Quoted 11 lines · Michal Privoznik
Quoted 10 lines · Michal Privoznik
For more description see patch 3. Long story short, if the bridge helper runs with SUID, the mechanism we rely on (DAC denying access to ACL files) does not work.
Michal Privoznik (3):
qemu-bridge-helper: Reverse return value setting logic
qemu-bridge-helper: Reverse return value setting logic in
parse_acl_file
qemu-bridge-helper: Take ACL file gid into account

Quoted 8 lines

Quoted 6 lines

Quoted 3 lines

 qemu-bridge-helper.c | 79 ++++++++++++++++++++++++++++------------------------
 1 file changed, 42 insertions(+), 37 deletions(-)

ping?

ping^2?

Sigh, this is one of the files for which we have no nominated maintainer listed, so it easily falls through the cracks.

Since this is network related, I wonder if Jason should be listed in the MAINTAINERS file for this. Or perhaps we should move the qemu-bridge-helper.c file into the net/ sub-directory instead ?

Regards,
Daniel

Signature · 3 lines

|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Jason Wang

Applied

2017-07-14 07:32 UTC #8 lore raw

Quoted 13 lines · Daniel P. Berrange

Quoted 12 lines · Michal Privoznik
Quoted 11 lines · Michal Privoznik
Quoted 10 lines · Michal Privoznik
For more description see patch 3. Long story short, if the bridge helper runs with SUID, the mechanism we rely on (DAC denying access to ACL files) does not work.
Michal Privoznik (3):
qemu-bridge-helper: Reverse return value setting logic
qemu-bridge-helper: Reverse return value setting logic in
parse_acl_file
qemu-bridge-helper: Take ACL file gid into account

Quoted 6 lines

Quoted 5 lines

Quoted 3 lines

  qemu-bridge-helper.c | 79 ++++++++++++++++++++++++++++------------------------
  1 file changed, 42 insertions(+), 37 deletions(-)

ping?

ping^2?

Applied.

Quoted 6 lines

Sigh, this is one of the files for which we have no nominated maintainer listed, so it easily falls through the cracks.
Since this is network related, I wonder if Jason should be listed in the MAINTAINERS file for this. Or perhaps we should move the qemu-bridge-helper.c file into the net/ sub-directory instead ?

Let me claim this in MAINTAINERS.

Thanks

Quoted 3 lines

Regards,
Daniel

Jason Wang

2017-07-14 07:40 UTC #9 lore raw

Quoted 20 lines · Jason Wang

Quoted 17 lines · Daniel P. Berrange
Quoted 16 lines · Michal Privoznik
Quoted 15 lines · Michal Privoznik
Quoted 14 lines · Michal Privoznik
For more description see patch 3. Long story short, if the bridge helper runs with SUID, the mechanism we rely on (DAC denying access to ACL files) does not work.
Michal Privoznik (3):
qemu-bridge-helper: Reverse return value setting logic
qemu-bridge-helper: Reverse return value setting logic in
parse_acl_file
qemu-bridge-helper: Take ACL file gid into account
  qemu-bridge-helper.c | 79 
++++++++++++++++++++++++++++------------------------
  1 file changed, 42 insertions(+), 37 deletions(-)

Quoted 5 lines

Quoted 3 lines
Quoted 3 lines
Quoted 2 lines
ping?
ping^2?
Applied.

Just notice Daniel's comment. Michal, can you please address that?

Thanks

Quoted 17 lines

Quoted 7 lines
Sigh, this is one of the files for which we have no nominated maintainer listed, so it easily falls through the cracks.
Since this is network related, I wonder if Jason should be listed in the MAINTAINERS file for this. Or perhaps we should move the qemu-bridge-helper.c file into the net/ sub-directory instead ?
Let me claim this in MAINTAINERS.
Thanks
Quoted 3 lines
Regards,
Daniel