Skip to content

msgpack, struct, zlib, buffer_helper: values from outside used unchecked - #11377

Open
peterbay wants to merge 1 commit into
adafruit:mainfrom
peterbay:bound-values-from-outside
Open

msgpack, struct, zlib, buffer_helper: values from outside used unchecked#11377
peterbay wants to merge 1 commit into
adafruit:mainfrom
peterbay:bound-values-from-outside

Conversation

@peterbay

Copy link
Copy Markdown

Code written by Claude Code, guided and corrected by @peterbay.

The problem

Six places where a value that comes from outside the program — a msgpack stream, a format string, an argument — is used without being checked. Two of them take the board down with a hard fault.

The changes

  • msgpack recursed once per container with no C stack check. The nesting on the unpack side comes straight from the input, so a stream of nested arrays runs the C stack out; 2000 of them fault the board. mp_cstack_check() in pack and unpack turns that into a RuntimeError.

  • A container's element count was multiplied by the element size with no overflow check. An array or map header carries a 32-bit count taken from the stream, and mp_obj_new_list / mp_obj_new_dict multiply it by sizeof(mp_obj_t) or sizeof(mp_map_elem_t). On a 32-bit build 0x40000001 * 4 wraps to 4, so a four-element allocation was filled with a billion entries.

  • msgpack wrote and read half a float. Type 0xca is a 32-bit float, but the union paired uint32_t with mp_float_t, which is a double on ports that have one. There it wrote the wrong half of the value and read back nonsense. The union holds a float now, converted at the boundary.

  • struct's repeat count went through MP_OBJ_SMALL_INT_VALUE. mp_parse_num_integer returns a big-int object when the literal does not fit a small int, and MP_OBJ_SMALL_INT_VALUE on one of those just reinterprets the object word: struct.calcsize("9999999999b") returned 268486792 here. Read through mp_obj_get_int, which raises for a value that does not fit.

  • zlib.decompress read wbits the same way, so a long int, a float, a string or None came through as a pointer shifted rather than being converted or rejected.

  • normalize_buffer_bounds left a negative start negative. A start below -length had the length added and was still below zero, and the caller then worked from buf + start with a length reaching past the end. bitbangio.SPI().write(buf, start=-100) on a ten byte buffer reads ninety bytes from in front of it.

Testing

Seeed XIAO nRF52840 Sense, on two builds differing only by these changes.

before after
struct.calcsize("9999999999b") returns 268486792 OverflowError
struct.calcsize("70000b") 70000 70000, unchanged
zlib.decompress(b"", "nonsense") ValueError: -3, from a pointer TypeError: can't convert str to int
msgpack.unpack of an array32 with count 0x40000001 EOFError ValueError: array too long
the same as a map32 EOFError ValueError: map too long
msgpack.unpack of 2000 nested arrays hard fault RuntimeError: maximum recursion depth exceeded
bitbangio.SPI().write(bytearray(10), start=-100) board down, USB gone until a double tap on reset returns

The two container-count rows stop at EOFError rather than at corruption because the stream ends before the loop has filled much; the allocation is still four bytes for a billion entries.

The float is not covered: mp_float_t is a single on this port, so the old union was the right size there and nothing changes. It is a defect only where mp_float_t is a double.

No new translatable strings: the two messages reuse "%q too long".

msgpack recurses once per container and the nesting on the unpack side comes
from the input, so a stream of nested arrays ran the C stack out; 2000 of them
fault the board. It calls mp_cstack_check() now. A container's 32-bit element
count was also multiplied by the element size with no overflow check, so on a
32-bit build a count of 0x40000001 allocated four bytes for a billion entries.
Type 0xca is a 32-bit float, but the union paired uint32_t with mp_float_t,
which is a double on some ports; there it wrote half the value.

struct's repeat count and zlib's wbits both went through
MP_OBJ_SMALL_INT_VALUE, which on anything that is not a small int just
reinterprets the object word: struct.calcsize("9999999999b") returned
268486792. Both read through the converting accessors now.

normalize_buffer_bounds added the length to a negative start and left it
negative when it was below -length, so the caller worked from before the
buffer with a length reaching past its end.
@peterbay

Copy link
Copy Markdown
Author

Testing and diagnostic script.
bound_values_from_outside.py

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant