-
Notifications
You must be signed in to change notification settings - Fork 803
Add vendor and image class UUID support #2409
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
base: main
Are you sure you want to change the base?
Add vendor and image class UUID support #2409
Conversation
ab8fffa
to
fc4c042
Compare
if (len != sizeof(img_uuid_vid)) { | ||
/* Vendor UUID is not valid. */ | ||
rc = -1; | ||
goto out; | ||
} | ||
|
||
rc = LOAD_IMAGE_DATA(hdr, fap, off, img_uuid_vid.raw, len); | ||
if (rc) { | ||
goto out; | ||
} |
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.
If it was decided to wrap uuid in fih, these should probably also report fih failure; i do not understand a difference between a failure in uuid check and inability to perform the check. And I know that we are inconsistent here, and maybe that is a good point to discuss 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.
There is a translation logic, based on the rc value:
if (rc) {
FIH_SET(fih_rc, FIH_FAILURE);
and the default value of uuid_vid_valid
and uuid_cid_valid
will either way mask the other failure reasons.
The difference is just that - inability (if possible) can report "incompatible image format" and the uuid check can report "invalid UUID value".
There is though no way to distinguish between those error reasons as there is just a single FIH_FAILURE
error code.
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.
out: seams can't mock success - it only could mock failure as a glith.
@@ -142,6 +142,8 @@ extern "C" { | |||
* ... | |||
* 0xffa0 - 0xfffe | |||
*/ | |||
#define IMAGE_TLV_UUID_VID 0x80 /* Vendor unique identifier */ |
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.
These go above comment explaining vendor specific ids. If these are vendor specific ids, then they have in-proper codes.
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.
And should't really be here.
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.
What do you mean by "comment explaining vendor specific ids"?
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.
@tomchy move this just after #define IMAGE_TLV_COMP_DEC_SIZE 0x73 /* Compressed decrypted image size */
line.
@de-nordic Clarification: these are new MCUboot generic TLVs
boot/zephyr/mcuboot_uuid.c
Outdated
|
||
static fih_ret boot_uuid_compare(const struct image_uuid *uuid1, const struct image_uuid *uuid2) | ||
{ | ||
return fih_ret_encode_zero_equality(memcmp(uuid1->raw, uuid2->raw, |
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.
Are we sure we want to get fih heavy on uuid comparisons? What is the attack vector here? As far as i cen tell they are already protected tlvs and, as such, are signed with image, and why would you bypass uid check? You would have to bypass signature first.
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 attack vector would be to install the FW not designed for a particular core as a result of glitching the UUID checks (skipping them or altering comparison results).
Honestly, I do not know where to draw a line between checks that should use FIH and checks that should not. The code was based on the way the security counter is implemented, thus by default it uses FIH routines.
fc4c042
to
54dd37c
Compare
54dd37c
to
25b5c04
Compare
@@ -142,6 +142,8 @@ extern "C" { | |||
* ... | |||
* 0xffa0 - 0xfffe | |||
*/ | |||
#define IMAGE_TLV_UUID_VID 0x80 /* Vendor unique identifier */ |
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.
@tomchy move this just after #define IMAGE_TLV_COMP_DEC_SIZE 0x73 /* Compressed decrypted image size */
line.
@de-nordic Clarification: these are new MCUboot generic TLVs
if (len != sizeof(img_uuid_vid)) { | ||
/* Vendor UUID is not valid. */ | ||
rc = -1; | ||
goto out; | ||
} | ||
|
||
rc = LOAD_IMAGE_DATA(hdr, fap, off, img_uuid_vid.raw, len); | ||
if (rc) { | ||
goto out; | ||
} |
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.
out: seams can't mock success - it only could mock failure as a glith.
25b5c04
to
09fa479
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.
very quick comment, need to review
@@ -612,6 +647,21 @@ def create(self, key, public_key_format, enckey, dependencies=None, | |||
if compression_tlvs is not None: | |||
for tag, value in compression_tlvs.items(): | |||
prot_tlv.add(tag, value) | |||
|
|||
if self.vid is not None: | |||
vid = parse_uuid(uuid.NAMESPACE_DNS, self.vid) |
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.
why is DNS used? What other options are there instead of DNS?
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.
Because it is the simplest thing with a registry to use to specify the vendor.
The python package includes the following namespaces:
- DNS: When this namespace is specified, the name string is a fully qualified domain name.
- URL: When this namespace is specified, the name string is a URL.
- OID: When this namespace is specified, the name string is an ISO OID.
- X500: When this namespace is specified, the name string is an X.500 DN in DER or a text output format.
It also aligns with the values used in SUIT:
The RECOMMENDED method to create a vendor ID is:
The "IANA UUID Namespace ID for DNS" is:
6ba7b810-9dad-11d1-80b4-00c04fd430c8
Vendor ID = UUID5(, vendor domain name)
namespace = uuid.UUID(bytes=vid) | ||
else: | ||
namespace = None |
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.
why does the above use DNS and this either use the above or nothing?
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.
Because it is easy to provide unique VIDs, based on DNS.
It is not easy to provide unique arbitrary CIDs, thus it is safer to use unique VID value as the namespace to calculate CID, thus providing unique, vendor-specific CIDs.
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 None
option is valid if and only if the user provides RAW CID UUID value.
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.
It also aligns with the recommendations in SUIT specification:
If the Vendor ID is a UUID, the RECOMMENDED method to create a Class ID is:
Class ID = UUID5(Vendor ID, Class-Specific-Information)
boot/zephyr/CMakeLists.txt
Outdated
# Include the sample implementation. | ||
if(CONFIG_MCUBOOT_UUID_VID OR CONFIG_MCUBOOT_UUID_CID) | ||
zephyr_library_sources( | ||
mcuboot_uuid.c | ||
) | ||
endif() |
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.
sample? If this is for a sample it should be added specifically by the sample, it should not be in the cmake file and it should not be added for all builds when these Kconfigs are enabled
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.
After second though - you're right: I cannot guarantee that this implementation will be sufficient for all use cases, thus it should not be placed inside MCUboot repository.
boot/zephyr/CMakeLists.txt
Outdated
if(CONFIG_MCUBOOT_UUID_VID OR CONFIG_MCUBOOT_UUID_CID) | ||
string(REGEX MATCHALL "^([0-9A-F][0-9A-F]|\-)+$" match_parts ${CONFIG_MCUBOOT_UUID_VID_VALUE}) |
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.
this runs if either is set, then assumes VID is the one that is set? Also make - optional
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.
Correct, the MCUBOOT_UUID_VID_VALUE
symbol is available if either CONFIG_MCUBOOT_UUID_VID
or CONFIG_MCUBOOT_UUID_CID
is enabled.
The use case is pretty simple: you want a simplified check (only CID), but still want to generate UUID values, based on the VID namespace.
boot/zephyr/CMakeLists.txt
Outdated
|
||
# Generate VID value(s) and raw value definition(s) | ||
if(CONFIG_MCUBOOT_UUID_CID) | ||
set(MCUBOOT_IMAGES_COUNT 4) |
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.
why is this using 4 images?
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'll try to use UPDATEABLE_IMAGE_NUMBER
instead.
boot/zephyr/mcuboot_uuid.c
Outdated
*/ | ||
#include <bootutil/mcuboot_uuid.h> | ||
|
||
#define IMAGE_ID_COUNT 5 |
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.
as above, if this is a sample it should be moved to a sample
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'll move it outside of the MCUboot repo too.
Also, this feature should be added to the simulator for testing |
Add a possibility to express vendor ID and image class ID inside image's TLVs. Signed-off-by: Tomasz Chyrowicz <[email protected]>
09fa479
to
7717a1c
Compare
Allow to specify VID and CID for an image. Signed-off-by: Tomasz Chyrowicz <[email protected]>
7717a1c
to
ee67a90
Compare
According to the PSA Platform Security Boot Guide:
The manifest (which can be interpreted as image header and additional metadata stored inside TLVs) must include:
Instance ID.
This PR introduces two identifiers to meet those requirements: