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

[Glenn Washburn]

On Thu, 25 Apr 2024 16:02:06 +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.

I'm glad we're getting a test with this feature. Its also unfortunate
that the test only works on the emu platform, which I suspect is tested
less.

> 
> 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>
> Signed-off-by: Gary Lin <glin@suse.com>
> ---
>  Makefile.util.def        |   6 +
>  tests/tpm2_test.in       | 308 +++++++++++++++++++++++++++++++++++++++
>  tests/util/grub-shell.in |   6 +-
>  3 files changed, 319 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..19cb21849
> --- /dev/null
> +++ b/tests/tpm2_test.in
> @@ -0,0 +1,308 @@
> +#! @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
> +  EUID=`id -u`
> +fi
> +
> +if [ "$EUID" != 0 ] ; then
> +   echo "not root; cannot test tpm2."
> +   exit 99
> +fi
> +
> +if ! which cryptsetup >/dev/null 2>&1; then

Should be using "command" here (and in the other tests) as the external
command "which" may not be available. So perhaps this line instead:

  if ! command -v cryptsetup >/dev/null 2>&1; then

Same for subsequent usages of which.

> +   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 ! which swtpm >/dev/null 2>&1; then
> +   echo "swtpm not installed; cannot test tpm2."
> +   exit 99
> +fi
> +
> +if ! which 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 20

Why exit 20 here instead of 99 like other tests? It looks like you want
to differentiate different error conditions, but this will make the
test show as error, not hard error, which is what it is.

> +
> +disksize=20M
> +
> +luksfile=$tpm2testdir/luks.disk

Use brace around variable name here as is done below, and also add
braces for unbraced uses below.

> +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"
> +
> +# 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

This test currently only tests the return code of grub's cryptmount. It
would be better to do a more end-to-end test where $vtext is written to
the LUKS device and then read back to verify encryption and decryption
works. When doing the luks tests, I made a fat filesystem and a test
file with $vtext as the contents of the file. I think that's more than
necessary here. The annoying thing is that grub has limited tools for
getting specific bytes from a device. Instead of doing the verification
check in grub, grub could just output the first block of the grub
crypto device (eg. "cat (crypto0)+1"). Then check in the grub-shell
output as is currently done. As the check is currently done. This will
mean that a new line should be added after $vtext to make
sure $vtext is the only output on the line.

> +
> +# 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
> +    exit $ret
> +fi

I don't see the purpose of the above 4 lines. The shell is run with -e,
so it will exit on any unprocessed errors. The swtpm command if it
errors is unprocessed (no || ...). So will not use the above 4 lines,
but should do the equivalent internally. If swtpm has a success return
code, the above 4 lines don't matter. Am I missing something?

> +
> +# Wait for tpm2 chardev
> +wait=3
> +while [ "${wait}" -gt 0 ]; do
> +    tpm2dev=$(grep "New TPM device" ${tpm2log} | cut -d' ' -f 4)
> +    if [ -n "${tpm2dev}" ] && [ -c "${tpm2dev}" ]; then

The "[ -n "${tpm2dev}" ] &&" is superfluous as "[ -c '' ]" will never
be true.

> +        break
> +    fi
> +    sleep 1
> +    ((wait--))
> +done
> +if [ "$wait" -le 0 ]; then

Perhaps what you really want is '[ -z "${tpm2dev}" ]'.

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

The '<<<' only works in bash (as far as I know), not sh. Let's not use
it and instead use: echo -n "test0" | sha256sum. Same for other such
uses.

> +ret=$?
> +if [ "$ret" -ne 0 ]; then
> +    exit $ret
> +fi

Again, this seems to be superfluous.

> +
> +# Extend PCR 1
> +tpm2_pcrextend 1:sha256=$(sha256sum <<< "test1" | cut -d ' ' -f 1)
> +ret=$?
> +if [ "$ret" -ne 0 ]; then
> +    exit $ret
> +fi
> +
> +tpm2_seal_unseal() {
> +    local srk_alg="$1"

"local" is not available in sh. If these variables clash with others,
please rename them. Either way don't use local. In principle I think
local should be used where we can, but we're more interested in
portability.

> +    local handle_type="$2"
> +    local srk_test="$3"
> +
> +    local grub_srk_alg=${srk_alg}
> +
> +    local extra_opt=""
> +    local extra_grub_opt=""
> +
> +    local 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
> +    fi

This is not superfluous because of the error message, but it is
unreachable code. To do what you want, put the "ret=$?" as " || ret=$?"
on the grub-protect command.

> +
> +    # Flip the asymmetric algorithm in grub.cfg to trigger fallback SRKs
> +    if [ "${srk_test}" == "fallback_srk" ]; then
> +        if [[ "${srk_alg}" == RSA* ]]; then

'[[' is another bashism, unless there is a binary for it (I think
busybox does have). So let's not use it. Instead do:

  if [ -z "${srk_alg##RSA*}" ]; then

Likewise for other uses.

> +            grub_srk_alg="ECC"
> +        elif [[ "${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
> +    echo "${vtext}"
> +fi
> +EOF
> +
> +    # Test TPM unsealing with the same PCR
> +    ${grubshell} --timeout=$timeout --grub-emu-opts="-t ${tpm2dev}" < 
> ${tpm2testdir}/testcase.cfg > ${testoutput}
> +    ret=$?

This ret=$? is equivalent to ret=0, and so is really not needed. I
think you want to '||' it with the grubshell command.

> +
> +    # Remove the persistent handle
> +    if [ "${handle_type}" == "persistent" ]; then
> +     grub-protect \
> +             --tpm2-device=${tpm2dev} \
> +             --protector=tpm2 \
> +             --action=remove \
> +             --tpm2-srk=${persistent_handle} \
> +             --tpm2-evict
> +    fi
> +
> +    if [ "$ret" -eq 0 ]; then
> +     if ! grep -q "^${vtext}$" "$testoutput"; then
> +         echo "error: test not verified [`cat $testoutput`]" >&2
> +         exit 1
> +     fi
> +    else
> +     echo "grub-emu exited with error: $ret" >&2
> +     exit $ret
> +    fi
> +}
> +
> +tpm2_seal_unseal_nv() {
> +    local persistent_handle="0x81000000"
> +    local primary_file=${tpm2testdir}/primary.ctx
> +    local session_file=${tpm2testdir}/session.dat
> +    local policy_file=${tpm2testdir}/policy.dat
> +    local keypub_file=${tpm2testdir}/key.pub
> +    local keypriv_file=${tpm2testdir}/key.priv
> +    local name_file=${tpm2testdir}/sealing.name
> +    local 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
> +
> +    # 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
> +    echo "${vtext}"
> +fi
> +EOF
> +
> +    # Test TPM unsealing with the same PCR
> +    ${grubshell} --timeout=$timeout --grub-emu-opts="-t ${tpm2dev}" < 
> ${tpm2testdir}/testcase.cfg > ${testoutput}
> +    ret=$?

Same comment as in function above.

> +
> +    # Remove the persistent handle
> +    grub-protect \
> +     --tpm2-device=${tpm2dev} \
> +     --protector=tpm2 \
> +     --action=remove \
> +     --tpm2-srk=${persistent_handle} \
> +     --tpm2-evict
> +
> +    if [ "$ret" -eq 0 ]; then
> +     if ! grep -q "^${vtext}$" "$testoutput"; then
> +         echo "error: test not verified [`cat $testoutput`]" >&2
> +         exit 1
> +     fi
> +    else
> +     echo "grub-emu exited with error: $ret" >&2
> +     exit $ret
> +    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..f8642543d 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=
> +grubemuopts=
>  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" ;;
> +    --grub-emu-opts=*)
> +     qs=`echo "$option" | sed -e 's/--grub-emu-opts=//'`
> +     grubemuopts="$grubemuopts $qs" ;;

I'm on the fence on this. '--qemu-opts' could just be reused, although
it might be a little confusing, as it would be misnamed. It would be
nice to think of a way to combine --qemu-opts and --grub-emu-opts into
one appropriately named argument (perhaps just --emu-opts?). They are
mutually exclusive in the sense that they will both never be in effect
for a given target. Also, prefixing with 'grub' goes against the
existing naming scheme, so regardless should not be done.

Glenn

>      --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" ${grubemuopts}
>  EOF
>  else
>      cat >"$work_directory/run.sh" <<EOF