[patch v2 1/1] virt-aa-helper: Add support for smartcard host-certificates

[Arnaud Patard]

Christian Ehrhardt <christian.ehrhardt at canonical.com> writes:

> On Thu, Dec 5, 2019 at 6:25 PM Arnaud Patard <apatard at hupstream.com> wrote:
>
>> When emulating smartcard with host certificates, qemu needs to
>> be able to read the certificates files. Add necessary code to
>> add the smartcard certificates file path to the apparmor profile.
>>
>> Passthrough support has been tested with spicevmc and remote-viewer.
>>
>> v2:
>> - Fix CodingStyle
>> - Add support for 'host' case.
>> - Add a comment to mention that the passthrough case doesn't need
>>   some configuration
>> - Use one rule with '{,*}' instead of two rules.
>>
>> Signed-off-by: Arnaud Patard <apatard at hupstream.com>
>> Index: libvirt/src/security/virt-aa-helper.c
>> ===================================================================
>> --- libvirt.orig/src/security/virt-aa-helper.c
>> +++ libvirt/src/security/virt-aa-helper.c
>> @@ -1271,6 +1271,39 @@ get_files(vahControl * ctl)
>>          }
>>      }
>>
>> +    for (i = 0; i < ctl->def->nsmartcards; i++) {
>> +        virDomainSmartcardDefPtr sc = ctl->def->smartcards[i];
>> +        virDomainSmartcardType sc_type = sc->type;
>> +        char *sc_db = (char *)VIR_DOMAIN_SMARTCARD_DEFAULT_DATABASE;
>> +        if (sc->data.cert.database)
>> +            sc_db = sc->data.cert.database;
>> +        switch (sc_type) {
>> +            /*
>> +             * Note: At time of writing, to get this working, qemu
>> seccomp sandbox has
>> +             * to be disabled or the host must be running QEMU with commit
>> +             * 9a1565a03b79d80b236bc7cc2dbce52a2ef3a1b8.
>> +             * It's possibly due to
>> libcacard:vcard_emul_new_event_thread(), which calls
>> +             * PR_CreateThread(), which calls {g,s}etpriority(). And
>> resourcecontrol seccomp
>> +             * filter forbids it (cf src/qemu/qemu_command.c which seems
>> to always use
>> +             * resourcecontrol=deny).
>> +             */
>> +            case VIR_DOMAIN_SMARTCARD_TYPE_HOST:
>> +                virBufferAddLit(&buf, "  \"/etc/pki/nssdb/{,*}\" rk,\n");
>>
>
> That path matches the examples in libvirt/qemu and also the fedora nss
> package
> [root at fedora~]# rpm -qf /etc/pki/nssdb/
> nss-3.47.0-2.fc29.x86_64
> [root at fedora ~]# ll /etc/pki/nssdb/
> total 8
> -rw-r--r-- 1 root root 65536 Jan  6  2017 cert8.db
> -rw-r--r-- 1 root root  9216 Jan  6  2017 cert9.db
> -rw-r--r-- 1 root root 16384 Jan  6  2017 key3.db
> -rw-r--r-- 1 root root 11264 Jan  6  2017 key4.db
> -rw-r--r-- 1 root root   451 Oct 23 11:23 pkcs11.txt
> -rw-r--r-- 1 root root 16384 Jan  6  2017 secmod.db
>
> But on Debian/Ubuntu the paths are slightly different.
> root at x:~# dpkg -L libnss3-nssdb
> [...]
> /var/lib/nssdb/key4.db
> /var/lib/nssdb/cert9.db
> /var/lib/nssdb/pkcs11.txt
> /var/lib/nssdb/secmod.db
>
> Therefore I'd ask you to add that path as well here.
>

I don't remember seeing this /var/lib/nssdb path on my Debian. I don't
even find the libnss3-nssdb package. Is it Ubuntu-specific and
documented somewhere ?

I can add this path, I've no problem with that. Nevertheless, I'm
wondering that if it's really distribution specific (like done only on
Ubuntu), wouldn't it be better to have the distribution patch libvirt to
add this path ?

>
> +                break;
>> +            case VIR_DOMAIN_SMARTCARD_TYPE_HOST_CERTIFICATES:
>> +                virBufferAsprintf(&buf, "  \"%s/{,*}\" rk,\n", sc_db);
>> +                break;
>>
>
>>From https://libvirt.org/formatdomain.html#elementsSmartcard
> "An additional sub-element <database> can specify the absolute path to an
> alternate directory ... if not present, it defaults to /etc/pki/nssdb."
> That is in "VIR_DOMAIN_SMARTCARD_DEFAULT_DATABASE".
> Have you tested if sc_db is actually set to
> "VIR_DOMAIN_SMARTCARD_DEFAULT_DATABASE"
> in that case?
> If it is e.g. undefined we need to check for that and add
> "VIR_DOMAIN_SMARTCARD_DEFAULT_DATABASE"
> instead.

I remember testing the case with certificates inside /etc/pki/nssdb and
not specifying the <database/> element at the early version of the
patch.

>
> Furthermore actually this lets us define:
>   <smartcard mode='host-certificates'>
>     <certificate>cert1</certificate>
>     <certificate>cert2</certificate>
>     <certificate>cert3</certificate>
>     <database>/etc/pki/nssdb/</database>
>   </smartcard>
>
> There could be two guests using rather different cert[1-3] and there is no
> need letting them cross read right?
> So instead of
>   virBufferAsprintf(&buf, "  \"%s/{,*}\" rk,\n", sc_db);
> maybe something like this is safer:
> iterate on certs-that-are-defined => cert
>     virBufferAsprintf(&buf, "  \"%s/%s\" rk,\n", sc_db, cert);

Not sure to understand. The certificates are inside a SQLite database,
for instance:
$ ls /etc/pki/nssdb
cert9.db  key4.db  pkcs11.txt

How your proposed change would be valid, since there's no specific file
for the certificate (for instance /etc/pki/nssdb/cert1). Is apparmor
able to filter the access of the content of a SQLite database ?

Arnaud