fix(rust/trezor-thp): channel desync after incorrect ACK
What the developer wrote
Authored by Martin Milata
fix(rust/trezor-thp): channel desync after incorrect ACK
What changed, and why it matters
This commit fixes a bug in the Trezor hardware wallet's custom transport protocol (THP). Previously, when the device received an incorrect or out-of-order acknowledgment (ACK), it would not advance its internal send state but would still log a generic warning. The fix makes the protocol explicitly reject bad ACK bits and only mark a message as delivered when the ACK bit matches what the device expects. Without the fix, a malformed or replayed ACK could leave the sending side stuck or desynchronized, potentially disrupting communication between the wallet and a host computer.
Treat as a hardening fix. Review the THP state machine for any remaining edge cases where a malformed, replayed, or reordered ACK/fragment can desynchronize the channel. If this fix corresponds to a known issue or external report, request the vendor to publish a security advisory.
Security signals we found
Protocol state-machine change around ACK handling
Explicit rejection of unexpected ACK bits
Desynchronization / channel sync issue mentioned in commit title
Transport layer (THP) change in hardware wallet firmware
Evidence from the diff
In rust/trezor-thp, send_mark_delivered() previously returned nothing and only incremented the send sync bit when the ACK bit matched. Callers then checked can_send() to decide whether to transition SendState::Idle. Because can_send() was not set to true when the ACK bit was wrong, the state machine already avoided advancing on a bad ACK. However, the new code makes the mismatch explicit: send_mark_delivered() now returns a bool, callers branch on that bool, and an unexpected ACK bit is logged separately from a fully unexpected ACK. The change is defensive and clarifies the state machine, but the diff alone does not show an exploitable vulnerability; it hardens the protocol against desynchronization.
Changed components
rust/trezor-thp/src/alternating_bit.rsrust/trezor-thp/src/channel/mod.rsTrezor THP (Trezor Host Protocol) transport layerInspect captured patch +8 / −6
diff --git a/rust/trezor-thp/src/alternating_bit.rs b/rust/trezor-thp/src/alternating_bit.rs
index 6a678e65..299a3c3e 100644
--- a/rust/trezor-thp/src/alternating_bit.rs
+++ b/rust/trezor-thp/src/alternating_bit.rs
@@ -112,12 +112,14 @@ impl ChannelSync {
self.can_send = false;
}
- /// Call after receiving an ACK message.
- pub fn send_mark_delivered(&mut self, sb: SyncBits) {
+ /// Call after receiving an ACK message. Return true if its ACK bit was correct.
+ pub fn send_mark_delivered(&mut self, sb: SyncBits) -> bool {
if self.sync_send == sb.ack_bit() {
self.sync_send.increment();
self.can_send = true;
+ return true;
}
+ false
}
/// Call after receving initial fragment of a message.
diff --git a/rust/trezor-thp/src/channel/mod.rs b/rust/trezor-thp/src/channel/mod.rs
index a730f045..1930d585 100644
--- a/rust/trezor-thp/src/channel/mod.rs
+++ b/rust/trezor-thp/src/channel/mod.rs
@@ -353,10 +353,11 @@ impl<R: Role, B: Backend> Channel<R, B> {
// Verify checksum.
let _ = Reassembler::<R>::single(packet_buffer)?;
let sb = SyncBits::try_from(packet_buffer)?;
- self.sync.send_mark_delivered(sb);
- if self.sync.can_send() {
+ if self.sync.send_mark_delivered(sb) {
self.send_state = SendState::Idle;
return Ok(());
+ } else {
+ log::warn!("[{:04x}] Unexpected ACK bit.", self.channel_id);
}
}
log::warn!("[{:04x}] Unexpected ACK.", self.channel_id);
@@ -447,8 +448,7 @@ impl<R: Role, B: Backend> Channel<R, B> {
&& self.sync.is_ack_piggybacking_allowed()
&& !self.sync.can_send()
{
- self.sync.send_mark_delivered(reassembler.sync_bits());
- if self.sync.can_send() {
+ if self.sync.send_mark_delivered(reassembler.sync_bits()) {
ack_received = true;
self.send_state = SendState::Idle;
} else {
Why this scored 58/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.