LUKS header change validation upon sealing and unsealing ops

[tlaurion]

Edit: UX change in screenshots at #1625 (comment)

---

Fixes #1092
Supersedes #1093

• Rework of ed1c23a (credit to @root-hardenedvault) thank you!)

• Addresses and correct self-review under Make it possible to report headers of which LUKSes to be unlocked via TPM change #1093 (@root-hardenedvault : you don't answer often here!)

  • kexec-unseal-key: Warn a user who attempts to default boot while his Disk Unlock Key passphrase fails to unseal because LUKS headers changed (Make it possible to report headers of which LUKSes to be unlocked via TPM change #1093 (comment)) EDIT: everything move to kexec-insert-key to check header checksum part of detached signed hash digest, before attempting TPM DUK unseal op.
  • kexec-seal-key: Identical as in ed1c23a
  • kexec-add-key: Tell the user that the Headers did not change when changing TPM released Disk Unlock Key
    (Through changing default boot at Options->Boot Options -> Show OS boot options: select a new boot option
    and set a Disk Unlock Key in TPM, accept to modify disk and sign /boot options)
    • Here, we cancel the diff output shown on screen Make it possible to report headers of which LUKSes to be unlocked via TPM change #1093 (comment)
      • EDIT: irrelevant in current workflow, since sealing/unsealing/tpm reset triggers TPM DUK resealing. We expect the last boot was not tampered with here prior of firmware upgrade. And then after firmware upgrade, the user should expect firmwware to have changed and engage in resealing TPM DUK (Ease TPM Disk Unlock Key sealing/resealing after TOTP mismatch (firmware upgrade) + warn and die changes #1482)
    • And we change the warning given to the user to past tense "Headers of LUKS containers to be unlocked via TPM Disk Unlock Key passphrase did not change." The user knows that unsealing error is not due to LUKS header having changed.
      • Edit: verbiage expose to end user less technical when unneeded: we talk about Encrypted disk keys, where sealing ops are related still to TPM Disk Unlock (DUK) verbiage.

  Workflow in pictures at LUKS header change validation upon sealing and unsealing ops #1625 (comment)

[tlaurion]

Simulating a LUKS attack/undesired key slot addition on Heads deployment with DUK setup (qemu with ubuntu deployed as OS):
cryptsetup luksKeyChange --key-slot 0 /dev/vda4

Result:

Debug trace showing LUKS header measured under PCR6, mismatch resulting in DUK unsealing failure and verification failing between detached signed LUKS header vs live cryptsetup luksHeaderBackup:

@JonathonHall-Purism please review (wording suggestions welcome as usual)

[JonathonHall-Purism]

@tlaurion Here's what I gather from this PR:

Goal: Advise more clearly whether LUKS header(s) have changed, if unsealing the TPM Disk Unlock Key fails.

(We already check it - PCR #6. But there's no specific output, user doesn't know why unsealing failed, which could be due to the LUKS header(s), any state in other PCRs, or the passphrase.)

Implementation:

• If DUK unseal fails, compare LUKS header digests with signed copies in /boot/kexec_lukshdr_hash.txt. If they differ, or are missing, print a warning. (kexec-unseal-key)
• If DUK unseal fails, compare LUKS header digests with signed copies in /boot/kexec_lukshdr_hash.txt. If they match, print an informative message. (kexec-insert-key)
• Also, improve wording relating to "TPM Disk Unlock Key"

---

I don't see how this solves #1092. I agree we should validate LUKS headers as well for boots without TPM Disk Unlock Key. The goal here is still valid, so #1092 could be addressed separately, but this doesn't address it and shouldn't close it, AFAICT.

Regarding implementation - why are the LUKS headers checked in two separate places: kexec-unseal-key / kexec-insert-key? It seems like kexec-insert-key could check it before even prompting for the passphrase, and it could tell you right away.

If the headers don't match, should we even bother trying to unlock DUK? Maybe we should assume DUK can't be unlocked then and ask whether to boot with the recovery passphrase?

It also seems like 'luksDump.txt' really should exist at this point, so we shouldn't ignore kexec_lukshdr_hash.txt if it doesn't - that would mean something has gone really wrong. (But if kexec_lukshdr_hash.txt doesn't exist due to having been created by a much older Heads, that seems reasonable, and the signatures on /boot cover it to prevent tampering.)

I'll post some review comments with some suggestions on details and wording too.

[JonathonHall-Purism]

Regarding the three key messages, how about the following wording:

• OK: "Encrypted disk keys have not been changed."
• Altered: "Encrypted disk keys have been changed. If you did not make this change, the disk may be compromised."
• Can't check: "Could not check for tampering of encrypted disk keys."
  • "Re-seal the TPM Disk Unlock Key by re-selecting your default boot option to enable this check (Options -> Boot Options -> Show OS boot menu)."

Original for comparison:

• OK: "Headers of LUKS containers to be unlocked via TPM Disk Unlock Key passphrase did not change since sealed in TPM."
• Altered: "Encrypted LUKS(es) container(s) headers changed since they were measured and sealed in TPM for Disk Unlock key. You might want to investigate."
• Can't check: "No encrypted LUKS container(s) headers were found/comparable under /boot/kexec_lukshdr_hash.txt"
  • "You might need to setup a new boot default and TPM Disk Unlock Key from Options->Boot Options->Show OS boot menu."

I get that the LUKS header contains other things besides the volume key and key slots, but virtually everything relates in some way to the keys (e.g. changing the cipher changes the way the key is applied to the data). This differentiates it from the encrypted disk data, which we do not check and could still have been tampered (though not in a predictable manner if the attacker does not know a key).

As usual I don't think we should require the user to know what a LUKS container is, what its header is, or what individual files in /boot mean to use Heads, which led me to the wording above.

[tlaurion]

@JonathonHall-Purism

Recap.
When sealing DUK, PCR6 is extended with LUKS header backup prior of the resulting measurements sealed into seperate TPM nvram+DUK passphrase.

So here, by attempting to unseal TPM nvram DUK passphrase protected region, two things could happen:

• DUK won't unseal because DUK passphrase is wrong, while measurements are good
• DUK won't unseal because measurements are wrong, while DUK passphrase is good (but cannot know because measurements+passphrase needs to be as in sealed state

So here, if undeal fails, we improve upon @root-hardenedvault PR by reporting not only diff of current header vs detached signed header backup that was given only after the 3 bad useal attempts. As of today, DUK would'nt unseal, but the user would not know that its because of the LUKS header having changed.

[JonathonHall-Purism]

Also a few detailed review comments - but some of the details may change if the logic is moved around as I suggested a couple of comments up, let me know what you think

[tlaurion]

@JonathonHall-Purism I think #1092 is badly named and would go forward changing its name since tits content and goals in description are all fixed by this PR.

As for having LUKS header verified outside of DUK, it would require refactoring with that goal in mind.
As for this PR, it fullfills its goals of telling user that DUK unsealing failed because of the LUKS header having changed.

I think validating LUKS header could be done independently without it being unsealed, but as of now, this is never the case.

[tlaurion]

@JonathonHall-Purism addressed review, opened #1629 and corrected name of #1092 which this PR should still close.

Will test under qemu now

[JonathonHall-Purism]

@tlaurion Did you see this part of the comment above? (Not trying to be snarky, just might have been overlooked)

Regarding implementation - why are the LUKS headers checked in two separate places: kexec-unseal-key / kexec-insert-key? It seems like kexec-insert-key could check it before even prompting for the passphrase, and it could tell you right away.

If the headers don't match, should we even bother trying to unlock DUK? Maybe we should assume DUK can't be unlocked then and ask whether to boot with the recovery passphrase?

It also seems like 'luksDump.txt' really should exist at this point, so we shouldn't ignore kexec_lukshdr_hash.txt if it doesn't - that would mean something has gone really wrong. (But if kexec_lukshdr_hash.txt doesn't exist due to having been created by a much older Heads, that seems reasonable, and the signatures on /boot cover it to prevent tampering.)

Would like to know your thoughts on these, did not see anything in the comments above 🤔

[JonathonHall-Purism]

IMO: keys instead of keys(s) (3x). I get that the idea is to permit the possibility that there's only one key, but I think key(s) is unnecessarily awkward. (keys(s) has two Ss too, reads like "Hobbitses" to me 😅 )

I think I'd only really use (s) when we're asking the user something and it needs to be abundantly clear that this is the option for one or more "thing(s)" - and there's no better way to rewrite the prompt, etc. Otherwise it could be unclear what to do for exactly one "thing".

I think the meaning is clear and we're watering down the real point by making it more awkward. It doesn't agree with have as written either, etc.

[JonathonHall-Purism]

I like the precision of specifying that they have not been changed relative to a specific point in time (sealing the TPM Disk Unlock Key). But IMO "You might want to investigate." is unnecessarily vague, we can say why they may want to investigate.

So I'd suggest:
Encrypted disk keys have changed since the TPM Disk Unlock Key was sealed. If you did not make this change, the disk may be compromised.

[tlaurion]

@tlaurion Did you see this part of the comment above? (Not trying to be snarky, just might have been overlooked)

Regarding implementation - why are the LUKS headers checked in two separate places: kexec-unseal-key / kexec-insert-key? It seems like kexec-insert-key could check it before even prompting for the passphrase, and it could tell you right away.

If the headers don't match, should we even bother trying to unlock DUK? Maybe we should assume DUK can't be unlocked then and ask whether to boot with the recovery passphrase?

It also seems like 'luksDump.txt' really should exist at this point, so we shouldn't ignore kexec_lukshdr_hash.txt if it doesn't - that would mean something has gone really wrong. (But if kexec_lukshdr_hash.txt doesn't exist due to having been created by a much older Heads, that seems reasonable, and the signatures on /boot cover it to prevent tampering.)

Would like to know your thoughts on these, did not see anything in the comments above 🤔

Answer was at #1625 (comment).
Unseal is done by insert, therefore the comparisons and check for file existence is done in unseal, where insert just confirms it?
@JonathonHall-Purism If you insist on this, then checks of unseal could be moved to insert code. Chain is:

• key file exists because it was sealed and that file is used to construct override of crypttab
• key file checked by detached signature, so we cannot go that far if detached signed integrity fails (header from sealing moment)
• arriving at unseal, if key file exist, we expect header backup hash. Where qubes measure script creates the tmp file so both should exists and can be compared against. No check done there, deletaged to unseal op.
• unseal will fail from tpm unseal op, where cmp is made to confirm header difference (verbiage now a bit less precise on this)
• unseal fails, so we go back to insert which gave more context, now also a bit diluted but I still find ok.

@JonathonHall-Purism still want the refactoring? Security is chain of trust here where higher level is TPM and then files are used to give context. I prefer to trust TPM here and give context on unseal failure more then use tpm file creating without TPM.

[JonathonHall-Purism]

Suggested change

	warn "Could not check for tampering of Encrypted disk keys(s)"
	warn "Could not check for tampering of Encrypted disk keys"

Please change keys(s) to keys here

[tlaurion]

still want the refactoring? Security is chain of trust here where higher level is TPM and then files are used to give context. I prefer to trust TPM here and give context on unseal failure more then use tpm file creating without TPM.

Has been refactored upon request. Now luks headers hashes compared before unsealing attempt.

[JonathonHall-Purism]

@tlaurion I don't see any changes to any initrd scripts now, were they lost in a force-push?

[tlaurion]

Normal boot (notice +++ Encrypted disk keys have not been changed since sealed in TPM Disk Unlock Key)

Tampering with the header:

Booting default again:

[tlaurion]

@JonathonHall-Purism Sorry about the past mess in commits (and thanks for the pointer). Should be clean now...

[JonathonHall-Purism]

LGTM, thanks @tlaurion