Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve TPM extend ops output in normal and DEBUG mode #1758

Merged
merged 5 commits into from
Sep 6, 2024
Merged

Improve TPM extend ops output in normal and DEBUG mode #1758

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
5299266
Improve TPM Extend infor in normal and DEBUG mode
tlaurion Aug 24, 2024
7ca8d42
tpmr : match desc of function in comment
tlaurion Aug 28, 2024
250a144
Merge remote-tracking branch 'osresearch/master' into HEAD
tlaurion Sep 6, 2024
de7902f
cbfs-init, insmod: Bring back params/filenames into PCR measurements
JonathonHall-Purism Sep 5, 2024
77d4be1
TPM extend ops: Augment output of TPM1/TMP22 for filename and file co…
tlaurion Sep 6, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 4 additions & 7 deletions initrd/bin/cbfs-init
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,17 +15,14 @@ cbfsfiles=`cbfs -t 50 -l 2>/dev/null | grep "^heads/initrd/"`
for cbfsname in `echo $cbfsfiles`; do
filename=${cbfsname:12}
if [ ! -z "$filename" ]; then
echo "Loading $filename from CBFS"
mkdir -p `dirname $filename` \
|| die "$filename: mkdir failed"
cbfs -t 50 -r $cbfsname > "$filename" \
echo "Extracting CBFS file $cbfsname into $filename"
cbfs -t 50 $CBFS_ARG -r $cbfsname > "$filename" \
|| die "$filename: cbfs file read failed"
if [ "$CONFIG_TPM" = "y" ]; then
TMPFILE=/tmp/cbfs.$$
echo "$filename" > $TMPFILE
cat $filename >> $TMPFILE
DEBUG "Extending TPM PCR $CONFIG_PCR with $filename"
tpmr extend -ix "$CONFIG_PCR" -if $TMPFILE \
echo "TPM: Extending PCR[$CONFIG_PCR with] with $filename"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This used to also extend with the CBFS file name. I think we should continue to do that:

tpmr extend -ix "$CONFIG_PCR" -ic "$filename"

While it's hard to reason about whether an attack could be possible by moving the entire content of one file to another in some way that doesn't invalidate the PCRs, it's easy to just extend with the file name and be sure there's no such attack.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@JonathonHall-Purism same comment #1758 (comment)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@JonathonHall-Purism We do not to hash the filenames; we want to hash with file content.

Only occurrences in code of TPM PCR exttends -ic ops are to extend PCR[4] with hashes of literals:

user@heads-tests-deb12-nix:~/heads$ grep -Rn "\-ic" ./initrd/
./initrd/bin/kexec-select-boot:388:			tpmr extend -ix 4 -ic generic ||
./initrd/bin/usb-init:11:	tpmr extend -ix 4 -ic usb
./initrd/bin/tpmr:241:		-ic)
./initrd/bin/kexec-insert-key:69:tpmr extend -ix 4 -ic generic ||
grep: ./initrd/etc/mtab: No such file or directory
./initrd/etc/ash_functions:245:		tpmr extend -ix 4 -ic recovery

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We do not to hash the filenames; we want to hash with file content.

It used to hash both. Now it only hashes the content.

This appears to open up an attack by pivoting file contents or renaming files.

For example, say there were only two such files:

  • heads/initrd/etc/config.user: echo CONFIG_RESTRICTED_BOOT=y....
  • heads/initrd/.gnupg/pubring.gpg: some GPG key data

An attacker could manipulate CBFS to the following without affecting the measured PCR state.

  • heads/initrd/etc/config.user.ignore_me_haha: echo CONFIG_RESTRICTED_BOOT=y...
  • heads/initrd/.gnupg/pubring.gpg.whoops_no_more_gpg: some GPG key data

Heads would construct the same PCR measurements in this PR because it is only measuring the file content. But the restricted boot config and GPG keyring will be gone, since they've been renamed in such a way that they'll be ignored.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(should spell this out 🙂 ) - So my point is you do need to hash both the filename and the content.

It used to do this by putting both the filename and content in a temp file, and hashing that.

A better strategy would be to extend once with the filename, then extend a second time with the file content. (That way the filename and content are clearly separated, you couldn't move the last few letters of the filename to the content, or something like that.)

So include tpmr extend -ix "$CONFIG_PCR" -ic "$filename" before hashing the file content.

tpmr extend -ix "$CONFIG_PCR" -if $filename \
|| die "$filename: tpm extend failed"
fi
fi
Expand Down
4 changes: 2 additions & 2 deletions initrd/bin/flash.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,9 +48,9 @@ flash_rom() {
dd if=/tmp/pchstrp9.bin bs=1 count=4 seek=292 of=/tmp/${CONFIG_BOARD}.rom conv=notrunc >/dev/null 2>&1
fi

warn "Do not power off computer. Updating firmware, this will take a few minutes..."
warn "Do not power off computer. Updating firmware, this will take a few minutes"
flashrom $CONFIG_FLASHROM_OPTIONS -w /tmp/${CONFIG_BOARD}.rom 2>&1 \
|| die "$ROM: Flash failed"
|| recovery "$ROM: Flash failed"
fi
}

Expand Down
2 changes: 1 addition & 1 deletion initrd/bin/kexec-insert-key
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ if ! kexec-unseal-key "$INITRD_DIR/secret.key"; then
fi

# Override PCR 4 so that user can't read the key
DEBUG "Extending TPM PCR 4 to prevent further secret unsealing"
echo "TPM: Extending PCR[4] to prevent any future secret unsealing"
JonathonHall-Purism marked this conversation as resolved.
Show resolved Hide resolved
tpmr extend -ix 4 -ic generic ||
die 'Unable to scramble PCR'

Expand Down
4 changes: 2 additions & 2 deletions initrd/bin/kexec-select-boot
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -384,9 +384,9 @@ while true; do
if [ "$CONFIG_TPM" = "y" ]; then
if [ ! -r "$TMP_KEY_DEVICES" ]; then
# Extend PCR4 as soon as possible
DEBUG "Extending TPM PCR 4 to prevent further secret unsealing"
DEBUG "TPM: Extending PCR[4] to prevent further secret unsealing"
tpmr extend -ix 4 -ic generic ||
die "Failed to extend PCR 4"
die "Failed to extend TPM PCR[4]"
fi
fi

Expand Down
2 changes: 1 addition & 1 deletion initrd/bin/qubes-measure-luks
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,6 @@ sha256sum /tmp/lukshdr-* >/tmp/luksDump.txt || die "Unable to hash LUKS headers"
DEBUG "Removing /tmp/lukshdr-*"
rm /tmp/lukshdr-*

DEBUG "Extending TPM PCR 6 with hash of LUKS headers from /tmp/luksDump.txt"
echo "TPM: Extending PCR[6] with hash of LUKS headers from /tmp/luksDump.txt"
tpmr extend -ix 6 -if /tmp/luksDump.txt ||
die "Unable to extend PCR"
24 changes: 17 additions & 7 deletions initrd/bin/tpmr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,11 +29,11 @@ else
. /etc/config
fi

TRACE_FUNC

# Busybox xxd lacks -r, and we get hex dumps from TPM1 commands. This converts
# a hex dump to binary data using sed and printf
hex2bin() {
TRACE_FUNC
sed 's/\([0-9A-F]\{2\}\)/\\\\\\x\1/gI' | xargs printf
}

Expand All @@ -43,6 +43,7 @@ hex2bin() {
# as a file still chokes if the password begins with 'hex:', oddly tpm2-tools
# accepts 'hex:' in the file content.)
tpm2_password_hex() {
TRACE_FUNC
echo "hex:$(echo -n "$1" | xxd -p | tr -d ' \n')"
}

Expand All @@ -61,7 +62,7 @@ tpm2_pcrread() {

if [ -z "$APPEND" ]; then
# Don't append - truncate file now so real command always
# appends
# overwrites
true >"$file"
fi

Expand All @@ -79,7 +80,7 @@ tpm1_pcrread() {

if [ -z "$APPEND" ]; then
# Don't append - truncate file now so real command always
# appends
# overwrites
true >"$file"
fi

Expand All @@ -102,9 +103,10 @@ is_hash() {
# usage:
# extend_pcr_state <alg> <initial_state> <files/hashes...>
tlaurion marked this conversation as resolved.
Show resolved Hide resolved
# alg - either 'sha1' or 'sha256' to specify algorithm
# initial_state - a hash value setting the initial state
# state - a hash value setting the initial state
# files/hashes... - any number of files or hashes, state is extended once for each item
extend_pcr_state() {
TRACE_FUNC
local alg="$1"
local state="$2"
local next extend
Expand Down Expand Up @@ -250,7 +252,8 @@ tpm2_extend() {
esac
done
tpm2 pcrextend "$index:sha256=$hash"
DO_WITH_DEBUG tpm2 pcrread "sha256:$index"
tpm2 pcrread "sha256:$index"
DEBUG "TPM: Extended PCR[$index] with $hash"
JonathonHall-Purism marked this conversation as resolved.
Show resolved Hide resolved
}

tpm2_counter_read() {
Expand Down Expand Up @@ -348,9 +351,9 @@ tpm2_startsession() {
die "tpm2_flushcontext: unable to flush saved session"
tpm2 readpublic -Q -c "$PRIMARY_HANDLE" -t "$PRIMARY_HANDLE_FILE"
#TODO: do the right thing to not have to suppress "WARN: check public portion the tpmkey manually" see https://github.com/linuxboot/heads/pull/1630#issuecomment-2075120429
tpm2 startauthsession -Q -c "$PRIMARY_HANDLE_FILE" --hmac-session -S "$ENC_SESSION_FILE" 2>&1 > /dev/null
tpm2 startauthsession -Q -c "$PRIMARY_HANDLE_FILE" --hmac-session -S "$ENC_SESSION_FILE" > /dev/null 2>&1
JonathonHall-Purism marked this conversation as resolved.
Show resolved Hide resolved
#TODO: do the right thing to not have to suppress "WARN: check public portion the tpmkey manually" see https://github.com/linuxboot/heads/pull/1630#issuecomment-2075120429
tpm2 startauthsession -Q -c "$PRIMARY_HANDLE_FILE" --hmac-session -S "$DEC_SESSION_FILE" 2>&1 > /dev/null
tpm2 startauthsession -Q -c "$PRIMARY_HANDLE_FILE" --hmac-session -S "$DEC_SESSION_FILE" > /dev/null 2>&1
tpm2 sessionconfig -Q --disable-encrypt "$DEC_SESSION_FILE"
}

Expand Down Expand Up @@ -381,6 +384,7 @@ cleanup_shred() {
# tpm2_destroy: Destroy a sealed file in the TPM. The mechanism differs by
# TPM version - TPM2 evicts the file object, so it no longer exists.
tpm2_destroy() {
TRACE_FUNC
index="$1" # Index of the sealed file
size="$2" # Size of zeroes to overwrite for TPM1 (unused in TPM2)

Expand All @@ -396,6 +400,7 @@ tpm2_destroy() {
# TPM version - TPM1 overwrites the file with zeroes, since this can be done
# without authorization. (Deletion requires authorization.)
tpm1_destroy() {
TRACE_FUNC
index="$1" # Index of the sealed file
size="$2" # Size of zeroes to overwrite for TPM1

Expand Down Expand Up @@ -761,6 +766,10 @@ if [ "$CONFIG_TPM2_TOOLS" != "y" ]; then
shift
tpm1_destroy "$@"
;;
extend)
DEBUG "TPM: Extending PCR[$3] with $5"
JonathonHall-Purism marked this conversation as resolved.
Show resolved Hide resolved
DO_WITH_DEBUG exec tpm "$@"
;;
seal)
shift
tpm1_seal "$@"
Expand Down Expand Up @@ -799,6 +808,7 @@ calcfuturepcr)
replay_pcr "sha256" "$@"
;;
extend)
DEBUG "TPM: Extending PCR[$2] with $4"
tpm2_extend "$@"
;;
counter_read)
Expand Down
2 changes: 1 addition & 1 deletion initrd/etc/ash_functions
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -241,7 +241,7 @@ recovery() {
DEBUG "Board $CONFIG_BOARD - version $(fw_version)"

if [ "$CONFIG_TPM" = "y" ]; then
DEBUG "Extending TPM PCR 4 for recovery shell access"
echo "TPM: Extending PCR[4] to prevent any further secret unsealing"
JonathonHall-Purism marked this conversation as resolved.
Show resolved Hide resolved
tpmr extend -ix 4 -ic recovery
fi

Expand Down
11 changes: 4 additions & 7 deletions initrd/sbin/insmod
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ fi
# Unify lsmod output to use - instead of _ for comparison
module_name=$(basename "$MODULE" | sed 's/_/-/g' | sed 's/\.ko$//')
if lsmod | sed 's/_/-/g' | grep -q "^$module_name\\b"; then
DEBUG "$MODULE: already loaded"
DEBUG "$MODULE: already loaded, skipping"
exit 0
fi

Expand All @@ -39,17 +39,14 @@ if [ ! -r /sys/class/tpm/tpm0/pcrs -o ! -x /bin/tpm ]; then
fi

if [ -z "$tpm_missing" ]; then
DEBUG "Extending TPM PCR $MODULE_PCR with $MODULE prior of usage"
echo "TPM: Extending PCR[$MODULE_PCR] with $MODULE prior of loading into kernel"
tpmr extend -ix "$MODULE_PCR" -if "$MODULE" \
|| die "$MODULE: tpm extend failed"
fi

if [ ! -z "$*" -a -z "$tpm_missing" ]; then
DEBUG "Extending TPM PCR $MODULE_PCR with $*"
TMPFILE=/tmp/insmod.$$
echo "$@" > $TMPFILE
DEBUG "Extending TPM PCR $MODULE_PCR with $MODULE prior of usage"
tpmr extend -ix "$MODULE_PCR" -if $TMPFILE \
echo "TPM: Extending PCR[$MODULE_PCR] with $MODULE prior of loading into kernel"
tpmr extend -ix "$MODULE_PCR" -if "$MODULE" \
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was supposed to extend the PCR with the module arguments if any were given, but now it's extending a second time with the module itself. So inserting the module with different arguments won't yield a different PCR digest.

I think you need to revert this change. If you don't want a temp file, maybe you could do something with -if /dev/stdin?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you could use `tpmr extend -ix "$MODULE_PCR" -ic "$*"

-ic extends with a literal value

Copy link
Collaborator Author

@tlaurion tlaurion Aug 28, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was supposed to extend the PCR with the module arguments if any were given, but now it's extending a second time with the module itself. So inserting the module with different arguments won't yield a different PCR digest.

I think you need to revert this change. If you don't want a temp file, maybe you could do something with -if /dev/stdin?

@JonathonHall-Purism : I'm not sure I follow. Traces better then words.
Called mount-usb to simulate HTOP auto-loading usb modules prior of HOTP related code, with same PCR[5] extend ops. Of course here in DEBUG so traces, so that hashes of files are outputted under /tmp/debug.log (which is the improvement here toward introspection):

~ # grep PCR /tmp/debug.log 
DEBUG: TPM: Extending PCR[7] with /.gnupg/pubring.kbx
DEBUG: TPM: Extended PCR[7] with 9cbb20eb7c942f83e25cba463d949ef2ac3b01ff092ffa9ebf74b7ce004e4368
DEBUG: TPM: Extending PCR[7] with /.gnupg/trustdb.gpg
DEBUG: TPM: Extended PCR[7] with 0f1a689ad3d6e75d26ab1ba9a99cb3b412f59f80e9e982b3cabdf175242e1d0b
DEBUG: TPM: Extending PCR[4] with recovery
DEBUG: TPM: Extended PCR[4] with 8c585378513f5f7a2e1456ee54042605fdb890392becefadd2ab180fd02fb341
DEBUG: TPM: Extending PCR[5] with /lib/modules/ehci-hcd.ko
DEBUG: TPM: Extended PCR[5] with 40c5206f06702e45d8e6632632255258af433be0641c96f514ea75ac14523a30
DEBUG: TPM: Extending PCR[5] with /lib/modules/uhci-hcd.ko
DEBUG: TPM: Extended PCR[5] with 1877332107fb8737a5636da26d4db2c10ffe4d1db2bcbde30b47774cdf05e02f
DEBUG: TPM: Extending PCR[5] with /lib/modules/ohci-hcd.ko
DEBUG: TPM: Extended PCR[5] with 8a12ce4abfc87f11a023d4f1c26c225f5cffae248f9dad1fd30e78022996df02
DEBUG: TPM: Extending PCR[5] with /lib/modules/ohci-pci.ko
DEBUG: TPM: Extended PCR[5] with 2065ee6544d78a5d31e67983166a9b8cf60dbe61bf0ee99c39e92816cc3a98db
DEBUG: TPM: Extending PCR[5] with /lib/modules/ehci-pci.ko
DEBUG: TPM: Extended PCR[5] with 116145df2c495dfd58354025799fe5bb9b4d8e078960e8d0d7ceda746e4f2d06
DEBUG: TPM: Extending PCR[5] with /lib/modules/xhci-hcd.ko
DEBUG: TPM: Extended PCR[5] with 7f5a6bd0f7de6104e49374e1e5ce421e11795fcc4f53014ef9259d630d7876bc
DEBUG: TPM: Extending PCR[5] with /lib/modules/xhci-pci.ko
DEBUG: TPM: Extended PCR[5] with 5502fa8c101f7e509145b9826094f06dd0e225c2311a14edc9ae9c812518a250
DEBUG: TPM: Extending PCR[5] with /lib/modules/usb-storage.ko
DEBUG: TPM: Extended PCR[5] with e01a3fbe9510da3ebb83f70126652edc99b8acac89d05ed0a4b6b8f35aafdf4c

~ # sha256sum /.gnupg/pubring.kbx
9cbb20eb7c942f83e25cba463d949ef2ac3b01ff092ffa9ebf74b7ce004e4368  /.gnupg/pubring.kbx
~ # sha256sum /.gnupg/trustdb.gpg
0f1a689ad3d6e75d26ab1ba9a99cb3b412f59f80e9e982b3cabdf175242e1d0b  /.gnupg/trustdb.gpg
~ # sha256sum /lib/modules/ehci-hcd.ko
40c5206f06702e45d8e6632632255258af433be0641c96f514ea75ac14523a30  /lib/modules/ehci-hcd.ko
~ # sha256sum /lib/modules/uhci-hcd.ko
1877332107fb8737a5636da26d4db2c10ffe4d1db2bcbde30b47774cdf05e02f  /lib/modules/uhci-hcd.ko
~ # sha256sum /lib/modules/ohci-hcd.ko
8a12ce4abfc87f11a023d4f1c26c225f5cffae248f9dad1fd30e78022996df02  /lib/modules/ohci-hcd.ko
~ # sha256sum /lib/modules/ohci-pci.ko
2065ee6544d78a5d31e67983166a9b8cf60dbe61bf0ee99c39e92816cc3a98db  /lib/modules/ohci-pci.ko
~ # sha256sum /lib/modules/ehci-pci.ko
116145df2c495dfd58354025799fe5bb9b4d8e078960e8d0d7ceda746e4f2d06  /lib/modules/ehci-pci.ko
~ # sha256sum /lib/modules/xhci-hcd.ko
7f5a6bd0f7de6104e49374e1e5ce421e11795fcc4f53014ef9259d630d7876bc  /lib/modules/xhci-hcd.ko
~ # sha256sum /lib/modules/xhci-pci.ko
5502fa8c101f7e509145b9826094f06dd0e225c2311a14edc9ae9c812518a250  /lib/modules/xhci-pci.ko
~ # sha256sum /lib/modules/usb-storage.ko
e01a3fbe9510da3ebb83f70126652edc99b8acac89d05ed0a4b6b8f35aafdf4c  /lib/modules/usb-storage.ko

So basically, I only changed removal of tmp file altogether in pointed code.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See also #1758 (comment)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So basically, I only changed removal of tmp file altogether in pointed code.

No, you changed what the PCR is extended with. It was not just extended with the module file content before, it was extended with the module parameters and the module file content.

In the PR, changing the module parameters will no longer affect the resulting PCR measurement, but it does affect the module behavior.

Granted, today we don't insert any modules with parameters. So I guess a change to actually include a module parameter would also necessarily change the initrd. But I think it's an unnecessary risk to introduce this possible vulnerability down the road, since it was there before, since it's easy to address.

In other words - we currently do the following: insmod /lib/modules/usbhid.ko

You could instead insert it with a module parameter, like quirks: insmod /lib/modules/usbhid.ko quirks=0x1234:0x5678:0x55AA

Prior to this PR, those resulted in different PCR values, which I think is important since this changes the behavior of the module in ways unknown to insmod. The PR no longer extends with the module parameters, so those two insmods now result in the same PCR but different kernel behavior.

So IMO add tpmr extend -ix "$MODULE_PCR" -ic "$*" before extending with the module file content.

|| die "$MODULE: tpm extend on arguments failed"
fi

Expand Down