fix(core): don't raise `ThpError` on low-level protocol errors
What the developer wrote
Authored by Roman Zeyde
fix(core): don't raise `ThpError` on low-level protocol errors
`ThpError` ends up being sent over the protocol as `MessageType_Failure`,
but in case of a low-level problem (e.g. unexpected SEQ bit), it is
probably better to ignore the offending message.
We should also not raise `ThpError` when a function is called with invalid
arguments (e.g. bit value not being 0 or 1).
[no changelog]
What changed, and why it matters
This commit changes how the Trezor hardware wallet handles low-level communication errors in its new THP (Trezor Host Protocol) code. Previously, certain malformed or unexpected low-level packets would cause the device to raise a ThpError, which gets sent back to the host as a Failure message. The patch makes the device silently ignore many of these low-level problems instead—such as unexpected control bytes, wrong sequence bits, bad checksums, invalid broadcast lengths, and reassembly overruns. Some internal argument checks are also converted from raising errors to assert statements, which only run in debug builds and are removed in production firmware. The stated intent is to make the protocol more robust against noisy or malformed traffic, but it also reduces the device's visibility into potential attacks or protocol fuzzing.
Treat this as a defensive-hardening change that warrants review rather than an confirmed vulnerability. A security reviewer should verify that ignoring each of these low-level errors cannot lead to state desynchronization, replay acceptance, buffer misuse, or denial of service. In particular, confirm that: (1) the reassembler reset on overread cannot be abused to drop legitimate payloads; (2) ignored unexpected sequence bits do not allow replay or sequence-number confusion; (3) assert-based validation in control_byte.py and alternating_bit_protocol.py is not reachable from untrusted host input in release builds; and (4) broadcast-channel invalid checksum/length/control-byte returns do not allow channel allocation spoofing. Consider adding counters or rate-limiting so that persistent malformed traffic is detectable.
Security signals we found
Error-silencing: multiple protocol error paths converted from raising ThpError/Failure to logging (debug-only) and continuing/returning
Defense weakening: malformed packets (bad checksum, wrong length, unexpected control byte, bad sequence bit) are now ignored rather than rejected
Debug-only assertions replace runtime argument validation in control_byte.py and alternating_bit_protocol.py
Reassembly overread no longer raises but resets state and returns False
Potential DoS / state confusion risk: an attacker or glitching host can send malformed THP traffic without receiving a Failure response
No explicit security advisory, CVE, or researcher attribution in commit or supplied references
Evidence from the diff
The patch modifies five files in core/src/trezor/wire/thp/. It removes ThpError raises for low-level protocol conditions and replaces them with logging (only in debug builds), returns, or continues. Specific changes: alternating_bit_protocol.py and control_byte.py now use assert for seq/ack bit validation instead of raising ThpError; channel.py ignores unexpected control bytes and wrong sequence bits and resets the reassembler on buffer overread; interface_context.py returns early on invalid checksum, invalid broadcast payload length, or unexpected control byte in broadcast packets; received_message_handler.py logs invalid handshake payload length and invalid channel state instead of raising. The commit message explicitly says ThpError should not be raised on low-level protocol errors or invalid function arguments, and that offending messages should be ignored.
Changed components
core/src/trezor/wire/thp/alternating_bit_protocol.pycore/src/trezor/wire/thp/channel.pycore/src/trezor/wire/thp/control_byte.pycore/src/trezor/wire/thp/interface_context.pycore/src/trezor/wire/thp/received_message_handler.pyInspect captured patch +50 / −36
diff --git a/core/src/trezor/wire/thp/alternating_bit_protocol.py b/core/src/trezor/wire/thp/alternating_bit_protocol.py
index 231b2830..4e84b395 100644
--- a/core/src/trezor/wire/thp/alternating_bit_protocol.py
+++ b/core/src/trezor/wire/thp/alternating_bit_protocol.py
@@ -1,7 +1,5 @@
from storage.cache_thp import ChannelCache
-from . import ThpError
-
def is_ack_valid(cache: ChannelCache, ack_bit: int) -> bool:
"""
@@ -72,8 +70,7 @@ def set_expected_receive_seq_bit(cache: ChannelCache, seq_bit: int) -> None:
Set the expected sequential number (bit) of the next message to be received
in the provided channel
"""
- if seq_bit not in (0, 1):
- raise ThpError("Unexpected receive sync bit")
+ assert seq_bit in (0, 1)
# set second bit to "seq_bit" value
cache.sync &= 0xBF
@@ -82,8 +79,7 @@ def set_expected_receive_seq_bit(cache: ChannelCache, seq_bit: int) -> None:
def _set_send_seq_bit(cache: ChannelCache, seq_bit: int) -> None:
- if seq_bit not in (0, 1):
- raise ThpError("Unexpected send seq bit")
+ assert seq_bit in (0, 1)
# set third bit to "seq_bit" value
cache.sync &= 0xDF
if seq_bit:
diff --git a/core/src/trezor/wire/thp/channel.py b/core/src/trezor/wire/thp/channel.py
index d524a983..a1d541ed 100644
--- a/core/src/trezor/wire/thp/channel.py
+++ b/core/src/trezor/wire/thp/channel.py
@@ -26,14 +26,7 @@ from trezor.loop import Timeout, race, sleep
from trezor.wire.context import UnexpectedMessageException
from ..protocol_common import Message
-from . import (
- ACK_MESSAGE,
- ENCRYPTED,
- ChannelState,
- PacketHeader,
- ThpDecryptionError,
- ThpError,
-)
+from . import ACK_MESSAGE, ENCRYPTED, ChannelState, PacketHeader, ThpDecryptionError
from . import alternating_bit_protocol as ABP
from . import control_byte, crypto, memory_manager
from .checksum import CHECKSUM_LENGTH, is_valid
@@ -113,7 +106,15 @@ class Reassembler:
return False
if self.bytes_read > self.buffer_len:
- raise ThpError("read more bytes than expected")
+ if __debug__:
+ log.warning(
+ __name__,
+ "Reassembled %d bytes, %d expected",
+ self.bytes_read,
+ self.buffer_len,
+ )
+ self.reset()
+ return False
if not is_checksum_valid(buffer):
return False
@@ -263,8 +264,12 @@ class Channel:
if expected_ctrl_byte is None or not expected_ctrl_byte(ctrl_byte):
if __debug__:
- self._log("Unexpected control byte: ", utils.hexlify_if_bytes(msg))
- raise ThpError("Unexpected control byte")
+ self._log(
+ "Unexpected control byte - ignoring ",
+ utils.hexlify_if_bytes(msg),
+ logger=log.warning,
+ )
+ continue
# 2: Handle message with unexpected sequential bit
if seq_bit != ABP.get_expected_receive_seq_bit(self.channel_cache):
@@ -273,7 +278,7 @@ class Channel:
"Received message with an unexpected sequential bit",
)
await send_ack(self, ack_bit=seq_bit)
- raise ThpError("Received message with an unexpected sequential bit")
+ continue
# 3: Send ACK in response
await send_ack(self, ack_bit=seq_bit)
diff --git a/core/src/trezor/wire/thp/control_byte.py b/core/src/trezor/wire/thp/control_byte.py
index 309661be..ddd0a8f9 100644
--- a/core/src/trezor/wire/thp/control_byte.py
+++ b/core/src/trezor/wire/thp/control_byte.py
@@ -6,7 +6,6 @@ from . import (
ENCRYPTED,
HANDSHAKE_COMP_REQ,
HANDSHAKE_INIT_REQ,
- ThpError,
)
_CONTINUATION_PACKET_MASK = const(0x80)
@@ -15,19 +14,19 @@ _DATA_MASK = const(0xE7)
def add_seq_bit_to_ctrl_byte(ctrl_byte: int, seq_bit: int) -> int:
- if seq_bit == 0:
- return ctrl_byte & 0xEF
- if seq_bit == 1:
+ assert seq_bit in (0, 1)
+ if seq_bit:
return ctrl_byte | 0x10
- raise ThpError("Unexpected sequence bit")
+ else:
+ return ctrl_byte & 0xEF
def add_ack_bit_to_ctrl_byte(ctrl_byte: int, ack_bit: int) -> int:
- if ack_bit == 0:
- return ctrl_byte & 0xF7
- if ack_bit == 1:
+ assert ack_bit in (0, 1)
+ if ack_bit:
return ctrl_byte | 0x08
- raise ThpError("Unexpected acknowledgement bit")
+ else:
+ return ctrl_byte & 0xF7
def get_ack_bit(ctrl_byte: int) -> int:
diff --git a/core/src/trezor/wire/thp/interface_context.py b/core/src/trezor/wire/thp/interface_context.py
index b9d9816f..941b582d 100644
--- a/core/src/trezor/wire/thp/interface_context.py
+++ b/core/src/trezor/wire/thp/interface_context.py
@@ -15,7 +15,6 @@ from . import (
CODEC_V1,
PING,
PacketHeader,
- ThpError,
ThpErrorType,
channel_manager,
checksum,
@@ -168,10 +167,20 @@ class InterfaceContext:
packet = packet[: PacketHeader.INIT_LENGTH + payload_length]
if not checksum.is_valid(packet[-CHECKSUM_LENGTH:], packet[:-CHECKSUM_LENGTH]):
- raise ThpError("Invalid checksum")
+ if __debug__:
+ log.debug(
+ __name__, "Invalid checksum: %s", utils.hexlify_if_bytes(packet)
+ )
+ return
if payload_length != _BROADCAST_PAYLOAD_LENGTH:
- raise ThpError("Invalid length in broadcast channel packet")
+ if __debug__:
+ log.debug(
+ __name__,
+ "Invalid length in broadcast channel packet: %d",
+ payload_length,
+ )
+ return
nonce = packet[PacketHeader.INIT_LENGTH : -CHECKSUM_LENGTH]
@@ -180,7 +189,13 @@ class InterfaceContext:
return await self.write_payload(response_header, nonce)
if ctrl_byte != CHANNEL_ALLOCATION_REQ:
- raise ThpError("Unexpected ctrl_byte in a broadcast channel packet")
+ if __debug__:
+ log.debug(
+ __name__,
+ "Unexpected ctrl_byte in a broadcast channel packet: %d",
+ ctrl_byte,
+ )
+ return
channel_cache = channel_manager.create_new_channel(self._iface)
response_data = get_channel_allocation_response(
diff --git a/core/src/trezor/wire/thp/received_message_handler.py b/core/src/trezor/wire/thp/received_message_handler.py
index d4307a19..56ae6c71 100644
--- a/core/src/trezor/wire/thp/received_message_handler.py
+++ b/core/src/trezor/wire/thp/received_message_handler.py
@@ -21,7 +21,6 @@ from . import (
SessionState,
ThpDecryptionError,
ThpDeviceLockedError,
- ThpError,
ThpErrorType,
ThpUnallocatedSessionError,
control_byte,
@@ -62,9 +61,8 @@ async def handle_received_message(channel: Channel) -> bool:
elif state is ChannelState.TH1:
await _handle_state_handshake(channel)
return channel.get_channel_state() == ChannelState.TC1
- else:
- raise ThpError("Unimplemented channel state")
-
+ if __debug__:
+ channel._log("Invalid channel state", logger=log.error)
except ThpUnallocatedSessionError as e:
error_message = Failure(code=FailureType.ThpUnallocatedSession)
await channel.write(error_message, e.session_id)
@@ -89,7 +87,8 @@ async def _handle_state_handshake(
payload = await ctx.recv_payload(control_byte.is_handshake_init_req)
if len(payload) != PUBKEY_LENGTH:
- raise ThpError("Message received is not a valid handshake init request!")
+ log.error(__name__, "Message received is not a valid handshake init request!")
+ return
if not config.is_unlocked():
raise ThpDeviceLockedError
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.