passt SELinux labelling (was: Re: [PATCH v2 1/3] qemu_passt: Don't make passt transition to svirt_t/libvirt_domain on start)

[Laine Stump]

On 2/23/23 5:47 AM, Daniel P. Berrangé wrote:
> On Thu, Feb 23, 2023 at 11:40:00AM +0100, Jiri Denemark wrote:
>> On Wed, Feb 22, 2023 at 14:21:29 +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.
>>>
>>> That is, if we switch to svirt_t, passt will run in the security
>>> context that's intended for QEMU, which allows a number of
>>> operations not needed by passt. On the other hand, with a switch
>>> to svirt_t, passt won't be able to create its own PID file.
>>>
>>> Usage of those new interfaces is implemented by this change in
>>> selinux-policy:
>>>    https://github.com/fedora-selinux/selinux-policy/pull/1613
>>>
>>> Replace qemuSecurityCommandRun() with virCommandRun(), and explicitly
>>> set the label, preserving the correct MCS range for the given VM
>>> instance. This is a temporary measure: eventually, we'll need a more
>>> generic and elegant mechanism for helper binaries.
>>>
>>> Fixes: a56f0168d576 ("qemu: hook up passt config to qemu domains")
>>> Signed-off-by: Stefano Brivio <sbrivio at redhat.com>
>>> ---
>>>   src/qemu/qemu_passt.c | 33 +++++++++++++++++++++++++++------
>>>   1 file changed, 27 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/src/qemu/qemu_passt.c b/src/qemu/qemu_passt.c
>>> index 1217a6a087..81f48dd630 100644
>>> --- a/src/qemu/qemu_passt.c
>>> +++ b/src/qemu/qemu_passt.c
>>> @@ -30,6 +30,11 @@
>>>   #include "virlog.h"
>>>   #include "virpidfile.h"
>>>   
>>> +#ifdef WITH_SELINUX
>>> +# include <selinux/selinux.h>
>>> +# include <selinux/context.h>
>>> +#endif
>>> +
>>>   #define VIR_FROM_THIS VIR_FROM_NONE
>>>   
>>>   VIR_LOG_INIT("qemu.passt");
>>> @@ -158,8 +163,11 @@ qemuPasstStart(virDomainObj *vm,
>>>       g_autofree char *errbuf = NULL;
>>>       char macaddr[VIR_MAC_STRING_BUFLEN];
>>>       size_t i;
>>> -    int exitstatus = 0;
>>> -    int cmdret = 0;
>>> +#ifdef WITH_SELINUX
>>> +    virSecurityLabelDef *seclabel;
>>> +    context_t s;
>>> +    const char *newLabel;
>>> +#endif
>>>   
>>>       cmd = virCommandNew(PASST);
>>>   
>>> @@ -271,18 +279,31 @@ qemuPasstStart(virDomainObj *vm,
>>>       if (qemuExtDeviceLogCommand(driver, vm, cmd, "passt") < 0)
>>>           return -1;
>>>   
>>> -    if (qemuSecurityCommandRun(driver, vm, cmd, -1, -1, &exitstatus, &cmdret) < 0)
>>> -        goto error;
>>> +#ifdef WITH_SELINUX
>>> +    /* TODO: Implement a new security manager function for helper binaries,
>>> +     * possibly deriving domain names from security attributes of the helper
>>> +     * binary itself.
>>> +     */
>>> +    seclabel = virDomainDefGetSecurityLabelDef(vm->def, "selinux");
>>> +    s = context_new(seclabel->label);
>>> +    context_type_set(s, "passt_t");
>>> +    newLabel = context_str(s);
>>> +
>>> +    virCommandSetSELinuxLabel(cmd, newLabel);
>>> +#endif
>>>   
>>> -    if (cmdret < 0 || exitstatus != 0) {
>>> +    if (virCommandRun(cmd, NULL)) {
>>>           virReportError(VIR_ERR_INTERNAL_ERROR,
>>>                          _("Could not start 'passt': %s"), NULLSTR(errbuf));
>>>           goto error;
>>>       }
>>>   
>>> +    context_free(s);
>>
>> This would need to be enclosed in #ifdef too.
>>
>>> +
>>>       return 0;
>>>   
>>>    error:
>>> -    qemuPasstKill(pidfile);
>>> +    context_free(s);
>>
>> And here as well.
>>
>>> +    qemuPasstKill(pidfile, passtSocketName);
>>>       return -1;
>>>   }
>>
>> I have to admit I'm not sure whether a proper solution via security
>> manager is feasible with a reasonable short time frame, i.e., ready to
>> be pushed ideally just after the release as I feel it would be too much
>> of a change for the freeze (but as I said, I may be wrong, I'm not
>> familiar with the details of the security manager code).
>>
>> This hacky approach has its downsides apart from the easily fixed nits
>> above. Due to not going through the security manager layers, this code
>> would try to use SELinux even if the driver is actually disabled in
>> runtime (which can happen for various reasons).
>>
>> So the question is, can a proper solution be implemented fast enough or
>> we need some form of this hack to have this new functionality working?
>> Michal, do you have any idea about the feasibility of providing a proper
>> solution to this?
> 
> This really isn't difficult to do in the security manager IMHO. It is
> just a variation on the existing virSecurityManagerSetChildProcessLabel
> method, which instead of using the standard QEMU svirt_t labels, queries
> the label by calling getfilecon on the binary to be execd and appending
> the MCS label.

It seems it's not as simple as this.

I have an implementation of this, available at

https://gitlab.com/lainestump/libvirt/-/commits/active-passt-6

The problem with it is that it doesn't end up setting the label to 
passt_t. Instead, it sets it to passt_exec_t. My understanding is that, 
similar to many other binaries (e.g. dnsmasq), the context type of the 
binary file is passt_exec_t, and the rules in the policies use that as 
an indicator to transition the process to passt_t.

Here's the results of a few different relabelling strategies:

In the case of Stefano's patch where libvirt's relabelling is completely 
removed (it just calls virCommandRun() directly), the passt process 
looks like this:

   unconfined_u:unconfined_r:passt_t:s0-s0:c0.c1023 97132

When I allow libvirt to do its normal relabelling, it ends up like this:

   unconfined_u:unconfined_r:svirt_t:s0:c239,c560 104213

When I run it with my patch (which calls getfilecon, then adds the MCS), 
it looks like this:

   system_u:object_r:passt_exec_t:s0:c239,c560

Am I correct to think that what everyone is suggesting is that the 
process ends up like this? (it's what would happen with Stefano's 
hard-coded hack that this message is replying to):

   unconfined_u:unconfined_r:passt_t:s0:c239,c560 104213

If so, how are we to derive "passt_t" from "passt_exec_t", since both 
those names are arbritrary, and some other selinux policy for some other 
binary might choose names that don't simply differ by having "_exec" 
added/removed. Is it possible to get the label of the process after it 
has been forked, and grab the context type from there?

In addition to this, after being able to run passt with selinux enabled, 
I was reminded that, while I've been doing my testing with privileged 
libvirt (qemu:///system), Stefano has been using unprivileged libvirt 
(qemu:///session); I've found that when passt is run by a privileged 
libvirt:

1) it is unable to write a pidfile to /var/run/libvirt/qemu/passt 
because its selinux policies restrict it /var/run/$UID for pidfiles 
(since that's where the pidfiles are expected by unprivileged libvirt).

2) it can't write to a logFile in /tmp, because passt's selinux policy 
restricts it to $HOME of the uid passt is run under (which is 
problematic since the user "qemu" doesn't have a $HOME :-))

3) No <portForward>s are possible, because the passt selinux policy 
forbids them.

I've let Stefano know about these. I'm still unsure what to do about 
getting the right label though. Any advice is welcome.