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

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&... >> >> 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.