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

[Michal Prívozník]

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);
  newLabel = context_str(s);

  virCommandSetSELinuxLabel(cmd, newLabel);

  virCommandRun();
}

Michal