fix(rust/trezor-thp): packet length edge cases
What the developer wrote
Authored by Martin Milata
fix(rust/trezor-thp): packet length edge cases
[no changelog]
What changed, and why it matters
This commit fixes how a Trezor hardware wallet's Rust code handles incoming USB-like packets. Previously, the code resized its receive buffer based on a size value returned after a packet was already accepted, which could lead to incorrect buffer sizing or missed edge cases around packet length. The new code parses the packet's control byte and channel length up front, and only resizes the buffer if the incoming data needs more space. The change is defensive and appears to prevent memory or protocol-handling bugs, but the commit message gives no explicit security claim and no external advisory is supplied.
Treat as a hardening/fix commit worth reviewing in context. Verify whether the prior buffer-resize-after-accept behavior could cause out-of-bounds writes, denial of service via malformed packet length, or protocol desynchronization. Review related THP packet_in implementations and fuzz the length parsing paths. No immediate CVE assignment is warranted from this diff alone without a demonstrated exploit or vendor security statement.
Security signals we found
Buffer sizing logic changed from post-accept resize to pre-accept size check
Packet length parsed from control byte/header before buffer use
Receive buffer now resized only when incoming length exceeds current capacity
Test updated to explicitly configure packet length on both endpoints
No changelog entry and no explicit security wording in commit message
Evidence from the diff
The patch modifies rust/trezor-thp/src/channel/buffered.rs. The old implementation called channel.packet_in() first, then, if the result was Accepted with buffer_size: Some(s), resized self.receive_buffer to s and returned a modified Accepted with buffer_size: None. The new implementation parses the packet header before calling packet_in(): it uses ControlByte::parse and parse_channel_length to extract the channel length, and only resizes the receive buffer if len exceeds the current buffer length. Then it passes the (possibly enlarged) buffer to packet_in(). A test file change sets packet lengths on both host and device sides and swaps argument order in take_turns. The change suggests the prior logic could accept a packet before ensuring the buffer was large enough, or could resize based on stale/optional size info. The fix is more robust but the diff alone does not prove an exploitable vulnerability exists.
Changed components
rust/trezor-thp/src/channel/buffered.rsrust/trezor-thp/src/channel/test.rsTrezor THP (Trezor Host Protocol) channel buffering layerInspect captured patch +13 / −21
diff --git a/rust/trezor-thp/src/channel/buffered.rs b/rust/trezor-thp/src/channel/buffered.rs
index 9ed591b3..48541b7f 100644
--- a/rust/trezor-thp/src/channel/buffered.rs
+++ b/rust/trezor-thp/src/channel/buffered.rs
@@ -1,7 +1,9 @@
use crate::{
ChannelIO,
channel::{APP_HEADER_LEN, PacketInResult, SEND_BUFFER_OVERHEAD},
+ control_byte::ControlByte,
error::Result,
+ header::parse_channel_length,
};
use std::ops::{Deref, DerefMut};
@@ -38,28 +40,16 @@ impl<C: ChannelIO> Buffered<C> {
}
pub fn packet_in(&mut self, packet_buffer: &[u8]) -> PacketInResult {
- let res = self
- .channel
- .packet_in(packet_buffer, self.receive_buffer.as_mut_slice());
- if let PacketInResult::Accepted {
- ack_received,
- message_ready,
- pong,
- buffer_size: Some(s),
- ..
- } = res
+ if let Ok((_, Some(len))) = ControlByte::parse(packet_buffer)
+ .and_then(|(cb, _)| parse_channel_length(cb, packet_buffer))
{
- let new_size: u16 = s.into();
- log::debug!("Resizing receive buffer to {}.", new_size);
- self.receive_buffer.resize(new_size.into(), 0u8);
- return PacketInResult::Accepted {
- ack_received,
- message_ready,
- pong,
- buffer_size: None,
- };
+ if usize::from(len) > self.receive_buffer.len() {
+ log::debug!("Resizing receive buffer to {}.", len);
+ self.receive_buffer.resize(len.into(), 0u8);
+ }
}
- res
+ self.channel
+ .packet_in(packet_buffer, self.receive_buffer.as_mut_slice())
}
pub fn packet_out(&mut self) -> Result<Vec<u8>> {
diff --git a/rust/trezor-thp/src/channel/test.rs b/rust/trezor-thp/src/channel/test.rs
index 7820f631..f5e87928 100644
--- a/rust/trezor-thp/src/channel/test.rs
+++ b/rust/trezor-thp/src/channel/test.rs
@@ -584,8 +584,10 @@ fn open_channel(
.channel_alloc(cids.get(), TestCredentialVerifier)?
.with_key(DEVICE_KEY)
.into_buffered();
- take_turns(&mut d, &mut hm)?;
+ d.set_packet_len(packet_len);
+ take_turns(&mut hm, &mut d)?;
let mut h = hm.channel_alloc(NullCredentialStore)?.into_buffered();
+ h.set_packet_len(packet_len);
if enable_piggybacking {
h.set_device_protocol_version(2, 1);
}
Why this scored 46/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.