Re: [PATCH v13 20/20] tests: Add tpm2_test - Gary Lin via Grub-devel

From: Gary Lin via Grub-devel <grub-devel@gnu.org>
To: Glenn Washburn <development@efficientek.com>
Cc: Gary Lin <glin@suse.com>,
	The development of GNU GRUB <grub-devel@gnu.org>,
	Hernan Gatta <hegatta@linux.microsoft.com>,
	Daniel Axtens <dja@axtens.net>,
	Daniel Kiper <daniel.kiper@oracle.com>,
	shkhisti@microsoft.com, jaskaran.khurana@microsoft.com,
	christopher.co@microsoft.com, daniel.mihai@microsoft.com,
	jaredz@redhat.com, jejb@linux.ibm.com, mchang@suse.com,
	patrick.colp@oracle.com, Stefan Berger <stefanb@linux.ibm.com>
Subject: Re: [PATCH v13 20/20] tests: Add tpm2_test
Date: Mon, 29 Apr 2024 16:10:05 +0800	[thread overview]
Message-ID: <20240429081005.t6jcnjvlh56bdjaz@GaryLaptop> (raw)
In-Reply-To: <20240426171747.15c9ba49@crass-HP-ZBook-15-G2>

On Fri, Apr 26, 2024 at 05:18:04PM -0500, Glenn Washburn wrote:
> 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.
> 
Doing a full test with QEMU requires a more powerful testsuite such as
openQA to manipulate the OS to seal the disk secret to the TPM of the
VM. With grub-emu, at least we can test the TPM2 stack and verify the
result of tpm2_key_protector_init.

> > 
> > 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.
> 
Got it. Will fix it in v14.

> > +   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.
> 
I followed tests/util/grub-shell-luks-tester.in to create the test
directory and didn't notice the difference between 20 and 99.
Will fix it in v14.

> > +
> > +disksize=20M
> > +
> > +luksfile=$tpm2testdir/luks.disk
> 
> Use brace around variable name here as is done below, and also add
> braces for unbraced uses below.
> 
Alright, I'll go through the whole script again to make sure all
variables are braced.

> > +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.
> 
I'll try to modify the script to write vtext into the block.

> > +
> > +# 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?
> 
Will remove those 4 lines in v14.

> > +
> > +# 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.
> 
It's added when I merged another check into this loop. But yes, it's not
necessary.

> > +        break
> > +    fi
> > +    sleep 1
> > +    ((wait--))
> > +done
> > +if [ "$wait" -le 0 ]; then
> 
> Perhaps what you really want is '[ -z "${tpm2dev}" ]'.
> 
Thanks! This is certainly a better check.

> > +    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.
> 
No problem. Will update in v14.

> > +ret=$?
> > +if [ "$ret" -ne 0 ]; then
> > +    exit $ret
> > +fi
> 
> Again, this seems to be superfluous.
> 
Ditto

> > +
> > +# 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.
> 
Will remove "local" and check the variable names again.

> > +    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.
> 
Got it.

> > +
> > +    # 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.
> 
Will remove those '[[' in v14.

> > +            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.
> 
Will update the 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.
> 
Ditto

> > +
> > +    # 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.
> 
Then I'd just append ${qemuopts} to the grub-emu command since renaming
the '--qemu-opts' will affect other test scripts. This has to be done in
a separate patch set.

Gary Lin

> 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

_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel