[Qemu-devel] [PATCH 0/3] Fix qemu-bridge-helper with SUID

[Qemu-devel] [PATCH 0/3] Fix qemu-bridge-helper with SUID

[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(-)

-- 
2.13.0

[Qemu-devel] [PATCH 1/3] qemu-bridge-helper: Reverse return value setting logic

[Michal Privoznik]

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 <mprivozn@redhat.com>
---
 qemu-bridge-helper.c | 22 +++++-----------------
 1 file changed, 5 insertions(+), 17 deletions(-)

diff --git a/qemu-bridge-helper.c b/qemu-bridge-helper.c
index 5396fbfbb6..af6613ea18 100644
--- a/qemu-bridge-helper.c
+++ b/qemu-bridge-helper.c
@@ -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) {
-- 
2.13.0

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

[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 <mprivozn@redhat.com>
---
 qemu-bridge-helper.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/qemu-bridge-helper.c b/qemu-bridge-helper.c
index af6613ea18..a7f9bf06cc 100644
--- a/qemu-bridge-helper.c
+++ b/qemu-bridge-helper.c
@@ -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)
-- 
2.13.0

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

[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 <mprivozn@redhat.com>
---
 qemu-bridge-helper.c | 44 ++++++++++++++++++++++++++++++--------------
 1 file changed, 30 insertions(+), 14 deletions(-)

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;
         }
     }
 
-- 
2.13.0

Re: [Qemu-devel] [PATCH 0/3] Fix qemu-bridge-helper with SUID

[Michal Privoznik]

On 05/30/2017 10:23 AM, Michal Privoznik wrote:
> 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(-)
> 

ping?

Michal

Re: [Qemu-devel] [PATCH 0/3] Fix qemu-bridge-helper with SUID

[Michal Privoznik]

On 06/22/2017 05:58 PM, Michal Privoznik wrote:
> On 05/30/2017 10:23 AM, Michal Privoznik wrote:
>> 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(-)
>>
> 
> ping?
> 

ping^2?

Michal

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

[Daniel P. Berrange]

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
-- 
|: 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 :|

Re: [Qemu-devel] [PATCH 0/3] Fix qemu-bridge-helper with SUID

[Daniel P. Berrange]

On Tue, Jul 11, 2017 at 03:10:43PM +0200, Michal Privoznik wrote:
> On 06/22/2017 05:58 PM, Michal Privoznik wrote:
> > On 05/30/2017 10:23 AM, Michal Privoznik wrote:
> >> 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(-)
> >>
> > 
> > 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
-- 
|: 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 :|

Re: [Qemu-devel] [PATCH 0/3] Fix qemu-bridge-helper with SUID

[Jason Wang]

On 2017年07月11日 22:54, Daniel P. Berrange wrote:
> On Tue, Jul 11, 2017 at 03:10:43PM +0200, Michal Privoznik wrote:
>> On 06/22/2017 05:58 PM, Michal Privoznik wrote:
>>> On 05/30/2017 10:23 AM, Michal Privoznik wrote:
>>>> 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(-)
>>>>
>>> ping?
>>>
>> ping^2?

Applied.

> 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

>
> Regards,
> Daniel

Re: [Qemu-devel] [PATCH 0/3] Fix qemu-bridge-helper with SUID

[Jason Wang]

On 2017年07月14日 15:31, Jason Wang wrote:
>
>
> On 2017年07月11日 22:54, Daniel P. Berrange wrote:
>> On Tue, Jul 11, 2017 at 03:10:43PM +0200, Michal Privoznik wrote:
>>> On 06/22/2017 05:58 PM, Michal Privoznik wrote:
>>>> On 05/30/2017 10:23 AM, Michal Privoznik wrote:
>>>>> 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(-)
>>>>>
>>>> ping?
>>>>
>>> ping^2?
>
> Applied.

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

Thanks

>
>> 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
>
>>
>> Regards,
>> Daniel
>
>