Re: [PATCH v2 0/3] Add ability to use SEV provisioned secrets for disk decryption

[Glenn Washburn]

On Fri, 13 Nov 2020 18:48:30 -0800
James Bottomley <jejb@linux.ibm.com> wrote:

> On Fri, 2020-11-13 at 19:50 -0600, Glenn Washburn wrote:
> > On Fri, 13 Nov 2020 14:25:07 -0800
> > James Bottomley <jejb@linux.ibm.com> wrote:
> > 
> > > v2: update geli.c to use conditional prompt and add callback for
> > >     variable message printing and secret destruction
> > > 
> > > To achieve encrypted disk images in the AMD SEV encrypted virtual
> > > machine, we need to add the ability for grub to retrieve the disk
> > > passphrase from the SEV launch secret.  To do this, we've modified
> > > OVMF to set aside an area for the injected secret and pass up a
> > > configuration table for it:
> > > 
> > > https://edk2.groups.io/g/devel/topic/78198617#67339
> > > 
> > > The patches in this series modify grub to look for the disk
> > > passphrase
> > > in the secret configuration table and use it to decrypt any disks
> > > in
> > > the system if they are found.  This is so an encrypted image with
> > > a properly injected password will boot without any user
> > > intervention.
> > > 
> > > The three patches firstly modify the cryptodisk consumers to allow
> > > arbitrary password getters instead of the current console based
> > > one.
> > 
> > I like this idea in general.
> > 
> > > The next patch adds a '-s' option to cryptodisk to allow it to use
> > > a
> > > saved password and the final one adds a sevsecret command to check
> > > for
> > > the secrets configuration table and provision the disk passphrase
> > > from
> > > it if an entry is found.
> > 
> > I'm not in favor of this approach.  This feels like a special case
> > of providing a key file to cryptomount.  We have working (and I
> > believe merge worthy) patches for adding key file support.
> > Unfortunately, due to the current position in the grub development
> > cycle, they have not been merged.  As a side note, it might be
> > interesting to re-work the key file patch series to use the
> > arbitrary password getter mechanism you've created.
> > 
> > What I would prefer, because it feels more generic, is to have the
> > sevsecret module create a procfs entry (perhaps (proc)/sevsecret),
> > which outputs the secret data when read (or NULL string if some
> > error in finding the secret).  Then to cryptomount all devices that
> > accept the sev secret do:
> > 
> >   cryptomount -a -k (proc)/sevsecret
> > 
> > In this case you could re-use most of the code in
> > grub_efi_sevsecret_find and creating the procfs entry would be
> > trivial (see bottom of cryptodisk.c for an example on how to do
> > this).
> 
> A file interface feels slightly wrong for this.  What we need is a
> use/release envelope ... effectively a way of enforcing the lifetime
> on the secret use.  This allows a wider variety of threat models than
> the simply file model because the latter assumes an infinite lifetime
> (even though that can be hacked around using temporary filesystems).

Yep, that's exactly what I'm proposing. I don't think the file model as
it exists as a concept in *NIX has any such assumption about data
lifetimes. Sockets, pipes, and character devices are accessed as files
in *NIX, and none of those have that assumption.  I think it makes a
lot of sense to expose the data through a file-like interface that is
read once.  After a read its wiped, which feels like a socket or pipe.

> From a generality point of view it feels better to implement a file
> interface as a special case of a use/release model because it simply
> has an empty release function.
> 
> If you follow this model, a file use case would have a special
> filesecret command and the use would go like:
> 
> filesecret <file>
> cryptomount -s

This feels like a clumsy way of passing data to the cryptodisk mounting
system.  Your approach might be akin to the kernel keyring in linux,
where you have a program to manage keys and other programs that can
access the kernel key ring.  This is done so that user space doesn't
have direct access to keys because its untrusted.  I don't think we
have that concern here.

If we were to go your route, I think there should be one secret
management command that manages a database of secrets.  Perhaps
something like:

# Actually don't need this, just have the module init for
# sevsecret do this.
# keys --load sev --name sevkey
cryptomount -a -k key:sevkey

Here the secrets command could take a list of arguments to send to a
registered loader (in this case named sev) and associate the loaded key
with a name (in this case sevkey).  The -k parameter to cryptomount
would take an argument that can be "key:" followed by the name of the
associated key and use that as key material.  For the file case,
something like:

cryptomount -a -k file:(dev1)/path/to/key

or

keys --load file --name mykey1 (dev1)/path/to/key
cryptomount -a -k key:mykey1

I know next to nothing about TPM, but my understanding is that it can
have key material.  Perhaps there could there be something like:

cryptomount -a -k tpm:key1

Glenn