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 <hega...@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 <stef...@linux.ibm.com>
> > > Cc: James Bottomley <j...@linux.ibm.com>
> > > Signed-off-by: Hernan Gatta <hega...@linux.microsoft.com>
> > > Signed-off-by: Gary Lin <g...@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
_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
