[libvirt] [PATCH v4 09/11] security: Label the external swtpm with SELinux labels

[Stefan Berger]

On 05/15/2018 11:45 AM, Stefan Berger wrote:
> On 05/15/2018 11:38 AM, Marc Hartmayer wrote:
>> On Thu, May 10, 2018 at 11:57 PM +0200, Stefan Berger 
>> <stefanb at linux.vnet.ibm.com> wrote:
>>> In this patch we label the swtpm process with SELinux labels. We 
>>> give it the
>>> same label as the QEMU process has. We label its state directory and 
>>> files
>>> as well. We restore the old security labels once the swtpm has 
>>> terminated.
>>>
>>> The file and process labels now look as follows:
>>>
>>> Directory: /var/lib/libvirt/swtpm
>>>
>>> [root at localhost swtpm]# ls -lZ
>>> total 4
>>> rwx------. 2 tss  tss system_u:object_r:svirt_image_t:s0:c254,c932 
>>> 4096 Apr  5 16:46 testvm
>>>
>>> [root at localhost testvm]# ls -lZ
>>> total 8
>>> -rw-r--r--. 1 tss tss system_u:object_r:svirt_image_t:s0:c254,c932 
>>> 3648 Apr  5 16:46 tpm-00.permall
>>>
>>> The log in /var/log/swtpm/libvirt/qemu is labeled as follows:
>>>
>>> -rw-r--r--. 1 tss tss system_u:object_r:svirt_image_t:s0:c254,c932 
>>> 2237 Apr  5 16:46 vtpm.log
>>>
>>> [root at localhost 485d0004-a48f-436a-8457-8a3b73e28567]# ps auxZ | 
>>> grep swtpm | grep ctrl | grep -v grep
>>> system_u:system_r:svirt_t:s0:c254,c932 tss 25664 0.0  0.0 28172  
>>> 3892 ?        Ss   16:57   0:00 /usr/bin/swtpm socket --daemon 
>>> --ctrl 
>>> type=unixio,path=/var/run/libvirt/qemu/swtpm/testvm-swtpm.sock,mode=0660 
>>> --tpmstate dir=/var/lib/libvirt/swtpm/testvm/tpm1.2 --log 
>>> file=/var/log/swtpm/libvirt/qemu/testvm-swtpm.log
>>>
>>> [root at localhost 485d0004-a48f-436a-8457-8a3b73e28567]# ps auxZ | 
>>> grep qemu | grep tpm | grep -v grep
>>> system_u:system_r:svirt_t:s0:c254,c932 qemu 25669 99.0  0.0 3096704 
>>> 48500 ?    Sl   16:57   3:28 /bin/qemu-system-x86_64 [..]
>>>
>>> Signed-off-by: Stefan Berger <stefanb at linux.vnet.ibm.com>
>>> ---
>>>   src/libvirt_private.syms        |   2 +
>>>   src/qemu/qemu_tpm.c             |  24 +++++-
>>>   src/security/security_driver.h  |   7 ++
>>>   src/security/security_manager.c |  36 +++++++++
>>>   src/security/security_manager.h |   6 ++
>>>   src/security/security_selinux.c | 164 
>>> ++++++++++++++++++++++++++++++++++++++++
>>>   src/security/security_stack.c   |  40 ++++++++++
>>>   7 files changed, 278 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
>>> index 75b8932..2ce67e7 100644
>>> --- a/src/libvirt_private.syms
>>> +++ b/src/libvirt_private.syms
>>> @@ -1319,6 +1319,7 @@ virSecurityManagerRestoreImageLabel;
>>>   virSecurityManagerRestoreInputLabel;
>>>   virSecurityManagerRestoreMemoryLabel;
>>>   virSecurityManagerRestoreSavedStateLabel;
>>> +virSecurityManagerRestoreTPMLabels;
>>>   virSecurityManagerSetAllLabel;
>>>   virSecurityManagerSetChardevLabel;
>>>   virSecurityManagerSetChildProcessLabel;
>>> @@ -1333,6 +1334,7 @@ virSecurityManagerSetProcessLabel;
>>>   virSecurityManagerSetSavedStateLabel;
>>>   virSecurityManagerSetSocketLabel;
>>>   virSecurityManagerSetTapFDLabel;
>>> +virSecurityManagerSetTPMLabels;
>>>   virSecurityManagerStackAddNested;
>>>   virSecurityManagerTransactionAbort;
>>>   virSecurityManagerTransactionCommit;
>>> diff --git a/src/qemu/qemu_tpm.c b/src/qemu/qemu_tpm.c
>>> index 024d24d..62f0146 100644
>>> --- a/src/qemu/qemu_tpm.c
>>> +++ b/src/qemu/qemu_tpm.c
>>> @@ -684,7 +684,26 @@ qemuExtTPMStartEmulator(virQEMUDriverPtr driver,
>>>
>>>       virCommandSetErrorBuffer(cmd, &errbuf);
>>>
>>> -    if (virCommandRun(cmd, &exitstatus) < 0 || exitstatus != 0) {
>>> +    if (virSecurityManagerSetTPMLabels(driver->securityManager,
>>> +                                       def) < 0)
>>> +        goto cleanup;
>>> +
>>> +    if 
>>> (virSecurityManagerSetChildProcessLabel(driver->securityManager,
>>> +                                               def, cmd) < 0)
>>> +        goto cleanup;
>>> +
>>> +    if (virSecurityManagerPreFork(driver->securityManager) < 0)
>>> +        goto cleanup;
>>> +
>>> +    /* make sure we run this with the appropriate user */
>>> +    virCommandSetUID(cmd, cfg->swtpm_user);
>>> +    virCommandSetGID(cmd, cfg->swtpm_group);
>>> +
>>> +    ret = virCommandRun(cmd, &exitstatus);
>>> +
>>> +    virSecurityManagerPostFork(driver->securityManager);
>>> +
>>> +    if (ret < 0 || exitstatus != 0) {
>>>           VIR_ERROR(_("Could not start 'swtpm'. exitstatus: %d "
>>>                       "stderr: %s"), exitstatus, errbuf);
>>>           virReportError(VIR_ERR_INTERNAL_ERROR,
>> Don't we have to set ret to -1 here (for the case where ret == 0 &&
>> exitstatus != 0)?
>
> Very true.

ret is initialized with '-1'. So we are good.

    Stefan