On Wed, Feb 22, 2023 at 12:38:23PM +0100, Stefano Brivio wrote: > On Wed, 22 Feb 2023 11:35:16 +0000 > Daniel P. Berrangé <berrange@xxxxxxxxxx> wrote: > > > On Wed, Feb 22, 2023 at 12:21:09PM +0100, Michal Prívozník wrote: > > > On 2/22/23 11:05, Stefano Brivio wrote: > > > > On Wed, 22 Feb 2023 09:46:42 +0000 > > > > Daniel P. Berrangé <berrange@xxxxxxxxxx> 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@xxxxxxxxxx> 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. > > > > > > > > > I don't think we need such drastic measure. I think you can use: > > > > > > qemuPasstStart() > > > { > > > > > > > > > seclabel = virDomainDefGetSecurityLabelDef(vm->def, "selinux"); > > > s = context_new(seclabel->label); > > > context_type_set(s, "virt_t); > > > > I presume you meant "passt_t" here. > > No, that needs to be "virtd_t" for the right domain transition to > happen. The transitions only happen on execve(). We're running in libvirtd (as virtd_t:c0.c1023) and will execve() passt (needs to be passt_t:c243,c2353). If we use virtd_t, then when passt is exec'd it'll get virtd_t:c243,c2353. Without a second execve it'll never transition onto passt_t:c243,c2353. We have no need to rely on automatic transitions when we know the target label we wish to use upfront. So we should be using passt_t:c243,c2353 with virCommand With regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|