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

[Gary Lin]

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.

> > 
> > 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.

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.

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
> > 
>