raito/lanzaboote
0
0
Fork 0
Code Issues 45 Pull requests 6 Projects Releases 6 Packages Wiki Activity Actions

feat: minimal poc for TPM measurements à la sd-stub #167

Merged
RaitoBezarius merged 2 commits from sd-stub-tpm2 into master 2023-05-18 17:16:30 +00:00
Conversation 21 Commits 2 Files changed 8 +399 -50
RaitoBezarius commented 2023-04-29 17:28:23 +00:00 (Migrated from github.com)
Copy link

Depends on #166.
Implements TPM part of https://github.com/nix-community/lanzaboote/issues/94.

Note to maintainers & reviewers

Lanzaboote customization requires a special handling of PCR11 measurements to include kernel and initrd properly.
To do this, we need to read the kernel and the initrd guarded with our hashes, then record them as "fake unified section" or measure them somewhere else.

This way, we can slowly recover lanzaboote as a "true UKI" in terms of semantics.

This is out of scope for this PR, but will done in #169 because they do bring the necessary infrastructure wrt to UnifiedSection to perform this.

~~Depends on #166.~~ Implements TPM part of https://github.com/nix-community/lanzaboote/issues/94. ### Note to maintainers & reviewers Lanzaboote customization requires a special handling of PCR11 measurements to include kernel and initrd properly. To do this, we need to read the kernel and the initrd guarded with our hashes, then record them as "fake unified section" or measure them somewhere else. This way, we can slowly recover lanzaboote as a "true UKI" in terms of semantics. This is out of scope for this PR, but will done in #169 because they do bring the necessary infrastructure wrt to UnifiedSection to perform this.
RaitoBezarius (Migrated from github.com) reviewed 2023-05-01 01:09:04 +00:00
rust/stub/src/main.rs
RaitoBezarius (Migrated from github.com) commented 2023-05-01 01:09:03 +00:00
Copy link

I'm not so sure about hard failing here.

I'm not so sure about hard failing here.
blitz commented 2023-05-04 09:47:53 +00:00 (Migrated from github.com)
Copy link

@RaitoBezarius It would be helpful to have a call one of these days about what's going on in these PRs to give some overview information. :)

@RaitoBezarius It would be helpful to have a call one of these days about what's going on in these PRs to give some overview information. :)
👍 1
RaitoBezarius commented 2023-05-04 13:17:14 +00:00 (Migrated from github.com)
Copy link

@RaitoBezarius It would be helpful to have a call one of these days about what's going on in these PRs to give some overview information. :)

When are you available :) ?

> @RaitoBezarius It would be helpful to have a call one of these days about what's going on in these PRs to give some overview information. :) When are you available :) ?
RaitoBezarius (Migrated from github.com) reviewed 2023-05-12 17:15:23 +00:00
rust/stub/src/measure.rs
@ -0,0 +24,4 @@
// safe, because we don't touch any data in the data sections that
// might conceivably change while we look at the slice.
// (data sections := all unified sections that can be measured.)
let pe_binary = unsafe { image.as_slice() };
RaitoBezarius (Migrated from github.com) commented 2023-05-12 17:15:23 +00:00
Copy link

add // SAFETY

add `// SAFETY`
RaitoBezarius commented 2023-05-14 17:14:18 +00:00 (Migrated from github.com)
Copy link

I met with @nikstur on the last NixOS Munich meetup and we discussed this PR.

I met with @nikstur on the last NixOS Munich meetup and we discussed this PR.
🎉 1
lovesegfault (Migrated from github.com) reviewed 2023-05-17 13:15:55 +00:00
rust/stub/src/unified_sections.rs
@ -0,0 +2,4 @@
/// UKI specification.
/// This is the canonical order in which they are measured into TPM
/// PCR 11.
/// !!! DO NOT REORDER !!!
lovesegfault (Migrated from github.com) commented 2023-05-17 13:15:54 +00:00
Copy link

Perhaps adding #[repr(u8)] and a unit test that checks their order would be nice?

std::mem::discriminant could also be used, perhaps

Perhaps adding `#[repr(u8)]` and a unit test that checks their order would be nice? [`std::mem::discriminant`](https://doc.rust-lang.org/stable/std/mem/fn.discriminant.html) could also be used, perhaps
nikstur (Migrated from github.com) reviewed 2023-05-17 19:30:56 +00:00
nikstur (Migrated from github.com) left a comment
Copy link

Looks good. Thank you for the work!

I'd love

  • a nice commit message
  • the code formatted with cargo fmt

Only the code formatting is a blocker for me.

Looks good. Thank you for the work! I'd love - a nice commit message - the code formatted with `cargo fmt` Only the code formatting is a blocker for me.
rust/stub/src/main.rs
nikstur (Migrated from github.com) commented 2023-05-16 19:39:46 +00:00
Copy link

It feels like we shouldn't. Under what circumstances can this fail?

It feels like we shouldn't. Under what circumstances can this fail?
rust/stub/src/main.rs
nikstur (Migrated from github.com) commented 2023-05-17 19:29:21 +00:00
Copy link

Shouldn't we just not measure anything if there is no TPM available? i.e. only call measure_image in this if clause.

Shouldn't we just not measure anything if there is no TPM available? i.e. only call `measure_image` in this `if` clause.
RaitoBezarius commented 2023-05-17 19:33:35 +00:00 (Migrated from github.com)
Copy link

Awesome, will fix the code formatting. :)

Awesome, will fix the code formatting. :)
RaitoBezarius (Migrated from github.com) reviewed 2023-05-17 19:33:51 +00:00
rust/stub/src/main.rs
RaitoBezarius (Migrated from github.com) commented 2023-05-17 19:33:51 +00:00
Copy link

Good point!

Good point!
RaitoBezarius (Migrated from github.com) reviewed 2023-05-17 19:34:22 +00:00
rust/stub/src/unified_sections.rs
@ -0,0 +2,4 @@
/// UKI specification.
/// This is the canonical order in which they are measured into TPM
/// PCR 11.
/// !!! DO NOT REORDER !!!
RaitoBezarius (Migrated from github.com) commented 2023-05-17 19:34:22 +00:00
Copy link

Hmm, I'm not so sure how to do it, I will go ahead and merge this, can you open a PR to propose something?

Hmm, I'm not so sure how to do it, I will go ahead and merge this, can you open a PR to propose something?
RaitoBezarius (Migrated from github.com) reviewed 2023-05-17 19:34:47 +00:00
rust/stub/src/main.rs
RaitoBezarius (Migrated from github.com) commented 2023-05-17 19:34:47 +00:00
Copy link

When we fail to parse our own memory as a PE for example, I'd say

When we fail to parse our own memory as a PE for example, I'd say
RaitoBezarius (Migrated from github.com) reviewed 2023-05-17 19:35:21 +00:00
rust/stub/src/main.rs
RaitoBezarius (Migrated from github.com) commented 2023-05-17 19:35:21 +00:00
Copy link

We don't want people doing strange things to the system making us fragile to security, so I'd say hardfailing here is not too shocking but… difficult to say.

We don't want people doing strange things to the system making us fragile to security, so I'd say hardfailing here is not too shocking but… difficult to say.
RaitoBezarius (Migrated from github.com) reviewed 2023-05-17 19:35:40 +00:00
rust/stub/src/main.rs
RaitoBezarius (Migrated from github.com) commented 2023-05-17 19:35:40 +00:00
Copy link

We can "not hard fail" then re-evaluate a "extreme stub mode" that tries to thwart active attacks.

We can "not hard fail" then re-evaluate a "extreme stub mode" that tries to thwart active attacks.
lovesegfault (Migrated from github.com) reviewed 2023-05-17 19:57:00 +00:00
rust/stub/src/unified_sections.rs
lovesegfault (Migrated from github.com) commented 2023-05-17 19:57:00 +00:00
Copy link
#[repr(u8)]
pub enum UnifiedSection {
    Linux = 0,
    OsRel = 1,
    CmdLine = 2,
    Initrd = 3,
    Splash = 4,
    DTB = 5,
    PcrSig = 6,
    PcrPkey = 7,
}

This way it's not as easy to get the values wrong by just changing the ordering

```suggestion #[repr(u8)] pub enum UnifiedSection { Linux = 0, OsRel = 1, CmdLine = 2, Initrd = 3, Splash = 4, DTB = 5, PcrSig = 6, PcrPkey = 7, } ``` This way it's not as easy to get the values wrong by just changing the ordering
lovesegfault (Migrated from github.com) reviewed 2023-05-17 19:57:11 +00:00
rust/stub/src/unified_sections.rs
@ -0,0 +2,4 @@
/// UKI specification.
/// This is the canonical order in which they are measured into TPM
/// PCR 11.
/// !!! DO NOT REORDER !!!
lovesegfault (Migrated from github.com) commented 2023-05-17 19:57:11 +00:00
Copy link

Added a suggestion :)

Added a suggestion :)
RaitoBezarius (Migrated from github.com) reviewed 2023-05-17 20:03:28 +00:00
rust/stub/src/unified_sections.rs
RaitoBezarius (Migrated from github.com) commented 2023-05-17 20:03:28 +00:00
Copy link

Oh I see your point now

Oh I see your point now
nikstur commented 2023-05-18 14:48:41 +00:00 (Migrated from github.com)
Copy link

Should we merge https://github.com/nix-community/lanzaboote/pull/177 first? This might make formatting easier and the diff easier to read.

Should we merge https://github.com/nix-community/lanzaboote/pull/177 first? This might make formatting easier and the diff easier to read.
RaitoBezarius commented 2023-05-18 15:06:59 +00:00 (Migrated from github.com)
Copy link

Sure thing, this PR has problem with TPM2 protocol being located in UEFI
anyway.

Le jeu. 18 mai 2023 à 16:48, nikstur @.***> a écrit :

Should we merge #177
https://github.com/nix-community/lanzaboote/pull/177 first? This might
make formatting easier and the diff easier to read.

—
Reply to this email directly, view it on GitHub
https://github.com/nix-community/lanzaboote/pull/167#issuecomment-1553178426,
or unsubscribe
https://github.com/notifications/unsubscribe-auth/AACMZRHSSUUIJGLEBMEEODDXGYZFHANCNFSM6AAAAAAXQLBHZA
.
You are receiving this because you were mentioned.Message ID:
@.***>

Sure thing, this PR has problem with TPM2 protocol being located in UEFI anyway. Le jeu. 18 mai 2023 à 16:48, nikstur ***@***.***> a écrit : > Should we merge #177 > <https://github.com/nix-community/lanzaboote/pull/177> first? This might > make formatting easier and the diff easier to read. > > — > Reply to this email directly, view it on GitHub > <https://github.com/nix-community/lanzaboote/pull/167#issuecomment-1553178426>, > or unsubscribe > <https://github.com/notifications/unsubscribe-auth/AACMZRHSSUUIJGLEBMEEODDXGYZFHANCNFSM6AAAAAAXQLBHZA> > . > You are receiving this because you were mentioned.Message ID: > ***@***.***> >
RaitoBezarius commented 2023-05-18 16:28:28 +00:00 (Migrated from github.com)
Copy link

OVMF has no TPM support compiled by default, I am fixing that.

OVMF has no TPM support compiled by default, I am fixing that.
RaitoBezarius (Migrated from github.com) reviewed 2023-05-18 16:34:28 +00:00
rust/stub/src/unified_sections.rs
@ -0,0 +2,4 @@
/// UKI specification.
/// This is the canonical order in which they are measured into TPM
/// PCR 11.
/// !!! DO NOT REORDER !!!
RaitoBezarius (Migrated from github.com) commented 2023-05-18 16:34:27 +00:00
Copy link

Applied :)

Applied :)
RaitoBezarius commented 2023-05-18 16:36:13 +00:00 (Migrated from github.com)
Copy link

Formatted and test fixed now.

Formatted and test fixed now.
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels   
bug

Something isn't working

  
dependency

Issues related to our dependencies

  
documentation

Improvements or additions to documentation

  
duplicate

This issue or pull request already exists

  
enhancement

New feature or request

  
good first issue

Good for newcomers

  
help wanted

Extra attention is needed

  
invalid

This doesn't seem right

  
question

Further information is requested

  
review-next

   
security

   
stub

   
tool

   
wontfix

This will not be worked on

No labels
bug
dependency
documentation
duplicate
enhancement
good first issue
help wanted
invalid
question
review-next
security
stub
tool
wontfix
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference: raito/lanzaboote#167
No description provided.
Delete branch "sd-stub-tpm2"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?