[libvirt] [PATCH v3 0/2] security: support for names on DAC labels
This patch series extends label parsing for DAC security driver and updates the related documentation. This version basically uses virReportError instead of VIR_DEBUG for error messages. 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 | 92 ++++++++++++++++++++++++++++++++++----------- 2 files changed, 82 insertions(+), 24 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 | 92 ++++++++++++++++++++++++++++++++++----------- 1 file changed, 70 insertions(+), 22 deletions(-) diff --git a/src/security/security_dac.c b/src/security/security_dac.c index 0fa66ce..bb2c6be 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -68,26 +68,79 @@ 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; + tmp_label = strdup(label); + if (tmp_label == NULL) { + virReportOOMError(); + goto cleanup; + } + + /* Split label */ + sep = strchr(tmp_label, ':'); + if (sep == NULL) { + virReportError(VIR_ERR_INVALID_ARG, + _("Missing separator ':' in DAC label \"%s\""), + label); + goto cleanup; + } + *sep = '\0'; + owner = tmp_label; + group = sep + 1; - if (virStrToLong_ui(label, &endptr, 10, &theuid) || - endptr == NULL || *endptr != ':') { - return -1; + /* Parse owner */ + if (*owner == '+') { + if (virStrToLong_ui(++owner, NULL, 10, &theuid) < 0) { + virReportError(VIR_ERR_INVALID_ARG, + _("Invalid uid \"%s\" in DAC label \"%s\""), + owner, label); + goto cleanup; + } + } else { + if (virGetUserID(owner, &theuid) < 0 && + virStrToLong_ui(owner, NULL, 10, &theuid) < 0) { + virReportError(VIR_ERR_INVALID_ARG, + _("Invalid owner \"%s\" in DAC label \"%s\""), + owner, label); + goto cleanup; + } } - if (virStrToLong_ui(endptr + 1, NULL, 10, &thegid)) - return -1; + /* Parse group */ + if (*group == '+') { + if (virStrToLong_ui(++group, NULL, 10, &thegid) < 0) { + virReportError(VIR_ERR_INVALID_ARG, + _("Invalid gid \"%s\" in DAC label \"%s\""), + group, label); + goto cleanup; + } + } else { + if (virGetGroupID(group, &thegid) < 0 && + virStrToLong_ui(group, NULL, 10, &thegid) < 0) { + virReportError(VIR_ERR_INVALID_ARG, + _("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 */ @@ -103,16 +156,14 @@ int virSecurityDACParseIds(virDomainDefPtr def, uid_t *uidPtr, gid_t *gidPtr) seclabel = virDomainDefGetSecurityLabelDef(def, SECURITY_DAC_NAME); if (seclabel == NULL || seclabel->label == NULL) { - VIR_DEBUG("DAC seclabel for domain '%s' wasn't found", def->name); + virReportError(VIR_ERR_INVALID_ARG, + _("DAC seclabel for domain '%s' wasn't found"), + def->name); 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; @@ -168,17 +219,14 @@ int virSecurityDACParseImageIds(virDomainDefPtr def, seclabel = virDomainDefGetSecurityLabelDef(def, SECURITY_DAC_NAME); if (seclabel == NULL || seclabel->imagelabel == NULL) { - VIR_DEBUG("DAC imagelabel for domain '%s' wasn't found", def->name); + virReportError(VIR_ERR_INVALID_ARG, + _("DAC imagelabel for domain '%s' wasn't found"), + def->name); 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; -- 1.7.12
On 10/02/12 19:57, 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 | 92 ++++++++++++++++++++++++++++++++++----------- 1 file changed, 70 insertions(+), 22 deletions(-)
diff --git a/src/security/security_dac.c b/src/security/security_dac.c index 0fa66ce..bb2c6be 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -68,26 +68,79 @@ void virSecurityDACSetDynamicOwnership(virSecurityManagerPtr mgr, static int parseIds(const char *label, uid_t *uidPtr, gid_t *gidPtr) {
/* returns 1 if label isn't found, 0 on success, -1 on error */ @@ -103,16 +156,14 @@ int virSecurityDACParseIds(virDomainDefPtr def, uid_t *uidPtr, gid_t *gidPtr)
seclabel = virDomainDefGetSecurityLabelDef(def, SECURITY_DAC_NAME); if (seclabel == NULL || seclabel->label == NULL) { - VIR_DEBUG("DAC seclabel for domain '%s' wasn't found", def->name); + virReportError(VIR_ERR_INVALID_ARG, + _("DAC seclabel for domain '%s' wasn't found"), + def->name);
This error will be masked so the VIR_DEBUG statement was fine here.
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; @@ -168,17 +219,14 @@ int virSecurityDACParseImageIds(virDomainDefPtr def,
seclabel = virDomainDefGetSecurityLabelDef(def, SECURITY_DAC_NAME); if (seclabel == NULL || seclabel->imagelabel == NULL) { - VIR_DEBUG("DAC imagelabel for domain '%s' wasn't found", def->name); + virReportError(VIR_ERR_INVALID_ARG, + _("DAC imagelabel for domain '%s' wasn't found"), + def->name);
And same here.
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; - }
Otherwise looks good. So I'm going to push this with following changes squashed in: diff --git a/src/security/security_dac.c b/src/security/security_dac.c index bb2c6be..a427e9d 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -156,9 +156,7 @@ int virSecurityDACParseIds(virDomainDefPtr def, uid_t *uidPtr, gid_t *gidPtr) seclabel = virDomainDefGetSecurityLabelDef(def, SECURITY_DAC_NAME); if (seclabel == NULL || seclabel->label == NULL) { - virReportError(VIR_ERR_INVALID_ARG, - _("DAC seclabel for domain '%s' wasn't found"), - def->name); + VIR_DEBUG("DAC seclabel for domain '%s' wasn't found", def->name); return 1; } @@ -219,9 +217,7 @@ int virSecurityDACParseImageIds(virDomainDefPtr def, seclabel = virDomainDefGetSecurityLabelDef(def, SECURITY_DAC_NAME); if (seclabel == NULL || seclabel->imagelabel == NULL) { - virReportError(VIR_ERR_INVALID_ARG, - _("DAC imagelabel for domain '%s' wasn't found"), - def->name); + VIR_DEBUG("DAC imagelabel for domain '%s' wasn't found", def->name); return 1; } Peter
On 10/02/2012 11:57 AM, 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>
Yuck. With this patch, I'm seeing lots of ugly error messages in the log: 2012-10-04 22:59:52.584+0000: 9225: error : virGetUserID:2535 : Failed to find user record for name '0': Success I think the correct fix is to move this logic...
+ /* Parse owner */ + if (*owner == '+') { + if (virStrToLong_ui(++owner, NULL, 10, &theuid) < 0) { + virReportError(VIR_ERR_INVALID_ARG, + _("Invalid uid \"%s\" in DAC label \"%s\""), + owner, label); + goto cleanup; + } + } else { + if (virGetUserID(owner, &theuid) < 0 && + virStrToLong_ui(owner, NULL, 10, &theuid) < 0) { + virReportError(VIR_ERR_INVALID_ARG, + _("Invalid owner \"%s\" in DAC label \"%s\""), + owner, label); + goto cleanup; + } }
...out of security_dac.c and into src/util/util.c:virGetUserID(), so that we are consistently parsing in this manner for ALL places where we convert a string into a user id, and also so that virGetUserID will quit logging such a bogus error message when it fails to find a given id string that happens to be a valid number. Likewise for virGetGroupID. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On Thu, Oct 04, 2012 at 05:46:31PM -0600, Eric Blake wrote:
On 10/02/2012 11:57 AM, 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>
Yuck. With this patch, I'm seeing lots of ugly error messages in the log:
2012-10-04 22:59:52.584+0000: 9225: error : virGetUserID:2535 : Failed to find user record for name '0': Success
I think the correct fix is to move this logic...
+ /* Parse owner */ + if (*owner == '+') { + if (virStrToLong_ui(++owner, NULL, 10, &theuid) < 0) { + virReportError(VIR_ERR_INVALID_ARG, + _("Invalid uid \"%s\" in DAC label \"%s\""), + owner, label); + goto cleanup; + } + } else { + if (virGetUserID(owner, &theuid) < 0 && + virStrToLong_ui(owner, NULL, 10, &theuid) < 0) { + virReportError(VIR_ERR_INVALID_ARG, + _("Invalid owner \"%s\" in DAC label \"%s\""), + owner, label); + goto cleanup; + } }
...out of security_dac.c and into src/util/util.c:virGetUserID(), so that we are consistently parsing in this manner for ALL places where we convert a string into a user id, and also so that virGetUserID will quit logging such a bogus error message when it fails to find a given id string that happens to be a valid number. Ok. I'll provide a patch for this.
I made a search in the code for virGetUserID and, other than in security_dac.c, it seems to be used just for parsing user name in qemu.conf.
Likewise for virGetGroupID.
Same for virGetGroupID.
-- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Just one question: do you think that makes sense for virGetUserID and virGetGroupID to check if a id exists when a numeric value is given? Regards, Marcelo On Fri, Oct 05, 2012 at 09:44:46AM -0300, Marcelo Cerri wrote:
On Thu, Oct 04, 2012 at 05:46:31PM -0600, Eric Blake wrote:
On 10/02/2012 11:57 AM, 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>
Yuck. With this patch, I'm seeing lots of ugly error messages in the log:
2012-10-04 22:59:52.584+0000: 9225: error : virGetUserID:2535 : Failed to find user record for name '0': Success
I think the correct fix is to move this logic...
+ /* Parse owner */ + if (*owner == '+') { + if (virStrToLong_ui(++owner, NULL, 10, &theuid) < 0) { + virReportError(VIR_ERR_INVALID_ARG, + _("Invalid uid \"%s\" in DAC label \"%s\""), + owner, label); + goto cleanup; + } + } else { + if (virGetUserID(owner, &theuid) < 0 && + virStrToLong_ui(owner, NULL, 10, &theuid) < 0) { + virReportError(VIR_ERR_INVALID_ARG, + _("Invalid owner \"%s\" in DAC label \"%s\""), + owner, label); + goto cleanup; + } }
...out of security_dac.c and into src/util/util.c:virGetUserID(), so that we are consistently parsing in this manner for ALL places where we convert a string into a user id, and also so that virGetUserID will quit logging such a bogus error message when it fails to find a given id string that happens to be a valid number. Ok. I'll provide a patch for this.
I made a search in the code for virGetUserID and, other than in security_dac.c, it seems to be used just for parsing user name in qemu.conf.
Likewise for virGetGroupID.
Same for virGetGroupID.
-- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 10/05/2012 06:53 AM, Marcelo Cerri wrote:
Just one question: do you think that makes sense for virGetUserID and virGetGroupID to check if a id exists when a numeric value is given?
Just because a uid hasn't been named in /etc/passwd doesn't mean it won't be added in the future. If the user requests a numeric value, I think we have to respect it as a uid_t value (assuming the name lookup failed or the leading '+' was present) regardless of whether we can do a lookup on that uid. One other thing - uid_t might be smaller than 'int', so we have to do overflow checking and make sure that a user requesting 65536 doesn't end up mapping to uid 0.
...out of security_dac.c and into src/util/util.c:virGetUserID(), so that we are consistently parsing in this manner for ALL places where we convert a string into a user id, and also so that virGetUserID will quit logging such a bogus error message when it fails to find a given id string that happens to be a valid number. Ok. I'll provide a patch for this.
I made a search in the code for virGetUserID and, other than in security_dac.c, it seems to be used just for parsing user name in qemu.conf.
I'm actually in favor of this change - it means qemu.conf also gains our consistent behavior (and consistent with coreutils' chown) for handling names vs. numbers. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
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 bc4cc4a..3cc6c51 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -4126,12 +4126,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 10/02/12 19:57, 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 && pushed. Peter
participants (3)
-
Eric Blake -
Marcelo Cerri -
Peter Krempa