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

[Michal Privoznik]

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.

Michal

> 
> Regards,
> Daniel
>