[libvirt] [PATCH v2] storage: support all file permissions

[Laine Stump]

On 08/07/2012 10:40 AM, Dave Allan wrote:
> On Tue, Aug 07, 2012 at 10:27:15AM +0200, Ján Tomko wrote:
>> On 08/07/12 07:17, Laine Stump wrote:
>>> On 07/26/2012 04:52 AM, Ján Tomko wrote:
>>>> sticky, setuid and setgid are no longer ignored.
>>> I'm always automatically wary of any code that allows setting the suid
>>> bit, in case it may allow some new security hole. I can't think of
>>> anything specific that could be allowed by setting the suid bit of a
>>> directory containing a disk image, but then again I haven't thought
>>> about it very hard :-) Can anyone convince me one way or the other?
>> SUID on directories is ignored on most systems, but you could be able to
>> create files belonging a group you're not a member of.
>>
>> But this patch enables it on files too, allowing everyone with access to
>> privileged libvirtd to create SUID files. I don't know if it's possible
>> to exploit this, but I don't like it, so NACK NACK NACK.
>>
>>> It might help to learn why you want to be able to set these bits.
>>> libvirt is generally very specific about explicitly setting the uid of
>>> disk images properly as required at runtime...
>> My issue was that vol-dumpxml reported wrong file permissions, as
>> described in https://bugzilla.redhat.com/show_bug.cgi?id=839463
>>
>> Writing the sticky bit seems harmless to me. 

Yeah, I think I agree with that.

(Heh - I'd never read the history of the sticky bit before (i.e. where
it got the name "sticky") - "retain the text segment
<http://en.wikipedia.org/wiki/Text_segment> of the program in swap space
<http://en.wikipedia.org/wiki/Virtual_memory> after the process
<http://en.wikipedia.org/wiki/Process_%28computing%29> exited". Remember
when loading the text segment of a program took long enough for that to
matter?)

>> Would it be safe to just
>> read SGID and SUID without ever setting them? Or is it not worth the risk?
> IMO we should preserve the existing behavior of not modifying the
> bits, but we should report them correctly, although I don't feel all
> that strongly about it.
>

That sounds reasonable to me, as long as the reported difference only
shows up during a dumpxml of the "live" XML (and not during a
pool-dumpxml --inactive).

I'm wondering if we should generate an error when someone tries to
specify those bits in a pool-define (and vol-define), or just ignore
them as we currently do. (no opinion, just wondering :-)