[libvirt] [PATCH v2 0/2] security: support for names on DAC labels
This patch series extends label parsing for DAC security driver and updates the related documentation. Marcelo Cerri (2): security: also parse user/group names instead of just IDs for DAC labels doc: update description about security labels on formatdomain.html docs/formatdomain.html.in | 14 ++++++++-- src/security/security_dac.c | 62 +++++++++++++++++++++++++++++++++++++++------ 2 files changed, 66 insertions(+), 10 deletions(-) -- 1.7.12
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; - 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); + 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); + 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); + 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); + 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); + 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 */ -- 1.7.12
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
This patch adds a brief description about labels for each security driver. --- docs/formatdomain.html.in | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index f05350e..ec57f4d 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -4118,12 +4118,22 @@ qemu-kvm -net nic,model=? /dev/null <dt><code>label</code></dt> <dd>If static labelling is used, this must specify the full security label to assign to the virtual domain. The format - of the content depends on the security driver in use + of the content depends on the security driver in use: + <ul> + <li>SELinux: a SELinux context.</li> + <li>AppArmor: an AppArmor profile.</li> + <li> + DAC: owner and group separated by colon. They can be + defined both as user/group names or uid/gid. The driver will first + try to parse these values as names, but a leading plus sign can + used to force the driver to parse them as uid or gid. + </li> + </ul> </dd> <dt><code>baselabel</code></dt> <dd>If dynamic labelling is used, this can optionally be used to specify the base security label. The format - of the content depends on the security driver in use + of the content depends on the security driver in use. </dd> <dt><code>imagelabel</code></dt> <dd>This is an output only element, which shows the -- 1.7.12
On 09/20/12 23:07, Marcelo Cerri wrote:
This patch adds a brief description about labels for each security driver. --- docs/formatdomain.html.in | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-)
ACK. Peter
Any thoughts on these patches? Regards, Marcelo On Thu, Sep 20, 2012 at 06:07:54PM -0300, Marcelo Cerri wrote:
This patch series extends label parsing for DAC security driver and updates the related documentation.
Marcelo Cerri (2): security: also parse user/group names instead of just IDs for DAC labels doc: update description about security labels on formatdomain.html
docs/formatdomain.html.in | 14 ++++++++-- src/security/security_dac.c | 62 +++++++++++++++++++++++++++++++++++++++------ 2 files changed, 66 insertions(+), 10 deletions(-)
-- 1.7.12
participants (2)
-
Marcelo Cerri -
Peter Krempa