On Wed, May 08, 2024 at 03:25:29PM -0500, Glenn Washburn wrote: > On Tue, 7 May 2024 16:19:19 +0800 > Gary Lin <g...@suse.com> wrote: > > > On Mon, May 06, 2024 at 02:09:12PM -0500, Glenn Washburn wrote: > > > On Fri, 3 May 2024 14:48:56 +0800 > > > Gary Lin <g...@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 <mch...@suse.com> > > > > Cc: Stefan Berger <stef...@linux.ibm.com> > > > > Cc: Glenn Washburn <developm...@efficientek.com> > > > > Signed-off-by: Gary Lin <g...@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. > > > > > Ok, will quote those variables. > > > > > > +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. > > > > > Thanks for the explanation. I'll change 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). > > > > > Oh, I copied the code from my test script which works on a blank file > > and forgot to change it. The dd command is certainly unnecessary here. > > > > > > +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. > > > > > Copy that. > > > > > > +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. > > > > > Good idea. I'll change the timeout variable. > > > > > > + 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. > > > > > Ah, I forgot to mention that I ran into a situation that tpm2dev was > > fetched but the char device was actually not ready, so I'd like to make > > sure that tpm2dev is really available. > > I meant to say '[ -z "$tpm2dev" ]', but I think you understood. But > even that isn't quite right and I should have said '[ -c "$tpm2dev" ]'. > Did you try the "-c" test and it succeeded while the device really > wasn't available? That test should work (because that's what sets > wait_count) and would be preferable to checking wait_count. > I was trying to avoid [ -c "$tpm2dev" ] after the timeout loop since it's already checked in the loop. Anyway I figured out what I really want:
tpm2timeout=${GRUB_TEST_SWTPM_DEFAULT_TIMEOUT:-3} for count in `seq 1 ${tpm2timeout}`; do sleep 1 tpm2dev=$(grep "New TPM device" ${tpm2log} | cut -d' ' -f 4) if [ -c "${tpm2dev}" ]; then break elif [ "${count}" -eq "${tpm2timeout}" ]; then echo "TPM device did not appear." exit 99 fi done > > > > > > + 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. > > > > > Will fix it. > > > > > > +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. > > > > > Got it > > > > > > + > > > > +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. > > > > > Originally the return status was handled outside this function and it > > ended up with several duplicate code. To improve the readability, I > > moved the error handling code inside the function and made it exit > > directly. I'll flip 'exit' back to 'return' and handle the return status > > with a shorter code. > > > > > > + 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? > > > > > Got it. > > > > > > + 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". > > > > > Will move them into another shell script. > > Since these commands aren't generally useful outside this specific > context, I don't think another shell script would be desirable. Just > wrap them in a subshell, or wrap them in a new function. > I'd prefer a new function. Those are the commands to seal the key into a given NV index. A generic function may be a good reference for someone who wants to use the feature. Gary Lin > Glenn > > > > > > > + > > > > + # 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 > > > > > Will fix the return code. > > > > Gary Lin > > > > > 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 _______________________________________________ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel