fix(core/bootloader): fix codec v1 overflow issues
What the developer wrote
Authored by tychovrahe
fix(core/bootloader): fix codec v1 overflow issues
[no changelog]
What changed, and why it matters
This patch fixes several integer-handling bugs in the Trezor bootloader's USB message decoder. The changes prevent small or maliciously crafted message sizes from causing arithmetic overflows or underflows when the device calculates how many USB packets to read. Such flaws could potentially let an attacker confuse the bootloader into reading memory out of bounds or behaving unpredictably during a firmware update.
Treat this as a security-relevant bootloader hardening patch. Verify the patch is included in release builds, and consider whether the same patterns exist in other codec/wire implementations in the repository. No independent exploit confirmation is present, so further review for related integer issues is prudent.
Security signals we found
Integer overflow/underflow hardening in bootloader message parsing
Attacker-controlled msg_size used in chunk-count arithmetic
Addition of bounds checks on packet_size before header-length subtraction
Explicit unsigned casts for multi-byte header field reconstruction
Evidence from the diff
The commit modifies core/embed/projects/bootloader/wire/codec_v1.c. It (1) casts header bytes to the expected unsigned widths before shifting, avoiding sign-extension/overflow when building msg_id and msg_size; (2) rejects packet sizes that are not larger than the message headers, preventing underflow in subsequent size arithmetic; and (3) performs the ‘remaining chunks’ calculation in 64-bit arithmetic before casting, avoiding a 32-bit wrap when msg_size is attacker-controlled. These are defensive fixes for integer overflow/underflow conditions in the bootloader wire protocol parser.
Changed components
Trezor core bootloadercore/embed/projects/bootloader/wire/codec_v1.cUSB/wire protocol v1 parserInspect captured patch +16 / −6
diff --git a/core/embed/projects/bootloader/wire/codec_v1.c b/core/embed/projects/bootloader/wire/codec_v1.c
index 9ed0f90e..74e3b833 100644
--- a/core/embed/projects/bootloader/wire/codec_v1.c
+++ b/core/embed/projects/bootloader/wire/codec_v1.c
@@ -49,8 +49,9 @@ secbool codec_parse_header(const uint8_t *buf, uint16_t *msg_id,
if (buf[0] != '?' || buf[1] != '#' || buf[2] != '#') {
return secfalse;
}
- *msg_id = (buf[3] << 8) + buf[4];
- *msg_size = (buf[5] << 24) + (buf[6] << 16) + (buf[7] << 8) + buf[8];
+ *msg_id = ((uint16_t)buf[3] << 8) | buf[4];
+ *msg_size = ((uint32_t)buf[5] << 24) | ((uint32_t)buf[6] << 16) |
+ ((uint32_t)buf[7] << 8) | buf[8];
return sectrue;
}
@@ -170,6 +171,10 @@ static bool read(pb_istream_t *stream, uint8_t *buf, size_t count) {
size_t packet_size = state->iface->rx_packet_size;
+ if (packet_size <= MSG_HEADER2_LEN) {
+ return false;
+ }
+
size_t read = 0;
// while we have data left
while (read < count) {
@@ -224,12 +229,17 @@ void codec_flush(wire_iface_t *iface, uint32_t msg_size, uint8_t *buf) {
size_t packet_size = iface->rx_packet_size;
+ if (packet_size <= MSG_HEADER1_LEN) {
+ return;
+ }
+
if (msg_size > (packet_size - MSG_HEADER1_LEN)) {
// calculate how many blocks need to be read to drain the message (rounded
- // up to not leave any behind)
- remaining_chunks = (msg_size - (packet_size - MSG_HEADER1_LEN) +
- ((packet_size - MSG_HEADER2_LEN) - 1)) /
- (packet_size - MSG_HEADER2_LEN);
+ // up to not leave any behind); use uint64_t to avoid 32-bit wrap for large
+ // attacker-supplied msg_size values
+ uint64_t num = (uint64_t)msg_size - (packet_size - MSG_HEADER1_LEN) +
+ (packet_size - MSG_HEADER2_LEN - 1);
+ remaining_chunks = (int)(num / (packet_size - MSG_HEADER2_LEN));
}
for (int i = 0; i < remaining_chunks; i++) {
Why this scored 63/100
Community notes
Notes can correct, qualify, or add evidence to the AI analysis. Every note shown here has been validated by a human moderator.
The AI analysis stands alone for now. Submit a note if you can add evidence or important context.