Re: [libvirt] [PATCH v2 03/18] security: Include security_util

On 12/6/18 3:34 PM, Daniel P. Berrangé wrote: > On Thu, Dec 06, 2018 at 03:17:47PM +0100, Michal Privoznik wrote: >> 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&gt; >>>> --- >>>> 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 > >>>> +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). > > I wasn't actually suggesting checking accessibility. > > IMHO if you are going to have 2 guests both accessing the same file, > we should simply declare that they *MUST* both use the same label. > > Simply reject the idea that the 2nd guest can change the label, using > a GID_B that magically still allows guest A to access it. Such scenarios > are way more likely to be an admin screw up than an intentional decision. > > As an admin - the guest A might set svirt_image_t:s0:c12,c35. The guest B > then comes along and sets 'svirt_image_t:s0' which grants access to > all guests. This is a clearly a mistake because the first guests' label > was an exclusive access label, while the second guests label was a shared > access label. The lock manager should have blocked this, but I still > think we should also block it here, as the lock manager won't block it > until after you've already set the labels. > > IOW we're justified in requiring strict equality of labels for all > guests, even if they technically might prevent something the kernel would > otherwise allow. Okay then, but this is not the correct place for that check then. In the XATTRs there is stored the original owner, not the current one. If domA is already running, then domB doesn't need to check if its seclabel == original owner rather than if its seclabel == current seclabel. What I can do is to have virSecuritySetRememberedLabel() return the updated value of the ref counter and if it is greater than 1 require the seclabel to be the one that @path currently has (in the caller). I mean, these virSecurity{Get,Set}RememberedLabel APIs really treat seclabels as an opaque data. I rather not have them understand seclabels.