-
-
Notifications
You must be signed in to change notification settings - Fork 40
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
Further improvements to TPM measurements #361
base: master
Are you sure you want to change the base?
Conversation
9927006
to
8e46bea
Compare
I originally wanted to add some more improvements to this PR, but I think they'll take some more time and the fix in this PR is kind of self contained, so I'm going to mark it as ready. |
8e46bea
to
2c4c991
Compare
// | ||
// As per reference: | ||
// "Measured hash covers the PE section name in ASCII (including a trailing NUL byte!)." | ||
let section_name_cs_utf8 = CString::new(section_name).unwrap(); |
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.
Is this unwrap safe?
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.
I think this might actually be an oversight on my part, thanks for catching it!
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.
I cleaned up the relevant flow by moving some section name logic into UnifiedSection
// "Measured hash covers the PE section name in ASCII (including a trailing NUL byte!)." | ||
let section_name_cs_utf8 = CString::new(section_name).unwrap(); | ||
|
||
if tpm_log_event_ascii( |
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.
@RaitoBezarius Do you know why this is called _ascii
?
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.
The name probably leans on the systemd equivalent, which is also called tpm_log_event_ascii
:
https://github.com/systemd/systemd/blob/fdd4263cac327a3bc447560dbd46713524046099/src/boot/efi/measure.c#L281-L288
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.
Yeah, it's just a TPM log event wrapper that applies preprocessing to the description so that userspace can read it.
if tpm_log_event_ascii( | ||
boot_services, | ||
TPM_PCR_INDEX_KERNEL_IMAGE, | ||
data, |
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.
nit: The control flow could be simplified by measuring the empty section in case there is no section data:
pe_section_data(...).unwrap_or(&[])
Not sure whether the compiler likes that, though.
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.
Hmm, do you have any examples in mind where sections might be empty?
From the Rust side, this looks good to me. I've commented some nits, feel free to ignore those. This also does what the comments claim. @RaitoBezarius If you have a second, can you double-check that this makes sense from a high-level perspective? If so, I'd merge it. |
2c4c991
to
a47b99d
Compare
I just realized there might be a small unhandled issue: Some sections are allowed to appear multiple times, but with this PR only one section would be measured. This is not terribly important for NixOS atm, but perhaps there's a simple solution. |
a47b99d
to
5470fd3
Compare
systemd-stub measures both the section name and the section contents. Furthermore it measures sections in a predefined order.
5470fd3
to
fffc7d5
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.
Looks good to me, I would only ask for a NixOS test to ensure that we don't regress this behavior, but I don't know what would be a good testing strategy here, so it's nit.
Great point! This sounds like a security issue, because the same measurements could lead to different running code. Is the simple solution to bail out when there are duplicated sections? Or is there another quick solution? |
I've been working towards getting systemd-pcrlock to work. This PR includes some further fixes, but some work remains.
There is an important caveat: While PCR 11 measurements line up with
systemd-pcrlock
's expectations, they don't really make sense at the moment since they measure the section contents of the PE image, so unless I'm mistaken we actually measure the kernel/initrd file paths and not their contents. This shouldn't be too complicated to implement, but probably needs some tinkering with the current architecture.I also noticed some race conditions in the NixOS test I introduced, but @nikstur was faster.