Patchwork [2/4] Add access control support to qemu-bridge-helper

login
register
mail settings
Submitter Anthony Liguori
Date Nov. 4, 2009, 12:28 a.m.
Message ID <1257294485-27015-3-git-send-email-aliguori@us.ibm.com>
Download mbox | patch
Permalink /patch/37541/
State New
Headers show

Comments

Anthony Liguori - Nov. 4, 2009, 12:28 a.m.
We go to great lengths to restrict ourselves to just cap_net_admin as an OS
enforced security mechanism.  However, we further restrict what we allow users
to do to simply adding a tap device to a bridge interface by virtue of the fact
that this is the only functionality we expose.

This is not good enough though.  An administrator is likely to want to restrict
the bridges that an unprivileged user can access, in particular, to restrict
an unprivileged user from putting a guest on what should be isolated networks.

This patch implements a ACL mechanism that is enforced by qemu-bridge-helper.
The ACLs are fairly simple whitelist/blacklist mechanisms with a wildcard of
'all'.

An interesting feature of this ACL mechanism is that you can include external
ACL files.  The main reason to support this is so that you can set different
file system permissions on those external ACL files.  This allows an
administrator to implement rather sophisicated ACL policies based on user/group
policies via the file system.

If we fail to include an acl file, we are silent about it making this mechanism
work pretty seamlessly.  As an example:

/etc/qemu/bridge.conf root:qemu 0640

 deny all
 allow br0
 include /etc/qemu/alice.conf
 include /etc/qemu/bob.conf

/etc/qemu/alice.conf root:alice 0640
 allow br1

/etc/qemu/bob.conf root:bob 0640
 allow br2

This ACL pattern allows any user in the qemu group to get a tap device
connected to br0 (which is bridged to the physical network).

Users in the alice group can additionally get a tap device connected to br1.
This allows br1 to act as a private bridge for the alice group.

Users in the bob group can additionally get a tap device connected to br2.
This allows br2 to act as a private bridge for the bob group.

Under no circumstance can the bob group get access to br1 or can the alice
group get access to br2.

Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
---
 configure            |    1 +
 qemu-bridge-helper.c |  138 ++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 139 insertions(+), 0 deletions(-)
Krumme, Chris - Nov. 4, 2009, 1:38 p.m.
Hello Anthony,

Cool patch series.
 

> -----Original Message-----
> From: 
> qemu-devel-bounces+chris.krumme=windriver.com@nongnu.org 
> [mailto:qemu-devel-bounces+chris.krumme=windriver.com@nongnu.o
> rg] On Behalf Of Anthony Liguori
> Sent: Tuesday, November 03, 2009 6:28 PM
> To: qemu-devel@nongnu.org
> Cc: Mark McLoughlin; Anthony Liguori; Arnd Bergmann; Dustin 
> Kirkland; Michael Tsirkin; Juan Quintela
> Subject: [Qemu-devel] [PATCH 2/4] Add access control support 
> toqemu-bridge-helper
> 
> We go to great lengths to restrict ourselves to just 
> cap_net_admin as an OS
> enforced security mechanism.  However, we further restrict 
> what we allow users
> to do to simply adding a tap device to a bridge interface by 
> virtue of the fact
> that this is the only functionality we expose.
> 
> This is not good enough though.  An administrator is likely 
> to want to restrict
> the bridges that an unprivileged user can access, in 
> particular, to restrict
> an unprivileged user from putting a guest on what should be 
> isolated networks.
> 
> This patch implements a ACL mechanism that is enforced by 
> qemu-bridge-helper.
> The ACLs are fairly simple whitelist/blacklist mechanisms 
> with a wildcard of
> 'all'.
> 
> An interesting feature of this ACL mechanism is that you can 
> include external
> ACL files.  The main reason to support this is so that you 
> can set different
> file system permissions on those external ACL files.  This allows an
> administrator to implement rather sophisicated ACL policies 
> based on user/group
> policies via the file system.
> 
> If we fail to include an acl file, we are silent about it 
> making this mechanism
> work pretty seamlessly.  As an example:
> 
> /etc/qemu/bridge.conf root:qemu 0640
> 
>  deny all
>  allow br0
>  include /etc/qemu/alice.conf
>  include /etc/qemu/bob.conf
> 
> /etc/qemu/alice.conf root:alice 0640
>  allow br1
> 
> /etc/qemu/bob.conf root:bob 0640
>  allow br2
> 
> This ACL pattern allows any user in the qemu group to get a tap device
> connected to br0 (which is bridged to the physical network).
> 
> Users in the alice group can additionally get a tap device 
> connected to br1.
> This allows br1 to act as a private bridge for the alice group.
> 
> Users in the bob group can additionally get a tap device 
> connected to br2.
> This allows br2 to act as a private bridge for the bob group.
> 
> Under no circumstance can the bob group get access to br1 or 
> can the alice
> group get access to br2.
> 
> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
> ---
>  configure            |    1 +
>  qemu-bridge-helper.c |  138 
> ++++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 139 insertions(+), 0 deletions(-)
> 
> diff --git a/configure b/configure
> index a341e77..7c98257 100755
> --- a/configure
> +++ b/configure
> @@ -1864,6 +1864,7 @@ printf " '%s'" "$0" "$@" >> $config_host_mak
>  echo >> $config_host_mak
>  
>  echo "CONFIG_QEMU_SHAREDIR=\"$prefix$datasuffix\"" >> 
> $config_host_mak
> +echo "CONFIG_QEMU_CONFDIR=\"/etc/qemu\"" >> $config_host_mak
>  
>  case "$cpu" in
>    
> i386|x86_64|alpha|cris|hppa|ia64|m68k|microblaze|mips|mips64|p
> pc|ppc64|s390|sparc|sparc64)
> diff --git a/qemu-bridge-helper.c b/qemu-bridge-helper.c
> index f10d37c..0d059ed 100644
> --- a/qemu-bridge-helper.c
> +++ b/qemu-bridge-helper.c
> @@ -33,6 +33,106 @@
>  
>  #include "net/tap-linux.h"
>  
> +#define MAX_ACLS (128)
> +#define DEFAULT_ACL_FILE CONFIG_QEMU_CONFDIR "/bridge.conf"
> +
> +enum {
> +    ACL_ALLOW = 0,
> +    ACL_ALLOW_ALL,
> +    ACL_DENY,
> +    ACL_DENY_ALL,
> +};
> +
> +typedef struct ACLRule
> +{
> +    int type;
> +    char iface[IFNAMSIZ];
> +} ACLRule;
> +
> +static int parse_acl_file(const char *filename, ACLRule 
> *acls, int *pacl_count)
> +{
> +    int acl_count = *pacl_count;
> +    FILE *f;
> +    char line[4096];
> +
> +    f = fopen(filename, "r");
> +    if (f == NULL) {
> +        return -1;
> +    }
> +
> +    while (acl_count != MAX_ACLS &&
> +           fgets(line, sizeof(line), f) != NULL) {
> +        char *ptr = line;
> +        char *cmd, *arg, *argend;
> +
> +        while (isspace(*ptr)) {
> +            ptr++;
> +        }
> +
> +        /* skip comments and empty lines */
> +        if (*ptr == '#' || *ptr == 0) {
> +            continue;
> +        }
> +
> +        cmd = ptr;
> +        arg = strchr(cmd, ' ');
> +        if (arg == NULL) {
> +            arg = strchr(cmd, '\t');
> +        }
> +
> +        if (arg == NULL) {
> +            fprintf(stderr, "Invalid config line:\n  %s\n", line);
> +            fclose(f);
> +            errno = EINVAL;
> +            return -1;
> +        }
> +
> +        *arg = 0;

No check is made for arg being in bounds.

Thanks

Chris

> +        arg++;
> +        while (isspace(*arg)) {
> +            arg++;
> +        }
> +
> +        argend = arg + strlen(arg);
> +        while (arg != argend && isspace(*(argend - 1))) {
> +            argend--;
> +        }
> +        *argend = 0;
> +
> +        if (strcmp(cmd, "deny") == 0) {
> +            if (strcmp(arg, "all") == 0) {
> +                acls[acl_count].type = ACL_DENY_ALL;
> +            } else {
> +                acls[acl_count].type = ACL_DENY;
> +                snprintf(acls[acl_count].iface, IFNAMSIZ, "%s", arg);
> +            }
> +            acl_count++;
> +        } else if (strcmp(cmd, "allow") == 0) {
> +            if (strcmp(arg, "all") == 0) {
> +                acls[acl_count].type = ACL_ALLOW_ALL;
> +            } else {
> +                acls[acl_count].type = ACL_ALLOW;
> +                snprintf(acls[acl_count].iface, IFNAMSIZ, "%s", arg);
> +            }
> +            acl_count++;
> +        } else if (strcmp(cmd, "include") == 0) {
> +            /* ignore errors */
> +            parse_acl_file(arg, acls, &acl_count);
> +        } else {
> +            fprintf(stderr, "Unknown command `%s'\n", cmd);
> +            fclose(f);
> +            errno = EINVAL;
> +            return -1;
> +        }
> +    }
> +
> +    *pacl_count = acl_count;
> +
> +    fclose(f);
> +
> +    return 0;
> +}
> +
>  static int has_vnet_hdr(int fd)
>  {
>      unsigned int features;
> @@ -95,6 +195,9 @@ int main(int argc, char **argv)
>      const char *bridge;
>      char iface[IFNAMSIZ];
>      int index;
> +    ACLRule acls[MAX_ACLS];
> +    int acl_count = 0;
> +    int i, access_allowed;
>  
>      /* parse arguments */
>      if (argc < 3 || argc > 4) {
> @@ -115,6 +218,41 @@ int main(int argc, char **argv)
>      bridge = argv[index++];
>      unixfd = atoi(argv[index++]);
>  
> +    /* parse default acl file */
> +    if (parse_acl_file(DEFAULT_ACL_FILE, acls, &acl_count) == -1) {
> +        fprintf(stderr, "failed to parse default acl file `%s'\n",
> +                DEFAULT_ACL_FILE);
> +        return -errno;
> +    }
> +
> +    /* validate bridge against acl -- default policy is to deny */
> +    access_allowed = 0;
> +    for (i = 0; i < acl_count; i++) {
> +        switch (acls[i].type) {
> +        case ACL_ALLOW_ALL:
> +            access_allowed = 1;
> +            break;
> +        case ACL_ALLOW:
> +            if (strcmp(bridge, acls[i].iface) == 0) {
> +                access_allowed = 1;
> +            }
> +            break;
> +        case ACL_DENY_ALL:
> +            access_allowed = 0;
> +            break;
> +        case ACL_DENY:
> +            if (strcmp(bridge, acls[i].iface) == 0) {
> +                access_allowed = 0;
> +            }
> +            break;
> +        }
> +    }
> +
> +    if (access_allowed == 0) {
> +        fprintf(stderr, "access denied by acl file\n");
> +        return -EPERM;
> +    }
> +
>      /* open a socket to use to control the network interfaces */
>      ctlfd = socket(AF_INET, SOCK_STREAM, 0);
>      if (ctlfd == -1) {
> -- 
> 1.6.2.5
> 
> 
> 
>
Anthony Liguori - Nov. 4, 2009, 2:23 p.m.
Krumme, Chris wrote:
> Hello Anthony,
>
> Cool patch series.
>   

Thanks.

>> +        cmd = ptr;
>> +        arg = strchr(cmd, ' ');
>> +        if (arg == NULL) {
>> +            arg = strchr(cmd, '\t');
>> +        }
>> +
>> +        if (arg == NULL) {
>> +            fprintf(stderr, "Invalid config line:\n  %s\n", line);
>> +            fclose(f);
>> +            errno = EINVAL;
>> +            return -1;
>> +        }
>> +
>> +        *arg = 0;
>>     
>
> No check is made for arg being in bounds.
>   

I don't get it.  arg is either going to be NULL (no ' ' or '\t' found in 
the string) or it will point to the first ' ' or '\t' in the string.  It 
will always be in bound in this second case and the first case is 
handled by the if().

Regards,

Anthony Liguori
Krumme, Chris - Nov. 4, 2009, 2:37 p.m.
Hello Anthony, 

> -----Original Message-----
> From: Anthony Liguori [mailto:anthony@codemonkey.ws] 
> Sent: Wednesday, November 04, 2009 8:23 AM
> To: Krumme, Chris
> Cc: qemu-devel@nongnu.org; Mark McLoughlin; Arnd Bergmann; 
> Michael Tsirkin; Juan Quintela; Dustin Kirkland
> Subject: Re: [Qemu-devel] [PATCH 2/4] Add access control 
> support toqemu-bridge-helper
> 
> Krumme, Chris wrote:
> > Hello Anthony,
> >
> > Cool patch series.
> >   
> 
> Thanks.
> 
> >> +        cmd = ptr;
> >> +        arg = strchr(cmd, ' ');
> >> +        if (arg == NULL) {
> >> +            arg = strchr(cmd, '\t');
> >> +        }
> >> +
> >> +        if (arg == NULL) {
> >> +            fprintf(stderr, "Invalid config line:\n  %s\n", line);
> >> +            fclose(f);
> >> +            errno = EINVAL;
> >> +            return -1;
> >> +        }
> >> +
> >> +        *arg = 0;
> >>     
> >
> > No check is made for arg being in bounds.
> >   
> 
> I don't get it.  arg is either going to be NULL (no ' ' or 
> '\t' found in 
> the string) or it will point to the first ' ' or '\t' in the 
> string.  It 

My concern is that the first space or tab may not be in the first
sizeof(line) characters.

Thanks

Chris

> will always be in bound in this second case and the first case is 
> handled by the if().
> 
> Regards,
> 
> Anthony Liguori
>
Daniel P. Berrange - Nov. 5, 2009, 3:06 p.m.
On Tue, Nov 03, 2009 at 06:28:03PM -0600, Anthony Liguori wrote:
> We go to great lengths to restrict ourselves to just cap_net_admin as an OS
> enforced security mechanism.  However, we further restrict what we allow users
> to do to simply adding a tap device to a bridge interface by virtue of the fact
> that this is the only functionality we expose.
> 
> This is not good enough though.  An administrator is likely to want to restrict
> the bridges that an unprivileged user can access, in particular, to restrict
> an unprivileged user from putting a guest on what should be isolated networks.
> 
> This patch implements a ACL mechanism that is enforced by qemu-bridge-helper.
> The ACLs are fairly simple whitelist/blacklist mechanisms with a wildcard of
> 'all'.
> 
> An interesting feature of this ACL mechanism is that you can include external
> ACL files.  The main reason to support this is so that you can set different
> file system permissions on those external ACL files.  This allows an
> administrator to implement rather sophisicated ACL policies based on user/group
> policies via the file system.
> 
> If we fail to include an acl file, we are silent about it making this mechanism
> work pretty seamlessly.  As an example:
> 
> /etc/qemu/bridge.conf root:qemu 0640
> 
>  deny all
>  allow br0
>  include /etc/qemu/alice.conf
>  include /etc/qemu/bob.conf
> 
> /etc/qemu/alice.conf root:alice 0640
>  allow br1
> 
> /etc/qemu/bob.conf root:bob 0640
>  allow br2
> 
> This ACL pattern allows any user in the qemu group to get a tap device
> connected to br0 (which is bridged to the physical network).
> 
> Users in the alice group can additionally get a tap device connected to br1.
> This allows br1 to act as a private bridge for the alice group.
> 
> Users in the bob group can additionally get a tap device connected to br2.
> This allows br2 to act as a private bridge for the bob group.
> 
> Under no circumstance can the bob group get access to br1 or can the alice
> group get access to br2.

If we're going to define an ACL file for this, then I'd like us to
try and get a file format that is suitable for all possible ACL
needs in QEMU. In particular to allow coverage of VNC server ACLs
which I previously did a proof of concept for

http://article.gmane.org/gmane.comp.emulators.qemu/38173

Daniel

Patch

diff --git a/configure b/configure
index a341e77..7c98257 100755
--- a/configure
+++ b/configure
@@ -1864,6 +1864,7 @@  printf " '%s'" "$0" "$@" >> $config_host_mak
 echo >> $config_host_mak
 
 echo "CONFIG_QEMU_SHAREDIR=\"$prefix$datasuffix\"" >> $config_host_mak
+echo "CONFIG_QEMU_CONFDIR=\"/etc/qemu\"" >> $config_host_mak
 
 case "$cpu" in
   i386|x86_64|alpha|cris|hppa|ia64|m68k|microblaze|mips|mips64|ppc|ppc64|s390|sparc|sparc64)
diff --git a/qemu-bridge-helper.c b/qemu-bridge-helper.c
index f10d37c..0d059ed 100644
--- a/qemu-bridge-helper.c
+++ b/qemu-bridge-helper.c
@@ -33,6 +33,106 @@ 
 
 #include "net/tap-linux.h"
 
+#define MAX_ACLS (128)
+#define DEFAULT_ACL_FILE CONFIG_QEMU_CONFDIR "/bridge.conf"
+
+enum {
+    ACL_ALLOW = 0,
+    ACL_ALLOW_ALL,
+    ACL_DENY,
+    ACL_DENY_ALL,
+};
+
+typedef struct ACLRule
+{
+    int type;
+    char iface[IFNAMSIZ];
+} ACLRule;
+
+static int parse_acl_file(const char *filename, ACLRule *acls, int *pacl_count)
+{
+    int acl_count = *pacl_count;
+    FILE *f;
+    char line[4096];
+
+    f = fopen(filename, "r");
+    if (f == NULL) {
+        return -1;
+    }
+
+    while (acl_count != MAX_ACLS &&
+           fgets(line, sizeof(line), f) != NULL) {
+        char *ptr = line;
+        char *cmd, *arg, *argend;
+
+        while (isspace(*ptr)) {
+            ptr++;
+        }
+
+        /* skip comments and empty lines */
+        if (*ptr == '#' || *ptr == 0) {
+            continue;
+        }
+
+        cmd = ptr;
+        arg = strchr(cmd, ' ');
+        if (arg == NULL) {
+            arg = strchr(cmd, '\t');
+        }
+
+        if (arg == NULL) {
+            fprintf(stderr, "Invalid config line:\n  %s\n", line);
+            fclose(f);
+            errno = EINVAL;
+            return -1;
+        }
+
+        *arg = 0;
+        arg++;
+        while (isspace(*arg)) {
+            arg++;
+        }
+
+        argend = arg + strlen(arg);
+        while (arg != argend && isspace(*(argend - 1))) {
+            argend--;
+        }
+        *argend = 0;
+
+        if (strcmp(cmd, "deny") == 0) {
+            if (strcmp(arg, "all") == 0) {
+                acls[acl_count].type = ACL_DENY_ALL;
+            } else {
+                acls[acl_count].type = ACL_DENY;
+                snprintf(acls[acl_count].iface, IFNAMSIZ, "%s", arg);
+            }
+            acl_count++;
+        } else if (strcmp(cmd, "allow") == 0) {
+            if (strcmp(arg, "all") == 0) {
+                acls[acl_count].type = ACL_ALLOW_ALL;
+            } else {
+                acls[acl_count].type = ACL_ALLOW;
+                snprintf(acls[acl_count].iface, IFNAMSIZ, "%s", arg);
+            }
+            acl_count++;
+        } else if (strcmp(cmd, "include") == 0) {
+            /* ignore errors */
+            parse_acl_file(arg, acls, &acl_count);
+        } else {
+            fprintf(stderr, "Unknown command `%s'\n", cmd);
+            fclose(f);
+            errno = EINVAL;
+            return -1;
+        }
+    }
+
+    *pacl_count = acl_count;
+
+    fclose(f);
+
+    return 0;
+}
+
 static int has_vnet_hdr(int fd)
 {
     unsigned int features;
@@ -95,6 +195,9 @@  int main(int argc, char **argv)
     const char *bridge;
     char iface[IFNAMSIZ];
     int index;
+    ACLRule acls[MAX_ACLS];
+    int acl_count = 0;
+    int i, access_allowed;
 
     /* parse arguments */
     if (argc < 3 || argc > 4) {
@@ -115,6 +218,41 @@  int main(int argc, char **argv)
     bridge = argv[index++];
     unixfd = atoi(argv[index++]);
 
+    /* parse default acl file */
+    if (parse_acl_file(DEFAULT_ACL_FILE, acls, &acl_count) == -1) {
+        fprintf(stderr, "failed to parse default acl file `%s'\n",
+                DEFAULT_ACL_FILE);
+        return -errno;
+    }
+
+    /* validate bridge against acl -- default policy is to deny */
+    access_allowed = 0;
+    for (i = 0; i < acl_count; i++) {
+        switch (acls[i].type) {
+        case ACL_ALLOW_ALL:
+            access_allowed = 1;
+            break;
+        case ACL_ALLOW:
+            if (strcmp(bridge, acls[i].iface) == 0) {
+                access_allowed = 1;
+            }
+            break;
+        case ACL_DENY_ALL:
+            access_allowed = 0;
+            break;
+        case ACL_DENY:
+            if (strcmp(bridge, acls[i].iface) == 0) {
+                access_allowed = 0;
+            }
+            break;
+        }
+    }
+
+    if (access_allowed == 0) {
+        fprintf(stderr, "access denied by acl file\n");
+        return -EPERM;
+    }
+
     /* open a socket to use to control the network interfaces */
     ctlfd = socket(AF_INET, SOCK_STREAM, 0);
     if (ctlfd == -1) {