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

[Daniel P. Berrange]

On Thu, Mar 28, 2013 at 01:06:12PM +0100, Michal Privoznik wrote:
> On 28.03.2013 12:52, Daniel P. Berrange wrote:
> > On Thu, Mar 28, 2013 at 12:47:25PM +0100, Michal Privoznik wrote:
> >> On 28.03.2013 12:12, Daniel P. Berrange wrote:
> >>> On Thu, Mar 28, 2013 at 11:38:04AM +0100, Michal Privoznik wrote:
> >>>> On 28.03.2013 10:46, Daniel P. Berrange wrote:
> >>>>> On Thu, Mar 21, 2013 at 05:50:49PM +0100, Michal Privoznik wrote:
> >>>>>>  #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"
> >>>>>
> >>>>> IMHO we don't need the 'trusted.' prefix on these.
> >>>>>
> >>>>> Daniel
> >>>>>
> >>>>
> >>>> An XATTR has to have a prefix. We can choose from several prefixes.
> >>>> attr(5) says:
> >>>>
> >>>>   Currently the security, system, trusted, and user extended attribute
> >>>>   classes are defined as described below. Additional classes may be
> >>>>   added in the future.
> >>>>
> >>>> security - should be used by kernel security modules, such as Security
> >>>> Enhanced Linux. As long as libvirt doesn't provide kernel module, we
> >>>> should not be polluting this prefix.
> >>>>
> >>>> system - used by the kernel to store system objects such as Access
> >>>> Control Lists and Capabilities. Again, we are not kernel.
> >>>>
> >>>> trusted - visible and accessible only to processes that have the
> >>>> CAP_SYS_ADMIN capability (the super user usually has this capability).
> >>>> Attributes  in  this  class are used to implement mechanisms in user
> >>>> space (i.e., outside the kernel) which keep information in extended
> >>>> attributes to which ordinary processes should not have access.
> >>>>
> >>>> Note, that the three above can be touched only by root (or
> >>>> CAP_SYS_ADMIN'ed process).
> >>>>
> >>>> user -  may be assigned to files and directories for storing arbitrary
> >>>> additional information such as the mime type, character set or encoding
> >>>> of a file.
> >>>>
> >>>> The user. can be manipulated by ordinary user.
> >>>>
> >>>> My rationale for not allowing ordinary user to play with our XATTRs is
> >>>> to prevent them chowning to arbitrary user.
> >>>
> >>> Ok, that makes more sense now. I wonder how portable this list of
> >>> prefixes is though - does anyone know if *BSD use the same conventions ?
> >>>
> >>>
> >>> Daniel
> >>>
> >>
> >> Aah. On BSD they support just 'system' and 'user':
> >>
> >> http://svnweb.freebsd.org/base/head/sys/sys/extattr.h?revision=184413&view=markup
> >>
> >> Does it mean we should move from 'trusted' to 'system'? Or is
> >> conditional prefix ('trusted' on linux, 'system' on BSD) sufficient?
> > 
> > You're not able to use 'system.' from userspace.
> > 
> >   # setfattr -n user.eek -v bar foo
> >   # setfattr -n system.eek -v bar foo
> >   setfattr: foo: Operation not supported
> > 
> > So 'user.' is the only option here for BSD. If we consider the (admittedly
> > unlikely) possibility of an NFS server access by 2 libvirt clients one
> > running Linux and one running BSD, we want compatibility. So this says
> > to me that we should use  'user.' as the prefer everywhere.
> 
> I don't think we should use 'user.' at all. It smells of CVE as soon as
> users find out they can effectively let libvirt chown a file for them in
> case ACLs are disabled. So the other solution is to not enable this on
> BSD (and other systems) unless they learn proper XATTRs.

It doesn't let any user do this. They have to have write access to the
files in question, which they will not ordinarily have. That said we're
actually giving QEMU such access, which is a problem.

I don't like the fact that we'd be forced into what is effectively a
Linux only solution for xattrs here. It makes me wonder if our entire
approach here is basically wrong.

We decided on using xattrs, instead of an in-memory record, because we
want the data to be accessible to multiple libvirtd daemons on different
hosts. This does not imply we actually need to store the xattrs on the
files themselves. Perhaps we should have been creating some parallel
files to record the original ownership, which are permanently root
owned.

eg For a file /var/lib/libvirt/images/foo.img add a
/var/lib/libvirt/images/.libvirt.dac.foo.img file to record the original
information.

Or as with the lock manager, just use a single directory like
/var/lib/libvirt/dac/ and create files in there based on the SHA256SUM
of the filename, and declare that you must share that directory between
hosts ?

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 :|