[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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



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

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;

[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]