[libvirt] [PATCH v2 1/2] security: also parse user/group names instead of just IDs for DAC labels

[Peter Krempa]

On 09/20/12 23:07, Marcelo Cerri wrote:
> The DAC driver is missing parsing of group and user names for DAC labels
> and currently just parses uid and gid. This patch extends it to support
> names, so the following security label definition is now valid:
>
>    <seclabel type='static' model='dac' relabel='yes'>
>        <label>qemu:qemu</label>
>        <imagelabel>qemu:qemu</imagelabel>
>    </seclabel>
>
> When it tries to parse an owner or a group, it first tries to resolve it as
> a name, if it fails or it's an invalid user/group name then it tries to
> parse it as an UID or GID. A leading '+' can also be used for both owner and
> group to force it to be parsed as IDs, so the following example is also
> valid:
>
>    <seclabel type='static' model='dac' relabel='yes'>
>        <label>+101:+101</label>
>        <imagelabel>+101:+101</imagelabel>
>    </seclabel>
>
> This ensures that UID 101 and GUI 101 will be used instead of an user or
> group named "101".
> ---
>   src/security/security_dac.c | 62 +++++++++++++++++++++++++++++++++++++++------
>   1 file changed, 54 insertions(+), 8 deletions(-)
>
> diff --git a/src/security/security_dac.c b/src/security/security_dac.c
> index 5f30f0f..00cbb8a 100644
> --- a/src/security/security_dac.c
> +++ b/src/security/security_dac.c
> @@ -68,26 +68,72 @@ void virSecurityDACSetDynamicOwnership(virSecurityManagerPtr mgr,
>   static
>   int parseIds(const char *label, uid_t *uidPtr, gid_t *gidPtr)
>   {
> +    int rc = -1;
>       unsigned int theuid;
>       unsigned int thegid;
> -    char *endptr = NULL;
> +    char *tmp_label = NULL;
> +    char *sep = NULL;
> +    char *owner = NULL;
> +    char *group = NULL;
>
>       if (label == NULL)
> -        return -1;
> +        goto cleanup;

"label" is guaranteed to be non-null here. Removing this condition will 
prevent you from inventing a new error message for this while changing 
to proper error reporting (see below.).

>
> -    if (virStrToLong_ui(label, &endptr, 10, &theuid) ||
> -        endptr == NULL || *endptr != ':') {
> -        return -1;
> +    tmp_label = strdup(label);
> +    if (tmp_label == NULL) {
> +        virReportOOMError();
> +        goto cleanup;
>       }
>
> -    if (virStrToLong_ui(endptr + 1, NULL, 10, &thegid))
> -        return -1;
> +    /* Split label */
> +    sep = strchr(tmp_label, ':');
> +    if (sep == NULL) {
> +        VIR_DEBUG("Missgin separator ':' in DAC label \"%s\"", label);

s/Missgin/Missing/

Also now when this function has a lot of more detail to report about the 
problems with the security label, I'd change it back to reporting a 
proper error and removing the error in the function that calls this that 
would mas errors from here.

I think a good error code here would be VIR_ERR_INVALID_ARG.

> +        goto cleanup;
> +    }
> +    *sep = '\0';
> +    owner = tmp_label;
> +    group = sep + 1;
> +
> +    /* Parse owner */
> +    if (*owner == '+') {
> +        if (virStrToLong_ui(++owner, NULL, 10, &theuid) < 0) {
> +            VIR_DEBUG("Invalid uid \"%s\" in DAC label \"%s\"", owner, label);

Also this is a nice proper error message.

> +            goto cleanup;
> +        }
> +    } else {
> +        if (virGetUserID(owner, &theuid) < 0 &&
> +            virStrToLong_ui(owner, NULL, 10, &theuid) < 0) {
> +            VIR_DEBUG("Invalid owner \"%s\" in DAC label \"%s\"", owner, label);

Here too.

> +            goto cleanup;
> +        }
> +    }
> +
> +    /* Parse group */
> +    if (*group == '+') {
> +        if (virStrToLong_ui(++group, NULL, 10, &thegid) < 0) {
> +            VIR_DEBUG("Invalid gid \"%s\" in DAC label \"%s\"", group, label);

Here.

> +            goto cleanup;
> +        }
> +    } else {
> +        if (virGetGroupID(group, &thegid) < 0 &&
> +            virStrToLong_ui(group, NULL, 10, &thegid) < 0) {
> +            VIR_DEBUG("Invalid group \"%s\" in DAC label \"%s\"", group, label);

And here :)

> +            goto cleanup;
> +        }
> +    }
>
>       if (uidPtr)
>           *uidPtr = theuid;
>       if (gidPtr)
>           *gidPtr = thegid;
> -    return 0;
> +
> +    rc = 0;
> +
> +cleanup:
> +    VIR_FREE(tmp_label);
> +
> +    return rc;
>   }
>
>   /* returns 1 if label isn't found, 0 on success, -1 on error */
>

When changing the the error messages as I suggest, you will have to 
squash in the attached patch to avoid masking the errors. Otherwise the 
code looks fine and I'm looking forward to review v3.

Peter

-------------- next part --------------
diff --git a/src/security/security_dac.c b/src/security/security_dac.c
index 1268f13..8e72aff 100644
--- a/src/security/security_dac.c
+++ b/src/security/security_dac.c
@@ -153,12 +153,8 @@ int virSecurityDACParseIds(virDomainDefPtr def, uid_t *uidPtr, gid_t *gidPtr)
         return 1;
     }

-    if (seclabel->label && parseIds(seclabel->label, &uid, &gid)) {
-       virReportError(VIR_ERR_INVALID_ARG,
-                       _("failed to parse DAC seclabel '%s' for domain '%s'"),
-                       seclabel->label, def->name);
+    if (parseIds(seclabel->label, &uid, &gid) < 0)
         return -1;
-    }

     if (uidPtr)
         *uidPtr = uid;
@@ -218,13 +214,8 @@ int virSecurityDACParseImageIds(virDomainDefPtr def,
         return 1;
     }

-    if (seclabel->imagelabel
-        && parseIds(seclabel->imagelabel, &uid, &gid)) {
-       virReportError(VIR_ERR_INVALID_ARG,
-                       _("failed to parse DAC imagelabel '%s' for domain '%s'"),
-                       seclabel->imagelabel, def->name);
+    if (parseIds(seclabel->imagelabel, &uid, &gid) < 0)
         return -1;
-    }

     if (uidPtr)
         *uidPtr = uid;