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

[Blue Swirl]

On Mon, Oct 24, 2011 at 13:44, Corey Bryant <address@hidden> wrote:
>
>
> On 10/23/2011 09:10 AM, Blue Swirl wrote:
>>
>> On Fri, Oct 21, 2011 at 15:07, Corey Bryant<address@hidden>
>>  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.

It can be added later.

>>> >  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<address@hidden>
>>> >  Signed-off-by: Richa Marwaha<address@hidden>
>>> >  Signed-off-by: Corey Bryant<address@hidden>
>>> >  ---
>>> >    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.

It's not necessary, more like cleanup _if_ it turns out to be even simpler.

>>> >  +        *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
>