Re: [Qemu-devel] [PATCH v2 2/4] Add access control support to qemu bridge helper

[Corey Bryant <coreyb@linux.vnet.ibm.com>]

On 10/23/2011 09:10 AM, Blue Swirl wrote:
> On Fri, Oct 21, 2011 at 15:07, Corey Bryant<coreyb@linux.vnet.ibm.com>  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 an ACL mechanism that is enforced by qemu-bridge-helper.
>> >  The ACLs are fairly simple whitelist/blacklist mechanisms with a wildcard of
>> >  'all'.  All users are blacklisted by default, and deny takes precedence over
>> >  allow.
>> >
>> >  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
> sophisticated
>

Yep, thanks.

>> >  policies via the file system.
>> >
>> >  As an example:
>> >
>> >  /etc/qemu/bridge.conf root:qemu 0640
>> >
>> >    allow br0
>> >    include /etc/qemu/alice.conf
>> >    include /etc/qemu/bob.conf
>> >    include /etc/qemu/charlie.conf
>> >
>> >  /etc/qemu/alice.conf root:alice 0640
>> >    allow br1
>> >
>> >  /etc/qemu/bob.conf root:bob 0640
>> >    allow br2
>> >
>> >  /etc/qemu/charlie.conf root:charlie 0640
>> >    deny all
> I think syntax 'include/etc/qemu/user.d/*.conf' or 'includedir
> /etc/qemu/user.d' could be also useful.
>

That could be useful, though I'm not sure it's necessary right now.

>> >  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.
>> >
>> >  Users in the charlie group cannot get a tap device connected to any bridge.
>> >
>> >  Under no circumstance can the bob group get access to br1 or can the alice
>> >  group get access to br2.  And under no cicumstance can the charlie group
>> >  get access to any bridge.
>> >
>> >  Signed-off-by: Anthony Liguori<aliguori@us.ibm.com>
>> >  Signed-off-by: Richa Marwaha<rmarwah@linux.vnet.ibm.com>
>> >  Signed-off-by: Corey Bryant<coreyb@linux.vnet.ibm.com>
>> >  ---
>> >    qemu-bridge-helper.c |  141 ++++++++++++++++++++++++++++++++++++++++++++++++++
>> >    1 files changed, 141 insertions(+), 0 deletions(-)
>> >
>> >  diff --git a/qemu-bridge-helper.c b/qemu-bridge-helper.c
>> >  index 2ce82fb..db257d5 100644
>> >  --- a/qemu-bridge-helper.c
>> >  +++ b/qemu-bridge-helper.c
>> >  @@ -33,6 +33,105 @@
>> >
>> >    #include "net/tap-linux.h"
>> >
>> >  +#define MAX_ACLS (128)
> If all users (or groups) in the system have an ACL, this number could
> be way too low. Please use a list instead.
>

I agree, we shouldn't be hard-coding the limit here.  I'll update this.

>> >  +#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--;
>> >  +        }
> These while loops to skip spaces are repeated, but the comment
> skipping part is not, so it is not possible to have comments after
> rules or split rules to several lines. I'd add a simple state variable
> to track at which stage we are in parsing instead.
>

That could be useful too, but again not sure it's necessary right now. 
I really like the simplicity we have with the existing approach.

>> >  +        *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 = 0;
>> >  @@ -95,6 +194,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, access_denied;
>> >
>> >       /* parse arguments */
>> >       if (argc<  3 || argc>  4) {
>> >  @@ -115,6 +217,45 @@ 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
>> >  +     * according acl policy if we have a deny and allow both
>> >  +     * then deny should always win over allow
>> >  +     */
>> >  +    access_allowed = 0;
>> >  +    access_denied = 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_denied = 1;
>> >  +            break;
>> >  +        case ACL_DENY:
>> >  +            if (strcmp(bridge, acls[i].iface) == 0) {
>> >  +                access_denied = 1;
>> >  +            }
>> >  +            break;
>> >  +        }
>> >  +    }
>> >  +
>> >  +    if ((access_allowed == 0) || (access_denied == 1)) {
>> >  +        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.7.3.4
>> >
>> >
>> >

-- 
Regards,
Corey