[libvirt] [PATCH 3/3] security_dac: Favour ACLs over chown()

[Daniel P. Berrange]

On Fri, Sep 06, 2013 at 09:50:25AM +0200, Michal Privoznik wrote:
> On 05.09.2013 16:14, Daniel P. Berrange wrote:
> > On Wed, Aug 28, 2013 at 12:27:40PM +0200, Michal Privoznik wrote:
> >> On filesystems supporting ACLs we don't need to do a chown but we
> >> can just set ACLs to gain access for qemu. However, since we are
> >> setting these on too low level, where we don't know if disk is
> >> just a read only or read write, we set read write access
> >> unconditionally.
> >>
> >> >From implementation POV, a reference counter is introduced, so ACL is
> >> restored only on the last restore attempt in order to not cut off other
> >> domains. And since a file may had an ACL for a user already set, we need
> >> to keep this as well. Both these, the reference counter and original ACL
> >> are stored as extended attributes named trusted.libvirt.dac.refCount and
> >> trusted.libvirt.dac.oldACL respectively.
> >>
> >> However, some filesystems doesn't support ACLs, XATTRs, or both. So the
> >> code is made to favour ACLs among with tracking the reference count. If
> >> this fails, we fall back to chown()  with best effort to remember the
> >> original owner of file.
> >>
> >> Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
> >> ---
> >>  src/security/security_dac.c | 297 +++++++++++++++++++++++++++++++++++++++-----
> >>  1 file changed, 268 insertions(+), 29 deletions(-)
> >>
> >> diff --git a/src/security/security_dac.c b/src/security/security_dac.c
> >> index 8cbb083..913b68e 100644
> >> --- a/src/security/security_dac.c
> >> +++ b/src/security/security_dac.c
> >> @@ -37,6 +37,9 @@
> >>  
> >>  #define VIR_FROM_THIS VIR_FROM_SECURITY
> >>  #define SECURITY_DAC_NAME "dac"
> >> +#define SECURITY_DAC_XATTR_OLD_ACL "trusted.libvirt.dac.oldACL"
> >> +#define SECURITY_DAC_XATTR_OLD_OWNER "trusted.libvirt.dac.oldOwner"
> >> +#define SECURITY_DAC_XATTR_REFCOUNT "trusted.libvirt.dac.refCount"
> > 
> > Use of "trusted." is non-portable, per the long mail thread last
> > time this was posted[1]. The discussion there was never resolved in
> > any way, so NACK to this patch as is.
> > 
> > Daniel
> > 
> > [1] https://www.redhat.com/archives/libvir-list/2013-March/msg01610.html
> > 
> 
> Okay then, consider this squashed in:
> 
> diff --git a/src/security/security_dac.c b/src/security/security_dac.c
> index 47af4e8..acc80c4 100644
> --- a/src/security/security_dac.c
> +++ b/src/security/security_dac.c
> @@ -37,9 +37,9 @@
> 
>  #define VIR_FROM_THIS VIR_FROM_SECURITY
>  #define SECURITY_DAC_NAME "dac"
> -#define SECURITY_DAC_XATTR_OLD_ACL "trusted.libvirt.dac.oldACL"
> -#define SECURITY_DAC_XATTR_OLD_OWNER "trusted.libvirt.dac.oldOwner"
> -#define SECURITY_DAC_XATTR_REFCOUNT "trusted.libvirt.dac.refCount"
> +#define SECURITY_DAC_XATTR_OLD_ACL "user.libvirt.dac.oldACL"
> +#define SECURITY_DAC_XATTR_OLD_OWNER "user.libvirt.dac.oldOwner"
> +#define SECURITY_DAC_XATTR_REFCOUNT "user.libvirt.dac.refCount"

In the thread above we determined that this is insecure. We give
QEMU write access to the files, so it would be able to change these
attributes to cause libvirt todo incorrect thing in its security
driver when shutting down.

  https://www.redhat.com/archives/libvir-list/2013-March/msg01612.html

My conclusion was that we can never use xattrs on the disk files
themselves for this, in a secure or portable manner. I suggested
we could possibly use a parallel file to maintain it, but this is
a little harder for block devices. 

  https://www.redhat.com/archives/libvir-list/2013-March/msg01615.html


Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|