← Watch feed
fix(core): fix OOB read in read_vendor_header
Public commit record
57/100 · Thin
What the developer wrote
Authored by tychovrahe
fix(core): fix OOB read in read_vendor_header
[no changelog]
✓ Descriptive subject✓ Names a concrete action or component✓ Uses a recognizable type or scope! No meaningful explanatory body
What changed, and why it matters
This commit fixes an out-of-bounds memory read bug in the Trezor bootloader's firmware header parser. The read_vendor_header function previously trusted size values inside a firmware file without first checking whether those sizes fit within the actual data buffer. A malformed firmware update or a corrupted on-device firmware image could have caused the bootloader to read memory beyond the allowed region. The patch adds size checks so the parser rejects too-small or oversized headers before using their contents.
Recommended action
Treat this as a security fix and include it in release notes or a security advisory. Users should update bootloader/firmware to a release containing this commit. Developers should audit other parsers that consume external length fields to ensure similar bounds checks are present.
Security signals we found
01
Out-of-bounds read in bootloader firmware-header parser
02
Untrusted length field (hdrlen, vsig_n) used without buffer-size validation
03
Signature offset could underflow when hdrlen < IMAGE_SIG_SIZE
04
Call-site changes propagate a new size parameter, indicating a systemic trust-boundary fix
05
Bootloader and bootloader_ci firmware-update paths are affected
Evidence from the diff
The patch changes read_vendor_header() in core/embed/sec/image/image.c to accept a data_size argument and validates: (1) at least 23 bytes exist before reading fixed fields, (2) declared hdrlen is within VENDOR_HEADER_MAX_SIZE and not smaller than IMAGE_SIG_SIZE, (3) hdrlen fits in the supplied buffer, (4) the vsig_n-derived public-key array plus trailing vstr_len byte fit before the trailing signature region, and (5) in check_firmware_header(), the image header still fits after the vendor header. All call sites are updated to pass the known buffer size. The bug is an OOB read: a crafted hdrlen, vsig_n, or short buffer could make the parser dereference data beyond the mapped firmware region.
Changed components
core/embed/sec/image/image.c - read_vendor_header()core/embed/sec/image/image.c - check_firmware_header()core/embed/projects/bootloader/emulator.ccore/embed/projects/bootloader/fw_check.ccore/embed/projects/bootloader/main.ccore/embed/projects/bootloader/workflow/wf_firmware_update.ccore/embed/projects/bootloader_ci/main.ccore/embed/projects/bootloader_ci/messages.ccore/embed/sec/fwutils/fwutils.ccore/embed/sec/storage/stm32u5/storage_salt.cInspect captured patch +54 / −18
diff --git a/core/embed/projects/bootloader/emulator.c b/core/embed/projects/bootloader/emulator.c
index eca99251..47890be1 100644
--- a/core/embed/projects/bootloader/emulator.c
+++ b/core/embed/projects/bootloader/emulator.c
@@ -75,7 +75,7 @@ bool load_firmware(const char *filename, uint8_t *hash) {
// read vendor and image header
vendor_header vhdr;
- if (sectrue != read_vendor_header(buffer, &vhdr)) {
+ if (sectrue != read_vendor_header(buffer, sizeof(buffer), &vhdr)) {
printf("File '%s' does not contain a valid vendor header.\n", filename);
return false;
}
diff --git a/core/embed/projects/bootloader/fw_check.c b/core/embed/projects/bootloader/fw_check.c
index 34360128..43cc59e7 100644
--- a/core/embed/projects/bootloader/fw_check.c
+++ b/core/embed/projects/bootloader/fw_check.c
@@ -64,8 +64,8 @@ void fw_check(fw_info_t *fw_info) {
volatile secbool version_ok = secfalse;
volatile secbool secmon_valid = secfalse;
- vhdr_present =
- read_vendor_header((const uint8_t *)FIRMWARE_START, &fw_info->vhdr);
+ vhdr_present = read_vendor_header((const uint8_t *)FIRMWARE_START,
+ VENDOR_HEADER_MAX_SIZE, &fw_info->vhdr);
if (sectrue == vhdr_present) {
vhdr_keys_ok = check_vendor_header_keys(&fw_info->vhdr);
diff --git a/core/embed/projects/bootloader/main.c b/core/embed/projects/bootloader/main.c
index 777a1bc3..70d94a61 100644
--- a/core/embed/projects/bootloader/main.c
+++ b/core/embed/projects/bootloader/main.c
@@ -124,7 +124,8 @@ static secbool is_manufacturing_mode(void) {
vendor_header vhdr;
memset(&vhdr, 0, sizeof(vhdr));
- (void)!read_vendor_header((const uint8_t *)FIRMWARE_START, &vhdr);
+ (void)!read_vendor_header((const uint8_t *)FIRMWARE_START,
+ VENDOR_HEADER_MAX_SIZE, &vhdr);
if ((vhdr.vtrust & VTRUST_ALLOW_PROVISIONING) != VTRUST_ALLOW_PROVISIONING) {
return secfalse;
@@ -419,7 +420,8 @@ void real_jump_to_firmware(void) {
const image_header *hdr = NULL;
vendor_header vhdr = {0};
- ensure(read_vendor_header((const uint8_t *)FIRMWARE_START, &vhdr),
+ ensure(read_vendor_header((const uint8_t *)FIRMWARE_START,
+ VENDOR_HEADER_MAX_SIZE, &vhdr),
"Firmware is corrupted");
ensure(check_vendor_header_keys(&vhdr), "Firmware is corrupted");
diff --git a/core/embed/projects/bootloader/workflow/wf_firmware_update.c b/core/embed/projects/bootloader/workflow/wf_firmware_update.c
index 154e79da..0e1086be 100644
--- a/core/embed/projects/bootloader/workflow/wf_firmware_update.c
+++ b/core/embed/projects/bootloader/workflow/wf_firmware_update.c
@@ -211,7 +211,8 @@ static upload_status_t process_msg_FirmwareUpload(protob_io_t *iface,
// first block and headers are not yet parsed
vendor_header vhdr;
- if (sectrue != read_vendor_header((uint8_t *)chunk_buffer, &vhdr)) {
+ if (sectrue != read_vendor_header((uint8_t *)chunk_buffer,
+ IMAGE_CHUNK_SIZE, &vhdr)) {
send_msg_failure(iface, FailureType_Failure_ProcessError,
"Invalid vendor header");
return UPLOAD_ERR_INVALID_VENDOR_HEADER;
@@ -308,8 +309,9 @@ static upload_status_t process_msg_FirmwareUpload(protob_io_t *iface,
secbool is_new = secfalse;
- if (sectrue !=
- read_vendor_header((const uint8_t *)FIRMWARE_START, ¤t_vhdr)) {
+ if (sectrue != read_vendor_header((const uint8_t *)FIRMWARE_START,
+ VENDOR_HEADER_MAX_SIZE,
+ ¤t_vhdr)) {
is_new = sectrue;
}
diff --git a/core/embed/projects/bootloader_ci/main.c b/core/embed/projects/bootloader_ci/main.c
index 0f2c222d..5e39a9c5 100644
--- a/core/embed/projects/bootloader_ci/main.c
+++ b/core/embed/projects/bootloader_ci/main.c
@@ -200,7 +200,8 @@ int main(void) {
// detect whether the device contains a valid firmware
secbool firmware_present = sectrue;
- if (sectrue != read_vendor_header((const uint8_t *)FIRMWARE_START, &vhdr)) {
+ if (sectrue != read_vendor_header((const uint8_t *)FIRMWARE_START,
+ VENDOR_HEADER_MAX_SIZE, &vhdr)) {
firmware_present = secfalse;
}
@@ -243,7 +244,8 @@ int main(void) {
return 1;
}
- ensure(read_vendor_header((const uint8_t *)FIRMWARE_START, &vhdr),
+ ensure(read_vendor_header((const uint8_t *)FIRMWARE_START,
+ VENDOR_HEADER_MAX_SIZE, &vhdr),
"invalid vendor header");
ensure(check_vendor_header_keys(&vhdr), "invalid vendor header signature");
diff --git a/core/embed/projects/bootloader_ci/messages.c b/core/embed/projects/bootloader_ci/messages.c
index 4d81b8ac..b42899fe 100644
--- a/core/embed/projects/bootloader_ci/messages.c
+++ b/core/embed/projects/bootloader_ci/messages.c
@@ -484,7 +484,8 @@ int process_msg_FirmwareUpload(uint8_t iface_num, uint32_t msg_size,
// first block and headers are not yet parsed
vendor_header vhdr;
- if (sectrue != read_vendor_header((uint8_t *)chunk_buffer, &vhdr)) {
+ if (sectrue != read_vendor_header((uint8_t *)chunk_buffer,
+ IMAGE_CHUNK_SIZE, &vhdr)) {
MSG_SEND_INIT(Failure);
MSG_SEND_ASSIGN_VALUE(code, FailureType_Failure_ProcessError);
MSG_SEND_ASSIGN_STRING(message, "Invalid vendor header");
@@ -536,8 +537,9 @@ int process_msg_FirmwareUpload(uint8_t iface_num, uint32_t msg_size,
secbool is_new = secfalse;
- if (sectrue !=
- read_vendor_header((const uint8_t *)FIRMWARE_START, ¤t_vhdr)) {
+ if (sectrue != read_vendor_header((const uint8_t *)FIRMWARE_START,
+ VENDOR_HEADER_MAX_SIZE,
+ ¤t_vhdr)) {
is_new = sectrue;
}
diff --git a/core/embed/sec/fwutils/fwutils.c b/core/embed/sec/fwutils/fwutils.c
index 71b646b7..e883f60a 100644
--- a/core/embed/sec/fwutils/fwutils.c
+++ b/core/embed/sec/fwutils/fwutils.c
@@ -111,7 +111,8 @@ secbool firmware_get_vendor(char* buff, size_t buff_size) {
memset(buff, 0, buff_size);
- if (data == NULL || sectrue != read_vendor_header(data, &vhdr)) {
+ if (data == NULL ||
+ sectrue != read_vendor_header(data, VENDOR_HEADER_MAX_SIZE, &vhdr)) {
return secfalse;
}
diff --git a/core/embed/sec/image/image.c b/core/embed/sec/image/image.c
index e40a29b0..a5159dd0 100644
--- a/core/embed/sec/image/image.c
+++ b/core/embed/sec/image/image.c
@@ -263,14 +263,25 @@ secbool check_secmon_contents(const secmon_header_t *const hdr,
#endif // USE_SECMON_VERIFICATION
-secbool __wur read_vendor_header(const uint8_t *const data,
+secbool __wur read_vendor_header(const uint8_t *const data, size_t data_size,
vendor_header *const vhdr) {
+ // Need at least 23 bytes to safely read all fixed-offset fields through
+ // fw_type at offset 22.
+ if (data_size < 23) return secfalse;
+
memcpy(&vhdr->magic, data, 4);
if (vhdr->magic != 0x565A5254) return secfalse; // TRZV
memcpy(&vhdr->hdrlen, data + 4, 4);
if (vhdr->hdrlen > VENDOR_HEADER_MAX_SIZE) return secfalse;
+ // hdrlen must be large enough to hold the IMAGE_SIG_SIZE-byte signature at
+ // its tail; otherwise the offset data + hdrlen - IMAGE_SIG_SIZE underflows.
+ if (vhdr->hdrlen < IMAGE_SIG_SIZE) return secfalse;
+
+ // The full declared header must fit within the provided buffer.
+ if (data_size < vhdr->hdrlen) return secfalse;
+
memcpy(&vhdr->expiry, data + 8, 4);
if (vhdr->expiry != 0) return secfalse;
@@ -288,6 +299,13 @@ secbool __wur read_vendor_header(const uint8_t *const data,
return secfalse;
}
+ // The public-key array and the vstr_len byte that follows it must all fit
+ // within the header body (the region before the trailing signature).
+ uint32_t vstr_len_offset = 32 + (uint32_t)vhdr->vsig_n * 32;
+ if (vstr_len_offset >= vhdr->hdrlen - IMAGE_SIG_SIZE) {
+ return secfalse;
+ }
+
for (int i = 0; i < vhdr->vsig_n; i++) {
vhdr->vpub[i] = data + 32 + i * 32;
}
@@ -466,13 +484,20 @@ secbool check_firmware_header(const uint8_t *header, size_t header_size,
firmware_header_info_t *info) {
// parse and check vendor header
vendor_header vhdr;
- if (sectrue != read_vendor_header(header, &vhdr)) {
+ if (sectrue != read_vendor_header(header, header_size, &vhdr)) {
return secfalse;
}
if (sectrue != check_vendor_header_keys(&vhdr)) {
return secfalse;
}
+ // Ensure the image header fits within the provided buffer after the vendor
+ // header.
+ if (header_size < vhdr.hdrlen ||
+ header_size - vhdr.hdrlen < IMAGE_HEADER_SIZE) {
+ return secfalse;
+ }
+
// parse and check image header
const image_header *ihdr;
if ((ihdr = read_image_header(header + vhdr.hdrlen, FIRMWARE_IMAGE_MAGIC,
diff --git a/core/embed/sec/image/inc/sec/image.h b/core/embed/sec/image/inc/sec/image.h
index d87b533a..88874c1a 100644
--- a/core/embed/sec/image/inc/sec/image.h
+++ b/core/embed/sec/image/inc/sec/image.h
@@ -163,7 +163,7 @@ secbool __wur check_image_header_sig(const image_header *const hdr,
uint8_t key_m, uint8_t key_n,
const uint8_t *const *keys);
-secbool __wur read_vendor_header(const uint8_t *const data,
+secbool __wur read_vendor_header(const uint8_t *const data, size_t data_Size,
vendor_header *const vhdr);
secbool __wur check_vendor_header_model(const vendor_header *const vhdr);
diff --git a/core/embed/sec/storage/stm32u5/storage_salt.c b/core/embed/sec/storage/stm32u5/storage_salt.c
index 7e23e73b..ae879179 100644
--- a/core/embed/sec/storage/stm32u5/storage_salt.c
+++ b/core/embed/sec/storage/stm32u5/storage_salt.c
@@ -37,7 +37,9 @@ void storage_salt_get(storage_salt_t* salt) {
memset(salt, 0, sizeof(*salt));
vendor_header vhdr = {0};
- ensure(read_vendor_header((const uint8_t*)FIRMWARE_START, &vhdr), NULL);
+ ensure(read_vendor_header((const uint8_t*)FIRMWARE_START,
+ VENDOR_HEADER_MAX_SIZE, &vhdr),
+ NULL);
_Static_assert(SECRET_KEY_STORAGE_SALT_SIZE <= sizeof(salt->bytes));
secbool retval = secret_key_storage_salt(vhdr.fw_type, salt->bytes);
Risk score
Our methodology →Why this scored 74/100
Human-validated context
Community notes
Notes can correct, qualify, or add evidence to the AI analysis. Every note shown here has been validated by a human moderator.
No validated notes yet.
The AI analysis stands alone for now. Submit a note if you can add evidence or important context.