Copilot commented on code in PR #3861:
URL: https://github.com/apache/avro/pull/3861#discussion_r3567559186
##########
lang/py/avro/io.py:
##########
@@ -832,24 +1045,43 @@ def read_map(self, writers_schema:
avro.schema.MapSchema, readers_schema: avro.s
The actual count in this case
is the absolute value of the count written.
"""
- read_items = {}
+ read_items: Dict[str, object] = {}
+ # Map keys are strings (>= 1 byte length prefix) plus the value.
+ min_bytes = 1 + _min_bytes_per_element(writers_schema.values)
+ zero_byte_limit, structural_limit = _collection_limits()
+ # Track decoded pairs separately: len(read_items) counts unique keys,
so
+ # duplicate keys (later entries overwrite earlier ones) would
undercount
+ # and let a multi-block map exceed the cumulative caps.
+ items_read = 0
block_count = decoder.read_long()
while block_count != 0:
if block_count < 0:
block_count = -block_count
decoder.skip_long()
+ self._ensure_collection_available(decoder, items_read,
block_count, min_bytes, zero_byte_limit, structural_limit)
+ items_read += block_count
for i in range(block_count):
key = decoder.read_utf8()
read_items[key] = self.read_data(writers_schema.values,
readers_schema.values, decoder)
block_count = decoder.read_long()
return read_items
def skip_map(self, writers_schema: avro.schema.MapSchema, decoder:
BinaryDecoder) -> None:
+ min_bytes = 1 + _min_bytes_per_element(writers_schema.values)
+ zero_byte_limit, structural_limit = _collection_limits()
+ items_skipped = 0
block_count = decoder.read_long()
while block_count != 0:
+ block_size = None
if block_count < 0:
+ block_count = -block_count
block_size = decoder.read_long()
- decoder.skip(block_size)
+ # Bound the (normalized) count on both the sized and unsized paths
so
+ # a negative block count cannot bypass the collection limits.
+ self._ensure_collection_available(decoder, items_skipped,
block_count, min_bytes, zero_byte_limit, structural_limit)
+ items_skipped += block_count
+ if block_size is not None:
+ self._skip_block_bytes(decoder, block_size)
Review Comment:
Same issue as `skip_array()`: in the sized-block skip path the decoder seeks
forward by the declared `block_size` without ensuring it can possibly contain
`block_count` map entries (keys are >=1 byte plus the value’s minimum). A
too-small `block_size` can leave the decoder positioned mid-block and cause
subsequent reads to interpret leftover bytes as the next field/block header.
Add a lower-bound check before skipping: for `min_bytes > 0`, reject when
`block_count > block_size // min_bytes`.
##########
lang/py/avro/io.py:
##########
@@ -795,23 +978,53 @@ def read_array(self, writers_schema:
avro.schema.ArraySchema, readers_schema: av
The actual count in this case
is the absolute value of the count written.
"""
- read_items = []
+ read_items: List[object] = []
+ min_bytes = _min_bytes_per_element(writers_schema.items)
+ zero_byte_limit, structural_limit = _collection_limits()
block_count = decoder.read_long()
while block_count != 0:
if block_count < 0:
block_count = -block_count
decoder.skip_long()
+ self._ensure_collection_available(decoder, len(read_items),
block_count, min_bytes, zero_byte_limit, structural_limit)
for i in range(block_count):
read_items.append(self.read_data(writers_schema.items,
readers_schema.items, decoder))
block_count = decoder.read_long()
return read_items
+ @staticmethod
+ def _skip_block_bytes(decoder: BinaryDecoder, block_size: int) -> None:
+ """Skip a sized-block's byte count, rejecting malformed sizes.
+
+ The block_size is attacker-controlled: a negative value would seek
+ backwards and an oversized value past EOF, either corrupting the
decoder
+ position. Reject both before skipping.
+ """
+ if block_size < 0:
+ raise avro.errors.InvalidAvroBinaryEncoding(f"Invalid negative
block size: {block_size}")
+ remaining = decoder.bytes_remaining()
+ if remaining is not None and block_size > remaining:
+ raise avro.errors.InvalidAvroBinaryEncoding(
+ f"Block size {block_size} exceeds the {remaining} bytes
remaining"
+ )
+ decoder.skip(block_size)
+
def skip_array(self, writers_schema: avro.schema.ArraySchema, decoder:
BinaryDecoder) -> None:
+ min_bytes = _min_bytes_per_element(writers_schema.items)
+ zero_byte_limit, structural_limit = _collection_limits()
+ items_skipped = 0
block_count = decoder.read_long()
while block_count != 0:
+ block_size = None
if block_count < 0:
+ block_count = -block_count
block_size = decoder.read_long()
- decoder.skip(block_size)
+ # Bound the (normalized) count on both the sized and unsized paths
so
+ # a negative block count cannot bypass the collection limits.
+ self._ensure_collection_available(decoder, items_skipped,
block_count, min_bytes, zero_byte_limit, structural_limit)
+ items_skipped += block_count
+ if block_size is not None:
+ self._skip_block_bytes(decoder, block_size)
Review Comment:
In the sized-block skip path (negative block count), the code trusts the
attacker-controlled `block_size` to reposition the decoder without verifying it
can possibly contain `block_count` elements. If `block_size` is smaller than
the minimum on-wire size for `block_count` elements, `skip_array()` can seek
into the middle of the block payload and corrupt decoding of subsequent fields
(and may accept malformed/truncated input when schema resolution skips this
field). Add a lower-bound validation: for `min_bytes > 0`, reject when
`block_count > block_size // min_bytes`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
