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@xxxxxxxxxx>
> >>>> ---
> >>>> 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 :|
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
