Copilot commented on code in PR #3861:
URL: https://github.com/apache/avro/pull/3861#discussion_r3567489457
##########
lang/py/avro/io.py:
##########
@@ -795,22 +971,35 @@ 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
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()
+ # 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:
decoder.skip(block_size)
else:
Review Comment:
When an array block is encoded with a negative count, the following
`block_size` comes from the input. Currently it is passed directly to
`decoder.skip(block_size)` with no validation, so a malicious negative
`block_size` would seek backwards and corrupt decoding (and an oversized
`block_size` can seek past EOF). Validate that `block_size` is non-negative
(and, when `bytes_remaining()` is known, that it does not exceed the remaining
bytes) before skipping.
##########
lang/py/avro/io.py:
##########
@@ -832,23 +1021,42 @@ 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()
+ # 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:
decoder.skip(block_size)
else:
Review Comment:
When a map block is encoded with a negative count, the subsequent
`block_size` is attacker-controlled. Passing it directly to
`decoder.skip(block_size)` without validating it allows negative sizes (seeking
backwards) and oversized sizes (seeking past EOF), which can corrupt the
decoder state. Validate `block_size` before skipping (and optionally ensure it
does not exceed `bytes_remaining()` when available).
--
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]
