minimum self-signed DICE #698

flihp · 2022-08-09T16:43:39Z

This PR adds a minimal, self-signed DeviceId and Alias cert creation process to stage0. The majority of the functionality is collected in a library that provides a types for various artifacts (Cdi, CdiL0, AliasOkm etc), functions for their derivation, and an interface to build the DeviceId and Alias certs. The artifacts created by stage0 (Alias seed, Alias cert & DeviceId cert) are then passed through RAM to a demonstration / test task in hubris. This task extracts the relevant artifacts, checks signatures on the certs and dumps the FWID out to a ringbuf.

NOTE: The demonstration task should be removed from this PR before merging as it serves no useful purpose beyond demo / debug.

chips/lpc55/chip.toml

lib/dice/src/handoff.rs

lib/dice/src/lib.rs

stage0/src/dice.rs

lib/dice/src/deviceid_cert_tmpl.rs

task/dice-data/src/main.rs

flihp · 2022-08-16T17:49:45Z

NOTE: In addressing review comments I noticed that I hadn't enabled dice in the app toml until the commit with the demo task (that should not be merged). This should have been included in the stage0.toml in bbd3260. This has been fixed and now the dice feature is enabled by default. If DICE is disabled on an lpc55 executing this code then none of the artifacts are generated and stage0 kicks off the hubris image like it always has.

lib/dice/src/cert.rs

mx-shift · 2022-08-20T00:19:31Z

lib/dice/src/handoff.rs

+
+// This memory is the USB peripheral SRAM that's 0x4000 bytes long. Changes
+// to this address must be coordinated with the [dice_*] tables in
+// chips/lpc55/chips.toml


build.rs could generate these from the config.

I spent a bit of time looking at this and while I agree in general it would require adding some plumbing to xtask. The data we would need is in the 'chip' key from the top level toml table but AFAIK the currently available methods for getting at the app toml don't expose it:

build_util::config exposes the app config that includes tables under [config]

build_util::task_config exposes tables under the [task_name] table

That's not to say it can't be done, but AFAIK doing so would require a new interface in build_util and some plumbing in xtask to expose the chip key / file path. This is definitely something that should be added but maybe it can wait till after this PR is merged? #731

lib/dice/src/handoff.rs

mx-shift · 2022-08-24T21:52:51Z

chips/lpc55/chip.toml

@@ -46,6 +46,12 @@ size = 4096
 address = 0x4003A000
 size = 4096

+# this is the start of the USB SRAM AHB peripheral (0x4000 bytes total)
+# we appropriate this SRAM for passing DICE artifacts
+[dice_alias]


I'd prefer that peripherals, especially those defined in the chip file, reflect the actual peripheral rather than our usage of it. In this case, renaming to usb_sram and extending to the full size.

Normally I'd agree with you here but eventually we'll be transferring other collections of artifacts across this boundary. In a future where we're transferring credentials to multiple recipients / hubris tasks we'll want guarantees that each task is only able to access the appropriate credentials. AFAIK, by dividing the USB SRAM into regions in the chip.toml we're able to grant access to each region using the existing mechanisms in the kernel. I wrote this up in RFD 282: https://rfd.shared.oxide.computer/rfd/0282#_ram

Use of the USB SRAM was a choice of convenience and so if the issue is with this memory being associated with a peripheral we could select any sufficiently large & unused region of memory for this purpose. This would still require that we list it in the chip.toml file as separate memory regions but it would keep us from labeling the USB SRAM as anything but what it is.

Could we use SRAM 4 instead (0x20040000-0x20043FFF)? That seems less weird than using spare USB SRAM, and isn't otherwise used in the system.

(I'm not sure if we'd need to jump through any hoops at startup to turn it on, or whether it's on immediately)

lib/dice/src/cert.rs

lib/dice/src/lib.rs

stage0/src/dice.rs

lib/dice/src/lib.rs

stage0/src/dice.rs

stage0/src/image_header.rs

mkeeter · 2022-09-01T18:48:55Z

lib/dice/src/cert.rs

+        &self.as_bytes()[Self::PUB_START..Self::PUB_END]
+    }
+
+    fn set_pub(mut self, pubkey: &[u8; PUBLICKEY_SERIALIZED_LENGTH]) -> Self


Nit: in cases where we have a _LENGTH constant, could we remove the _END and use *_START + *LENGTH instead?

We could. This case is a bit weird since this _LENGTH is coming from a different library. But either way _START, _END, _LENGTH stuff from the *_tmpl.rs is mostly generated at this point so the _LENGTH values in these files are just calculated values from _START and _END behind the scenes.

mkeeter · 2022-09-01T18:51:30Z

lib/dice/src/alias_cert_tmpl.rs

+#[allow(dead_code)]
+pub const SIZE: usize = 608;
+#[allow(dead_code)]
+pub const SERIAL_NUMBER_START: usize = 15;


Nit: these could be more compactly expressed as ranges, e.g.

pub const SERIAL_NUMBER_RANGE: core::ops::Range<usize> = 15..16;

Doing so would reduce the potential for copy-paste errors!

TIL core::ops::Range exists. The code that generates these tmpl files can't go into the hubris tree cleanly since I'm using a bunch of external tools to do ASN.1 / x509 crimes but I'll queue this change up for the code gen. This may also help w/ your previous point about _LENGTH values.

mkeeter · 2022-09-01T18:51:49Z

lib/dice/src/cert.rs

+    fn as_bytes(&self) -> &[u8];
+    fn as_mut_bytes(&mut self) -> &mut [u8];
+
+    const SERIAL_NUMBER_START: usize;


(same comment here about using core::ops::Range<usize>)

lib/dice/src/cert.rs

mkeeter · 2022-09-01T19:07:10Z

lib/dice/src/cert.rs

+    const SERIAL_NUMBER_START: usize;
+    const SERIAL_NUMBER_END: usize;
+
+    fn get_serial_number(&self) -> u8 {


Optional:

If you want to get fancy here, you could add a helper function of the form

fn read_range<'a, T>(&'a self, r: core::ops::Range<usize>) -> T where T: TryFrom<&'a [u8]> { self.as_bytes()[r].try_into().unwrap_lite() }

Then, get_serial_number would become

fn get_serial_number(&self) -> u8 { u8::from_be_bytes(self.read_range(Self::SERIAL_NUMBER_RANGE)) }

(and similarly, the other getters would be a little less verbose)

lib/dice/src/cert.rs

task/dice-data/src/main.rs

mkeeter · 2022-09-01T19:20:14Z

lib/dice/src/cert.rs

+            .set_issuer_sn(dname_sn)
+            .set_subject_sn(dname_sn)
+            .set_pub(keypair.public.as_bytes())
+            .sign(keypair)


Vague architectural vibes (no need to take action): the API would be more robust if we split up the intermediate vs signed certificate, e.g. if we had CertBuilder::sign(self, keypair: &Keypair) -> Cert

This would protect us from editing the certificate after it was signed. I'm not exactly sure how this would work with the generics – maybe the Cert type would be an associated type on CertBuilder, and it would store the various fields?

I definitely like the idea of minimizing potential mistakes. I'm not sure how the generics would work either though. The two cert types have different fields (only one, but still different) so we'd need a different builder for each cert type?

Ah, I didn't notice that there was a difference in the Wall Of Fields. In that case, we'd need separate builder types for each one (which might even let us kill the Cert trait entirely?)

This will probably cause everything in the Cert trait to move into another trait for the builders. Just moves things around.

mx-shift · 2022-09-02T18:53:06Z

lib/dice/src/cert.rs

+        0x88, 0x35, 0x50, 0x0E,
+    ];
+
+    // Each time this library is built the notBefore time in the validity


I don't see anything in this library dealing with notBefore. Is this a TODO?

I had initially set this up such that the cert templates were generated at build time. I pulled this out because it had weird dependencies on external tools used to create the x.509 structures. In constructing the templates I was initially setting the notBefore field with the system time before I realized that this would break reproducible builds. This can be done in such a way it won't break reproducible builds but it requires support from xtask. I'll make a ticket and add a TODO.

lib/dice/src/handoff.rs

mx-shift · 2022-09-02T19:12:06Z

lib/dice/src/handoff.rs

+        // 1) The associated memory region has been enabled / turned on if
+        // necessary. This happens in the constructor / 'turn_on' function.
+        // 2) The function call is made by code in stage0.
+        let dst: &mut [u8] = unsafe { &mut *dst_ptr };


Suggested change

let dst: &mut [u8] = unsafe { &mut *dst_ptr };

let dst: &mut [u8] = unsafe { core::slice::from_raw_parts_mut(ALIAS_START as *mut u8, AliasData::MAX_SIZE) };

Should also check that AliasData::MAX_SIZE fits in the dice_alias peripheral's range. That should be a compile-time check of some sort.

mx-shift · 2022-09-02T19:14:22Z

lib/dice/src/handoff.rs

+        // conditions are met:
+        // 1) The associated memory region has been enabled / turned on if
+        // necessary. This happens in the constructor / 'turn_on' function.
+        // 2) The function call is made by code in stage0.


Feels worth clarifying in the safety comment that this is safe even if these conditions aren't met since it will cause a fault.

mx-shift · 2022-09-02T19:20:21Z

lib/dice/src/handoff.rs

+        // necessary. This should be done by code in stage0.
+        // 2) The task making the call has been granted access to the memory
+        // region by the kernel.
+        let src: &[u8] = unsafe { &*src_ptr };


Suggested change

let src: &[u8] = unsafe { &*src_ptr };

let src: &[u8] = unsafe { core::slice::from_raw_parts(ALIAS_START as *const u8, AliasData::MAX_SIZE) };

Same comment about checking that AliasData::MAX_SIZE is less than or equal to dice_alias peripheral size.

mx-shift · 2022-09-02T19:21:19Z

lib/dice/src/handoff.rs

+        // 1) The associated memory region has been enabled / turned on if
+        // necessary. This should be done by code in stage0.
+        // 2) The task making the call has been granted access to the memory
+        // region by the kernel.


Same comment about noting that this is safe even if these conditions are violated as it will cause a fault.

mx-shift · 2022-09-02T19:47:30Z

lib/dice/src/lib.rs

+        if self.0.iter().all(|&w| w == 0) {
+            true
+        } else {
+            false
+        }


Suggested change

if self.0.iter().all(|&w| w == 0) {

true

} else {

false

}

self.0.iter().all(|&w| w == 0)

mx-shift · 2022-09-02T19:51:32Z

lib/dice/src/lib.rs

+
+        for (dst, src) in cdi
+            .chunks_exact_mut(mem::size_of::<u32>())
+            .zip(cdi_reg.0.as_ref())


impl SeedBuf on CdiReg instead of cdi_reg.0.as_ref()?

CdiReg is a slice of u32s not bytes like the SeedBufs. The type exists pretty much exclusively to ensure the registers that it wraps are zeroed when it's dropped.

mx-shift · 2022-09-02T19:59:11Z

stage0/src/dice.rs

+pub fn run(image: &Image) {
+    let cdi = match Cdi::from_reg() {
+        Some(cdi) => cdi,
+        None => return,


If the CDI isn't available, this is going to skip turning on the USB SRAM. That means that any task that attempts to access the DICE artifacts in stage1 will fault when they try to access it. Probably better to enable the USB SRAM and write out a well-known pattern indicating that DICE was disabled in stage0 so that can reported upstream.

This came up elsewhere in the review and I 💯 agree. For now the well-known pattern is 0's but I'll do some testing to see how well this works in practice.

The current cert implementation is limited to the creation of a self signed cert for the DeviceId & the Alias cert. Cert creation is as simple as possible with a template for each cert (DER) that is populated with fixed length data like the platform serial number, certificate serial number, public key & signature. Future work will address programatic generation of these templates at build time to replace the currently static ones.

The memory used in this commit is SRAM intended for use by USB. The choice of using this memory region was mostly driven by convenience and pivoting to a different range should be straight forward. The size of this region was chosen based on the estimated size of the data passed. The DeviceId and Alias cert are each ~600 bytes and the seed required to reconstruct the Alias key is 32 bytes. All of this fits comfortably in 2k.

…ials. This commit collects the DICE CDI from the NXP ROM, creates an ed25519 key pair from it (effectively the DeviceId) and uses this key to create a self-signed DeviceId cert. The hash of the hubris image that will be launched by stage0 is then hashed and the output used to generate the Alias credentials (used for attestation). Finally the keying material used to create the Alias key, and the cert chain from the Alias back to the DeviceId are written to memory for use in an attestation exchange by a future hubris task. If run on an lpc55 with DICE disabled (the default config) none of the DICE artifacts will be created and hubris will be booted as usual.

* Update lpc55_support for compatibility with latest zeroize crate. * lib-dice: Add new library with types for DICE artifacts and certs. The current cert implementation is limited to the creation of a self signed cert for the DeviceId & the Alias cert. Cert creation is as simple as possible with a template for each cert (DER) that is populated with fixed length data like the platform serial number, certificate serial number, public key & signature. Future work will address programatic generation of these templates at build time to replace the currently static ones. * lpc55/chip.toml: Add memory regions for DICE artifact handoff. The memory used in this commit is SRAM intended for use by USB. The choice of using this memory region was mostly driven by convenience and pivoting to a different range should be straight forward. The size of this region was chosen based on the estimated size of the data passed. The DeviceId and Alias cert are each ~600 bytes and the seed required to reconstruct the Alias key is 32 bytes. All of this fits comfortably in 2k. * stage0: Use the dice library to implement minimal self-signed credentials. This commit collects the DICE CDI from the NXP ROM, creates an ed25519 key pair from it (effectively the DeviceId) and uses this key to create a self-signed DeviceId cert. The hash of the hubris image that will be launched by stage0 is then hashed and the output used to generate the Alias credentials (used for attestation). Finally the keying material used to create the Alias key, and the cert chain from the Alias back to the DeviceId are written to memory for use in an attestation exchange by a future hubris task. If run on an lpc55 with DICE disabled (the default config) none of the DICE artifacts will be created and hubris will be booted as usual.

flihp requested a review from labbott August 9, 2022 16:43

labbott reviewed Aug 11, 2022

View reviewed changes

flihp force-pushed the dice-self-min branch 2 times, most recently from 43d36a9 to 24f50bb Compare August 16, 2022 16:57

mx-shift reviewed Aug 20, 2022

View reviewed changes

flihp force-pushed the dice-self-min branch 4 times, most recently from 47998cd to 78d82c3 Compare August 24, 2022 17:56

mx-shift reviewed Aug 25, 2022

View reviewed changes

flihp force-pushed the dice-self-min branch 9 times, most recently from 6bb2a73 to fce77ef Compare August 30, 2022 18:07

mkeeter reviewed Sep 1, 2022

View reviewed changes

lib/dice/src/cert.rs Show resolved Hide resolved

mkeeter reviewed Sep 1, 2022

View reviewed changes

lib/dice/src/cert.rs Outdated Show resolved Hide resolved

mkeeter reviewed Sep 1, 2022

View reviewed changes

task/dice-data/src/main.rs Outdated Show resolved Hide resolved

mkeeter reviewed Sep 1, 2022

View reviewed changes

flihp mentioned this pull request Sep 1, 2022

use core::ops::Range<usize> in place of start / end / length values in DICE tmpls #747

Closed

flihp force-pushed the dice-self-min branch from fce77ef to 60e857a Compare September 1, 2022 22:54

mx-shift reviewed Sep 2, 2022

View reviewed changes

flihp added 4 commits September 2, 2022 14:52

Update lpc55_support for compatibility with latest zeroize crate.

e2f69c6

flihp force-pushed the dice-self-min branch from 60e857a to f08ebad Compare September 2, 2022 23:14

flihp merged commit 58dd4d3 into oxidecomputer:master Sep 2, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

minimum self-signed DICE #698

minimum self-signed DICE #698

flihp commented Aug 9, 2022

flihp commented Aug 16, 2022

mx-shift Aug 20, 2022

flihp Aug 24, 2022 •

edited

Loading

mx-shift Aug 24, 2022

flihp Aug 25, 2022

mkeeter Sep 1, 2022

mkeeter Sep 1, 2022

flihp Sep 1, 2022

mkeeter Sep 1, 2022

flihp Sep 1, 2022

flihp Sep 1, 2022

mkeeter Sep 1, 2022

mkeeter Sep 1, 2022

mkeeter Sep 1, 2022

flihp Sep 1, 2022

mkeeter Sep 2, 2022

flihp Sep 2, 2022

mx-shift Sep 2, 2022

flihp Sep 2, 2022

flihp Sep 2, 2022

mx-shift Sep 2, 2022

mx-shift Sep 2, 2022

flihp Sep 2, 2022

mx-shift Sep 2, 2022

mx-shift Sep 2, 2022

mx-shift Sep 2, 2022

mx-shift Sep 2, 2022

mx-shift Sep 2, 2022

flihp Sep 2, 2022

mx-shift Sep 2, 2022

flihp Sep 2, 2022

mx-shift Sep 2, 2022

flihp Sep 2, 2022

	let dst: &mut [u8] = unsafe { &mut *dst_ptr };
	let dst: &mut [u8] = unsafe { core::slice::from_raw_parts_mut(ALIAS_START as *mut u8, AliasData::MAX_SIZE) };

	let src: &[u8] = unsafe { &*src_ptr };
	let src: &[u8] = unsafe { core::slice::from_raw_parts(ALIAS_START as *const u8, AliasData::MAX_SIZE) };

minimum self-signed DICE #698

minimum self-signed DICE #698

Conversation

flihp commented Aug 9, 2022

flihp commented Aug 16, 2022

Choose a reason for hiding this comment

flihp Aug 24, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flihp Aug 24, 2022 •

edited

Loading