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

[Daniel P. Berrangé]

On Thu, Dec 06, 2018 at 04:12:45PM +0100, Michal Privoznik wrote:
> 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 at 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
> > 
> >>>> +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.

Yes, that makes sense to me.


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|