8:17 a.m.
On 12/6/18 12:38 PM, Daniel P. Berrangé wrote:
On Thu, Nov 29, 2018 at 02:52:18PM +0100, Michal Privoznik wrote:
> This file implements wrappers over XATTR getter/setter. It
> ensures the proper XATTR namespace is used.
>
> Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
> ---
> src/security/Makefile.inc.am | 2 +
> src/security/security_util.c | 226 +++++++++++++++++++++++++++++++++++
> src/security/security_util.h | 32 +++++
> 3 files changed, 260 insertions(+)
> create mode 100644 src/security/security_util.c
> create mode 100644 src/security/security_util.h
>
> +/**
> + * virSecurityGetRememberedLabel:
> + * @name: security driver name
> + * @path: file name
> + * @label: label
> + *
> + * For given @path and security driver (@name) fetch remembered
> + * @label. The caller must not restore label if an error is
> + * indicated or if @label is NULL upon return.
> + *
> + * Returns: 0 on success,
> + * -1 otherwise (with error reported)
> + */
> +int
> +virSecurityGetRememberedLabel(const char *name,
> + const char *path,
> + char **label)
> +{
> + char *ref_name = NULL;
> + char *attr_name = NULL;
> + char *value = NULL;
> + unsigned int refcount = 0;
> + int ret = -1;
> +
> + *label = NULL;
> +
> + if (!(ref_name = virSecurityGetRefCountAttrName(name)))
> + goto cleanup;
> +
> + if (virFileGetXAtrr(path, ref_name, &value) < 0) {
> + if (errno == ENOSYS || errno == ENODATA || errno == ENOTSUP) {
> + ret = 0;
> + } else {
> + virReportSystemError(errno,
> + _("Unable to get XATTR %s on %s"),
> + ref_name,
> + path);
> + }
> + goto cleanup;
> + }
> +
> + if (virStrToLong_ui(value, NULL, 10, &refcount) < 0) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("malformed refcount %s on %s"),
> + value, path);
> + goto cleanup;
> + }
> +
> + VIR_FREE(value);
> +
> + refcount--;
> +
> + if (refcount > 0) {
> + if (virAsprintf(&value, "%u", refcount) < 0)
> + goto cleanup;
> +
> + if (virFileSetXAtrr(path, ref_name, value) < 0)
> + goto cleanup;
> + } else {
> + if (virFileRemoveXAttr(path, ref_name) < 0)
> + goto cleanup;
> +
> + if (!(attr_name = virSecurityGetAttrName(name)))
> + goto cleanup;
> +
> + if (virFileGetXAtrr(path, attr_name, label) < 0)
> + goto cleanup;
I'm not understanding why we only fetch the label value in
this half of the conditional ? Shouldn't we be unconditionally
getting the label, regardless of the updated refcount value.
If not, the method description could have an explanation of
intended behaviour.
The idea is that the first time we set a label for a file the owner is
stored in XATTRs and refcount is set to 1. Any other attempt to chown()
will increase the counter (and do the chown() too). Then, restore
decrements the counter and only if it reaches 0 the original owner is
read from XATTRs and passed to secdriver which is a signal to it that it
needs to actually restore the owner.
Anyway, I will put it into a comment.
> +
> + if (virFileRemoveXAttr(path, attr_name) < 0)
> + goto cleanup;
> + }
> +
> + ret = 0;
> + cleanup:
> + VIR_FREE(value);
> + VIR_FREE(attr_name);
> + VIR_FREE(ref_name);
> + return ret;
> +}
> +
> +
> +int
> +virSecuritySetRememberedLabel(const char *name,
> + const char *path,
> + const char *label)
> +{
> + char *ref_name = NULL;
> + char *attr_name = NULL;
> + char *value = NULL;
> + unsigned int refcount = 0;
> + int ret = -1;
> +
> + if (!(ref_name = virSecurityGetRefCountAttrName(name)))
> + goto cleanup;
> +
> + if (virFileGetXAtrr(path, ref_name, &value) < 0) {
> + if (errno == ENOSYS || errno == ENOTSUP) {
> + ret = 0;
> + goto cleanup;
> + } else if (errno != ENODATA) {
> + virReportSystemError(errno,
> + _("Unable to get XATTR %s on %s"),
> + ref_name,
> + path);
> + goto cleanup;
> + }
> + }
> +
> + if (value &&
> + virStrToLong_ui(value, NULL, 10, &refcount) < 0) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("malformed refcount %s on %s"),
> + value, path);
> + goto cleanup;
> + }
> +
> + VIR_FREE(value);
> +
> + refcount++;
> +
> + if (refcount == 1) {
> + if (!(attr_name = virSecurityGetAttrName(name)))
> + goto cleanup;
> +
> + if (virFileSetXAtrr(path, attr_name, label) < 0)
> + goto cleanup;
> + }
Do we need to have a
} else {
.... check the currently remember label matches
what was passed into this method ?
}
if not, can you add API docs for this method explainng the
intended semantics when label is already remembered.
I don't think that's a good idea because that sets us off onto a path
where we'd have to determine whether a file is accessible or not. I
mean, if the current owner is UID_A:GID_A (and qemu has access through
both) and this new label passed into here is UID_B:GID_B that doesn't
necessarily mean that qemu loses the access (UID_A can be a member of
GID_B).
Sure, this doesn't prevent us from the following scenario:
1) set a seclabel for domA on path X
2) user tries to start domB with a different seclabel which also
requires path X
3) while starting domB, libvirt overwrites seclabel on X effectively
cutting of domA
4) imagine domB can't be started (or is destroyed later or whatever),
libvirt starts restore, but since X's refcount = 1, the original owner
is not restored (domA is still cut off)
But I don't see any easy solution to that. I mean:
a) As I say we don't want to check in 3) if the new seclabel would cut
off domA. That's for kernel to decide.
b) If we would want to rollback in 4) and set domA seclabel on X we
would need a very clever algorithm. We would need to store an array of
seclabels in XATTR and match them with domains that are doing the
restore. For instance: there are three domains domA, domB and domC and
assume they all share path X and have their own seclabels defined
(A:UID, B:UID, C:UID). Lets start them all and the XATTR on X would look
like this:
{[original owner],[A:UID],[B:UID]} (and the X is currently owned by C:UID)
Now, domA is shut off. How would I know what item in the array to
remove? Note that [A:UID] was added when starting domB, and in fact
[original owner] was added by domA.
Michal