[libvirt PATCH v2 09/12] tools: support generating SEV secret injection tables

[Daniel P. Berrangé]

On Tue, Oct 25, 2022 at 07:38:43PM -0400, Cole Robinson wrote:
> On 10/19/22 6:17 AM, Daniel P. Berrangé wrote:
> > It is possible to build OVMF for SEV with an embedded Grub that can
> > fetch LUKS disk secrets. This adds support for injecting secrets in
> > the required format.
> > 
> > Signed-off-by: Daniel P. Berrangé <berrange at redhat.com>
> > ---
> 
> > diff --git a/tools/virt-qemu-sev-validate b/tools/virt-qemu-sev-validate
> > index 5ce5763d5b..2d15edb933 100755
> > --- a/tools/virt-qemu-sev-validate
> > +++ b/tools/virt-qemu-sev-validate
> > @@ -36,16 +36,19 @@
> >  
> >  import abc
> >  import argparse
> > -from base64 import b64decode
> > +from base64 import b64decode, b64encode
> >  from hashlib import sha256
> >  import hmac
> >  import logging
> > +import os
> >  import re
> >  import socket
> >  from struct import pack
> >  import sys
> >  import traceback
> >  from uuid import UUID
> > +from cryptography.hazmat.primitives.ciphers import Cipher, algorithms, modes
> > +
> >  
> >  from lxml import etree
> >  import libvirt
> > @@ -573,7 +576,26 @@ class KernelTable(GUIDTable):
> >          return entries
> >  
> >  
> > -class ConfidentialVM(object):
> > +class SecretsTable(GUIDTable):
> > +
> > +    TABLE_GUID = UUID('{1e74f542-71dd-4d66-963e-ef4287ff173b}').bytes_le
> > +    DISK_PW_GUID = UUID('{736869e5-84f0-4973-92ec-06879ce3da0b}').bytes_le
> > +
> > +    def __init__(self):
> > +        super().__init__(guid=self.TABLE_GUID,
> > +                         lenlen=4)
> > +        self.disk_password = None
> > +
> > +    def load_disk_password(self, path):
> > +        with open(path, 'rb') as fh:
> > +            self.disk_password = fh.read()
> > +
> > +    def entries(self):
> > +        return self.build_entry(self.DISK_PW_GUID,
> > +                                self.disk_password + bytes([0]), 4)
> > +
> 
> This bytes([0]) NUL byte ends up in the efi_secret /sys path. Dropping
> it doesn't seem to impact injecting the secret at all

The specs/sev-guest-firmware.rst files does not specify anything in
particular about the injected secret format. The rules for the format
will vary according to the GUID used for the entry. In this case what
I've called the DISK_PW_GUID is the same as the GUID used in James
Bottomley's grub patch:

  https://lists.gnu.org/archive/html/grub-devel/2020-12/msg00260.html

In particular note

  +      /*
  +       * the passphrase must be a zero terminated string because the
  +       * password routines call grub_strlen () to find its size
  +       */

As written, the code just passes around a pointer to the data in the
secret table, so it can't simply add a NUL terminator itself.

See also James' python demo script for injection which adds a NUL:

  https://lists.gnu.org/archive/html/qemu-devel/2021-02/msg03691.html

> FWIW once that's dropped, getting automatic luks unlock is really simple
> with /etc/crypttab + kernel 5.19
> 
> sed -i -e "s| none |
> /sys/kernel/security/secrets/coco/736869e5-84f0-4973-92ec-06879ce3da0b
> |g" /etc/crypttab
> dracut --force --add-drivers efi_secret
> shutdown -r now

Now one option here is to invent a different GUID and call it the
"CRYTPTAB_PW_GUID", and have the host side injection tools not use
a NUL terminator for this option.

What I don't like about this is that in both cases we're just passing
in the same data - a passphrase for unlocking a LUKS keyslot.

IMHO, we should have a "LUKS_PASSPHARSE_GUID" secret entry which can
be consumed by any software that wants to unlock LUKS, not have
software specific GUIDs.

The crypttab keyfiles can contain arbitrary bytes, so we can't just
drop the trailing NUL as that might be relevant key material.

Reading cryttab(5) I see that /etc/crypttab supports an option
called 'keyfile-size', which could be used to truncate the file.
That's not at all user friendly for us, as the user pw data can be
arbitrary length and when building the disk image template we
don't know how many bytes are sensitive.IOW, we can't predict
what keyfile-size to use.

So it seems we have a few options

 1. modify grub patches to remove the requirement for NUL
    termination
 2. modify systemd to add a 'keyfile-nul-terminated' option
    to instruct it to stip a trailing NUL from key-file.
 3. modify systemd to add a 'efi-secret=UUID' option and
    ignore key-file field entirely

Incidentally I think we should document well known secret GUIDs
in specs/sev-guest-firmware.rst.

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|