Re: [PATCH 1/3] qemu_passt: Don't make passt transition to svirt_t/virt_domain on start

On Wed, 22 Feb 2023 09:46:42 +0000 Daniel P. Berrangé <berrange(a)redhat.com&gt; wrote: > On Tue, Feb 21, 2023 at 10:49:46PM +0100, Stefano Brivio wrote: >> On Tue, 21 Feb 2023 19:43:33 +0000 >> Daniel P. Berrangé <berrange(a)redhat.com&gt; wrote: >> >>> On Tue, Feb 21, 2023 at 08:19:05PM +0100, Stefano Brivio wrote: >>>> qemuSecurityCommandRun() causes an explicit domain transition of the >>>> new process, but passt ships with its own SELinux policy, with >>>> external interfaces for libvirtd, so we simply need to transition >>>> from virtd_t to passt_t as passt is executed. The qemu type >>>> enforcement rules have little to do with it. >>> >>> Can you clarify the difference here ? >> >> Between...? >> >> I mean, virCommandRun() will just keep things running under virtd_t, so >> that passt later can transition to passt_t. >> >> With qemuSecurityCommandRun(), there would be a transition from virtd_t >> to svirt_t (it's the function that's called to start qemu, but >> shouldn't be called here), and not to passt_t. >> >> But I'm not really sure that's what you were asking for. > > Yes, it is. > >> >>> Runing passt under 'svirt_t' is not desirable as that allows >>> many actions that are only relevant to QEMU. >> >> Right, that's what this patch avoids. There are also actions, such as >> starting passt or killing it, that we don't want to allow QEMU to do. >> >>> Running passt under the MCS label that is associated with the >>> VM is highly desirable though. Two passt instances belonging >>> to separate VMs are isolated from each other if they each use >>> the VM specific MCS label, than if they use the global default >>> MCS label. >>> >>> To use the VM specific MCS label would require libvirt to >>> explicitly set the desired selinux label on exec, it can't >>> happen automatically via an SELinux transition rule. >>> >>> We do stil want to use the passt_t type though. >>> >>> IOW, if we have a VM running >>> >>> svirt_t:s0:c710,c716 >>> >>> IMHO we would its corresponding passt instance to be >>> running >>> >>> passt_t:s0:c710,c716 >>> >>> >>> not >>> >>> passt_t:s0:c0.c1023 >> >> Practically speaking, it doesn't make a huge difference for passt >> because it unshares any relevant namespace right after it starts -- >> that's *in theory* a strictly stronger isolation compared to what >> SELinux provides (at least once we reach the main loop). > > Even docker/podman will apply SELinux isolation per container, > rather than only relying on namespaces. Sure, I'm not saying it's not desirable -- but still, many (most?) host-facing services they rely on are not isolated in this sense. Same for the current implementation of libvirt with dnsmasq, for example. >> But it makes sense, and I guess we should relabel to a specific MCS >> with still 'virtd_t' as a type, then later the domain would transition >> to passt_t. This could probably be done as an extension of >> qemuSecurityCommandRun(), I haven't looked into it yet. I will. >> >> Anyway, right now, I think this provides better security than >> 'setenforce 0', which is the only way to run passt from libvirt at the >> moment on some distributions. > > If running with SELinux permissive, this patch has no effect, as it > is unconfined regardless. That's not a situation we care about. If > people want to run without protection they get to keep the pieces > when it all goes wrong. The current implementation simply does not and cannot work with SELinux in enforcing mode, so users have no other choice. >> I'm not sure how you handle these cases on libvirt, but generally >> speaking, this patch is a vast improvement on the current situation, >> and I can follow up with an extension or a different version of >> qemuSecurityCommandRun() later. > > No, I don't think it is a vast improvement. > > The goal of sVirt is to guarantee isolation between VMs. > > Running passt under svirt_t:MCS is not ideal because the svirt_t > policy allows things that passt should not get. It does still > guarantee isolation between VMs, because the MCS label is present. It's a bit more than that: it's not ideal because libvirt simply won't start passt. There's no isolation and no VMs. > Switching to running passt_t:c0.c1023 will be more correct in terms > of what permissions passt should be allowed, but it has disabled > isolation of passt between VMs. This is a clear degradation of > capabilities from the POV of sVirt's goals. It's not a degradation because VMs can't start passt with SELinux in enforcing mode, without this patch. No service, no degradation. I looked into options to rework virSecurityManagerSetChildProcessLabel() and friends with a more flexible modeling/building of labels -- just doing some trick all the way down from qemuSecurityCommandRun() implies a number of layering violations that I would like to avoid. It all looks doable, but implementing the type of functionality/API that's currently missing there isn't a small rework -- some refactoring of interfaces is definitely needed. I started, but it's not quick. So for the moment being I would suggest that, if passt can't be relabeled as passt_t (i.e. if this patch or equivalent can't be applied), the whole passt back-end should be dropped.