-
-
Notifications
You must be signed in to change notification settings - Fork 196
HOTP asked to be resealed even if TOTP good (Picks up on a reinstalled OS even if firmware measurements haven't changed) #1935
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.
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
base: master
Are you sure you want to change the base?
Conversation
@JonathonHall-Purism in debug. This is To show 280cb1f So now user is locked in into following steps:
Full debug log of the combined steps (user doing Reset TPM as asked up to setting TPM DUK) with way more interesting call stack for everyone to understand: Note: |
To be clear - previously after OS reinstall (including removal of /boot/kexec* files) the recommendation was to do full TPM reset, and now just resealing the HOTP secret should work, right? |
@@ -29,7 +29,13 @@ mount_boot_or_die | |||
#counter_value=$(read_tpm_counter $counter | cut -f2 -d ' ' | awk 'gsub("^000e","")') | |||
# | |||
|
|||
counter_value=$(cat $HOTP_COUNTER) | |||
#if HOTP_COUNTER is not present, bail out | |||
if [ ! -f $HOTP_COUNTER ]; then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@marmarek the issue was that previously, if TOTPM previously sealed/unsealed (measured boot from coreboot+heads), Heads was not looking to see if HOTP counter under /boot/kexec_hotp_counter was still present.
@@ -280,7 +269,10 @@ update_hotp() { | |||
HOTP='N/A' | |||
fi | |||
|
|||
if [[ "$CONFIG_TPM" = n && "$HOTP" = "Invalid code" ]]; then | |||
if [[ "$HOTP" = "Invalid code" ]]; then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This check only verified if HOTP was invalid if no TPM was in use.
So now, if there is no /boot/kexec_hotp_counter and TPMTOTP can unseal, user is promoted to reseal HOTP alone (OS reinstall use case without firmware upgrade)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But what is we have kexec_rollback.txt? All of this doesn't make any sense: user should reset TPM here of more logic needs to be refactored.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@marmarek added comments in code: with this PR, HOTP counter not being present will guide user to only reseal HOTP and generate hashes and /boot detached signed digest files, as well as selecting default boot and propose to set TPM DUK.
So TPM reset is not needed anymore in case of OS re-installation for TPM1.
For TPM2, TPM primary handle still needs to be created+hashed, which is TPM Reset is advised for creation and hash creation advised for in output as well.
This is workaround for you issue.
6159012
to
75e766a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @tlaurion, strategy looks good to me and I left comments on some of the details 💯
initrd/etc/functions
Outdated
@@ -789,12 +820,17 @@ increment_tpm_counter() { | |||
TRACE_FUNC | |||
tpmr counter_increment -ix "$1" -pwdc '' | | |||
tee /tmp/counter-$1 >/dev/null 2>&1 || | |||
die "TPM counter increment failed for rollback prevention. Please reset the TPM" | |||
die "TPM counter increment failed for rollback prevention. Please reset the TPM. Press Enter to continue" | |||
#TODO: why the die here needs to say to Press Enter to continue? Should be part of die call? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe because oem-factory-reset has its own die()? It's not equivalent to this one though so you can't just delete it (kills some TOP_PID it captured rather than exiting) 🤔
5cfcc73
to
47c696f
Compare
@@ -719,7 +719,7 @@ tpm1_reset() { | |||
DO_WITH_DEBUG tpm physicalsetdeactivated -c &>/dev/null | |||
DO_WITH_DEBUG tpm forceclear &>/dev/null | |||
DO_WITH_DEBUG tpm physicalenable &>/dev/null | |||
DO_WITH_DEBUG tpm takeown -pwdo "$tpm_owner_password" &>/dev/null | |||
DO_WITH_DEBUG --mask-position 3 tpm takeown -pwdo "$tpm_owner_password" &>/dev/null |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DEBUG was exposing TPM owner passphrase on log + debug.log
c38c13b
to
dae9e85
Compare
@@ -5,9 +5,9 @@ find /boot/kexec*.txt | gpg --verify /boot/kexec.sig - | |||
#remove invalid kexec_* signed files | |||
mount /dev/sda1 /boot && mount -o remount,rw /boot && rm /boot/kexec* && mount -o remount,ro /boot | |||
#Generate keys on OpenPGP smartcard: | |||
mount-usb && gpg --home=/.gnupg/ --card-edit | |||
mount-usb --mode rw && gpg --home=/.gnupg/ --card-edit |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bash history now promotes mount-usb --mode rw. nitpick
…ives call hierarchy, fix HOTP resealing only on OS reinstall, clarify TPM increment workflow Signed-off-by: Thierry Laurion <[email protected]>
dae9e85
to
1f6a975
Compare
@@ -183,17 +183,6 @@ update_totp() { | |||
TOTP="NO TPM" | |||
else | |||
TOTP=$(unseal-totp) | |||
# On platforms using CONFIG_BOOT_EXTRA_TTYS multiple processes may try to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No more 3 attempts on boot to unseal TPMTOTP: if multiple consoles (Eg Talos-2 with display console + BMC, at worst we could intruduce small delay if race condition still happening, while die asks user to press Enter now, guiding to reseal TPMTOTP or reset TPM if unable to access TPM NVRAM.
|
…hecksums. Warn user prior of effectively booting (shows console warning, wait 2s then reboot) Signed-off-by: Thierry Laurion <[email protected]>
This comment was marked as off-topic.
This comment was marked as off-topic.
…red,yellow,green) Signed-off-by: Thierry Laurion <[email protected]>
… prompt for recovery shell access, state where debug logs are in centralized way Note for linuxboot#1888: warn in code is used mostly to actually warn user of something requiring his attention, and pausing for 2 seconds. Goal is: die: blocking: tell user that something failed, requiring acknowledgement for corrective actions. warn: display "WARNING:" prepended messages which pauses for 2 seconds prior of continuing. This is not an error, nor INFO INFO: gives a trace to the user when in QUIET mode, under /tmp/debug.log related to core components output, typically related to measurements traces. Consequently, putting what is currently under warn->INFO wold be console silenced. We want to get rid of manual "echo +++++" messages. So it seems we lack what is currently named INFO to go into measurement_log, and INFO (green), warn (yellow) and die (red) messages to console. Signed-off-by: Thierry Laurion <[email protected]>
Signed-off-by: Thierry Laurion <[email protected]>
… being set: observed in fbwhiptail-tpm2-hotp-prod_quiet 991 root 3272 S {gui-init} /bin/bash /bin/gui-init 2024 root 2792 S {kexec-select-bo} /bin/bash /bin/kexec-select-boot - 2025 root 1364 S sha256sum -c /tmp/kexec/kexec_default_hashes.txt 2105 root 2068 S /bin/bash Signed-off-by: Thierry Laurion <[email protected]>
…. Logs for first under usb.raw to check against HOTP reseal Signed-off-by: Thierry Laurion <[email protected]>
…hrase equiv) + easthetic fixes Signed-off-by: Thierry Laurion <[email protected]>
…p_without_firmware_upgrade_boot_wiped-staging Signed-off-by: Thierry Laurion <[email protected]>
Asking to press Enter is more forgiving than "any key" and good, but we also have to actually continue on Enter instead of any key. Signed-off-by: Jonathon Hall <[email protected]>
… sleep one second before continuing Signed-off-by: Thierry Laurion <[email protected]>
…mpts Signed-off-by: Thierry Laurion <[email protected]>
…l selected containers prior of prompting for new DUK Signed-off-by: Thierry Laurion <[email protected]>
…nderstanding and debugging Signed-off-by: Thierry Laurion <[email protected]>
…d leaves 1 second to the user to read the notice Signed-off-by: Thierry Laurion <[email protected]>
Tested under QEMU - wipe of /boot/kexec_* - TPM reset + boot default + define default + TPM DUK - remove qemu *.rom files (so keyring injected is unique and triggers TPM unseal error on boot) - Reseal TPMTOTP+HOTP succeeds giving debug output of TPM counter increment succeeding - comparing hashes under /boot/kexec_rollback.txt validates TPM increment works and is validated (rollback is to prevent copying old kexec*.txt + kexec.sig under /boot) Signed-off-by: Thierry Laurion <[email protected]>
#1935 (comment) testing showed:
@marmarek if you could restest with 1a8d685 I think this is good to merge after testing + @JonathonHall-Purism final code review is needed (die now wait for input; warn waits 1 second; both of which don't require docs changes after reading. I could clean commit logs once testing is reported fixing everything reported here, (and making console output clear and code clearer and having warnings that can actually be seen, errors that ask user acknowledgement etc, which are all input for now merged first steps of #1888 but I have not much more time to put here after that. |
Thanks, I'll test it soon, but it will take me some time. That test system got hit with #1882 again :( |
HOTP part seems to be okay now, but signing boot files fails with |
Recontextualization of your use case:
@marmarek what would you like to see here? I agree the diff/file non existience is not helpful here. But what would be?
|
…ious tree file exists to be compared against in diff Signed-off-by: Thierry Laurion <[email protected]>
Ok, so it seems the issue is about the message - normally the "Missing file" error is about signing /boot content with gpg, and the next step is about signing them (see https://openqa.qubes-os.org/tests/145090#step/firstboot/4 for example). TPM owner password is unexpected/surprising prompt. So, maybe just add one line before that prompt explaining why it's asking for it? Alternatively, change the "Missing file" error to include that info? |
Fixes #1562
Supeseeds #1934 + reviewed changes
@marmarek/ @JonathonHall-Purism : good enough for merge?
EDIT: Thanks @JonathonHall-Purism for your review. All comments addressed.
Changes: