lpc55-rot-startup: Use a better method to generate an FWID. #1500

flihp · 2023-08-17T00:04:47Z

The current implementation captures all of the RoT software. It does not however include other pages in the RoT image flash range. Expanding our definition for the RoT FWID as not only the expected image, but also any other programmed flash pages in the range of possible pages mitigates a potential persistence mechanism for an attacker.

This commit:

adds a new environment variable HUBRIS_FLASH_OUTPUT that holds the config::Output structure for the flash region assigned to the image being built
generates a Range from HUBRIS_FLASH_OUTPUT usable by the lpc55-rot-starup code
uses this Range to walk through all possible flash pages for the image including each page that's been programmed in the FWID

flihp · 2023-08-17T16:45:45Z

This can be tested in a few different ways if folks want to reproduce. The easiest is to (assuming we're using imagea on an RoT)

erase the flash range 0x10_000..0x50_000 (consult lpc55 memory.toml for values):
NOTE: this is not strictly necessary but my dev board had a random flash page programmed well outside of the expected range likely from past work so this may be required
boot the image
copy out all of the flash pages programmed
NOTE: humility readmem can be used for this but you'll have to read out the flash in 0x10_000 byte blocks and convert the hex dump back to binary. This works but it's easier to use gdb dump binary memory foo.bin 0x10000 0xEND
1. the start address should be 0x10_000 per above
2. the end address should be 0x10_000 + sizeof(final.bin) + FLASH_PAGE_SIZE - sizeof(final.bin) % FLASH_PAGE_SIZE
  1. sizeof(final.bin) = 225736
  2. FLASH_PAGE_SIZE = 512
hash the memory dumped by gdb: $ openssl sha3-256 foo.bin
NOTE the expected digest is 897408be5039aec1fcb581c01c5afdc4d4f433a28a6d19a52c4563235b7493d0

get the FWID calculated by hubris on boot (from this PR) by querying the update server for the rot_boot_info: humility --target rot-carrier --archive-name imagea -- hiffy --call Update.rot_boot_info
NOTE: the output should look something like:

Update.rot_boot_info() => RotBootInfo { active: A, persistent_boot_preference: A, pending_persistent_boot_preference: None, transient_boot_preference: None, slot_a_sha3_256_digest: Some([ 0x89, 0x74, 0x8, 0xbe, 0x50, 0x39, 0xae, 0xc1, 0xfc, 0xb5, 0x81, 0xc0, 0x1c, 0x5a, 0xfd, 0xc4, 0xd4, 0xf4, 0x33, 0xa2, 0x8a, 0x6d, 0x19, 0xa5, 0x2c, 0x45, 0x63, 0x23, 0x5b, 0x74, 0x93, 0xd0 ]), slot_b_sha3_256_digest: None }

lib/lpc55-rot-startup/build.rs

labbott · 2023-08-17T17:25:06Z

lib/lpc55-rot-startup/src/images.rs

+
+        for start in IMAGE_FLASH.step_by(FLASH_PAGE_SIZE) {
+            let start: u32 = start.try_into().unwrap_lite();
+            if lpc55_romapi::validate_programmed(start, PAGE_SIZE) {


Does this handle partially programmed pages correctly? I need to go review exactly how this would work on trailing pages (aside, I should sit down and finally rip out usage of this ROM API and just use the flash driver)

ROM API rip / replace: #1431

Does this handle partially programmed pages correctly?

I think so, assuming my reading of UM11126 § 8.6.7 is correct. This implementation includes the 0xff trailing data added by the write command in the FWID hash.

andrewjstone · 2023-08-18T20:30:55Z

@flihp asked me to take a look at this because we plumb the digest all the way up through wicket. AFAICT this looks good and closes a security hole. Approved from my POV, although you may want to build atop @labbott's recent changes that rip out the rom api.

labbott

I ran the tests you suggested and was able to get a reliable answer. Even longer term we might just want to write to the entire image length space.

labbott · 2023-08-22T14:40:23Z

lib/lpc55-rot-startup/src/images.rs

+    pub fn get_fwid(&self) -> [u8; 32] {
+        let mut hash = Sha3_256::new();
+
+        for start in IMAGE_FLASH.step_by(FLASH_PAGE_SIZE) {


Okay while testing I realize this doesn't actually work. We need to be able to get the base of the other image, otherwise they both end up with the same hash:

$ humility -a target/rot-carrier/dist/a/build-rot-carrier-image-a.zip hiffy --call Update.rot_boot_info humility: attached via CMSIS-DAP Update.rot_boot_info() => RotBootInfo { active: A, persistent_boot_preference: A, pending_persistent_boot_preference: None, transient_boot_preference: None, slot_a_sha3_256_digest: Some([ 0x9d, 0xce, 0xc5, 0x7e, 0x78, 0xb8, 0x74, 0xb0, 0xc8, 0x78, 0x71, 0x7e, 0xf1, 0x58, 0x8b, 0x53, 0x88, 0xea, 0xc, 0x7, 0xb6, 0x23, 0x3, 0x69, 0x3b, 0x8f, 0xd4, 0xd8, 0x1d, 0x10, 0x8f, 0x1 ]), slot_b_sha3_256_digest: Some([ 0x9d, 0xce, 0xc5, 0x7e, 0x78, 0xb8, 0x74, 0xb0, 0xc8, 0x78, 0x71, 0x7e, 0xf1, 0x58, 0x8b, 0x53, 0x88, 0xea, 0xc, 0x7, 0xb6, 0x23, 0x3, 0x69, 0x3b, 0x8f, 0xd4, 0xd8, 0x1d, 0x10, 0x8f, 0x1 ]) }

I'll give this a think if there's another option

I see what you mean. This doesn't become apparent until you've flashed the 'b' image before pulling the rot_boot_info:

(hubris) [flihp@slag hubris]$ humility --target rot-carrier --archive-name imagea -- hiffy --call Update.rot_boot_info humility: attached to 1fc9:0143:IYJ1NYJRCW2TC via CMSIS-DAP Update.rot_boot_info() => RotBootInfo { active: B, persistent_boot_preference: A, pending_persistent_boot_preference: None, transient_boot_preference: None, slot_a_sha3_256_digest: Some([ 0x3d, 0x4d, 0x34, 0x69, 0x37, 0x24, 0x8b, 0xef, 0x93, 0xd7, 0x18, 0xd1, 0x25, 0xf3, 0xe0, 0xab, 0x15, 0xbc, 0x15, 0x98, 0x33, 0xaa, 0x46, 0x67, 0x91, 0xa4, 0x26, 0x53, 0xde, 0x53, 0x10, 0x41 ]), slot_b_sha3_256_digest: Some([ 0x3d, 0x4d, 0x34, 0x69, 0x37, 0x24, 0x8b, 0xef, 0x93, 0xd7, 0x18, 0xd1, 0x25, 0xf3, 0xe0, 0xab, 0x15, 0xbc, 0x15, 0x98, 0x33, 0xaa, 0x46, 0x67, 0x91, 0xa4, 0x26, 0x53, 0xde, 0x53, 0x10, 0x41 ]) }

It's pretty much impossible for the two images to have the same hash (and still be functional) so this is definitely wrong. I'll sort out some options and update the PR.

I've just pushed two commits with a new approach. Instead of exporting the range for the image being built we now export all flash ranges for the chip from xtask through the environment. This allows us to use the appropriate range for A or B images. This also duplicates data in the HUBRIS_DICE_MFG env var so I've included another commit removing it.

flihp · 2023-08-23T16:09:40Z

@flihp asked me to take a look at this because we plumb the digest all the way up through wicket. AFAICT this looks good and closes a security hole. Approved from my POV, although you may want to build atop @labbott's recent changes that rip out the rom api.

Those recent changes went in after this PR was opened. I'll definitely rebase before merging but didn't want to do so while folks were actively reviewing.

flihp · 2023-08-23T16:43:15Z

@flihp asked me to take a look at this because we plumb the digest all the way up through wicket. AFAICT this looks good and closes a security hole. Approved from my POV, although you may want to build atop @labbott's recent changes that rip out the rom api.

Those recent changes went in after this PR was opened. I'll definitely rebase before merging but didn't want to do so while folks were actively reviewing.

Ah that PR hasn't been merged yet. Either way there will be rebasing.

labbott

Good solution!

flihp · 2023-08-25T16:32:46Z

I'm rebasing to clean up history from this review & will merge once those changes pass CI.

The current implementation captures all of the RoT software. It does not however include other pages in the RoT image flash range. Expanding our definition for the RoT FWID as not only the expected image, but also any other programmed flash pages in the range of possible pages mitigates a potential persistence mechanism for an attacker. This commit: - adds a new environment variable HUBRIS_FLASH_OUTPUT that holds all config::Output structures for the flash regions assigned to the image being built - generates a Range for each flash region in HUBRIS_FLASH_OUTPUTS for use by the lpc55-rot-startup crate - uses the A & B Ranges to walk through all possible flash pages for the given image including each page that's been programmed in the FWID

HUBRIS_DICE_MFG is no longer needed.

flihp requested review from cbiffle and labbott as code owners August 17, 2023 00:04

flihp requested a review from lzrd August 17, 2023 15:35

flihp linked an issue Aug 17, 2023 that may be closed by this pull request

improve RoT stage0 FWID calculation #736

Closed

labbott reviewed Aug 17, 2023

View reviewed changes

flihp requested a review from andrewjstone August 18, 2023 16:52

labbott approved these changes Aug 21, 2023

View reviewed changes

labbott requested changes Aug 22, 2023

View reviewed changes

flihp requested a review from labbott August 23, 2023 23:40

flihp force-pushed the fwid-calc branch 2 times, most recently from 296f6ad to 576c06a Compare August 24, 2023 02:14

labbott approved these changes Aug 24, 2023

View reviewed changes

flihp force-pushed the fwid-calc branch 2 times, most recently from dc62478 to 22b3557 Compare August 25, 2023 16:40

flihp added 2 commits August 25, 2023 09:49

lpc55-rot-startup: Get DICE mfg flash region from HUBRIS_FLASH_OUTPUTs.

435032a

HUBRIS_DICE_MFG is no longer needed.

flihp force-pushed the fwid-calc branch from 22b3557 to 435032a Compare August 25, 2023 16:49

flihp merged commit b2b4861 into master Aug 25, 2023
71 checks passed

flihp mentioned this pull request Aug 29, 2023

Move away from ROM APIs for flash writing #1504

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

lpc55-rot-startup: Use a better method to generate an FWID. #1500

lpc55-rot-startup: Use a better method to generate an FWID. #1500

flihp commented Aug 17, 2023

flihp commented Aug 17, 2023

labbott Aug 17, 2023

flihp Aug 17, 2023

flihp Aug 17, 2023

andrewjstone commented Aug 18, 2023

labbott left a comment

labbott Aug 22, 2023

flihp Aug 23, 2023

flihp Aug 23, 2023 •

edited

Loading

flihp commented Aug 23, 2023

flihp commented Aug 23, 2023

labbott left a comment

flihp commented Aug 25, 2023

lpc55-rot-startup: Use a better method to generate an FWID. #1500

lpc55-rot-startup: Use a better method to generate an FWID. #1500

Conversation

flihp commented Aug 17, 2023

flihp commented Aug 17, 2023

labbott Aug 17, 2023

Choose a reason for hiding this comment

flihp Aug 17, 2023

Choose a reason for hiding this comment

flihp Aug 17, 2023

Choose a reason for hiding this comment

andrewjstone commented Aug 18, 2023

labbott left a comment

Choose a reason for hiding this comment

labbott Aug 22, 2023

Choose a reason for hiding this comment

flihp Aug 23, 2023

Choose a reason for hiding this comment

flihp Aug 23, 2023 • edited Loading

Choose a reason for hiding this comment

flihp commented Aug 23, 2023

flihp commented Aug 23, 2023

labbott left a comment

Choose a reason for hiding this comment

flihp commented Aug 25, 2023

flihp Aug 23, 2023 •

edited

Loading