Re: [PATCH v17 11/20] key_protector: Add TPM2 Key Protector

[Daniel Kiper]

On Thu, Jun 20, 2024 at 03:35:32PM +0800, Gary Lin wrote:
> On Wed, Jun 19, 2024 at 06:34:13PM +0200, Daniel Kiper wrote:
> > On Fri, Jun 14, 2024 at 02:45:44PM +0800, Gary Lin wrote:
> > > From: Hernan Gatta <hegatta@linux.microsoft.com>
> > >
> > > The TPM2 key protector is a module that enables the automatic retrieval
> > > of a fully-encrypted disk's unlocking key from a TPM 2.0.
> > >
> > > The theory of operation is such that the module accepts various
> > > arguments, most of which are optional and therefore possess reasonable
> > > defaults. One of these arguments is the keyfile/tpm2key parameter, which
> > > is mandatory. There are two supported key formats:
> > >
> > > 1. Raw Sealed Key (--keyfile)
> > >    When sealing a key with TPM2_Create, the public portion of the sealed
> > >    key is stored in TPM2B_PUBLIC, and the private portion is in
> > >    TPM2B_PRIVATE. The raw sealed key glues the fully marshalled
> > >    TPM2B_PUBLIC and TPM2B_PRIVATE into one file.
> > >
> > > 2. TPM 2.0 Key (--tpm2key)
> > >    The following is the ASN.1 definition of TPM 2.0 Key File:
> > >
> > >    TPMPolicy ::= SEQUENCE {
> > >      CommandCode   [0] EXPLICIT INTEGER
> > >      CommandPolicy [1] EXPLICIT OCTET STRING
> > >    }
> > >
> > >    TPMAuthPolicy ::= SEQUENCE {
> > >      Name    [0] EXPLICIT UTF8STRING OPTIONAL
> > >      Policy  [1] EXPLICIT SEQUENCE OF TPMPolicy
> > >    }
> > >
> > >    TPMKey ::= SEQUENCE {
> > >      type        OBJECT IDENTIFIER
> > >      emptyAuth   [0] EXPLICIT BOOLEAN OPTIONAL
> > >      policy      [1] EXPLICIT SEQUENCE OF TPMPolicy OPTIONAL
> > >      secret      [2] EXPLICIT OCTET STRING OPTIONAL
> > >      authPolicy  [3] EXPLICIT SEQUENCE OF TPMAuthPolicy OPTIONAL
> > >      description [4] EXPLICIT UTF8String OPTIONAL,
> > >      rsaParent   [5] EXPLICIT BOOLEAN OPTIONAL,
> > >      parent      INTEGER
> > >      pubkey      OCTET STRING
> > >      privkey     OCTET STRING
> > >    }
> > >
> > >   The TPM2 key protector only expects a "sealed" key in DER encoding,
> > >   so 'type' is always 2.23.133.10.1.5, 'emptyAuth' is 'TRUE', and
> > >   'secret' is empty. 'policy' and 'authPolicy' are the possible policy
> > >   command sequences to construst the policy digest to unseal the key.
> > >   Similar to the raw sealed key, the public portion (TPM2B_PUBLIC) of
> > >   the sealed key is stored in 'pubkey', and the private portion
> > >   (TPM2B_PRIVATE) is in 'privkey'.
> > >
> > >   For more details: 
> > > https://www.hansenpartnership.com/draft-bottomley-tpm2-keys.html
> > >
> > > This sealed key file is created via the grub-protect tool. The tool
> > > utilizes the TPM's sealing functionality to seal (i.e., encrypt) an
> > > unlocking key using a Storage Root Key (SRK) to the values of various
> > > Platform Configuration Registers (PCRs). These PCRs reflect the state
> > > of the system as it boots. If the values are as expected, the system
> > > may be considered trustworthy, at which point the TPM allows for a
> > > caller to utilize the private component of the SRK to unseal (i.e.,
> > > decrypt) the sealed key file. The caller, in this case, is this key
> > > protector.
> > >
> > > The TPM2 key protector registers two commands:
> > >
> > > - tpm2_key_protector_init: Initializes the state of the TPM2 key
> > >                            protector for later usage, clearing any
> > >                            previous state, too, if any.
> > >
> > > - tpm2_key_protector_clear: Clears any state set by 
> > > tpm2_key_protector_init.
> > >
> > > The way this is expected to be used requires the user to, either
> > > interactively or, normally, via a boot script, initialize/configure
> > > the key protector and then specify that it be used by the 'cryptomount'
> > > command (modifications to this command are in a different patch).
> > >
> > > For instance, to unseal the raw sealed key file:
> > >
> > > tpm2_key_protector_init --keyfile=(hd0,gpt1)/efi/grub2/sealed-1.key
> > > cryptomount -u <PART1_UUID> -P tpm2
> > >
> > > tpm2_key_protector_init --keyfile=(hd0,gpt1)/efi/grub2/sealed-2.key 
> > > --pcrs=7,11
> > > cryptomount -u <PART2_UUID> -P tpm2
> > >
> > > Or, to unseal the TPM 2.0 Key file:
> > >
> > > tpm2_key_protector_init --tpm2key=(hd0,gpt1)/efi/grub2/sealed-1.tpm
> > > cryptomount -u <PART1_UUID> -P tpm2
> > >
> > > tpm2_key_protector_init --tpm2key=(hd0,gpt1)/efi/grub2/sealed-2.tpm 
> > > --pcrs=7,11
> > > cryptomount -u <PART2_UUID> -P tpm2
> > >
> > > If a user does not initialize the key protector and attempts to use it
> > > anyway, the protector returns an error.
> > >
> > > Before unsealing the key, the TPM2 key protector follows the "TPMPolicy"
> > > sequences to enforce the TPM policy commands to construct a valid policy
> > > digest to unseal the key.
> > >
> > > For the TPM 2.0 Key files, 'authPolicy' may contain multiple "TPMPolicy"
> > > sequences, the TPM2 key protector iterates 'authPolicy' to find a valid
> > > sequence to unseal key. If 'authPolicy' is empty or all sequences in
> > > 'authPolicy' fail, the protector tries the one from 'policy'. In case
> > > 'policy' is also empty, the protector creates a "TPMPolicy" sequence
> > > based on the given PCR selection.
> > >
> > > For the raw sealed key, the TPM2 key protector treats the key file as a
> > > TPM 2.0 Key file without 'authPolicy' and 'policy', so the "TPMPolicy"
> > > sequence is always based on the PCR selection from the command
> > > parameters.
> > >
> > > This commit only supports one policy command: TPM2_PolicyPCR. The
> > > command set will be extended to support advanced features, such as
> > > authorized policy, in the later commits.
> > >
> > > Cc: Stefan Berger <stefanb@linux.ibm.com>
> > > Cc: James Bottomley <jejb@linux.ibm.com>
> > > Signed-off-by: Hernan Gatta <hegatta@linux.microsoft.com>
> > > Signed-off-by: Gary Lin <glin@suse.com>
> > > ---
> > >  grub-core/Makefile.core.def       |   14 +
> > >  grub-core/tpm2/args.c             |  140 ++++
> > >  grub-core/tpm2/module.c           | 1225 +++++++++++++++++++++++++++++
> > >  grub-core/tpm2/tpm2key.asn        |   33 +
> > >  grub-core/tpm2/tpm2key.c          |  475 +++++++++++
> > >  grub-core/tpm2/tpm2key_asn1_tab.c |   45 ++
> > >  include/grub/tpm2/internal/args.h |   49 ++
> > >  include/grub/tpm2/tpm2key.h       |   86 ++
> > >  8 files changed, 2067 insertions(+)
> > >  create mode 100644 grub-core/tpm2/args.c
> > >  create mode 100644 grub-core/tpm2/module.c
> > >  create mode 100644 grub-core/tpm2/tpm2key.asn
> > >  create mode 100644 grub-core/tpm2/tpm2key.c
> > >  create mode 100644 grub-core/tpm2/tpm2key_asn1_tab.c
> > >  create mode 100644 include/grub/tpm2/internal/args.h
> > >  create mode 100644 include/grub/tpm2/tpm2key.h
> > >
> > > diff --git a/grub-core/Makefile.core.def b/grub-core/Makefile.core.def
> > > index 457eb2e41..4adfbd175 100644
> > > --- a/grub-core/Makefile.core.def
> > > +++ b/grub-core/Makefile.core.def
> > > @@ -2566,6 +2566,20 @@ module = {
> > >    enable = efi;
> > >  };
> > >
> > > +module = {
> > > +  name = tpm2;
> > > +  common = tpm2/args.c;
> > > +  common = tpm2/buffer.c;
> > > +  common = tpm2/module.c;
> > > +  common = tpm2/mu.c;
> > > +  common = tpm2/tpm2.c;
> > > +  common = tpm2/tpm2key.c;
> > > +  common = tpm2/tpm2key_asn1_tab.c;
> > > +  efi = tpm2/tcg2.c;
> > > +  enable = efi;
> > > +  cppflags = '-I$(srcdir)/lib/libtasn1-grub';
> > > +};
> > > +
> >
> > I think the TPM2 key protector should be in separate GRUB module,
> > i.e. not integrated with tss2 and tpm2 modules.
> >
>
> Hmmm, I'm thinking about putting the tss2 parts in grub-core/lib/ since
> buffer.c/mu.c/tpm2.c/tcg2 do not provide any grub commands and are more
> like a library.

Good point! Go ahead!

> > >  module = {
> > >    name = tr;
> > >    common = commands/tr.c;
> > > diff --git a/grub-core/tpm2/args.c b/grub-core/tpm2/args.c
> > > new file mode 100644
> > > index 000000000..c11280ab9
> > > --- /dev/null
> > > +++ b/grub-core/tpm2/args.c
> > > @@ -0,0 +1,140 @@
> > > +/*
> > > + *  GRUB  --  GRand Unified Bootloader
> > > + *  Copyright (C) 2022 Microsoft Corporation
> > > + *
> > > + *  GRUB is free software: you can redistribute it and/or modify
> > > + *  it under the terms of the GNU General Public License as published by
> > > + *  the Free Software Foundation, either version 3 of the License, or
> > > + *  (at your option) any later version.
> > > + *
> > > + *  GRUB is distributed in the hope that it will be useful,
> > > + *  but WITHOUT ANY WARRANTY; without even the implied warranty of
> > > + *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > > + *  GNU General Public License for more details.
> > > + *
> > > + *  You should have received a copy of the GNU General Public License
> > > + *  along with GRUB.  If not, see <http://www.gnu.org/licenses/>.
> > > + */
> > > +
> > > +#include <grub/err.h>
> > > +#include <grub/mm.h>
> > > +#include <grub/misc.h>
> > > +#include <grub/tpm2/internal/args.h>
> > > +
> > > +grub_err_t
> > > +grub_tpm2_protector_parse_pcrs (char *value, grub_uint8_t *pcrs,
> > > +                         grub_uint8_t *pcr_count)
> > > +{
> > > +  char *current_pcr = value;
> > > +  char *next_pcr;
> > > +  unsigned long pcr;
> > > +  grub_uint8_t i;
> > > +
> > > +  if (grub_strlen (value) == 0)
> > > +    return GRUB_ERR_BAD_ARGUMENT;
> > > +
> > > +  *pcr_count = 0;
> > > +  for (i = 0; i < TPM_MAX_PCRS; i++)
> > > +    {
> > > +      next_pcr = grub_strchr (current_pcr, ',');
> > > +      if (next_pcr == current_pcr)
> > > + return grub_error (GRUB_ERR_BAD_ARGUMENT,
> > > +                    N_("Empty entry in PCR list"));
> > > +      if (next_pcr)
> > > + *next_pcr = '\0';
> > > +
> > > +      grub_errno = GRUB_ERR_NONE;
> > > +      pcr = grub_strtoul (current_pcr, NULL, 10);
> > > +      if (grub_errno != GRUB_ERR_NONE)
> >
> > This check is unreliable. Please take a look at commit ac8a37dda
> > (net/http: Allow use of non-standard TCP/IP ports) how is should
> > be done properly.
> >
> Thanks for the hint. Will improve the check in v18.

You are welcome!

> > > + return grub_error (grub_errno,
> > > +                    N_("Entry '%s' in PCR list is not a number"),
> > > +                    current_pcr);
> > > +
> > > +      if (pcr > TPM_MAX_PCRS)
> > > + return grub_error (GRUB_ERR_OUT_OF_RANGE,
> > > +                    N_("Entry %lu in PCR list is too large to be a PCR "
> > > +                       "number, PCR numbers range from 0 to %u"),
> > > +                    pcr, TPM_MAX_PCRS);
> > > +
> > > +      pcrs[i] = (grub_uint8_t)pcr;
> >
> > Missing space after ")".
> >
> Will fix it in v18.
>
> > > +      *pcr_count += 1;
> >
> > ++(*pcr_count); ???
> >
> Those look the same to me here. Is there any functional difference?

No, just matter of taste.

[...]

> > > +static int
> > > +asn1_read_uint32 (asn1_node node, const char *name, grub_uint32_t *out)
> > > +{
> > > +  grub_uint32_t tmp = 0;
> > > +  grub_uint8_t *ptr;
> > > +  void *data = NULL;
> > > +  grub_size_t data_size;
> > > +  int ret;
> > > +
> > > +  ret = asn1_allocate_and_read (node, name, &data, &data_size);
> > > +  if (ret != ASN1_SUCCESS)
> > > +    return ret;
> > > +
> > > +  if (data_size > 4)
> >
> > Is it possible to get 3 or less here? If yes then we should check for
> > this too. Or s/>/!=/...
> >
> ASN.1 encoding only tells the number of octects for the integer so there
> could be 1 to hundreds of octects. Here we want a UINT32, so 1~4 octects
> are valid.

OK, we need a comment before the function then. Otherwise its name and
the code is confusing.

> > > +    {
> > > +      ret = ASN1_MEM_ERROR;
> > > +      goto error;
> > > +    }
> > > +
> > > +  /* convert the big-endian integer to host uint32 */
> > > +  ptr = (grub_uint8_t *)&tmp + (4 - data_size);
> > > +  grub_memcpy (ptr, data, data_size);
> >
> > Could you explain this? Why grub_be_to_cpu32() is not enough?
> > Is it related to alignment? If yes you could use grub_get_unaligned32().
> >
> The data_size could 1 to 4, so I have to copy data to tmp to make it a
> big-endian uint32 and convert it.

Please be more verbose in the comment then.

> > > +  tmp = grub_be_to_cpu32 (tmp);
> > > +
> > > +  *out = tmp;
> > > +error:
> > > +  if (data)
> > > +    grub_free (data);
> > > +  return ret;
> > > +}
> >
> > [...]
> >
> > > +grub_err_t
> > > +grub_tpm2key_get_authpolicy_seq (asn1_node tpm2key, tpm2key_authpolicy_t 
> > > *authpol_seq)
> > > +{
> > > +  tpm2key_authpolicy_t tmp_seq = NULL;
> > > +  tpm2key_authpolicy_t authpol = NULL;
> > > +  int authpol_n;
> > > +  char authpol_pol[AUTHPOLICY_POL_MAX];
> > > +  int i;
> > > +  int ret;
> > > +  grub_err_t err;
> > > +
> > > +  ret = asn1_number_of_elements (tpm2key, "authPolicy", &authpol_n);
> > > +  if (ret == ASN1_ELEMENT_NOT_FOUND)
> > > +    {
> > > +      /* "authPolicy" is optional, so it may not be available */
> > > +      *authpol_seq = NULL;
> > > +      return GRUB_ERR_NONE;
> > > +    }
> > > +  else if (ret != ASN1_SUCCESS)
> > > +    return grub_error (GRUB_ERR_READ_ERROR, N_("Failed to retrieve 
> > > authPolicy"));
> > > +
> > > +  /* Limit the number of authPolicy elements to two digits (99) */
> > > +  if (authpol_n > 100 || authpol_n < 1)
> >
> > I would define high/low policy limits as constants and use them everywhere.
> >
> At least 1 elements is expected, so we only need to define the upper
> limit.

I would define both for completeness.

Daniel