fix(clear_signing): fix bytes[] and strings[] parsing. [no changelog]
What the developer wrote
Authored by PrisionMike
fix(clear_signing): fix bytes[] and strings[] parsing.
[no changelog]
What changed, and why it matters
This commit fixes how Trezor firmware parses certain Ethereum transaction data types—specifically arrays of byte blobs (bytes[]) and arrays of strings (string[])—when showing clear signing details on the device screen. Before the fix, the code would follow an offset pointer twice, likely reading the wrong memory location and producing incorrect decoded values. That could cause the Trezor to display misleading transaction details to the user, potentially tricking them into approving a transaction that does what the screen does not show.
Treat this as a security-relevant correctness fix. Review whether the prior behavior could be exploited to craft an Ethereum transaction whose clear-signing display misrepresents the actual call data, and assess whether a security advisory or changelog entry is warranted. Verify the fix with unit tests covering bytes[], string[], nested arrays, and struct arrays.
Security signals we found
Incorrect offset handling in transaction data decoder
Potential display of misleading clear-signing information
Double pointer dereference in dynamic array parsing
User-interface / transaction approval safety issue
Evidence from the diff
The patch refactors ABI decoding in core/src/apps/ethereum/clear_signing.py. It introduces a parse_body helper on Dynamic types that reads a length-prefixed value at an already-resolved absolute offset. The Array parser is updated so that when its elements are Dynamic (bytes/string), it dereferences the element head once and calls parse_body, instead of calling parse which would dereference a second time. The change also clarifies handling for Atomic, nested Array, and Tuple element types. The bug appears to be a double-dereference: for bytes[]/string[] elements, the existing code called parse(raw_data, i_pointer), which treated the element head as a pointer again, rather than as a relative offset to the dynamic body.
Changed components
core/src/apps/ethereum/clear_signing.pyEthereum clear signing / ABI decodingbytes[] and string[] parameter displayInspect captured patch +41 / −9
diff --git a/core/src/apps/ethereum/clear_signing.py b/core/src/apps/ethereum/clear_signing.py
index 9cc65386..811b1f5e 100644
--- a/core/src/apps/ethereum/clear_signing.py
+++ b/core/src/apps/ethereum/clear_signing.py
@@ -605,8 +605,15 @@ class Dynamic(ABIValue):
if offset + 32 > len(raw_data):
raise OutOfBounds
pointer = int.from_bytes(raw_data[offset : offset + 32], "big")
- data = _read_dynamic_data(raw_data, pointer)
- return self.parser(data), 32
+ return self.parse_body(raw_data, pointer), 32
+
+ def parse_body(self, raw_data: memoryview, body_start: int) -> AnyValue:
+ """Parse the length-prefixed value located directly at `body_start`.
+
+ Unlike `parse`, this expects `body_start` to already point at the value
+ body (no pointer indirection). Used when an enclosing `Array` has
+ already resolved the element's absolute offset."""
+ return self.parser(_read_dynamic_data(raw_data, body_start))
class Tuple(ABIValue):
@@ -680,19 +687,44 @@ class Array(ABIValue):
value = []
for i in range(array_length):
- p = array_start + 32 + (i * 32)
- if p + 32 > len(raw_data):
+ i_pointer = array_start + 32 + (i * 32)
+ if i_pointer + 32 > len(raw_data):
raise OutOfBounds
if isinstance(self.element_definition, Atomic):
- # atomic types are encoded in place
- data, _ = self.element_definition.parse(raw_data, p)
+ # e.g. `uint256[]` / `address[]`: each element is a static leaf,
+ # encoded in place (no pointer indirection), so `parse` reads the
+ # 32-byte value directly at the element head position.
+ data, _ = self.element_definition.parse(raw_data, i_pointer)
elif isinstance(self.element_definition, Array):
- # inner arrays: element head is a relative offset to the inner array body
- element_pointer = int.from_bytes(raw_data[p : p + 32], "big")
+ # e.g. `uint256[][]`: each element is itself a (dynamic) array, so
+ # the element head is a relative offset to the inner array body.
+ # Dereference it, then `_parse_body` consumes the inner length
+ # prefix -- this is the array form of the "dynamic dance".
+ element_pointer = int.from_bytes(
+ raw_data[i_pointer : i_pointer + 32], "big"
+ )
inner_array_start = array_start + 32 + element_pointer
data = self.element_definition._parse_body(raw_data, inner_array_start)
+ elif isinstance(self.element_definition, Dynamic):
+ # e.g. `bytes[]` / `string[]`: each element is a dynamic leaf, so
+ # the element head is a relative offset to the length-prefixed
+ # element body. Read the body directly, mirroring the inner-array
+ # case above -- calling `parse` here would dereference the offset
+ # a second time.
+ element_pointer = int.from_bytes(
+ raw_data[i_pointer : i_pointer + 32], "big"
+ )
+ element_absolute_pointer = array_start + 32 + element_pointer
+ data = self.element_definition.parse_body(
+ raw_data, element_absolute_pointer
+ )
else:
- element_pointer = int.from_bytes(raw_data[p : p + 32], "big")
+ # e.g. `MyStruct[]`: each element is a struct (Tuple). Inside an
+ # array a struct is encoded via a relative offset head (and parsed
+ # as static -- see `from_proto`), so dereference then `parse`.
+ element_pointer = int.from_bytes(
+ raw_data[i_pointer : i_pointer + 32], "big"
+ )
element_absolute_pointer = array_start + 32 + element_pointer
data, _ = self.element_definition.parse(
raw_data, element_absolute_pointer
Why this scored 59/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.