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

Wed Feb 22 11:38:23 UTC 2023

On Wed, 22 Feb 2023 11:35:16 +0000
Daniel P. Berrangé <berrange at redhat.com> 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 at redhat.com> 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 at redhat.com> 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.

-- 
Stefano