grub-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH 3/5] protectors: Add TPM2 Key Protector


From: Glenn Washburn
Subject: Re: [PATCH 3/5] protectors: Add TPM2 Key Protector
Date: Thu, 27 Jan 2022 21:40:26 -0600

On Thu, 27 Jan 2022 06:11:03 -0800 (PST)
Hernan Gatta
<hegatta@linuxonhyperv3.guj3yctzbm1etfxqx2vob5hsef.xx.internal.cloudapp.net>
wrote:

> 
> 
> On Mon, 24 Jan 2022, Glenn Washburn wrote:
> 
> > On Mon, 24 Jan 2022 06:12:16 -0800
> > Hernan Gatta <hegatta@linux.microsoft.com> wrote:
> >
> >> From: Hernan Gatta <hegatta@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 therefore possess reasonable defaults. One of these
> >> arguments is the keyfile parameter, which is mandatory.
> >>
> >> The value of this parameter must be a path to a sealed key file (e.g.,
> >> (hd0,gpt1)/boot/grub2/sealed_key). 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.
> >>
> >
> > There are a lot of points in this code where an error can be returned.
> > In this case, all I'm going to get is a non-descript error code (almost
> > always GRUB_ERR_BAD_ARGUMENT). So as a user, how am I to know where the
> > error is actually coming from? For this reason, I like the use of
> > grub_error(error_code, error_format_string, fmtstr_args...), which will
> > set grub_errno and an error message, which can be retrieved up the stack
> > and presented to the user. I'm not sure that every return of a naked
> > error code should be replaced by grub_error(), depends on how granular
> > of an error message you want to see. I would error on the side of more
> > granular. For instance, its really nice that GCC will tell me which
> > format code type specifier and a argument its choking on, rather than a
> > catch all "I'm failing to compile your code because you have an
> > incorrect format code somewhere in your format string".
> >
> 
> Agreed. I'll add calls to grub_error to indicate why things fail and, when 
> they do, capture the error up the stack (I was thinking in the command 
> handler for cryptomount), and print it.

IIRC, you shouldn't have to do anything, just return from cryptomount
with the error code and have grub_errno and grub_errmsg set. If you
swallow any errors, you should print the swallowed errors in
grub_dprintf() (maybe not always, judgement required).

> 
> >> Signed-off-by: Hernan Gatta <hegatta@linux.microsoft.com>
> >> ---
> >>  grub-core/Makefile.core.def |   9 +
> >>  grub-core/tpm2/module.c     | 742 
> >> ++++++++++++++++++++++++++++++++++++++++++++
> >>  2 files changed, 751 insertions(+)
> >>  create mode 100644 grub-core/tpm2/module.c
> >>
> >> diff --git a/grub-core/Makefile.core.def b/grub-core/Makefile.core.def
> >> index e4ae78b..8691440 100644
> >> --- a/grub-core/Makefile.core.def
> >> +++ b/grub-core/Makefile.core.def
> >> @@ -2498,6 +2498,15 @@ module = {
> >>  };
> >>
> >>  module = {
> >> +  name = tpm2;
> >> +  common = tpm2/buffer.c;
> >> +  common = tpm2/module.c;
> >> +  common = tpm2/mu.c;
> >> +  common = tpm2/tpm2.c;
> >> +  efi = tpm2/tcg2.c;
> >> +};
> >> +
> >> +module = {
> >>    name = tr;
> >>    common = commands/tr.c;
> >>  };
> >> diff --git a/grub-core/tpm2/module.c b/grub-core/tpm2/module.c
> >> new file mode 100644
> >> index 0000000..cd97452
> >> --- /dev/null
> >> +++ b/grub-core/tpm2/module.c
> >> @@ -0,0 +1,742 @@
> >> +/*
> >> + *  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/dl.h>
> >> +#include <grub/file.h>
> >> +#include <grub/misc.h>
> >> +#include <grub/mm.h>
> >> +#include <grub/protector.h>
> >> +#include <grub/tpm2/buffer.h>
> >> +#include <grub/tpm2/mu.h>
> >> +#include <grub/tpm2/tpm2.h>
> >> +#include <grub/types.h>
> >> +
> >> +GRUB_MOD_LICENSE ("GPLv3+");
> >> +
> >> +typedef enum grub_tpm2_protector_args
> >> +{
> >> +  GRUB_TPM2_PROTECTOR_ARG_MODE       = 1 << 0,
> >> +  GRUB_TPM2_PROTECTOR_ARG_PCRS       = 1 << 1,
> >> +  GRUB_TPM2_PROTECTOR_ARG_ASYMMETRIC = 1 << 2,
> >> +  GRUB_TPM2_PROTECTOR_ARG_BANK       = 1 << 3,
> >> +  GRUB_TPM2_PROTECTOR_ARG_KEYFILE    = 1 << 4,
> >> +  GRUB_TPM2_PROTECTOR_ARG_SRK        = 1 << 5,
> >> +  GRUB_TPM2_PROTECTOR_ARG_NV         = 1 << 6
> >> +} grub_tpm2_protector_args_t;
> >> +
> >> +typedef enum grub_tpm2_protector_mode
> >> +{
> >> +  GRUB_TPM2_PROTECTOR_MODE_SRK = 1,
> >> +  GRUB_TPM2_PROTECTOR_MODE_NV  = 2
> >> +} grub_tpm2_protector_mode_t;
> >> +
> >> +struct grub_tpm2_protector_context
> >> +{
> >> +  grub_tpm2_protector_args_t args;
> >> +  grub_tpm2_protector_mode_t mode;
> >> +  grub_uint8_t pcrs[TPM_MAX_PCRS];
> >> +  grub_uint8_t pcr_count;
> >> +  TPM_ALG_ID asymmetric;
> >> +  TPM_ALG_ID bank;
> >> +  const char *keyfile;
> >> +  TPM_HANDLE srk;
> >> +  TPM_HANDLE nv;
> >> +};
> >> +
> >> +static grub_err_t
> >> +grub_tpm2_protector_parse_mode (struct grub_tpm2_protector_context *ctx,
> >> +                                const char *value)
> >> +{
> >> +  if (grub_strcmp (value, "srk") == 0)
> >> +    ctx->mode = GRUB_TPM2_PROTECTOR_MODE_SRK;
> >> +  else if (grub_strcmp (value, "nv") == 0)
> >> +    ctx->mode = GRUB_TPM2_PROTECTOR_MODE_NV;
> >> +  else
> >> +    return GRUB_ERR_OUT_OF_RANGE;
> >> +
> >> +  return GRUB_ERR_NONE;
> >> +}
> >> +
> >> +static grub_err_t
> >> +grub_tpm2_protector_parse_pcrs (struct grub_tpm2_protector_context *ctx,
> >> +                                char *value)
> >> +{
> >> +  grub_err_t err;
> >> +  char *current_pcr = value;
> >> +  char *next_pcr;
> >> +  unsigned long pcr;
> >> +  grub_uint8_t i;
> >> +
> >> +  if (grub_strlen (value) == 0)
> >> +    return GRUB_ERR_BAD_ARGUMENT;
> >> +
> >> +  ctx->pcr_count = 0;
> >> +  for (i = 0; i < TPM_MAX_PCRS; i++)
> >> +    {
> >> +      next_pcr = grub_strchr (current_pcr, ',');
> >> +      if (next_pcr)
> >> +        *next_pcr = '\0';
> >> +      if (next_pcr == current_pcr)
> >> +        return GRUB_ERR_BAD_ARGUMENT;
> >> +
> >> +      grub_errno = GRUB_ERR_NONE;
> >> +      pcr = grub_strtoul (current_pcr, NULL, 10);
> >> +      if (grub_errno != GRUB_ERR_NONE)
> >> +        {
> >> +          err = grub_errno;
> >> +          grub_errno = GRUB_ERR_NONE;
> >> +          return err;
> >> +        }
> >> +
> >> +      if (pcr > GRUB_UCHAR_MAX)
> >> +        return GRUB_ERR_OUT_OF_RANGE;
> >> +
> >> +      ctx->pcrs[i] = (grub_uint8_t)pcr;
> >> +      ctx->pcr_count++;
> >> +
> >> +      if (!next_pcr)
> >> +        break;
> >> +
> >> +      current_pcr = next_pcr + 1;
> >> +      if (*current_pcr == '\0')
> >> +        return GRUB_ERR_BAD_ARGUMENT;
> >> +    }
> >> +
> >> +  if (i == TPM_MAX_PCRS)
> >> +    return GRUB_ERR_OUT_OF_RANGE;
> >> +
> >> +  return GRUB_ERR_NONE;
> >> +}
> >> +
> >> +static grub_err_t
> >> +grub_tpm2_protector_parse_asymmetric (struct grub_tpm2_protector_context 
> >> *ctx,
> >> +                                      const char *value)
> >> +{
> >> +  if (grub_strcasecmp (value, "ECC") == 0)
> >> +    ctx->asymmetric = TPM_ALG_ECC;
> >> +  else if (grub_strcasecmp (value, "RSA") == 0)
> >> +    ctx->asymmetric = TPM_ALG_RSA;
> >> +  else
> >> +    return GRUB_ERR_OUT_OF_RANGE;
> >> +
> >> +  return GRUB_ERR_NONE;
> >> +}
> >> +
> >> +static grub_err_t
> >> +grub_tpm2_protector_parse_bank (struct grub_tpm2_protector_context *ctx,
> >> +                                const char *value)
> >> +{
> >> +  if (grub_strcasecmp (value, "SHA1") == 0)
> >> +    ctx->bank = TPM_ALG_SHA1;
> >> +  else if (grub_strcasecmp (value, "SHA256") == 0)
> >> +    ctx->bank = TPM_ALG_SHA256;
> >> +  else if (grub_strcasecmp (value, "SHA384") == 0)
> >> +    ctx->bank = TPM_ALG_SHA384;
> >> +  else
> >> +    return GRUB_ERR_OUT_OF_RANGE;
> >> +
> >> +  return GRUB_ERR_NONE;
> >> +}
> >> +
> >> +static grub_err_t
> >> +grub_tpm2_protector_parse_keyfile (struct grub_tpm2_protector_context 
> >> *ctx,
> >> +                                   const char *value)
> >> +{
> >> +  if (grub_strlen (value) == 0)
> >> +    return GRUB_ERR_BAD_ARGUMENT;
> >> +
> >> +  ctx->keyfile = value;
> >> +
> >> +  return GRUB_ERR_NONE;
> >> +}
> >> +
> >> +static grub_err_t
> >> +grub_tpm2_protector_parse_tpm_handle (const char *value, TPM_HANDLE 
> >> *handle)
> >> +{
> >> +  grub_err_t err;
> >> +  unsigned long num;
> >> +
> >> +  grub_errno = GRUB_ERR_NONE;
> >> +  num = grub_strtoul (value, NULL, 0);
> >> +  if (grub_errno != GRUB_ERR_NONE)
> >> +    {
> >> +      err = grub_errno;
> >> +      grub_errno = GRUB_ERR_NONE;
> >> +      return err;
> >> +    }
> >> +
> >> +  if (num > GRUB_UINT_MAX)
> >> +    return GRUB_ERR_OUT_OF_RANGE;
> >> +
> >> +  *handle = (TPM_HANDLE)num;
> >> +
> >> +  return GRUB_ERR_NONE;
> >> +}
> >> +
> >> +static grub_err_t
> >> +grub_tpm2_protector_parse_srk (struct grub_tpm2_protector_context *ctx,
> >> +                               const char *value)
> >> +{
> >> +  return grub_tpm2_protector_parse_tpm_handle (value, &ctx->srk);
> >> +}
> >> +
> >> +static grub_err_t
> >> +grub_tpm2_protector_parse_nvindex (struct grub_tpm2_protector_context 
> >> *ctx,
> >> +                                   const char *value)
> >> +{
> >> +  return grub_tpm2_protector_parse_tpm_handle (value, &ctx->nv);
> >> +}
> >> +
> >> +static grub_err_t
> >> +grub_tpm2_protector_match_arg (struct grub_tpm2_protector_context *ctx,
> >> +                               const char *key, char *value)
> >> +{
> >> +  grub_err_t err;
> >> +
> >> +  if (grub_strlen (key) == 0 || grub_strlen (value) == 0)
> >> +    return GRUB_ERR_BAD_ARGUMENT;
> >> +
> >> +  if (grub_strcmp (key, "mode") == 0)
> >> +  {
> >> +    if (ctx->args & GRUB_TPM2_PROTECTOR_ARG_MODE)
> >> +      return GRUB_ERR_BAD_ARGUMENT;
> >> +
> >> +    err = grub_tpm2_protector_parse_mode (ctx, value);
> >> +    if (!err)
> >> +      ctx->args |= GRUB_TPM2_PROTECTOR_ARG_MODE;
> >> +
> >> +    return err;
> >> +  }
> >> +
> >> +  if (grub_strcmp (key, "pcrs") == 0)
> >
> > Minor nit, why wouldn't this be better as an "else if"? Seems
> > unnecessary to check all the "if" statements if we've already found a
> > match.
> >
> 
> The reason why I wrote this in this way without an "else if" is because 
> each "if" clause has an unconditional return. So, in the end, the entire 
> sequence behaves in the manner you describe.
> 
> Would you prefer that I add the "else if"'s anyway for readability?

Somehow I missed the returns. I don't care as much now. Maybe Daniel
will have an opinion when he gets to this.

Glenn



reply via email to

[Prev in Thread] Current Thread [Next in Thread]