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

[Glenn Washburn]

Hi Gary,

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

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

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

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

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

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