feat(clear_signing): tuple supports abi values instead of parsers directly
What changed, and why it matters
This commit refactors how Trezor firmware parses Ethereum 'clear signing' data so that tuple/struct fields can themselves contain nested tuples or arrays, instead of only simple leaf values. The change removes a previous restriction and updates the parser to walk nested structures correctly. It also removes an old safety guard that rejected empty tuples, though the code still rejects them elsewhere. The commit is described as a feature, not a security fix, but the prior code's leaf-only limitation could have caused mis-parsing of legitimate nested ABI structures, and the old empty-tuple guard was explicitly motivated by preventing an unbounded parse loop.
Treat as a hardening/feature patch rather than an active vulnerability fix. Review that empty tuple rejection remains robust in all code paths (including future callers that might instantiate `Tuple` directly), and verify that recursive parsing of deeply nested or maliciously crafted ABI payloads has adequate bounds checks and cannot be driven into stack exhaustion or unbounded loops by attacker-controlled length/pointer words.
Security signals we found
Parser now supports nested tuples and arrays, reducing risk of mis-parsing valid ABI payloads that were previously unsupported
Old empty-tuple guard comment that cited prevention of unbounded parse loop was removed; rejection still occurs via other paths
Tuple field size calculation changed from fixed word stride to sum of field head sizes, fixing incorrect offsets for nested static structs
Dynamic tuple offsets are now resolved recursively relative to enclosing body start, fixing nested dynamic struct parsing
No changelog entry and no explicit security framing in commit message
Evidence from the diff
The patch changes clear_signing.py so that Tuple holds ABIValue nodes rather than raw parser callables, and TupleValue is redefined to allow nested AnyValue elements. Tuple.__init__ now derives dynamism from its fields (any(f.is_dynamic for f in fields)) and computes fields_size as the sum of each field’s head_size. parse_body walks fields accumulating consumed head bytes and delegates to field.parse, enabling nested static and dynamic structs. The previous DYNAMIC_DATA_PARSERS list and _get_leaf_parser helper are removed; _get_leaf_value builds Atomic/DynamicLeaf nodes instead. Tests are added for nested static and dynamic structs. The README is updated to remove the ‘leaf fields only’ limitation. The explicit zero-field rejection comment in Tuple.__init__ is removed, though from_proto still cannot create an empty tuple because _get_leaf_value raises InvalidFormatDefinition for non-leaf/non-atomic info, and the test still asserts empty tuples are rejected.
Changed components
core/src/apps/ethereum/clear_signing.pycore/src/apps/ethereum/clear_signing_definitions.pycore/src/apps/ethereum/README.mdcore/tests/test_apps.ethereum.clear_signing.pyInspect captured patch +112 / −65
### core/src/apps/ethereum/README.md
@@ -32,7 +32,7 @@ subclass only describes what its *body* looks like, in `parse_body()`:
| `Atomic` | `uintN`, `address`, `bool`, `bytes32`, ... | never | the one-word value itself |
| `DynamicLeaf` | `bytes`, `string` | always | one-word **byte length**, then that many bytes |
| `Array` | `T[]` | always | one-word **element count**, then the elements' heads |
-| `Tuple` | structs (leaf fields only) | iff any field is dynamic | the fields' heads, back to back |
+| `Tuple` | structs | iff any field is dynamic | the fields' heads, back to back |
Note the two length prefixes mean different things: a `DynamicLeaf` counts
*bytes*, an `Array` counts *elements*.
@@ -52,7 +52,7 @@ Note the two length prefixes mean different things: a `DynamicLeaf` counts
static struct, with its own dynamic-field offsets relative to the body
start. ("Parse the body in place" is universal; only the head dereference
depends on the type's dynamism.)
-* Whether a leaf-only tuple is dynamic is fully derivable from its fields.
+* Whether a tuple is dynamic is fully derivable from its fields.
For tuples nested inside arrays, `ABIValue.from_proto` therefore *derives*
the flag and ignores the wire descriptor's `is_dynamic` (which was
historically ignored in that position anyway).
### core/src/apps/ethereum/clear_signing.py
@@ -33,7 +33,9 @@
# into our internal representation.
# TODO: Revisit simplifying this.
Value = int | bytes | bool | str | None | list["Value"]
- TupleValue = tuple[Value, ...]
+ # a tuple field may itself be a tuple or an array, so its elements are
+ # the full `AnyValue`, not just leaf `Value`s
+ TupleValue = tuple["AnyValue", ...]
ListValue = list[TupleValue]
AnyValue = Value | TupleValue | list["AnyValue"]
@@ -196,9 +198,6 @@ def parse_string(raw_data: memoryview) -> Value:
return bytes(raw_data).decode("utf-8")
-DYNAMIC_DATA_PARSERS = [parse_bytes, parse_string]
-
-
def _get_parser(t: int, is_dynamic: bool) -> Parser:
"""Get a parser for a type we received over the wire protocol.
`is_dynamic` selects whether the type is being used in a dynamic
@@ -261,15 +260,6 @@ def _get_parser(t: int, is_dynamic: bool) -> Parser:
raise InvalidFormatDefinition
-def _get_leaf_parser(info: EthereumABIValueInfo) -> Parser:
- """Get a parser for a leaf (atomic or dynamic) value. Raises for nested structures."""
- if info.atomic is not None:
- return _get_parser(info.atomic, is_dynamic=False)
- elif info.dynamic is not None:
- return _get_parser(info.dynamic, is_dynamic=True)
- raise InvalidFormatDefinition
-
-
# Field formatters: https://eips.ethereum.org/EIPS/eip-7730#field-formats
@@ -663,8 +653,7 @@ def from_proto(info: EthereumABIValueInfo) -> "ABIValue":
return DynamicLeaf(_get_parser(info.dynamic, is_dynamic=True))
elif info.tuple is not None:
return Tuple(
- tuple(_get_leaf_parser(f) for f in info.tuple.fields),
- info.tuple.is_dynamic,
+ tuple(_get_leaf_value(f) for f in info.tuple.fields),
)
elif info.array is not None:
element = info.array
@@ -673,17 +662,12 @@ def from_proto(info: EthereumABIValueInfo) -> "ABIValue":
elif element.dynamic is not None:
return Array(DynamicLeaf(_get_parser(element.dynamic, is_dynamic=True)))
elif element.tuple is not None:
- fields = tuple(_get_leaf_parser(f) for f in element.tuple.fields)
+ fields = tuple(_get_leaf_value(f) for f in element.tuple.fields)
# A non-array (leaf) struct/tuple is dynamic if any of its fields is dynamic.
# E.g. of dynamic members: bytes, string, uint256[], bytes[], bytes[][] etc.
# An array (this outer structure) is always* dynamic regardless of its fields.
# (*Unless it's of fixed length, which generally don't exist.)
- return Array(
- Tuple(
- fields,
- is_dynamic=any(p in DYNAMIC_DATA_PARSERS for p in fields),
- )
- )
+ return Array(Tuple(fields))
elif element.array is not None:
inner = element.array
if inner.atomic is not None:
@@ -723,6 +707,15 @@ def _read_dynamic_data(raw_data: memoryview, pointer: int) -> memoryview:
return raw_data[body_start : body_start + length]
+def _get_leaf_value(info: EthereumABIValueInfo) -> ABIValue:
+ """Build a leaf (atomic or dynamic) node. Raises for nested structures."""
+ if info.atomic is not None:
+ return Atomic(_get_parser(info.atomic, is_dynamic=False))
+ elif info.dynamic is not None:
+ return DynamicLeaf(_get_parser(info.dynamic, is_dynamic=True))
+ raise InvalidFormatDefinition
+
+
class DynamicLeaf(ABIValue):
"""`strings` or `bytes`. Their body is a
one-word byte length followed by that many bytes of data."""
@@ -738,9 +731,7 @@ def parse_body(self, raw_data: memoryview, pos: int) -> AnyValue:
class Tuple(ABIValue):
"""Tuples (or Structs - the same thing as far as the ABI is concerned)
- contain multiple values of possibly different types. Only leaf fields
- (atomic types, `bytes`, `string`) are supported here i.e. no more nesting; the dynamic
- fields are the ones whose parser is in DYNAMIC_DATA_PARSERS.
+ contain multiple values of possibly different types.
A tuple is a dynamic type iff at least one of its fields is dynamic.
Its body is the concatenation of its fields' heads: a static field is
@@ -749,35 +740,26 @@ class Tuple(ABIValue):
therefore just its field values back to back - no length prefix and no
offset words anywhere."""
- def __init__(self, fields: tuple[Parser, ...], is_dynamic: bool) -> None:
+ def __init__(self, fields: tuple[ABIValue, ...]) -> None:
if not fields:
- # Zero-field structs do not exist in Solidity. Rejecting them here
- # also guarantees head_size >= _EVM_WORD_SIZE for every type: a
- # static tuple with head_size == 0 inside an Array would defeat
- # the heads bounds pre-check (array_length * 0) and let an
- # attacker-controlled length word drive an unbounded parse loop.
raise InvalidFormatDefinition
self.fields = fields
- self.is_dynamic = is_dynamic
- self.fields_size = len(fields) * _EVM_WORD_SIZE
- if not is_dynamic:
- # a static tuple is encoded in place, so its head is its whole body
+ # `head_size` is already the head *span* of any node: one word for
+ # dynamic types, the full in-place size for static ones.
+ self.fields_size = sum(f.head_size for f in fields)
+ self.is_dynamic = any(f.is_dynamic for f in fields)
+ if not self.is_dynamic:
self.head_size = self.fields_size
def parse_body(self, raw_data: memoryview, pos: int) -> AnyValue:
if pos + self.fields_size > len(raw_data):
raise OutOfBounds
-
- value: list[Value] = [None] * len(self.fields)
-
- for i, parser in enumerate(self.fields):
- field_head_pos = pos + (i * _EVM_WORD_SIZE)
- raw_field = raw_data[field_head_pos : field_head_pos + _EVM_WORD_SIZE]
- if parser not in DYNAMIC_DATA_PARSERS:
- value[i] = parser(raw_field)
- else:
- field_pointer = pos + int.from_bytes(raw_field, "big")
- value[i] = parser(_read_dynamic_data(raw_data, field_pointer))
+ value: list[AnyValue] = []
+ head_offset = pos
+ for field in self.fields:
+ data, consumed = field.parse(raw_data, head_offset, pos)
+ value.append(data)
+ head_offset += consumed
return tuple(value)
### core/src/apps/ethereum/clear_signing_definitions.py
@@ -236,8 +236,11 @@ def all_display_formats() -> Generator[DisplayFormat, None, None]:
DynamicLeaf(parse_bytes), # 1 packedPath (sliced for token addresses)
Array( # 2 swapData: (sendingAssetId, receivingAssetId, fromAmount)[]
Tuple(
- (parse_address, parse_address, parse_uint256),
- is_dynamic=False,
+ (
+ Atomic(parse_address),
+ Atomic(parse_address),
+ Atomic(parse_uint256),
+ )
)
),
],
### core/tests/test_apps.ethereum.clear_signing.py
@@ -199,8 +199,7 @@ def test_uint256_max(self):
def test_static_struct_valid(self):
static_struct = Tuple(
- (parse_address, parse_uint160, parse_bool),
- is_dynamic=False,
+ (Atomic(parse_address), Atomic(parse_uint160), Atomic(parse_bool))
)
addr_bytes = bytes.fromhex("d8da6bf26964af9d7eed9e03e53415d37aa96045")
@@ -220,8 +219,7 @@ def test_static_struct_valid(self):
def test_static_struct_uint160_overflow(self):
static_struct = Tuple(
- (parse_address, parse_uint160, parse_bool),
- is_dynamic=False,
+ (Atomic(parse_address), Atomic(parse_uint160), Atomic(parse_bool))
)
addr_bytes = bytes.fromhex("d8da6bf26964af9d7eed9e03e53415d37aa96045")
@@ -240,7 +238,7 @@ def test_static_struct_uint160_overflow(self):
static_struct.parse(data, len(FIVE_RANDOM_BYTES))
def test_dynamic_struct_valid(self):
- dynamic_struct = Tuple((parse_address, parse_string), is_dynamic=True)
+ dynamic_struct = Tuple((Atomic(parse_address), DynamicLeaf(parse_string)))
addr = bytes.fromhex("d8da6bf26964af9d7eed9e03e53415d37aa96045")
# left padded with zeroes
@@ -276,7 +274,9 @@ def test_dynamic_struct_valid(self):
self.assertEqual(consumed, 32) # only the initial pointer is consumed
def test_dynamic_struct_out_of_bounds(self):
- dynamic_struct = Tuple((parse_uint256,), is_dynamic=True)
+ # the `bytes` field is what makes the struct a dynamic type, and so
+ # what puts its body behind the pointer this test aims out of bounds
+ dynamic_struct = Tuple((DynamicLeaf(parse_bytes),))
# pointer says struct starts at byte 1000, but data is only 32 bytes long
payload = to_bytes(1000)
@@ -285,6 +285,74 @@ def test_dynamic_struct_out_of_bounds(self):
with self.assertRaises(OutOfBounds):
dynamic_struct.parse(data, 0)
+ def test_nested_static_struct_head_accumulation(self):
+ # (uint256 a, (address b, uint256 c) inner, uint256 d)
+ #
+ # A nested *static* struct occupies its whole body in the enclosing
+ # struct's head area - 64 bytes here, not one word - so the field heads
+ # have to be walked by accumulating each field's size. Reading field
+ # `i` at a fixed stride of one word would find `d` at word 2 (`c`)
+ # instead of word 3.
+ nested_struct = Tuple(
+ (
+ Atomic(parse_uint256),
+ Tuple((Atomic(parse_address), Atomic(parse_uint256))),
+ Atomic(parse_uint256),
+ )
+ )
+
+ self.assertFalse(nested_struct.is_dynamic)
+ self.assertEqual(nested_struct.head_size, 128) # 32 + 64 + 32
+
+ addr = bytes.fromhex("d8da6bf26964af9d7eed9e03e53415d37aa96045")
+ payload = (
+ to_bytes(11) # 0 a
+ + pad_left(addr) # 32 inner.b
+ + to_bytes(22) # 64 inner.c
+ + to_bytes(33) # 96 d
+ )
+ data = memoryview(FIVE_RANDOM_BYTES + payload + SEVEN_RANDOM_BYTES)
+
+ parsed, consumed = nested_struct.parse(data, len(FIVE_RANDOM_BYTES))
+
+ self.assertEqual(parsed, (11, (addr, 22), 33))
+ self.assertEqual(consumed, 128)
+
+ def test_nested_dynamic_struct_offset_is_relative_to_enclosing_body(self):
+ # (address a, (uint256 x, bytes y) inner)
+ #
+ # `inner` is dynamic (it holds a `bytes`), so its head is a pointer -
+ # and that pointer is relative to the start of the *enclosing struct's
+ # body*, not to the parameter block. The outer body deliberately does
+ # not start at 0 here, so the two readings resolve to different words.
+ nested_struct = Tuple(
+ (
+ Atomic(parse_address),
+ Tuple((Atomic(parse_uint256), DynamicLeaf(parse_bytes))),
+ )
+ )
+
+ self.assertTrue(nested_struct.is_dynamic)
+ self.assertEqual(nested_struct.head_size, 32)
+
+ addr = bytes.fromhex("d8da6bf26964af9d7eed9e03e53415d37aa96045")
+ blob = b"\xca\xfe"
+ data = memoryview(
+ to_bytes(64) # 0 outer head -> outer body at 64
+ + to_bytes(0) # 32 unused gap, keeps the outer body off 0
+ + pad_left(addr) # 64 outer field 0
+ + to_bytes(64) # 96 outer field 1, rel to 64 -> inner body at 128
+ + to_bytes(7) # 128 inner field 0
+ + to_bytes(64) # 160 inner field 1, rel to 128 -> bytes body at 192
+ + to_bytes(len(blob)) # 192 byte length
+ + pad_right(blob) # 224 data
+ )
+
+ parsed, consumed = nested_struct.parse(data, 0)
+
+ self.assertEqual(parsed, (addr, (7, blob)))
+ self.assertEqual(consumed, 32) # only the outer pointer is consumed
+
def test_array_of_addresses_valid(self):
address_array_parser = Array(Atomic(parse_address))
@@ -337,10 +405,9 @@ def test_array_of_addresses_with_dirty_element(self):
def test_array_of_dynamic_structs(self):
array_parser = Array(
Tuple(
- (parse_address, parse_string),
# the `string` field makes the struct a dynamic type, so the
# array encodes its elements via offset heads
- is_dynamic=True,
+ (Atomic(parse_address), DynamicLeaf(parse_string))
)
)
@@ -390,10 +457,7 @@ def test_array_of_static_structs(self):
# the length word, at a stride of the struct size (96 bytes), with no
# offset heads at all.
array_parser = Array(
- Tuple(
- (parse_address, parse_address, parse_uint256),
- is_dynamic=False,
- )
+ Tuple((Atomic(parse_address), Atomic(parse_address), Atomic(parse_uint256)))
)
addr1 = bytes.fromhex("6666666666666666666666666666666666666666")
@@ -440,9 +504,7 @@ def test_empty_tuple_rejected(self):
# the heads bounds pre-check (array_length * 0) and let an
# attacker-controlled length word drive an unbounded parse loop.
with self.assertRaises(InvalidFormatDefinition):
- Tuple((), is_dynamic=False)
- with self.assertRaises(InvalidFormatDefinition):
- Tuple((), is_dynamic=True)
+ Tuple(())
def test_bytes32_parsing(self):
atomic_bytes32 = Atomic(parse_bytes32)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.