Re: [PATCH v2 00/11] Automatic Disk Unlock with TPM2

[Glenn Washburn]

On 3/24/23 09:06, Gary Lin wrote:
> On Fri, Mar 24, 2023 at 03:22:43AM +0000, Glenn Washburn wrote:
> > Hi Gary,
> Hi Glenn,
>
> > Usually a v2 series will also include what changed from v1 -> v2 and
> > subsequent versions will keep the changelog. Also, for such a large series,
> > using --range-diff with git format-patch can be helpful for reviewers (it
> > will include here the exact changes to each patch from the last version of
> > the series).
> The difference between v1 and v2 is a bit huge. libtasn1 was added,
> several follow-up patches in v1 are merged into Hernan's patches. Not to
> mention the newly implemented TPM 2.0 Key. It made me feel that v2 is
> more like a fresh start.
>
> > On 3/22/23 08:10, Gary Lin wrote:
> > > This patch series is based on "Automatic TPM Disk Unlock"(*1) posted by
> > > Hernan Gatta to introduce the key protector framework and TPM2 stack
> > > to GRUB2, and this could be a useful feature for the systems to
> > > implement full disk encryption.
> > >
> > > To support TPM 2.0 Key File format(*2), patch 1~6 are grabbed from
> > > Daniel Axtens's "appended signature secure boot support" (*3) to import
> > > libtasn1 into grub2. Besides, the libtasn1 version is upgraded to
> > > 4.19.0 instead of 4.16.0 in the original patch.
> >
> > How was the upgrade process? Were there any gotchas? Regardless when
> > including new external libraries the update process should be included in
> > docs/grub-dev.texi under the section "Updating External Code".
> I followed the steps in the comment of Patch 2 to upgrade the lib. Will
> check how to write the steps into the document.
>
> > Similarly, I am not in favor of including this new user functionality
> > without properly documenting it in docs/grub.texi.
> >
> > Also, and I know this is a longshot, but it would be great to add tests for
> > the protector and TPM code. Probably the easiest option would be to create
> > non-native (ie QEMU) make check tests. QEMU can use an emulated software
> > TPM[1].
> Sure, I mainly tested the code with QEMU + swtpm.

Great, so sounds like you have a good idea how things need to be setup, 
that's a large part of making the test.

> > > Patch 7~11 are Hernan Gatta's patches with the follow-up fixes:
> > > - Converting 8 spaces into 1 tab
> >
> > This is good. However, I noticed that there are places in the patch
> > "cryptodisk: Support key protectors" where this did not happen. I've not
> > checked other patches. I'll note a specific instance in a reply to that
> > patch.
> I probably overlooked cryptodisk.c. Will check all affect files again.
>
> > > - Merging the minor build fix from Michael Chang
> > >     - Replacing "lu" with "PRIuGRUB_SIZE" for grub_dprintf
> > >     - Adding "enable = efi" to the tpm2 module in grub-core/Makefile.core.def
> > > - Rebasing "cryptodisk: Support key protectors" to the git master
> > > - Removing the measurement on the sealed key
> > >     - Based ont the patch from Olaf Kirch <OKir@suse.com>
> > > - Adjusting the input parameters of TPM2_EvictControl to match the order
> > >     in "TCG TPM2 Part3 Commands"
> > > - Declaring the input arguments of TPM2 functions as const
> > > - Resending TPM2 commands on TPM_RC_RETRY
> > > - Adding checks for the parameters of TPM2 commands
> > > - Packing the missing authorization command for TPM2_PCR_Read
> > > - Tweaking the TPM2 command functions to allow some parameters to be
> > >     NULL
> > > - Only enabling grub-protect for "efi" since the TPM2 stack currently
> > >     relies on the EFI TCG2 protocol to send TPM2 commands
> > > - Using grub_cpu_to_be*() in the TPM2 stack instead grub_swap_bytes*()
> > >     for the marshal functions
> > > - Changing the short name of "--protector" of "cryptomount" from "-k" to
> > >     "-P" to avoid the conflict with "--key-file"
> > > - Adding the primitive support for TPM 2.0 Key File Format besides the
> > >     raw sealed key
> > > - Adding the external libtasn1 dependency for grub-protect since
> > >     "asn1_write_value()" is disabled in the embedded libtasn1
> > >
> > > To utilize the TPM2 key protector to unlock the encrypted partition
> > > (sdb1), here are the sample steps:
> > >
> > > 1. Add an extra random key for LUKS (luks-key)
> > >      $ dd if=/dev/urandom of=luks-key bs=1 count=32
> > >      $ sudo cryptsetup luksAddKey /dev/sdb1 luks-key --pbkdf=pbkdf2
> > >
> > > 2. Seal the key
> > >      $ sudo grub-protect --action=add \
> > >                          --protector=tpm2 \
> > >                          --tpm2key \
> > >                          --tpm2-keyfile=luks-key \
> > >                          --tpm2-outfile=/boot/efi/boot/grub2/sealed.tpm
> > >
> > > 3. Unseal the key with the proper commands in grub.cfg:
> > >      tpm2_key_protector_init --tpm2key=(hd0,gpt1)/boot/grub2/sealed.tpm
> > >      cryptomount -u SDB1_UUID -P tpm2
> >
> > This is great, but as I'm sure you know this won't appear in the commit log
> > and as far as I can tell is not documented anywhere else. This would be nice
> > to add to the user documentation.
> The similar steps are also mentioned in the comment of Patch 11. Those
> steps give the overview of what these patches do, so I want them in the
> cover letter.

Great, I hadn't checked patch 11. And its more detailed there, so even 
better.

> Since this patchset introduces several new things, before we reach the
> consensus on the implementation, arguments, etc., I'd like to defer the
> user documentation work.

Yep, completely agree, my comment was meant to put/keep the 
documentation on the radar (lest it be forgot).

Glenn

> Thanks,
>
> Gary Lin
>
> > Glenn
> >
> > [1] 
> > https://qemu.readthedocs.io/en/latest/specs/tpm.html#the-qemu-tpm-emulator-device
> >
> > > (*1) https://lists.gnu.org/archive/html/grub-devel/2022-02/msg00006.html
> > > (*2) https://www.hansenpartnership.com/draft-bottomley-tpm2-keys.html
> > > (*3) https://lists.gnu.org/archive/html/grub-devel/2021-06/msg00044.html
> > >
> > > Daniel Axtens (6):
> > >     posix_wrap: tweaks in preparation for libtasn1
> > >     libtasn1: import libtasn1-4.19.0
> > >     libtasn1: disable code not needed in grub
> > >     libtasn1: changes for grub compatibility
> > >     libtasn1: compile into asn1 module
> > >     test_asn1: test module for libtasn1
> > >
> > > Hernan Gatta (5):
> > >     protectors: Add key protectors framework
> > >     tpm2: Add TPM Software Stack (TSS)
> > >     protectors: Add TPM2 Key Protector
> > >     cryptodisk: Support key protectors
> > >     util/grub-protect: Add new tool
> > >
> > >    .gitignore                                    |    2 +
> > >    Makefile.util.def                             |   29 +
> > >    configure.ac                                  |    9 +
> > >    grub-core/Makefile.am                         |    1 +
> > >    grub-core/Makefile.core.def                   |   42 +
> > >    grub-core/disk/cryptodisk.c                   |  175 +-
> > >    grub-core/kern/protectors.c                   |   75 +
> > >    grub-core/lib/libtasn1/COPYING                |   16 +
> > >    grub-core/lib/libtasn1/README.md              |   98 +
> > >    grub-core/lib/libtasn1/lib/coding.c           | 1433 ++++++++++
> > >    grub-core/lib/libtasn1/lib/decoding.c         | 2504 +++++++++++++++++
> > >    grub-core/lib/libtasn1/lib/element.c          | 1110 ++++++++
> > >    grub-core/lib/libtasn1/lib/element.h          |   42 +
> > >    grub-core/lib/libtasn1/lib/errors.c           |  103 +
> > >    grub-core/lib/libtasn1/lib/gstr.c             |   74 +
> > >    grub-core/lib/libtasn1/lib/gstr.h             |   50 +
> > >    grub-core/lib/libtasn1/lib/int.h              |  221 ++
> > >    grub-core/lib/libtasn1/lib/parser_aux.c       | 1179 ++++++++
> > >    grub-core/lib/libtasn1/lib/parser_aux.h       |  172 ++
> > >    grub-core/lib/libtasn1/lib/structure.c        | 1227 ++++++++
> > >    grub-core/lib/libtasn1/lib/structure.h        |   46 +
> > >    .../tests/CVE-2018-1000654-1_asn1_tab.h       |   32 +
> > >    .../tests/CVE-2018-1000654-2_asn1_tab.h       |   36 +
> > >    .../libtasn1_wrap/tests/CVE-2018-1000654.c    |   61 +
> > >    .../lib/libtasn1_wrap/tests/Test_overflow.c   |  138 +
> > >    .../lib/libtasn1_wrap/tests/Test_simple.c     |  207 ++
> > >    .../lib/libtasn1_wrap/tests/Test_strings.c    |  150 +
> > >    .../libtasn1_wrap/tests/object-id-decoding.c  |  116 +
> > >    .../libtasn1_wrap/tests/object-id-encoding.c  |  120 +
> > >    .../lib/libtasn1_wrap/tests/octet-string.c    |  211 ++
> > >    .../lib/libtasn1_wrap/tests/reproducers.c     |   81 +
> > >    grub-core/lib/libtasn1_wrap/wrap.c            |   26 +
> > >    grub-core/lib/libtasn1_wrap/wrap_tests.c      |   75 +
> > >    grub-core/lib/libtasn1_wrap/wrap_tests.h      |   38 +
> > >    grub-core/lib/posix_wrap/limits.h             |    1 +
> > >    grub-core/lib/posix_wrap/stdlib.h             |    8 +
> > >    grub-core/lib/posix_wrap/sys/types.h          |    1 +
> > >    grub-core/tpm2/args.c                         |  129 +
> > >    grub-core/tpm2/buffer.c                       |  145 +
> > >    grub-core/tpm2/module.c                       |  833 ++++++
> > >    grub-core/tpm2/mu.c                           |  807 ++++++
> > >    grub-core/tpm2/tcg2.c                         |  143 +
> > >    grub-core/tpm2/tpm2.c                         |  761 +++++
> > >    grub-core/tpm2/tpm2key.asn                    |   31 +
> > >    grub-core/tpm2/tpm2key.c                      |  218 ++
> > >    grub-core/tpm2/tpm2key_asn1_tab.c             |   34 +
> > >    include/grub/cryptodisk.h                     |   14 +
> > >    include/grub/libtasn1.h                       |  645 +++++
> > >    include/grub/protector.h                      |   48 +
> > >    include/grub/tpm2/buffer.h                    |   65 +
> > >    include/grub/tpm2/internal/args.h             |   39 +
> > >    include/grub/tpm2/internal/functions.h        |  117 +
> > >    include/grub/tpm2/internal/structs.h          |  675 +++++
> > >    include/grub/tpm2/internal/types.h            |  372 +++
> > >    include/grub/tpm2/mu.h                        |  292 ++
> > >    include/grub/tpm2/tcg2.h                      |   34 +
> > >    include/grub/tpm2/tpm2.h                      |   38 +
> > >    include/grub/tpm2/tpm2key.h                   |   40 +
> > >    tests/test_asn1.in                            |   12 +
> > >    util/grub-protect.c                           | 1472 ++++++++++
> > >    60 files changed, 16841 insertions(+), 32 deletions(-)
> > >    create mode 100644 grub-core/kern/protectors.c
> > >    create mode 100644 grub-core/lib/libtasn1/COPYING
> > >    create mode 100644 grub-core/lib/libtasn1/README.md
> > >    create mode 100644 grub-core/lib/libtasn1/lib/coding.c
> > >    create mode 100644 grub-core/lib/libtasn1/lib/decoding.c
> > >    create mode 100644 grub-core/lib/libtasn1/lib/element.c
> > >    create mode 100644 grub-core/lib/libtasn1/lib/element.h
> > >    create mode 100644 grub-core/lib/libtasn1/lib/errors.c
> > >    create mode 100644 grub-core/lib/libtasn1/lib/gstr.c
> > >    create mode 100644 grub-core/lib/libtasn1/lib/gstr.h
> > >    create mode 100644 grub-core/lib/libtasn1/lib/int.h
> > >    create mode 100644 grub-core/lib/libtasn1/lib/parser_aux.c
> > >    create mode 100644 grub-core/lib/libtasn1/lib/parser_aux.h
> > >    create mode 100644 grub-core/lib/libtasn1/lib/structure.c
> > >    create mode 100644 grub-core/lib/libtasn1/lib/structure.h
> > >    create mode 100644 
> > > grub-core/lib/libtasn1_wrap/tests/CVE-2018-1000654-1_asn1_tab.h
> > >    create mode 100644 
> > > grub-core/lib/libtasn1_wrap/tests/CVE-2018-1000654-2_asn1_tab.h
> > >    create mode 100644 grub-core/lib/libtasn1_wrap/tests/CVE-2018-1000654.c
> > >    create mode 100644 grub-core/lib/libtasn1_wrap/tests/Test_overflow.c
> > >    create mode 100644 grub-core/lib/libtasn1_wrap/tests/Test_simple.c
> > >    create mode 100644 grub-core/lib/libtasn1_wrap/tests/Test_strings.c
> > >    create mode 100644 grub-core/lib/libtasn1_wrap/tests/object-id-decoding.c
> > >    create mode 100644 grub-core/lib/libtasn1_wrap/tests/object-id-encoding.c
> > >    create mode 100644 grub-core/lib/libtasn1_wrap/tests/octet-string.c
> > >    create mode 100644 grub-core/lib/libtasn1_wrap/tests/reproducers.c
> > >    create mode 100644 grub-core/lib/libtasn1_wrap/wrap.c
> > >    create mode 100644 grub-core/lib/libtasn1_wrap/wrap_tests.c
> > >    create mode 100644 grub-core/lib/libtasn1_wrap/wrap_tests.h
> > >    create mode 100644 grub-core/tpm2/args.c
> > >    create mode 100644 grub-core/tpm2/buffer.c
> > >    create mode 100644 grub-core/tpm2/module.c
> > >    create mode 100644 grub-core/tpm2/mu.c
> > >    create mode 100644 grub-core/tpm2/tcg2.c
> > >    create mode 100644 grub-core/tpm2/tpm2.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/libtasn1.h
> > >    create mode 100644 include/grub/protector.h
> > >    create mode 100644 include/grub/tpm2/buffer.h
> > >    create mode 100644 include/grub/tpm2/internal/args.h
> > >    create mode 100644 include/grub/tpm2/internal/functions.h
> > >    create mode 100644 include/grub/tpm2/internal/structs.h
> > >    create mode 100644 include/grub/tpm2/internal/types.h
> > >    create mode 100644 include/grub/tpm2/mu.h
> > >    create mode 100644 include/grub/tpm2/tcg2.h
> > >    create mode 100644 include/grub/tpm2/tpm2.h
> > >    create mode 100644 include/grub/tpm2/tpm2key.h
> > >    create mode 100644 tests/test_asn1.in
> > >    create mode 100644 util/grub-protect.c