feat(core/thp): support receive-side THP ACK piggybacking
What the developer wrote
Authored by Roman Zeyde
feat(core/thp): support receive-side THP ACK piggybacking
The host will detect device support via the bumped minor version
(from `2.0` to `2.1`) - will be done in a subsequent commit, since
it affects the pairing code entropy - resulting in UI fixture diffs.
The device will detect host support via the
ACK bit of `HANDSHAKE_INIT_REQ` message (previously set to 0).
Due to event loop restarts, the last message sent to the host should
be ACKed using a non-piggybacked THP ACK message.
Otherwise, the piggybacking message will be dropped and the host will
need to re-transmit it.
What changed, and why it matters
This commit adds a new optional efficiency feature to Trezor's THP (Trezor Host Protocol) that lets acknowledgements (ACKs) be attached to normal messages instead of sent separately. It is a feature implementation, not a fix for a known vulnerability. The change includes a compatibility handshake so older hosts fall back to non-piggybacked ACKs. There is no direct evidence in the commit that this introduces a security bug, but any protocol change carries some risk of subtle state-machine issues.
Treat as a normal feature commit. Reviewers should verify that the new ACK-piggybacking state machine correctly handles retransmissions, sequence-bit toggling, and the event-loop-restart edge case described in the commit message. No immediate security response is indicated by the supplied materials.
Security signals we found
Protocol state-machine change in alternating-bit reliability layer
New handshake negotiation bit derived from host control byte
Addition of `EMPTY_ACK_PAYLOAD` return path and reassembler state preservation
Conditional ACK handling branch added to `recv_payload()` loop
No security-relevant keywords (fix, CVE, vulnerability, etc.) in commit message or changelog
Evidence from the diff
The patch implements receive-side ACK piggybacking in the THP alternating-bit protocol layer. A new ack_piggybacking bit in the channel sync byte is negotiated: the host signals support via the ACK bit of HANDSHAKE_INIT_REQ, and the device enables piggybacking when it sees that bit. The device then embeds the last-received sequence bit into outgoing control bytes and processes piggybacked ACKs in recv_payload(). A special case is added so the final message before an event-loop restart is sent as a standalone ACK, avoiding a dropped piggybacked message. The commit is purely additive and does not patch an existing CVE or acknowledged flaw.
Changed components
core/src/trezor/wire/thp/alternating_bit_protocol.pycore/src/trezor/wire/thp/channel.pycore/src/trezor/wire/thp/received_message_handler.pycore/src/storage/cache_thp.pyInspect captured patch +58 / −8
diff --git a/core/.changelog.d/6202.added b/core/.changelog.d/6202.added
new file mode 100644
index 00000000..c50964dc
--- /dev/null
+++ b/core/.changelog.d/6202.added
@@ -0,0 +1 @@
+Support receive-side THP ACK piggybacking.
diff --git a/core/src/storage/cache_thp.py b/core/src/storage/cache_thp.py
index 5fdeee8a..256db94f 100644
--- a/core/src/storage/cache_thp.py
+++ b/core/src/storage/cache_thp.py
@@ -70,7 +70,7 @@ class ChannelCache(ThpDataCache):
@property
def sync(self) -> int:
- # can_send_bit | sync_receive_bit | sync_send_bit | rfu(5)
+ # can_send_bit | sync_receive_bit | sync_send_bit | ack_piggybacking | rfu(4)
return self.get_int(CHANNEL_SYNC) or 0x00
@sync.setter
diff --git a/core/src/trezor/wire/thp/alternating_bit_protocol.py b/core/src/trezor/wire/thp/alternating_bit_protocol.py
index 6bfa2013..98ebe71f 100644
--- a/core/src/trezor/wire/thp/alternating_bit_protocol.py
+++ b/core/src/trezor/wire/thp/alternating_bit_protocol.py
@@ -40,6 +40,13 @@ def is_sending_allowed(cache: ChannelCache) -> bool:
return bool(cache.sync & 0x80)
+def get_send_ack_bit(cache: ChannelCache) -> int:
+ """
+ Returns the sequential number (bit) of the last message successfully received on this channel.
+ """
+ return 1 - get_expected_receive_seq_bit(cache)
+
+
def get_send_seq_bit(cache: ChannelCache) -> int:
"""
Returns the sequential number (bit) of the next message to be sent
@@ -92,3 +99,11 @@ def set_send_seq_bit_to_opposite(cache: ChannelCache) -> None:
i.e. 1 -> 0 and 0 -> 1
"""
_set_send_seq_bit(cache=cache, seq_bit=1 - get_send_seq_bit(cache))
+
+
+def is_ack_piggybacking_allowed(cache: ChannelCache) -> bool:
+ return bool(cache.sync & 0x10)
+
+
+def allow_ack_piggybacking(cache: ChannelCache) -> None:
+ cache.sync |= 0x10
diff --git a/core/src/trezor/wire/thp/channel.py b/core/src/trezor/wire/thp/channel.py
index f74e7810..3ba24ccd 100644
--- a/core/src/trezor/wire/thp/channel.py
+++ b/core/src/trezor/wire/thp/channel.py
@@ -64,16 +64,18 @@ _WRITE_TIMEOUT = sleep(_WRITE_TIMEOUT_MS)
# It allows interrupting a "stuck" THP workflow using a different channel on the same interface.
_PREEMPT_TIMEOUT_MS = const(1_000)
+EMPTY_ACK_PAYLOAD = memoryview(b"")
+
class Reassembler:
def __init__(self, read_buf: ThpBuffer) -> None:
self.thp_read_buf = read_buf
self.reset()
- def reset(self) -> None:
+ def reset(self, message: memoryview | None = None) -> None:
self.bytes_read: int = 0
self.buffer_len: int = 0
- self.message: memoryview | None = None
+ self.message = message
def handle_packet(self, packet: memoryview) -> bool:
"""
@@ -242,6 +244,10 @@ class Channel:
"""
return_after_ack = expected_ctrl_byte is None
+ is_ack_piggybacking_allowed = ABP.is_ack_piggybacking_allowed(
+ self.channel_cache
+ )
+
while True:
# Handle an existing message (if already reassembled).
# Otherwise, receive and reassemble a new one.
@@ -256,9 +262,17 @@ class Channel:
if control_byte.is_ack(ctrl_byte):
handle_ack(self, control_byte.get_ack_bit(ctrl_byte))
if return_after_ack:
- return payload
+ assert not payload
+ return EMPTY_ACK_PAYLOAD
continue
+ if is_ack_piggybacking_allowed:
+ handle_ack(self, control_byte.get_ack_bit(ctrl_byte))
+ if return_after_ack and ABP.is_sending_allowed(self.channel_cache):
+ # A valid ACK has been received - keep the payload for the next `recv_payload()` call
+ self.reassembler.reset(msg)
+ return EMPTY_ACK_PAYLOAD
+
if return_after_ack or not expected_ctrl_byte(ctrl_byte):
if __debug__:
self._log(
@@ -415,6 +429,11 @@ class Channel:
payload_len = len(payload) + CHECKSUM_LENGTH
sync_bit = ABP.get_send_seq_bit(self.channel_cache)
ctrl_byte = control_byte.add_seq_bit_to_ctrl_byte(ctrl_byte, sync_bit)
+
+ if ABP.is_ack_piggybacking_allowed(self.channel_cache):
+ ack_bit = ABP.get_send_ack_bit(self.channel_cache)
+ ctrl_byte = control_byte.add_ack_bit_to_ctrl_byte(ctrl_byte, ack_bit)
+
header = PacketHeader(ctrl_byte, self.get_channel_id_int(), payload_len)
async def _write_loop() -> None:
diff --git a/core/src/trezor/wire/thp/received_message_handler.py b/core/src/trezor/wire/thp/received_message_handler.py
index 74b20896..b0b6e310 100644
--- a/core/src/trezor/wire/thp/received_message_handler.py
+++ b/core/src/trezor/wire/thp/received_message_handler.py
@@ -23,10 +23,9 @@ from . import (
ThpDeviceLockedError,
ThpErrorType,
ThpUnallocatedSessionError,
- control_byte,
- get_encoded_device_properties,
- session_manager,
)
+from . import alternating_bit_protocol as ABP
+from . import control_byte, get_encoded_device_properties, session_manager
from .crypto import PUBKEY_LENGTH, Handshake
from .session_context import SeedlessSessionContext
@@ -104,7 +103,23 @@ async def _handle_state_handshake(
if __debug__:
log.debug(__name__, "handle_state_handshake", iface=ctx.iface)
- payload = await ctx.recv_payload(control_byte.is_handshake_init_req)
+ def _handshake_callback(ctrl_byte: int) -> bool:
+ success = control_byte.is_handshake_init_req(ctrl_byte)
+
+ if success and control_byte.get_ack_bit(ctrl_byte) == 1:
+ # Newer Suite versions will send `handshake_init_req` with a non-zero ACK bit.
+ # The device should not use ACK piggybacking with older Suite versions.
+ ABP.allow_ack_piggybacking(ctx.channel_cache)
+
+ if __debug__:
+ ctx._log(
+ "THP ACK piggybacking = ",
+ str(ABP.is_ack_piggybacking_allowed(ctx.channel_cache)),
+ )
+
+ return success
+
+ payload = await ctx.recv_payload(_handshake_callback)
if len(payload) != PUBKEY_LENGTH + 1:
if __debug__:
Why this scored 20/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.