From 35d40cc164bf398c3593bf69fdce60c78bf45ab4 Mon Sep 17 00:00:00 2001 From: Pavol Rusnak Date: Wed, 5 May 2021 17:24:30 +0200 Subject: [PATCH] fix(core): change logic of vendor header comparison Previously we checked whether the current vendor header and the new vendor header are the same by comparing the embedded keyset. What originally looked like a good idea is not that good, because this disallows us from ever changing the vendor header signing keys without causing erasure of the storage during the version update. This commit fixes that by changing the logic to comparing just the vendor string. Change of function names is purely cosmetic: * vendor_keys_hash -> vendor_header_hash * check_vendor_keys_lock -> check_vendor_header_lock --- core/embed/bootloader/.changelog.d/1599.changed | 1 + core/embed/bootloader/main.c | 10 +++++----- core/embed/bootloader/messages.c | 6 +++--- .../bootloader_ci/.changelog.d/1599.changed | 1 + core/embed/bootloader_ci/main.c | 10 +++++----- core/embed/bootloader_ci/messages.c | 6 +++--- core/embed/trezorhal/flash.h | 2 +- core/embed/trezorhal/image.c | 17 +++-------------- core/embed/trezorhal/image.h | 2 +- 9 files changed, 23 insertions(+), 32 deletions(-) create mode 100644 core/embed/bootloader/.changelog.d/1599.changed create mode 100644 core/embed/bootloader_ci/.changelog.d/1599.changed diff --git a/core/embed/bootloader/.changelog.d/1599.changed b/core/embed/bootloader/.changelog.d/1599.changed new file mode 100644 index 0000000000..2a8ff94f76 --- /dev/null +++ b/core/embed/bootloader/.changelog.d/1599.changed @@ -0,0 +1 @@ +Update logic of vendor header comparison. diff --git a/core/embed/bootloader/main.c b/core/embed/bootloader/main.c index f9522893cc..129a3a5d53 100644 --- a/core/embed/bootloader/main.c +++ b/core/embed/bootloader/main.c @@ -191,9 +191,9 @@ secbool load_vendor_header_keys(const uint8_t *const data, BOOTLOADER_KEYS, vhdr); } -static secbool check_vendor_keys_lock(const vendor_header *const vhdr) { +static secbool check_vendor_header_lock(const vendor_header *const vhdr) { uint8_t lock[FLASH_OTP_BLOCK_SIZE]; - ensure(flash_otp_read(FLASH_OTP_BLOCK_VENDOR_KEYS_LOCK, 0, lock, + ensure(flash_otp_read(FLASH_OTP_BLOCK_VENDOR_HEADER_LOCK, 0, lock, FLASH_OTP_BLOCK_SIZE), NULL); if (0 == @@ -204,7 +204,7 @@ static secbool check_vendor_keys_lock(const vendor_header *const vhdr) { return sectrue; } uint8_t hash[32]; - vendor_keys_hash(vhdr, hash); + vendor_header_hash(vhdr, hash); return sectrue * (0 == memcmp(lock, hash, 32)); } @@ -269,7 +269,7 @@ int main(void) { secbool firmware_present = load_vendor_header_keys((const uint8_t *)FIRMWARE_START, &vhdr); if (sectrue == firmware_present) { - firmware_present = check_vendor_keys_lock(&vhdr); + firmware_present = check_vendor_header_lock(&vhdr); } if (sectrue == firmware_present) { firmware_present = load_image_header( @@ -328,7 +328,7 @@ int main(void) { ensure(load_vendor_header_keys((const uint8_t *)FIRMWARE_START, &vhdr), "invalid vendor header"); - ensure(check_vendor_keys_lock(&vhdr), "unauthorized vendor keys"); + ensure(check_vendor_header_lock(&vhdr), "unauthorized vendor keys"); ensure(load_image_header((const uint8_t *)(FIRMWARE_START + vhdr.hdrlen), FIRMWARE_IMAGE_MAGIC, FIRMWARE_IMAGE_MAXSIZE, diff --git a/core/embed/bootloader/messages.c b/core/embed/bootloader/messages.c index 97dd267c77..0948db419b 100644 --- a/core/embed/bootloader/messages.c +++ b/core/embed/bootloader/messages.c @@ -295,7 +295,7 @@ static void send_msg_features(uint8_t iface_num, MSG_SEND_ASSIGN_VALUE(fw_patch, ((hdr->version >> 16) & 0xFF)); MSG_SEND_ASSIGN_STRING_LEN(fw_vendor, vhdr->vstr, vhdr->vstr_len); uint8_t hash[32]; - vendor_keys_hash(vhdr, hash); + vendor_header_hash(vhdr, hash); MSG_SEND_ASSIGN_BYTES(fw_vendor_keys, hash, 32); } else { MSG_SEND_ASSIGN_VALUE(firmware_present, false); @@ -446,8 +446,8 @@ static void detect_installation(vendor_header *current_vhdr, return; } uint8_t hash1[32], hash2[32]; - vendor_keys_hash(new_vhdr, hash1); - vendor_keys_hash(current_vhdr, hash2); + vendor_header_hash(new_vhdr, hash1); + vendor_header_hash(current_vhdr, hash2); if (0 != memcmp(hash1, hash2, 32)) { return; } diff --git a/core/embed/bootloader_ci/.changelog.d/1599.changed b/core/embed/bootloader_ci/.changelog.d/1599.changed new file mode 100644 index 0000000000..2a8ff94f76 --- /dev/null +++ b/core/embed/bootloader_ci/.changelog.d/1599.changed @@ -0,0 +1 @@ +Update logic of vendor header comparison. diff --git a/core/embed/bootloader_ci/main.c b/core/embed/bootloader_ci/main.c index b6ddfcef74..4f10468f4e 100644 --- a/core/embed/bootloader_ci/main.c +++ b/core/embed/bootloader_ci/main.c @@ -169,9 +169,9 @@ secbool load_vendor_header_keys(const uint8_t *const data, BOOTLOADER_KEYS, vhdr); } -static secbool check_vendor_keys_lock(const vendor_header *const vhdr) { +static secbool check_vendor_header_lock(const vendor_header *const vhdr) { uint8_t lock[FLASH_OTP_BLOCK_SIZE]; - ensure(flash_otp_read(FLASH_OTP_BLOCK_VENDOR_KEYS_LOCK, 0, lock, + ensure(flash_otp_read(FLASH_OTP_BLOCK_VENDOR_HEADER_LOCK, 0, lock, FLASH_OTP_BLOCK_SIZE), NULL); if (0 == @@ -182,7 +182,7 @@ static secbool check_vendor_keys_lock(const vendor_header *const vhdr) { return sectrue; } uint8_t hash[32]; - vendor_keys_hash(vhdr, hash); + vendor_header_hash(vhdr, hash); return sectrue * (0 == memcmp(lock, hash, 32)); } @@ -235,7 +235,7 @@ int main(void) { secbool firmware_present = load_vendor_header_keys((const uint8_t *)FIRMWARE_START, &vhdr); if (sectrue == firmware_present) { - firmware_present = check_vendor_keys_lock(&vhdr); + firmware_present = check_vendor_header_lock(&vhdr); } if (sectrue == firmware_present) { firmware_present = load_image_header( @@ -263,7 +263,7 @@ int main(void) { ensure(load_vendor_header_keys((const uint8_t *)FIRMWARE_START, &vhdr), "invalid vendor header"); - ensure(check_vendor_keys_lock(&vhdr), "unauthorized vendor keys"); + ensure(check_vendor_header_lock(&vhdr), "unauthorized vendor keys"); ensure(load_image_header((const uint8_t *)(FIRMWARE_START + vhdr.hdrlen), FIRMWARE_IMAGE_MAGIC, FIRMWARE_IMAGE_MAXSIZE, diff --git a/core/embed/bootloader_ci/messages.c b/core/embed/bootloader_ci/messages.c index 82083cfcf6..274f42e7d0 100644 --- a/core/embed/bootloader_ci/messages.c +++ b/core/embed/bootloader_ci/messages.c @@ -295,7 +295,7 @@ static void send_msg_features(uint8_t iface_num, MSG_SEND_ASSIGN_VALUE(fw_patch, ((hdr->version >> 16) & 0xFF)); MSG_SEND_ASSIGN_STRING_LEN(fw_vendor, vhdr->vstr, vhdr->vstr_len); uint8_t hash[32]; - vendor_keys_hash(vhdr, hash); + vendor_header_hash(vhdr, hash); MSG_SEND_ASSIGN_BYTES(fw_vendor_keys, hash, 32); } else { MSG_SEND_ASSIGN_VALUE(firmware_present, false); @@ -446,8 +446,8 @@ static void detect_installation(vendor_header *current_vhdr, return; } uint8_t hash1[32], hash2[32]; - vendor_keys_hash(new_vhdr, hash1); - vendor_keys_hash(current_vhdr, hash2); + vendor_header_hash(new_vhdr, hash1); + vendor_header_hash(current_vhdr, hash2); if (0 != memcmp(hash1, hash2, 32)) { return; } diff --git a/core/embed/trezorhal/flash.h b/core/embed/trezorhal/flash.h index 8bd0bc2c98..50e5f6210f 100644 --- a/core/embed/trezorhal/flash.h +++ b/core/embed/trezorhal/flash.h @@ -110,7 +110,7 @@ secbool __wur flash_write_word(uint8_t sector, uint32_t offset, uint32_t data); // OTP blocks allocation #define FLASH_OTP_BLOCK_BATCH 0 #define FLASH_OTP_BLOCK_BOOTLOADER_VERSION 1 -#define FLASH_OTP_BLOCK_VENDOR_KEYS_LOCK 2 +#define FLASH_OTP_BLOCK_VENDOR_HEADER_LOCK 2 #define FLASH_OTP_BLOCK_RANDOMNESS 3 secbool __wur flash_otp_read(uint8_t block, uint8_t offset, uint8_t *data, diff --git a/core/embed/trezorhal/image.c b/core/embed/trezorhal/image.c index cf12f02a23..4ee74aedd8 100644 --- a/core/embed/trezorhal/image.c +++ b/core/embed/trezorhal/image.c @@ -161,22 +161,11 @@ secbool load_vendor_header(const uint8_t *const data, uint8_t key_m, *(const ed25519_signature *)vhdr->sig)); } -void vendor_keys_hash(const vendor_header *const vhdr, uint8_t *hash) { +void vendor_header_hash(const vendor_header *const vhdr, uint8_t *hash) { BLAKE2S_CTX ctx; blake2s_Init(&ctx, BLAKE2S_DIGEST_LENGTH); - blake2s_Update(&ctx, &(vhdr->vsig_m), sizeof(vhdr->vsig_m)); - blake2s_Update(&ctx, &(vhdr->vsig_n), sizeof(vhdr->vsig_n)); - for (int i = 0; i < MAX_VENDOR_PUBLIC_KEYS; i++) { - if (vhdr->vpub[i] != 0) { - blake2s_Update(&ctx, vhdr->vpub[i], 32); - } else { - blake2s_Update( - &ctx, - "\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00" - "\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00", - 32); - } - } + blake2s_Update(&ctx, vhdr->vstr, vhdr->vstr_len); + blake2s_Update(&ctx, "Trezor Vendor Header", 20); blake2s_Final(&ctx, hash, BLAKE2S_DIGEST_LENGTH); } diff --git a/core/embed/trezorhal/image.h b/core/embed/trezorhal/image.h index a92d365671..95d079980a 100644 --- a/core/embed/trezorhal/image.h +++ b/core/embed/trezorhal/image.h @@ -87,7 +87,7 @@ secbool __wur load_vendor_header(const uint8_t *const data, uint8_t key_m, uint8_t key_n, const uint8_t *const *keys, vendor_header *const vhdr); -void vendor_keys_hash(const vendor_header *const vhdr, uint8_t *hash); +void vendor_header_hash(const vendor_header *const vhdr, uint8_t *hash); secbool __wur check_single_hash(const uint8_t *const hash, const uint8_t *const data, int len);