Add vendor commands to inject crypto materials #239
Conversation
jmichelp
force-pushed
the
transparency
branch
from
f443252
to
c1ddb51
Compare
|
I reviewed all code except the patches, and most comments are minor! |
src/ctap/mod.rs
Outdated
|
|
||
| if params.attestation_material.is_some() { | ||
| let data = params.attestation_material.unwrap(); | ||
| if !has_cert { |
There was a problem hiding this comment.
We don't want to return an error (e.g. invalid argument) if there is already attestation material in the store?
There was a problem hiding this comment.
I thought about it. But what if, for example, writing the certificate works but fails for the private key?
I saw 3 solutions:
- modify the cbor command to allow setting certificate and private key individually or together
- accept but ignore an already set value which allows to recover from the described scenario
- delete the certificate if the private key fails.
There was a problem hiding this comment.
I see. Both 2 and 3 look good (2 being the current code). For 2 we could additionally check that the ignored value is equal to the existing one. But I'm fine with the current code, was just asking.
There was a problem hiding this comment.
Why is overwriting so bad on a non-locked-down device? Maybe just make writing conditional, and since lockdown depends on both being set, you should be good?
There was a problem hiding this comment.
I think the code deals equally well with none of the 2 being set, or only 1 being set.
There was a problem hiding this comment.
So solution 4 would be to have a is_locked_down in persistent storage, that reflects that. And make writing to the certificate conditional on that. It would also allow to return the state of is_locked_down as part of this command as you planned.
There was a problem hiding this comment.
Doing this means that we have one value stored at 2 places which can go out of sync.
There was a problem hiding this comment.
From another thread I understood that we can't know if the device is locked, so we don't really have it in 2 places. If it's possible to fetch the value from some register (maybe adding a syscall to the new driver, e.g. command 0 could return whether the device is protected), then that would be the best solution no?
There was a problem hiding this comment.
I updated HIL + capsule + syscalls to have a get/set model and support different levels of protection. For the vendor specific command I keep a boolean because it doesn't make sense in our context to partially protect.
Regarding our implementation and allowing overwriting the certificate and the private key on a non-locked device, I still see an issue allowing it: it would invalidate all credentials that have been created using U2F. This would only make sense in combination with a reset command (although I'm not familiar enough with batch attestation and the implications it may have). With CTAP2.1 though, some credentials may persist even after a reset and could still risk to be invalidated.
There was a problem hiding this comment.
I think the current implementation is solid, then.
| _ => ProtectionLevel::Unknown, | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Probably not actionable, but ideally this kind of stuff should be in a shared library between the kernel and user-land. I don't know if Tock already has this kind of shared library.
There was a problem hiding this comment.
I don't recall such shared library, but agree that it could be useful. CC @jrvanwhy
There was a problem hiding this comment.
I do not believe there is any such library. If I recall my past communications with other Tock core WG members correctly, they had an explanation for why they don't want to share code between the kernel and libtock-rs.
How the OpenSK project structures code in its own repository is up to OpenSK, but that may not be helpful if you would like to upstream this driver in the future.
Discussing this on the Tock core WG call is an option as well.
There was a problem hiding this comment.
they don't want to share code
Just to clarify, the idea here is not to share code but to share APIs (e.g. types, enum values), i.e. share what is used to define the interface of kernel and user-land.
There was a problem hiding this comment.
Mostly took a look at patches/tock, nothing major to flag assuming that a corresponding pull request will happen on https://github.com/tock/tock soon (if not yet started).
| -//! Minimal implementation to support activation of the reset button on | ||
| -//! nRF52-DK. |
There was a problem hiding this comment.
What changed w.r.t. this comment? Is the code now complete, or does it support more features (but not all of them) than the reset button?
There was a problem hiding this comment.
We already added support for voltage regulator and NFC pin protection.
This patch adds support for code read protection and the PR I'll prepare for Tock will add on top of that support for Nordic bootloader (2 words are stored in UICR and could be wiped, which could result in a bricked device).
| @@ -0,0 +1,423 @@ | |||
| diff --git a/boards/components/src/firmware_protection.rs b/boards/components/src/firmware_protection.rs | |||
There was a problem hiding this comment.
Is there an upstream pull-request to discuss the capsule/component/chip additions (which should stay in this patch as shortly as possible)?
There was a problem hiding this comment.
Not yet. I needed the feature for OpenSK first. Starting the PR for Tock is next but will clearly happen.
| +//! # use kernel::static_init; | ||
| +//! | ||
| +//! let crp = static_init!( | ||
| +//! capsules::firware_protection::FirmwareProtection<'static>, |
| + } | ||
| + } | ||
| + | ||
| + fn set_protection(&self, level: ProtectionLevel) -> ReturnCode { |
There was a problem hiding this comment.
Nit: might make sense to add a few blank lines to separate the logical parts within this function.
| + self.registers | ||
| + .debugctrl | ||
| + .write(DebugControl::CPUNIDEN::DISABLED + DebugControl::CPUFPBEN::DISABLED); | ||
| + // TODO(jmichel): Kill bootloader if present |
There was a problem hiding this comment.
Nit: Could make sense to be more specific than "kill" here, does it mean "disable" (temporarily), or "erase from flash", or something else?
There was a problem hiding this comment.
Hopefully made it clearer.
When a bootloader is present and we want firmware protection we're left with 2 options:
- configure the bootloader to only accept signed updates, sign all our firmware, etc.
- prevent returning into bootloader and handle the firmware updates at Tock level.
I was experimenting with 2, thinking that erasing the 2 values stored in UICR would be enough but it bricked the dongles (still haven't found a way to recover them). I left a TODO at the moment but I guess the path will probably to modify deploy.py and go for signed firmware.
jmichelp
force-pushed
the
transparency
branch
from
f225b08
to
712fa0f
Compare
Fixes #195
The PR is rather big and involves many modifications so I tried to keep the individual commits clean (they could be reviewed separately instead of the PR as a whole):
Fully tested on the nRF52840DK board. I had issues with the nRF52840 dongle I have so I'll test it in parallel of the review.
At the moment, the Makerdiairy dongle is excluded on purpose (the case is sealed so any mistake with the lockdown would require a destructive opening to recover the chip).