Re: [libvirt] [PATCH] security: also parse user/group names instead of just IDs for DAC labels
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