Re: [PATCH v14 20/20] tests: Add tpm2_test

[Glenn Washburn]

On Fri,  3 May 2024 14:48:56 +0800
Gary Lin <glin@suse.com> wrote:

> For the tpm2 module, the TCG2 command submission function is the only
> difference between the a QEMU instance and grub-emu. To test TPM key
> unsealing with a QEMU instance, it requires an extra OS image to invoke
> grub-protect to seal the LUKS key, rather than a simple grub-shell rescue
> CD image. On the other hand, grub-emu can share the emulated TPM device
> with the host, so that we can seal the LUKS key on host and test key
> unsealing with grub-emu.
> 
> This test script firstly creates a simple LUKS image to be loaded as a
> loopback device in grub-emu. Then an emulated TPM device is created by
> swtpm_cuse and PCR 0 and 1 are extended.
> 
> There are several test cases in the script to test various settings. Each
> test case uses grub-protect or tpm2-tools to seal the LUKS password
> against PCR 0 and PCR 1. Then grub-emu is launched to load the LUKS image,
> try to mount the image with tpm2_key_protector_init and cryptomount, and
> verify the result.
> 
> Based on the idea from Michael Chang.
> 
> Cc: Michael Chang <mchang@suse.com>
> Cc: Stefan Berger <stefanb@linux.ibm.com>
> Cc: Glenn Washburn <development@efficientek.com>
> Signed-off-by: Gary Lin <glin@suse.com>
> ---
>  Makefile.util.def        |   6 +
>  tests/tpm2_test.in       | 306 +++++++++++++++++++++++++++++++++++++++
>  tests/util/grub-shell.in |   6 +-
>  3 files changed, 317 insertions(+), 1 deletion(-)
>  create mode 100644 tests/tpm2_test.in
> 
> diff --git a/Makefile.util.def b/Makefile.util.def
> index 40bfe713d..8d4c53a03 100644
> --- a/Makefile.util.def
> +++ b/Makefile.util.def
> @@ -1281,6 +1281,12 @@ script = {
>    common = tests/asn1_test.in;
>  };
>  
> +script = {
> +  testcase = native;
> +  name = tpm2_test;
> +  common = tests/tpm2_test.in;
> +};
> +
>  program = {
>    testcase = native;
>    name = example_unit_test;
> diff --git a/tests/tpm2_test.in b/tests/tpm2_test.in
> new file mode 100644
> index 000000000..75c042274
> --- /dev/null
> +++ b/tests/tpm2_test.in
> @@ -0,0 +1,306 @@
> +#! @BUILD_SHEBANG@ -e
> +
> +# Test GRUBs ability to unseal a LUKS key with TPM 2.0
> +# Copyright (C) 2024  Free Software Foundation, Inc.
> +#
> +# 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/>.
> +
> +grubshell=@builddir@/grub-shell
> +
> +. "@builddir@/grub-core/modinfo.sh"
> +
> +if [ x${grub_modinfo_platform} != xemu ]; then
> +  exit 77
> +fi
> +
> +builddir="@builddir@"
> +
> +# Force build directory components
> +PATH="${builddir}:${PATH}"
> +export PATH
> +
> +if [ "x${EUID}" = "x" ] ; then

Braces in quoted variables at the end of the quoted string don't need
braces. But no need to change it back.

> +  EUID=`id -u`
> +fi
> +
> +if [ "${EUID}" != 0 ] ; then
> +   echo "not root; cannot test tpm2."
> +   exit 99
> +fi
> +
> +if ! command -v cryptsetup >/dev/null 2>&1; then
> +   echo "cryptsetup not installed; cannot test tpm2."
> +   exit 99
> +fi
> +
> +if ! grep -q tpm_vtpm_proxy /proc/modules && ! modprobe tpm_vtpm_proxy; then
> +   echo "no tpm_vtpm_proxy support; cannot test tpm2."
> +   exit 99
> +fi
> +
> +if ! command -v swtpm >/dev/null 2>&1; then
> +   echo "swtpm not installed; cannot test tpm2."
> +   exit 99
> +fi
> +
> +if ! command -v tpm2_startup >/dev/null 2>&1; then
> +   echo "tpm2-tools not installed; cannot test tpm2."
> +   exit 99
> +fi
> +
> +tpm2testdir="`mktemp -d "${TMPDIR:-/tmp}/$(basename "$0").XXXXXXXXXX"`" || 
> exit 99
> +
> +disksize=20M
> +
> +luksfile=${tpm2testdir}/luks.disk

Its a good idea to have variables that can be user provided and thus
have spaces to be quoted. tpm2testdir and any variable contructed from
it fall in that category. Note that putting a variable in braces
unquoted, does not make implicit quoting.

> +lukskeyfile=${tpm2testdir}/password.txt
> +
> +# Choose a low iteration number to reduce the time to decrypt the disk
> +csopt="--type luks2 --pbkdf pbkdf2 --iter-time 1000"
> +
> +tpm2statedir=${tpm2testdir}/tpm
> +tpm2ctrl=${tpm2statedir}/ctrl
> +tpm2log=${tpm2statedir}/logfile
> +
> +sealedkey=${tpm2testdir}/sealed.tpm
> +
> +timeout=20
> +
> +testoutput=${tpm2testdir}/testoutput
> +
> +vtext="TEST VERIFIED"
> +
> +ret=0
> +
> +# Create the password file
> +echo -n "top secret" > ${lukskeyfile}
> +
> +# Setup LUKS2 image
> +truncate -s ${disksize} ${luksfile} || exit 21
> +cryptsetup luksFormat -q ${csopt} ${luksfile} ${lukskeyfile} || exit 22

You should never be exiting with an explicit value other than 77 or 99.
Everything else indicates a real failure in the test. I think you got
confused by reading grub-shell-luks-tester and seeing that it does not
follow that rule. The reason for that is because it is expected that
scripts using it will convert those exit codes to 99. Take a look at
grub_cmd_cryptomount and you will see that in the _testcase function.
So the big difference in this script and grub-shell-luks-tester is that
this script is run directly by the test harness, but
grub-shell-luks-tester is not.  Please change the rest of the exit
codes to 99.

> +
> +# Write vtext into the first block of the LUKS2 image
> +luksdev=/dev/mapper/`basename ${tpm2testdir}`
> +cryptsetup open --key-file ${lukskeyfile} ${luksfile} `basename ${luksdev}` 
> || exit 23
> +echo "${vtext}" | dd of=${luksdev} conv=notrunc status=none

I would think `echo "${vtext}" | cat > "${luksdev}"` would be sufficient
here. I'm curious if I'm wrong about that. Certainly "conv=notrunc"
seems superfluous as ${luksdev} is a device so no truncation happens
anyway. If true, then as is it may be confusing to readers. In general,
I think its better to use simpler commands when available (in this case
no need for arg parsing).

> +cryptsetup close ${luksdev}
> +
> +# Shutdown the swtpm instance on exit
> +cleanup() {
> +    RET=$?
> +    if [ -e "${tpm2ctrl}" ]; then
> +     swtpm_ioctl -s --unix ${tpm2ctrl}
> +    fi
> +    if [ "${RET}" -eq 0 ]; then
> +     rm -rf "$tpm2testdir" || :
> +    fi
> +}
> +trap cleanup EXIT INT TERM KILL QUIT
> +
> +mkdir -p ${tpm2statedir}
> +
> +# Create the swtpm chardev instance
> +swtpm chardev --vtpm-proxy --tpmstate dir=${tpm2statedir} \
> +     --tpm2 --ctrl type=unixio,path=${tpm2ctrl} \
> +     --flags startup-clear --daemon > ${tpm2log} || ret=$?
> +if [ "${ret}" -ne 0 ]; then
> +    echo "Failed to start swtpm chardev"
> +    exit ${ret}

This should also exit with 99, as this is not an actual failure of the
test, but a failure to setup the test. You could put the $ret in the
message echo'd, which would be helpful to someone debugging this
failure.

> +fi
> +
> +# Wait for tpm2 chardev
> +wait_count=3
> +for count in `seq 1 ${wait_count}`; do

Considering that this may run in a virtual machine where things will be
going much slower, perhaps instead use:

  `seq 1 ${tpm2timeout}`

And above after setting tpm2log, have a line like:

  tpm2timeout=${GRUB_TEST_SWTPM_DEFAULT_TIMEOUT:-3}

This way this timeout can be overridden if needed.

> +    sleep 1
> +
> +    tpm2dev=$(grep "New TPM device" ${tpm2log} | cut -d' ' -f 4)
> +    if [ -c "${tpm2dev}" ]; then
> +     wait_count=0
> +     break
> +    fi
> +done
> +if [ "${wait_count}" -ne 0 ]; then

I thought we agreed that it would be better to use [ -z "$tmp2log" ]
here. In which case, the wait_count variable seem unnecessary.

> +    echo "TPM device did not appear."
> +    exit QUIT

I believe this invalid usage of the exit command (I get errors using
this in both bash and sh). Regardless we should be exiting with 99 here.

> +fi
> +
> +# Export the TCTI variable for tpm2-tools
> +export TPM2TOOLS_TCTI="device:${tpm2dev}"
> +
> +# Extend PCR 0
> +tpm2_pcrextend 0:sha256=$(echo "test0" | sha256sum | cut -d ' ' -f 1)
> +
> +# Extend PCR 1
> +tpm2_pcrextend 1:sha256=$(echo "test1" | sha256sum | cut -d ' ' -f 1)

Both of the above commands should have " || exit 99" at the end to make
these hard errors.

> +
> +tpm2_seal_unseal() {
> +    srk_alg="$1"
> +    handle_type="$2"
> +    srk_test="$3"
> +
> +    grub_srk_alg=${srk_alg}
> +
> +    extra_opt=""
> +    extra_grub_opt=""
> +
> +    persistent_handle="0x81000000"
> +
> +    if [ "${handle_type}" = "persistent" ]; then
> +     extra_opt="--tpm2-srk=${persistent_handle}"
> +    fi
> +
> +    if [ "${srk_alg}" != "default" ]; then
> +     extra_opt="${extra_opt} --tpm2-asymmetric=${srk_alg}"
> +    fi
> +
> +    # Seal the password with grub-protect
> +    grub-protect ${extra_opt} \
> +     --tpm2-device=${tpm2dev} \
> +     --action=add \
> +     --protector=tpm2 \
> +     --tpm2key \
> +     --tpm2-bank=sha256 \
> +     --tpm2-pcrs=0,1 \
> +     --tpm2-keyfile=${lukskeyfile} \
> +     --tpm2-outfile=${sealedkey} || ret=$?
> +    if [ "${ret}" -ne 0 ]; then
> +     echo "Failed to seal the secret key"
> +     exit 1

The error code should be 99 as this is a hard error (ie a failure in
the setup). Perhaps this should be a "return" instead of "exit"
statement. This allows the caller to handle this. Since currently the
caller does nothing, using either one is effectively the same, but I
think return is better. Same for other uses of exit in functions.

> +    fi
> +
> +    # Flip the asymmetric algorithm in grub.cfg to trigger fallback SRKs
> +    if [ "${srk_test}" = "fallback_srk" ]; then
> +     if [ -z "${srk_alg##RSA*}" ]; then
> +         grub_srk_alg="ECC"
> +     elif [ -z "${srk_alg##ECC*}" ]; then
> +         grub_srk_alg="RSA"
> +     fi
> +    fi
> +
> +    if [ "${grub_srk_alg}" != "default" ] && [ "${handle_type}" != 
> "persistent" ]; then
> +     extra_grub_opt="-a ${grub_srk_alg}"
> +    fi
> +
> +    # Write the TPM unsealing script
> +    cat > ${tpm2testdir}/testcase.cfg <<EOF
> +loopback luks (host)${luksfile}
> +tpm2_key_protector_init -T (host)${sealedkey} ${extra_grub_opt}
> +if cryptomount -a --protector tpm2; then
> +    cat (crypto0)+1
> +fi
> +EOF
> +
> +    # Test TPM unsealing with the same PCR
> +    ${grubshell} --timeout=${timeout} --emu-opts="-t ${tpm2dev}" < 
> ${tpm2testdir}/testcase.cfg > ${testoutput} || ret=$?
> +
> +    # Remove the persistent handle
> +    if [ "${handle_type}" = "persistent" ]; then
> +     grub-protect \
> +             --tpm2-device=${tpm2dev} \
> +             --protector=tpm2 \
> +             --action=remove \
> +             --tpm2-srk=${persistent_handle} \
> +             --tpm2-evict

Probably should " || :" here to ignore a potential failure?

> +    fi
> +
> +    if [ "${ret}" -eq 0 ]; then
> +     if ! grep -q "^${vtext}$" "${testoutput}"; then
> +         echo "error: test not verified [`cat ${testoutput}`]" >&2
> +         exit 1

return as mentioned above.

> +     fi
> +    else
> +     echo "grub-emu exited with error: ${ret}" >&2
> +     exit ${ret}

ditto

> +    fi
> +}
> +
> +tpm2_seal_unseal_nv() {
> +    persistent_handle="0x81000000"
> +    primary_file=${tpm2testdir}/primary.ctx
> +    session_file=${tpm2testdir}/session.dat
> +    policy_file=${tpm2testdir}/policy.dat
> +    keypub_file=${tpm2testdir}/key.pub
> +    keypriv_file=${tpm2testdir}/key.priv
> +    name_file=${tpm2testdir}/sealing.name
> +    sealing_ctx_file=${tpm2testdir}/sealing.ctx
> +
> +    # Since we don't run a resource manager on our swtpm instance, it has
> +    # to flush the transient handles after tpm2_createprimary, tpm2_create
> +    # and tpm2_load to avoid the potential out-of-memory (0x902) errors.
> +    # Ref: 
> https://github.com/tpm2-software/tpm2-tools/issues/1338#issuecomment-469689398
> +
> +    # Create the primary object
> +    tpm2_createprimary -C o -g sha256 -G ecc -c ${primary_file} -Q
> +    tpm2_flushcontext -t
> +
> +    # Create the policy object
> +    tpm2_startauthsession -S ${session_file}
> +    tpm2_policypcr -S ${session_file} -l sha256:0,1 -L ${policy_file} -Q
> +    tpm2_flushcontext ${session_file}
> +
> +    # Seal the key into TPM
> +    tpm2_create -C ${primary_file} -u ${keypub_file} -r ${keypriv_file} -L 
> ${policy_file} -i ${lukskeyfile} -Q
> +    tpm2_flushcontext -t
> +    tpm2_load -C ${primary_file} -u ${keypub_file} -r ${keypriv_file} -n 
> ${name_file} -c ${sealing_ctx_file}
> +    tpm2_flushcontext -t
> +    tpm2_evictcontrol -C o -c ${sealing_ctx_file} ${persistent_handle} -Q

All of these can fail and their failure should cause a 99 error code.
Perhaps wrap them in a subshell and do " || return 99".

> +
> +    # Write the TPM unsealing script
> +    cat > ${tpm2testdir}/testcase.cfg <<EOF
> +loopback luks (host)${luksfile}
> +tpm2_key_protector_init --mode=nv --nvindex=${persistent_handle} --pcrs=0,1
> +if cryptomount -a --protector tpm2; then
> +    cat (crypto0)+1
> +fi
> +EOF
> +
> +    # Test TPM unsealing with the same PCR
> +    ${grubshell} --timeout=${timeout} --emu-opts="-t ${tpm2dev}" < 
> ${tpm2testdir}/testcase.cfg > ${testoutput} || ret=$?
> +
> +    # Remove the persistent handle
> +    grub-protect \
> +     --tpm2-device=${tpm2dev} \
> +     --protector=tpm2 \
> +     --action=remove \
> +     --tpm2-srk=${persistent_handle} \
> +     --tpm2-evict

||:, as mentioned above

> +
> +    if [ "${ret}" -eq 0 ]; then
> +     if ! grep -q "^${vtext}$" "${testoutput}"; then
> +         echo "error: test not verified [`cat ${testoutput}`]" >&2
> +         exit 1

return

> +     fi
> +    else
> +     echo "grub-emu exited with error: ${ret}" >&2
> +     exit ${ret}

return

Glenn

> +    fi
> +}
> +
> +tpm2_seal_unseal default transient no_fallback_srk
> +
> +tpm2_seal_unseal RSA transient no_fallback_srk
> +
> +tpm2_seal_unseal ECC transient no_fallback_srk
> +
> +tpm2_seal_unseal RSA persistent no_fallback_srk
> +
> +tpm2_seal_unseal ECC persistent no_fallback_srk
> +
> +tpm2_seal_unseal RSA transient fallback_srk
> +
> +tpm2_seal_unseal ECC transient fallback_srk
> +
> +tpm2_seal_unseal_nv
> +
> +exit 0
> diff --git a/tests/util/grub-shell.in b/tests/util/grub-shell.in
> index 496e1bab3..6847d869e 100644
> --- a/tests/util/grub-shell.in
> +++ b/tests/util/grub-shell.in
> @@ -75,6 +75,7 @@ work_directory=${WORKDIR:-`mktemp -d 
> "${TMPDIR:-/tmp}/grub-shell.XXXXXXXXXX"`} |
>  
>  . "${builddir}/grub-core/modinfo.sh"
>  qemuopts=
> +emuopts=
>  serial_port=com0
>  serial_null=
>  halt_cmd=halt
> @@ -281,6 +282,9 @@ for option in "$@"; do
>      --qemu-opts=*)
>       qs=`echo "$option" | sed -e 's/--qemu-opts=//'`
>       qemuopts="$qemuopts $qs" ;;
> +    --emu-opts=*)
> +     qs=`echo "$option" | sed -e 's/--emu-opts=//'`
> +     emuopts="$emuopts $qs" ;;
>      --disk=*)
>       dsk=`echo "$option" | sed -e 's/--disk=//'`
>       if [ ${grub_modinfo_platform} = emu ]; then
> @@ -576,7 +580,7 @@ elif [ x$boot = xemu ]; then
>      cat >"$work_directory/run.sh" <<EOF
>  #! @BUILD_SHEBANG@
>  SDIR=\$(realpath -e \${0%/*})
> -exec "$(realpath -e "${builddir}")/grub-core/grub-emu" -m 
> "\$SDIR/${device_map##*/}" --memdisk "\$SDIR/${roottar##*/}" -r memdisk -d 
> "/boot/grub"
> +exec "$(realpath -e "${builddir}")/grub-core/grub-emu" -m 
> "\$SDIR/${device_map##*/}" --memdisk "\$SDIR/${roottar##*/}" -r memdisk -d 
> "/boot/grub" ${emuopts}
>  EOF
>  else
>      cat >"$work_directory/run.sh" <<EOF