Skip to content
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

Enforce TLV protected entries #1938

Merged
merged 3 commits into from
Apr 18, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions boot/bootutil/include/bootutil/image.h
Original file line number Diff line number Diff line change
Expand Up @@ -186,6 +186,7 @@ int bootutil_tlv_iter_begin(struct image_tlv_iter *it,
bool prot);
int bootutil_tlv_iter_next(struct image_tlv_iter *it, uint32_t *off,
uint16_t *len, uint16_t *type);
int bootutil_tlv_iter_is_prot(struct image_tlv_iter *it, uint32_t off);

int32_t bootutil_get_img_security_cnt(struct image_header *hdr,
const struct flash_area *fap,
Expand Down
45 changes: 45 additions & 0 deletions boot/bootutil/src/image_validate.c
Original file line number Diff line number Diff line change
Expand Up @@ -349,6 +349,30 @@ bootutil_get_img_security_cnt(struct image_header *hdr,
return 0;
}

#ifndef ALLOW_ROGUE_TLVS
/*
* The following list of TLVs are the only entries allowed in the unprotected
* TLV section. All other TLV entries must be in the protected section.
*/
static const uint16_t allowed_unprot_tlvs[] = {
IMAGE_TLV_KEYHASH,
IMAGE_TLV_PUBKEY,
IMAGE_TLV_SHA256,
IMAGE_TLV_SHA384,
IMAGE_TLV_RSA2048_PSS,
IMAGE_TLV_ECDSA224,
IMAGE_TLV_ECDSA_SIG,
IMAGE_TLV_RSA3072_PSS,
IMAGE_TLV_ED25519,
IMAGE_TLV_ENC_RSA2048,
IMAGE_TLV_ENC_KW,
IMAGE_TLV_ENC_EC256,
IMAGE_TLV_ENC_X25519,
Comment on lines +358 to +370
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the current situation - considering how imgtool works.
In theory the keyhash/pubkey and encryption TLVs could be in the protected area.
Do you think it would make sense to move these TLVs in the future?
As for the keyhash/pubkey TLV, I prefer to have it together with the signature TLV to make re-signing of an image more convenient.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the keyhash/pubhash can stay in the unprotected. The things directly related to signature verification make sense to keep there. It probably makes sense to move the enc ones, but it isn't urgent.

I'll fix the whitespace.

/* Mark end with ANY. */
IMAGE_TLV_ANY,
};
#endif

/*
* Verify the integrity of the image.
* Return non-zero if image could not be validated/does not validate.
Expand Down Expand Up @@ -420,6 +444,27 @@ bootutil_img_validate(struct enc_key_data *enc_state, int image_index,
break;
}

#ifndef ALLOW_ROGUE_TLVS
/*
* Ensure that the non-protected TLV only has entries necessary to hold
* the signature. We also allow encryption related keys to be in the
* unprotected area.
*/
if (!bootutil_tlv_iter_is_prot(&it, off)) {
bool found = false;
for (const uint16_t *p = allowed_unprot_tlvs; *p != IMAGE_TLV_ANY; p++) {
if (type == *p) {
found = true;
break;
}
}
if (!found) {
FIH_SET(fih_rc, FIH_FAILURE);
goto out;
}
}
#endif

if (type == EXPECTED_HASH_TLV) {
/* Verify the image hash. This must always be present. */
if (len != sizeof(hash)) {
Expand Down
20 changes: 20 additions & 0 deletions boot/bootutil/src/tlv.c
Original file line number Diff line number Diff line change
Expand Up @@ -132,3 +132,23 @@ bootutil_tlv_iter_next(struct image_tlv_iter *it, uint32_t *off, uint16_t *len,

return 1;
}

/*
* Return if a TLV entry is in the protected area.
*
* @param it The image TLV iterator struct
* @param off The offset of the entry to check.
*
* @return 0 if this TLV iterator entry is not protected.
* 1 if this TLV iterator entry is in the protected region
* -1 if the iterator is invalid.
*/
int
bootutil_tlv_iter_is_prot(struct image_tlv_iter *it, uint32_t off)
{
if (it == NULL || it->hdr == NULL || it->fap == NULL) {
return -1;
}

return off < it->prot_end;
}
2 changes: 2 additions & 0 deletions docs/release-notes.d/bootutil-check-tlv.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
- Enforce that TLV entries that should be protected are.
This can be disabled by defining `ALLOW_ROGUE_TLVS`
Loading