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

improvements on the direct boot using UKI #299

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

Conversation

hector-cao
Copy link
Collaborator

@hector-cao hector-cao commented Dec 10, 2024

the improvements are results of the user's feedback on the closed MR : #296

  • grammar check on README

@hector-cao hector-cao changed the title grammar checks on readme and some minor improvements improvements on the direct boot using UKI Dec 10, 2024
@hector-cao hector-cao requested a review from a team December 10, 2024 09:31
README.md Outdated Show resolved Hide resolved
Copy link

@mythi mythi left a comment

Choose a reason for hiding this comment

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

LGTM, thanks! Nice to see -kernel working easily for you as well

@hector-cao
Copy link
Collaborator Author

LGTM, thanks! Nice to see -kernel working easily for you as well

Thanks for the feedback that greatly improved the instructions and script

README.md Outdated
@@ -593,7 +593,8 @@ you proceed to [step 4](#verify-itts-client-version).

One of the key components remote attestation is based on is the runtime measurement values (stored in RTMRs
registers for each TD by the TDX module). These RTMR values are computed from the digests of the entries of
the TD boot event log.
the TD boot event log. During the system's boot, each event's digest (SHA384) is extended (appended) to the
Copy link
Contributor

Choose a reason for hiding this comment

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

If you're using "appended" to clarify the meaning of "extended", maybe we should just go with "appended" unless "extended" is baked into the language of this component

Copy link
Collaborator Author

@hector-cao hector-cao Dec 10, 2024

Choose a reason for hiding this comment

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

the CC terminology uses extended for this operation, i personally did not find it intuitive so try to clarify for people who have the same feeling, but if we have to keep one, it will be extended

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done 57bef18

@@ -46,15 +46,13 @@ EOM

cleanup() {
echo "cleanup ..."
Copy link
Contributor

Choose a reason for hiding this comment

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

If you're deleting the functional part of the cleanup, is there a reason you're keeping the function that just echoes?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

thanks, i will remove it

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done 57bef18

README.md Outdated
registers for each TD by the TDX module). These RTMR values are computed from the digests of the entries of
the TD boot event log. During the system's boot, each event's digest (SHA384) is extended (appended) to the
current value of the associated RTMR. The resulting value's digest becomes the new value of the RTMR.
One of the key components remote attestation is the runtime measurement values (stored in RTMRs
Copy link
Contributor

Choose a reason for hiding this comment

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

Add "of" after "componenets"

Copy link
Contributor

Choose a reason for hiding this comment

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

"One of" implies a singular subject, but "values" is plural. Might be better to just flip the sentence around and say something like, "Runtime measurement values (paren contents) are key components of remote attestation."

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done 57bef18

README.md Outdated
current value of the associated RTMR. The resulting value's digest becomes the new value of the RTMR.
One of the key components remote attestation is the runtime measurement values (stored in RTMRs
registers for each TD by the TDX module). During the system boot, each component of the boot process (binary or conf)
is measured into a digest. This digest value is extended (appended) to the RTMR's current value. The digest of the result value
Copy link
Contributor

Choose a reason for hiding this comment

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

Same thing with appended/extended.

Also, I'm not sure what is meant by "measured into a digest". I think what you're meaning here is that each component of the boot process becomes input for the digest/hash function?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

digest is the value from the hash function that is applied to data that needs to be measured

@hector-cao hector-cao requested a review from mckees December 16, 2024 11:59
@qinkunbao
Copy link

Hi Hector,

I've been reviewing the TPM measurement sequence during direct boot and noticed a potential discrepancy that could have security implications. I'm seeing conflicting behavior between how OVMF and the EFI handle measurements for RTMR and vTPM. I just wondered if you have checked the event log of vTPM and RTMR on a TDX CVM using an image that supports the direct boot.

OVMF (https://github.com/tianocore/edk2/blob/62de957185d71feb9c4d09f4eddcdac1980632fb/SecurityPkg/Library/DxeTpm2MeasureBootLib/DxeTpm2MeasureBootLib.c#L478) attempts to measure into RTMR first and skips vTPM if the RTMR measurement succeeds.

On the other hand, the EFI/libstub, as implemented in the Linux kernel patches (https://patches.linaro.org/project/linux-efi/list/?series=236617), prioritizes measurement into vTPM and only proceeds to RTMR if the vTPM measurement fails.

This conflicting approach results in an incomplete TPM and RTMR event log on systems using direct boot. Because either OVMF or EFI skips its part of the measurement, neither RTMR nor vTPM has a complete log. This incomplete log is a security concern as it undermines the integrity and verifiability of the boot process.

@mythi
Copy link

mythi commented Jan 3, 2025

(the TCG vs CC measurement protocol topic could be a separate issue)

This conflicting approach results in an incomplete TPM and RTMR event log on systems using direct boot.

I think the general consensus still is that only one measurement protocol is available at a time.

@qinkunbao
Copy link

qinkunbao commented Jan 3, 2025

(the TCG vs CC measurement protocol topic could be a separate issue)

Yes, I agree. I just wanted to say the direct boot doesn't work for TDX CVMs.

I think the general consensus still is that only one measurement protocol is available at a time.

The firmware should enforce that only one of the protocol is used. The current combination of OVMF and EFI/libstub has a security issue. The boot chain software should measure into both to avoid the security issue such as https://github.com/google/security-research/blob/master/pocs/bios/tpm-carte-blanche/writeup.md

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.

4 participants