Re: [PATCH v20 25/33] util/grub-protect: Add new tool

[Daniel Kiper]

On Mon, Oct 21, 2024 at 04:07:03PM +0800, Gary Lin wrote:
> From: Hernan Gatta <hegatta@linux.microsoft.com>
>
> To utilize the key protectors framework, there must be a way to protect
> full-disk encryption keys in the first place. The grub-protect tool
> includes support for the TPM2 key protector but other protectors that
> require setup ahead of time can be supported in the future.
>
> For the TPM2 key protector, the intended flow is for a user to have a
> LUKS 1 or LUKS 2-protected fully-encrypted disk. The user then creates a
> new LUKS key file, say by reading /dev/urandom into a file, and creates
> a new LUKS key slot for this key. Then, the user invokes the grub-protect
> tool to seal this key file to a set of PCRs using the system's TPM 2.0.
> The resulting sealed key file is stored in an unencrypted partition such
> as the EFI System Partition (ESP) so that GRUB may read it. The user also
> has to ensure the cryptomount command is included in GRUB's boot script
> and that it carries the requisite key protector (-P) parameter.
>
> Sample usage:
>
> $ dd if=/dev/urandom of=luks-key bs=1 count=32
> $ sudo cryptsetup luksAddKey /dev/sdb1 luks-key --pbkdf=pbkdf2 --hash=sha512
>
> To seal the key with TPM 2.0 Key File (recommended):
>
> $ sudo grub-protect --action=add \
>                     --protector=tpm2 \
>                     --tpm2-pcrs=0,2,4,7,9 \
>                     --tpm2key \
>                     --tpm2-keyfile=luks-key \
>                     --tpm2-outfile=/boot/efi/efi/grub/sealed.tpm
>
> Or, to seal the key with the raw sealed key:
>
> $ sudo grub-protect --action=add \
>                     --protector=tpm2 \
>                     --tpm2-pcrs=0,2,4,7,9 \
>                     --tpm2-keyfile=luks-key \
>                     --tpm2-outfile=/boot/efi/efi/grub/sealed.key
>
> Then, in the boot script, for TPM 2.0 Key File:
>
> tpm2_key_protector_init --tpm2key=(hd0,gpt1)/efi/grub/sealed.tpm
> cryptomount -u <SDB1_UUID> -P tpm2
>
> Or, for the raw sealed key:
>
> tpm2_key_protector_init --keyfile=(hd0,gpt1)/efi/grub/sealed.key 
> --pcrs=0,2,4,7,9
> cryptomount -u <SDB1_UUID> -P tpm2
>
> The benefit of using TPM 2.0 Key File is that the PCR set is already
> written in the key file, so there is no need to specify PCRs when
> invoking tpm2_key_protector_init.
>
> Cc: Stefan Berger <stefanb@linux.ibm.com>
> Signed-off-by: Hernan Gatta <hegatta@linux.microsoft.com>
> Signed-off-by: Gary Lin <glin@suse.com>
> ---
>  .gitignore                |    2 +
>  Makefile.util.def         |   26 +
>  configure.ac              |   30 +
>  docs/man/grub-protect.h2m |    4 +
>  util/grub-protect.c       | 1394 +++++++++++++++++++++++++++++++++++++
>  5 files changed, 1456 insertions(+)
>  create mode 100644 docs/man/grub-protect.h2m
>  create mode 100644 util/grub-protect.c

[...]

> +static grub_err_t
> +protect_write_file (const char *filepath, void *buffer, size_t buffer_size)
> +{
> +  grub_err_t err;
> +  FILE *f;
> +
> +  f = fopen (filepath, "wb");
> +  if (f == NULL)
> +    return GRUB_ERR_FILE_NOT_FOUND;
> +
> +  if (fwrite (buffer, buffer_size, 1, f) != 1)
> +  {
> +    err = GRUB_ERR_WRITE_ERROR;
> +    goto exit;
> +  }
> +
> +  err = GRUB_ERR_NONE;
> +
> + exit:
> +  fclose (f);
> +
> +  return err;
> +}
> +
> +grub_err_t
> +grub_tcg2_get_max_output_size (grub_size_t *size)
> +{
> +  if (size == NULL)
> +    return GRUB_ERR_BAD_ARGUMENT;
> +
> +  *size = GRUB_TPM2_BUFFER_CAPACITY;
> +
> +  return GRUB_ERR_NONE;
> +}
> +
> +grub_err_t
> +grub_tcg2_submit_command (grub_size_t input_size, grub_uint8_t *input,
> +                       grub_size_t output_size, grub_uint8_t *output)
> +{
> +  static const grub_size_t header_size = sizeof (grub_uint16_t) +
> +                                      (2 * sizeof(grub_uint32_t));

AIUI this is somehow related to the TPM structs defined elsewhere. So,
I think these structs should have defined header as a separate struct
(probably preferred). Then you could use, e.g., sizeof (tpm_header) and be
done. Or you can define a constant next to TPM structs as you do here.
Of course it would be nice if the constant has relevant name and/or
a comment...

> +  if (write (protector_tpm2_fd, input, input_size) != input_size)
> +    return GRUB_ERR_BAD_DEVICE;
> +
> +  if (read (protector_tpm2_fd, output, output_size) < header_size)
> +    return GRUB_ERR_BAD_DEVICE;
> +
> +  return GRUB_ERR_NONE;
> +}

[...]

> +static grub_err_t
> +protect_tpm2_export_tpm2key (const protect_args_t *args,
> +                          tpm2_sealed_key_t *sealed_key)
> +{
> +  const char *sealed_key_oid = "2.23.133.10.1.5";

Please name this OID in a comment or define properly named constant.

> +  asn1_node asn1_def = NULL;
> +  asn1_node tpm2key = NULL;
> +  grub_uint32_t parent;
> +  grub_uint32_t cmd_code;
> +  struct grub_tpm2_buffer pol_buf;
> +  TPML_PCR_SELECTION_t pcr_sel = {
> +    .count = 1,
> +    .pcrSelections = {
> +      {
> +     .hash = args->tpm2_bank,
> +     .sizeOfSelect = 3,
> +     .pcrSelect = {0}
> +      },
> +    }
> +  };
> +  struct grub_tpm2_buffer pub_buf;
> +  struct grub_tpm2_buffer priv_buf;
> +  void *der_buf = NULL;
> +  int der_buf_size = 0;
> +  int i;
> +  int ret;
> +  grub_err_t err;
> +
> +  for (i = 0; i < args->tpm2_pcr_count; i++)
> +    TPMS_PCR_SELECTION_SelectPCR (&pcr_sel.pcrSelections[0], 
> args->tpm2_pcrs[i]);
> +
> +  /*
> +   * Prepare the parameters for TPM_CC_PolicyPCR:
> +   * empty pcrDigest and the user selected PCRs
> +   */
> +  grub_tpm2_buffer_init (&pol_buf);
> +  grub_tpm2_buffer_pack_u16 (&pol_buf, 0);
> +  grub_Tss2_MU_TPML_PCR_SELECTION_Marshal (&pol_buf, &pcr_sel);
> +
> +  grub_tpm2_buffer_init (&pub_buf);
> +  grub_Tss2_MU_TPM2B_PUBLIC_Marshal (&pub_buf, &sealed_key->public);
> +  grub_tpm2_buffer_init (&priv_buf);
> +  grub_Tss2_MU_TPM2B_Marshal (&priv_buf, sealed_key->private.size,
> +                           sealed_key->private.buffer);
> +  if (pub_buf.error != 0 || priv_buf.error != 0)
> +    return GRUB_ERR_BAD_ARGUMENT;
> +
> +  ret = asn1_array2tree (tpm2key_asn1_tab, &asn1_def, NULL);
> +  if (ret != ASN1_SUCCESS)
> +    return GRUB_ERR_BAD_ARGUMENT;
> +
> +  ret = asn1_create_element (asn1_def, "TPM2KEY.TPMKey" , &tpm2key);
> +  if (ret != ASN1_SUCCESS)
> +    return GRUB_ERR_BAD_ARGUMENT;
> +
> +  /* Set 'type' to "sealed key" */
> +  ret = asn1_write_value (tpm2key, "type", sealed_key_oid, 1);
> +  if (ret != ASN1_SUCCESS)
> +    {
> +      fprintf (stderr, "Failed to set 'type': 0x%u\n", ret);
> +      err = GRUB_ERR_BAD_ARGUMENT;
> +      goto error;
> +    }
> +
> +  /* Set 'emptyAuth' to TRUE */
> +  ret = asn1_write_value (tpm2key, "emptyAuth", "TRUE", 1);
> +  if (ret != ASN1_SUCCESS)
> +    {
> +      fprintf (stderr, "Failed to set 'emptyAuth': 0x%x\n", ret);
> +      err = GRUB_ERR_BAD_ARGUMENT;
> +      goto error;
> +    }
> +
> +  /* Set 'policy' */
> +  ret = asn1_write_value (tpm2key, "policy", "NEW", 1);
> +  if (ret != ASN1_SUCCESS)
> +    {
> +      fprintf (stderr, "Failed to set 'policy': 0x%x\n", ret);
> +      err = GRUB_ERR_BAD_ARGUMENT;
> +      goto error;
> +    }
> +  cmd_code = grub_cpu_to_be32 (TPM_CC_PolicyPCR);
> +  ret = asn1_write_value (tpm2key, "policy.?LAST.CommandCode", &cmd_code,
> +                       sizeof (cmd_code));
> +  if (ret != ASN1_SUCCESS)
> +    {
> +      fprintf (stderr, "Failed to set 'policy CommandCode': 0x%x\n", ret);
> +      err = GRUB_ERR_BAD_ARGUMENT;
> +      goto error;
> +    }
> +  ret = asn1_write_value (tpm2key, "policy.?LAST.CommandPolicy", 
> &pol_buf.data,
> +                       pol_buf.size);
> +  if (ret != ASN1_SUCCESS)
> +    {
> +      fprintf (stderr, "Failed to set 'policy CommandPolicy': 0x%x\n", ret);
> +      err = GRUB_ERR_BAD_ARGUMENT;
> +      goto error;
> +    }
> +
> +  /* Remove 'secret' */
> +  ret = asn1_write_value (tpm2key, "secret", NULL, 0);
> +  if (ret != ASN1_SUCCESS)
> +    {
> +      fprintf (stderr, "Failed to remove 'secret': 0x%x\n", ret);
> +      err = GRUB_ERR_BAD_ARGUMENT;
> +      goto error;
> +    }
> +
> +  /* Remove 'authPolicy' */
> +  ret = asn1_write_value (tpm2key, "authPolicy", NULL, 0);
> +  if (ret != ASN1_SUCCESS)
> +    {
> +      fprintf (stderr, "Failed to remove 'authPolicy': 0x%x\n", ret);
> +      err = GRUB_ERR_BAD_ARGUMENT;
> +      goto error;
> +    }
> +
> +  /* Remove 'description' */
> +  ret = asn1_write_value (tpm2key, "description", NULL, 0);
> +  if (ret != ASN1_SUCCESS)
> +    {
> +      fprintf (stderr, "Failed to remove 'description': 0x%x\n", ret);
> +      err = GRUB_ERR_BAD_ARGUMENT;
> +      goto error;
> +    }
> +
> +  /*
> +   *  Use the SRK handle as the parent handle if specified
> +   *  Otherwise, Use TPM_RH_OWNER as the default parent handle
> +  */
> +  if (args->tpm2_srk != 0)
> +    parent = grub_cpu_to_be32 (args->tpm2_srk);
> +  else
> +    parent = grub_cpu_to_be32 (TPM_RH_OWNER);
> +  ret = asn1_write_value (tpm2key, "parent", &parent, sizeof (parent));
> +  if (ret != ASN1_SUCCESS)
> +    {
> +      fprintf (stderr, "Failed to set 'parent': 0x%x\n", ret);
> +      err = GRUB_ERR_BAD_ARGUMENT;
> +      goto error;
> +    }
> +
> +  /*
> +   * Set 'rsaParent' to TRUE if the RSA SRK is specified and the SRK
> +   * handle is not persistent. Otherwise, remove 'rsaParent'.
> +   */
> +  if (args->tpm2_srk == 0 && args->srk_type.type == TPM_ALG_RSA)
> +    ret = asn1_write_value (tpm2key, "rsaParent", "TRUE", 1);
> +  else
> +    ret = asn1_write_value (tpm2key, "rsaParent", NULL, 0);
> +
> +  if (ret != ASN1_SUCCESS)
> +    {
> +      fprintf (stderr, "Failed to set 'rsaParent': 0x%x\n", ret);
> +      err = GRUB_ERR_BAD_ARGUMENT;
> +      goto error;
> +    }
> +
> +  /* Set the pubkey */
> +  ret = asn1_write_value (tpm2key, "pubkey", pub_buf.data, pub_buf.size);
> +  if (ret != ASN1_SUCCESS)
> +    {
> +      fprintf (stderr, "Failed to set 'pubkey': 0x%x\n", ret);
> +      err = GRUB_ERR_BAD_ARGUMENT;
> +      goto error;
> +    }
> +
> +  /* Set the privkey */
> +  ret = asn1_write_value (tpm2key, "privkey", priv_buf.data, priv_buf.size);
> +  if (ret != ASN1_SUCCESS)
> +    {
> +      fprintf (stderr, "Failed to set 'privkey': 0x%x\n", ret);
> +      err = GRUB_ERR_BAD_ARGUMENT;
> +      goto error;
> +    }
> +
> +  /* Create the DER binary */
> +  der_buf_size = 0;
> +  ret = asn1_der_coding (tpm2key, "", NULL, &der_buf_size, NULL);
> +  if (ret != ASN1_MEM_ERROR)
> +    {
> +      fprintf (stderr, "Failed to get DER size: 0x%x\n", ret);
> +      err = GRUB_ERR_BAD_ARGUMENT;
> +      goto error;
> +    }
> +
> +  der_buf = grub_malloc (der_buf_size);
> +  if (der_buf == NULL)
> +    {
> +      fprintf (stderr, "Failed to allocate memory for DER encoding\n");
> +      err = GRUB_ERR_OUT_OF_MEMORY;
> +      goto error;
> +    }
> +
> +  ret = asn1_der_coding (tpm2key, "", der_buf, &der_buf_size, NULL);
> +  if (ret != ASN1_SUCCESS)
> +    {
> +      fprintf (stderr, "DER coding error: 0x%x\n", ret);
> +      err = GRUB_ERR_BAD_ARGUMENT;
> +      goto error;
> +    }
> +
> +  err = protect_write_file (args->tpm2_outfile, der_buf, der_buf_size);
> +  if (err != GRUB_ERR_NONE)
> +    fprintf (stderr, "Could not write tpm2key file (Error: %u).\n", errno);
> +
> + error:
> +  grub_free (der_buf);
> +
> +  if (tpm2key)
> +    asn1_delete_structure (&tpm2key);
> +
> +  return err;
> +}

[...]

> +static grub_err_t
> +protect_tpm2_args_verify (protect_args_t *args)
> +{
> +  switch (args->action)
> +    {
> +    case PROTECT_ACTION_ADD:
> +      if (args->args & PROTECT_ARG_TPM2_EVICT)
> +     {
> +       fprintf (stderr, N_("--tpm2-evict is invalid when --action is 
> 'add'.\n"));
> +       return GRUB_ERR_BAD_ARGUMENT;
> +     }
> +
> +      if (args->tpm2_keyfile == NULL)
> +     {
> +       fprintf (stderr, N_("--tpm2-keyfile must be specified.\n"));
> +       return GRUB_ERR_BAD_ARGUMENT;
> +     }
> +
> +      if (args->tpm2_outfile == NULL)
> +     {
> +       fprintf (stderr, N_("--tpm2-outfile must be specified.\n"));
> +       return GRUB_ERR_BAD_ARGUMENT;
> +     }
> +
> +      if (args->tpm2_device == NULL)
> +     args->tpm2_device = "/dev/tpm0";

When you move this "if" to the beginning of function or its end then you
can do this check once and drop one below...

> +      if (args->tpm2_pcr_count == 0)
> +     {
> +       args->tpm2_pcrs[0] = 7;
> +       args->tpm2_pcr_count = 1;
> +     }
> +
> +      if (args->srk_type.type == TPM_ALG_ERROR)
> +     {
> +       args->srk_type.type = TPM_ALG_ECC;
> +       args->srk_type.detail.ecc_curve = TPM_ECC_NIST_P256;
> +     }
> +
> +      if (args->tpm2_bank == TPM_ALG_ERROR)
> +     args->tpm2_bank = TPM_ALG_SHA256;
> +
> +      break;
> +
> +    case PROTECT_ACTION_REMOVE:
> +      if (args->args & PROTECT_ARG_TPM2_ASYMMETRIC)
> +     {
> +       fprintf (stderr, N_("--tpm2-asymmetric is invalid when --action is 
> 'remove'.\n"));
> +       return GRUB_ERR_BAD_ARGUMENT;
> +     }
> +
> +      if (args->args & PROTECT_ARG_TPM2_BANK)
> +     {
> +       fprintf (stderr, N_("--tpm2-bank is invalid when --action is 
> 'remove'.\n"));
> +       return GRUB_ERR_BAD_ARGUMENT;
> +     }
> +
> +      if (args->args & PROTECT_ARG_TPM2_KEYFILE)
> +     {
> +       fprintf (stderr, N_("--tpm2-keyfile is invalid when --action is 
> 'remove'.\n"));
> +       return GRUB_ERR_BAD_ARGUMENT;
> +     }
> +
> +      if (args->args & PROTECT_ARG_TPM2_OUTFILE)
> +     {
> +       fprintf (stderr, N_("--tpm2-outfile is invalid when --action is 
> 'remove'.\n"));
> +       return GRUB_ERR_BAD_ARGUMENT;
> +     }
> +
> +      if (args->args & PROTECT_ARG_TPM2_PCRS)
> +     {
> +       fprintf (stderr, N_("--tpm2-pcrs is invalid when --action is 
> 'remove'.\n"));
> +       return GRUB_ERR_BAD_ARGUMENT;
> +     }
> +
> +      if (args->tpm2_srk == 0)
> +     {
> +       fprintf (stderr, N_("--tpm2-srk is not specified when --action is 
> 'remove'.\n"));
> +       return GRUB_ERR_BAD_ARGUMENT;
> +     }
> +
> +      if (args->tpm2_device == NULL)
> +     args->tpm2_device = "/dev/tpm0";

... I mean from here...

> +      break;
> +
> +    default:
> +      fprintf (stderr, N_("The TPM2 key protector only supports the 
> following actions: add, remove.\n"));
> +      return GRUB_ERR_BAD_ARGUMENT;
> +    }
> +
> +  return GRUB_ERR_NONE;
> +}

[...]

> +static grub_err_t
> +protect_args_verify (protect_args_t *args)
> +{
> +  if (args->action == PROTECT_ACTION_ERROR)
> +    {
> +      fprintf (stderr, N_("--action is mandatory.\n"));
> +      return GRUB_ERR_BAD_ARGUMENT;
> +    }
> +
> +  /* At the moment, the only configurable key protector is the TPM2 one, so 
> it
> +   * is the only key protector supported by this tool. */

Wrong coding style for the comment...

> +  if (args->protector != PROTECT_TYPE_TPM2)
> +    {
> +      fprintf (stderr, N_("--protector is mandatory and only 'tpm2' is 
> currently supported.\n"));
> +      return GRUB_ERR_BAD_ARGUMENT;
> +    }
> +
> +  switch (args->protector)
> +    {
> +    case PROTECT_TYPE_TPM2:
> +      return protect_tpm2_args_verify (args);
> +    default:
> +      return GRUB_ERR_BAD_ARGUMENT;
> +    }
> +
> +  return GRUB_ERR_NONE;
> +}

[...]

> +int
> +main (int argc, char *argv[])
> +{
> +  grub_err_t err;

s/grub_err_t/int/

> +  protect_args_t args = {0};
> +
> +  if (argp_parse (&protect_argp, argc, argv, 0, 0, &args) != 0)
> +    {
> +      fprintf (stderr, N_("Could not parse arguments.\n"));
> +      return GRUB_ERR_BAD_ARGUMENT;

You expose GRUB internals to the user space and mix types. There is no
guarantee GRUB_ERR_BAD_ARGUMENT value will not change in the future.
So, I think you should return EXIT_FAILURE here.

> +    }
> +
> +  protect_init (&argc, &argv);
> +
> +  err = protect_args_verify (&args);

Ditto... The EXIT_SUCCESS and EXIT_FAILURE are your friends...

> +  if (err != GRUB_ERR_NONE)
> +    goto exit;
> +
> +  err = protect_dispatch (&args);

Ditto...

I did not check other patches but if you do the same thing elsewhere
please fix it.

> +  if (err != GRUB_ERR_NONE)
> +    goto exit;
> +
> + exit:
> +  protect_fini ();
> +
> +  return err;
> +}

Daniel