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

[Marcelo Cerri]

On Thu, Sep 20, 2012 at 02:53:52PM +0200, Peter Krempa wrote:
> On 09/19/12 23:32, 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>
> >---
> >  src/security/security_dac.c | 49 ++++++++++++++++++++++++++++++++++++---------
> >  1 file changed, 40 insertions(+), 9 deletions(-)
> >
> >diff --git a/src/security/security_dac.c b/src/security/security_dac.c
> >index be65d6e..7e11e31 100644
> >--- a/src/security/security_dac.c
> >+++ b/src/security/security_dac.c
> >@@ -66,28 +66,59 @@ void virSecurityDACSetDynamicOwnership(virSecurityManagerPtr mgr,
> >  }
> >
> >  static
> >+int parseId(const char *str, unsigned int *id)
> >+{
> >+    char *endptr = NULL;
> >+
> >+    if (str == NULL || id == NULL)
> >+        return -1;
> >+
> >+    if (virStrToLong_ui(str, &endptr, 10, id) || endptr != NULL)
> 
> After successful parse "endptr" will point to the '\0' byte at the
> end of the string always thiggering this condition. Call
> virStrToLong_ui with NULL instead of endptr and the wrapper will do
> the expected thing.

You're right.

> 
> >+        return -1;
> >+
> >+    return 0;
> >+}
> >+
> >+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 *sep = NULL;
> >+    char *tmp_label = NULL;
> >
> >      if (label == NULL)
> >-        return -1;
> >+        goto done;
> >
> >-    if (virStrToLong_ui(label, &endptr, 10, &theuid) ||
> >-        endptr == NULL || *endptr != ':') {
> >-        return -1;
> >-    }
> >+    tmp_label = strdup(label);
> >+    if (tmp_label == NULL)
> 
> You need to raise an OOM error with virReportOOMError() here even if
> it's masked by the upper layer. This error gets recorded to the logs
> and might help debugging.

I agree.

> 
> I'm thinking if it's worth reporting other errors in this function
> as they get masked. They might be useful for debugging purposes even
> if they don't get propagated to the user.

Do you think that describing each specific error with VIR_DEBUG should
be enough?

> 
> >+        goto done;
> >
> >-    if (virStrToLong_ui(endptr + 1, NULL, 10, &thegid))
> >-        return -1;
> >+    sep = strchr(tmp_label, ':');
> >+    if (sep == NULL)
> >+        goto done;
> >+    *sep = '\0';
> >+
> >+    if (virGetUserID(tmp_label, &theuid) < 0 &&
> >+        parseId(tmp_label, &theuid) < 0)
> >+        goto done;
> >+
> >+    if (virGetGroupID(sep + 1, &thegid) < 0 &&
> >+        parseId(sep + 1, &thegid) < 0)
> >+        goto done;
> >
> >      if (uidPtr)
> >          *uidPtr = theuid;
> >      if (gidPtr)
> >          *gidPtr = thegid;
> >-    return 0;
> >+
> >+    rc = 0;
> >+
> >+done:
> 
> I'd rename this label to "cleanup" for consistency with other libvirt code.

Ok.

> 
> >+    VIR_FREE(tmp_label);
> >+
> >+    return rc;
> >  }
> >
> >  /* returns 1 if label isn't found, 0 on success, -1 on error */
> >
> 
> Peter
>