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.

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

WiP: Improve checksum verification upon totp unseal errors #1508

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

tlaurion
Copy link
Collaborator

@tlaurion tlaurion commented Oct 11, 2023

Raw notes:

  • Writing multi-line output to /dev/kmsg didn't work. We now output the message passed to DEBUG one line at a time.
  • When TOTP unseal errors happened, the user needed to also sign /boot without knowing if /boot was tampered with.
    • Now integrity report is given when totp unseal fails
    • If gpg detached signature or hashes are invalid, we give the user a detailed report of hashes being different if detached signature is good (kexec.sig of kexec*.txt)
  • There were multiple instances of verify_global_hashes
    • The one from gui-init has been moved to etc/functions while others have been deleted, and calls requiring to update the hashes now pass optional parameter "update"
  • Are some /tmp/hash_output relevance missed somewhere?

What should be improved further?

Raw notes:
- Writing multi-line output to /dev/kmsg didn't work. We now output one line of message in DEBUG calls
- When TOTP unseal errors happened, the use needed to also sign /boot without knowing if things were right.
  - Now integrity report is given
   - If gpg detached signature or hashes are invalid, we give the user a detailed report of hashes being different is detaches signature is good
- There were multiple instances of verify_global_hashes
  - The one from gui-init has been moved to etc/functions, and calls requiring to update the hashes now pass optional parameter
- Are some /tmp/hash_output relevance missed somewhere?

What should be improved further?
@tlaurion
Copy link
Collaborator Author

Played around with qemu-fbwhiptail-tpm1-hotp, qemu-fbwhiptail-tpm2-hotp and love the improvements.

@tlaurion
Copy link
Collaborator Author

Currently fails on default boot and select boot option. Putting as draft

@tlaurion tlaurion marked this pull request as draft October 12, 2023 17:05
@JonathonHall-Purism
Copy link
Collaborator

From reviewing verify_global_hashes, it looks like there were significant differences between these two functions. We can't just replace one with the other.

In particular:

  • The one in gui-init returned 0 or 1 for success/failure. The one in kexec-select-boot always returned 0, instead it set valid_hash=y and valid_global_hash=y for success and left them untouched for failure.
  • The one in kexec-select-boot also checks root file hashes. The one in gui-init did not.
  • The one in kexec-select-boot used "$gui_menu" to control whether differences are shown or just checked silently, the one in gui-init did not.

I do think this needs to be refactored, but we can't just replace one with the other.

Comment on lines +489 to +505
if [ -f "$TMP_PACKAGE_TRIGGER_PRE" ]; then
PRE_CHANGED_FILES=$(grep '^CHANGED_FILES' $TMP_PACKAGE_TRIGGER_POST | cut -f 2 -d '=' | tr -d '"')
TEXT="The following files failed the verification process BEFORE package updates ran:\n${PRE_CHANGED_FILES}\n\nCompare against the files $CONFIG_BRAND_NAME has detected have changed:\n${CHANGED_FILES}\n\nThis could indicate a compromise!"
TEXT+="$UPDATE_TEXT"
# if files changed after package manager started, probably caused by package manager
elif [ -f "$TMP_PACKAGE_TRIGGER_POST" ]; then
LAST_PACKAGE_LIST=$(grep -E "^(Install|Remove|Upgrade|Reinstall):" $TMP_PACKAGE_TRIGGER_POST)
UPDATE_INITRAMFS_PACKAGE=$(grep '^UPDATE_INITRAMFS_PACKAGE' $TMP_PACKAGE_TRIGGER_POST | cut -f 2 -d '=' | tr -d '"')

if [ "$UPDATE_INITRAMFS_PACKAGE" != "" ]; then
TEXT="The following files failed the verification process AFTER package updates ran:\n${CHANGED_FILES}\n\nThis is likely due to package triggers in$UPDATE_INITRAMFS_PACKAGE.\n\nYou will need to update your checksums for all files in /boot."
TEXT+="$UPDATE_TEXT"
else
TEXT="The following files failed the verification process AFTER package updates ran:\n${CHANGED_FILES}\n\nThis might be due to the following package updates:\n$LAST_PACKAGE_LIST.\n\nYou will need to update your checksums for all files in /boot."
TEXT+="$UPDATE_TEXT"
fi
else
Copy link
Collaborator

Choose a reason for hiding this comment

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

This logic isn't new (moved from gui-init), but while we're here....

This looks like logic to allow an OS package manager to place files on /boot indicating:

  • what files failed validation before updating packages
  • what files failed validation after updating packages
  • what packages were updated

This then controls the prompts shown by Heads if files do not validate. But kexec_package_trigger_{pre/post}.txt aren't themselves authenticated in any way (which would have to somehow "authenticate" that your package manager really made these changes). So as far as I can tell, somebody tampering /boot could just place these files here with whatever content they want and thereby control what Heads will say when it observes the tampering.

Does this package manager support exist in any OS?

I noted some additional details from my read-through here, but really the key question to me is - why should we trust these kexec_package_trigger_* files?

Comment on lines +605 to +606
whiptail $MAIN_MENU_BG_COLOR --title "Measured Integrity Report" --msgbox "$date\n\nTOTP: $TOTP\nHOTP: $HOTP\nBOOT INTEGRITY SIGNATURE: $SIGNED\nBOOT INTEGRITY HASHES: $HASH\n\nPress OK to continue with detailed information in case of errors" 0 80

Copy link
Collaborator

Choose a reason for hiding this comment

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

IMO:

  • All the CAPS introduced in the error text, as well as the much longer labels for integrity, make this difficult to read. There's no longer any separation between the "labels" and "content" for errors (screenshot below)
  • I think the separation between "signature" and "hashes" should be an implementation detail of Heads - that's how we authenticate /boot. The normal user flow should just say whether /boot integrity was authenticated or not, so it's easier to understand if you are not an expert in Heads. (I'd be OK with providing detailed information as an optional thing or on the console, etc. to cater to advanced users too, but putting it front-and-center IMO makes it a requirement to be an knowledgeable in Heads internals to use Heads.)

Re: the caps, this is what I was greeted with when firing this up in qemu:

Screenshot_20231012_171341

From a glance it is really hard to parse this and know what to look at. The text-only prompt is a bit limiting but I'd suggest:

  • Don't use all caps for the errors, this doesn't add anything and just makes it harder to read
  • Go back to just "INTEGRITY: " instead of the last two lines, which would make the labels all similar-length again and reduce a lot of verbosity
  • Take out the colon from the HOTP error so the colons form a bit of separation between the labels and contents

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh and also, it did not show me any detailed information when I pressed Enter :-/

@JonathonHall-Purism
Copy link
Collaborator

Thinking back to the actual intent of the change - why do you want to report /boot integrity when TOTP/HOTP unseal fails? Seems like this is adding another prompt in an already nontrivial flow, and I think it'd be hard for users who aren't experts in Heads to understand what this information has to do with the issue at hand.

When TOTP unseal errors happened, the use needed to also sign /boot without knowing if things were right.

TOTP/HOTP re-seal doesn't affect the integrity of /boot. Why would you need to sign /boot without knowing the prior status in this case?

@tlaurion
Copy link
Collaborator Author

tlaurion commented Oct 15, 2023

The intent of this was once again for tpm disk unlock key specifically but more globally for users to not sign /boot content when totp unsealing fails and resealing totp.

When one updates totp, tpm Disk Unlock Key needs to be updated (which takes totp + runtime measurements into another sealed secret which is authenticated) , but doing so, LUKS header changes and its local hash kept under /boot changes which is signed under kexec.sig and needs to be updated.

On tpm2, primary handle needs to be updated as well.

As for pre trigger and post trigger files under /boot, I will have to check history to see which os populates those and if they are still relevant. I do not use any OS doing those but thought probably wrongly that pureos was the intent of support here. Will check later but as of now I think the unification of those functions will need more thinking.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants