[libvirt] [RFC] secdrivers remembering original labels

[Michal Prívozník]

On 07/27/2018 11:01 AM, Daniel P. Berrangé wrote:
> On Fri, Jul 27, 2018 at 09:56:23AM +0200, Michal Privoznik wrote:
>> Dear list,
>>
>> we have this very old bug [1] that I just keep pushing in front of me. I
>> made several attempts to fix it. However, none of them made into the
>> tree. I guess it's time to have discussion what to do about it. IIRC,
>> the algorithm that I implemented last was to keep original label in
>> XATTRs (among with some ref counter) and the last one to restore the
>> label will look there and find the original label. There was a problem
>> with two libvirtds fighting over a file on shared FS.
> 
> IIRC there was a second problem around security of XATTRs. eg, if we set
> an XATTR it was possible for QEMU to change it once we given QEMU privs
> on the image. Thus a malicious QEMU can cause libvirtd to change the
> image to an arbitrary user on shutdown.
> 
> I think it was possible to deal with that on Linux, because the kernel
> hsa some xattr namespacing that is reserved for only root. This protection
> is worthless though when using NFS images as you can't trust the remote NFS
> client to honour the same restriction.
> 
>> So I guess my question is can we come up with a design that would work?
>> Or at least work to the extent that we're satisfied with?
>>
>> Personally, I like the XATTRs approach. And to resolve the NFS race we
>> can invent yet another lockspace to guard labelling - I also have a bug
>> for that [2] (although, I'm not that familiar with lockspaces). I guess
>> doing disk metadata locking is not going to be trivial, is it?
> 
> Yeah, for sure we need two distinct locking regimes. Our current locking
> aims to protect disk content, and the lifetime of the lock is the entire
> duration of the QEMU process. For XATTRs, we only need write protection
> for the short time window in which the security drivers execute, which
> is at start, during hotplug/unplug, during shutdown, and finally migration.
> 
> I think we could achieve this with the virtlockd if we make it use the
> same locking file, but a different byte offset within the file. Just
> need to make sure it doesn't clash with the byte offset QEMU has chosen,
> nor the current offset we use.

So do we really need virtlockd for this? I mean, the whole purpose of it
is to hold locks so that libvirtd can restart independently, without
losing the lock attached. However, since the metadata lock will be held
only for a fraction of a second and will be not held through more than a
single API aren't we just fine with libvirtd holding the lock? The way I
imagine this to work is the following:

lock_for_metadata(vm->def); // locks an unique byte in all the disks

qemuSecuritySetAllLabel();

...

qemuProcessStartCPUs();  // disk content lock is acquired here

unlock_for_metadata(vm->def);

Another way to look at it is: it's libvirtd who relabels all the paths
and the metadata lock should guard just that. Speaking of which, we need
these locks to wait for each other, not just set-or-fail. Again,
something that goes against virtlockd view of locking.

Frankly, I've started implementing this with virtlockd already, but the
changes I made so far simply do not feel right, e.g. I have to change
lock_protocol.x so that virLockSpaceProtocolAcquireResourceArgs has this
'type' argument which tells virtlockd which byte we want to lock.

Michal