iemejia commented on code in PR #3862:
URL: https://github.com/apache/avro/pull/3862#discussion_r3566893761
##########
lang/ruby/lib/avro/io.rb:
##########
@@ -496,12 +550,99 @@ def skip_record(writers_schema, decoder)
end
private
- def skip_blocks(decoder, &blk)
+ # Minimum number of bytes a single value of the given schema can occupy
on
+ # the wire. Used to reject an array/map block count that could not be
+ # backed by the bytes remaining. A type that can encode to zero bytes
+ # (null) returns 0, which disables the check for it (so an array of nulls
+ # is not falsely rejected).
+ def min_bytes_per_element(schema, visited = nil)
+ visited ||= {}.compare_by_identity
+ case schema.type_sym
+ when :null then 0
+ when :float then 4
+ when :double then 8
+ when :fixed then schema.size
+ when :record, :error, :request
+ return 0 if visited[schema]
+ visited[schema] = true
+ total = schema.fields.sum { |field|
min_bytes_per_element(field.type, visited) }
+ visited.delete(schema)
+ total
+ else
+ # boolean, int, long, bytes, string, enum, union, array, map: >= 1
byte
+ # (a union encodes at least a 1-byte branch index).
+ 1
+ end
+ end
+
+ # Reject a collection (array or map) block that could drive an unbounded
+ # allocation, before iterating. A block whose declared element count
could
+ # not be backed by the bytes actually remaining is rejected; a block of
+ # zero-byte elements (where the bytes-remaining check does not apply) is
+ # bounded by a cumulative item cap; and every collection is bounded by a
+ # structural cap. Returns the running total across blocks.
+ #
+ # Both limits default to the same values as the other Avro SDKs and can
be
+ # overridden (to a single value capping both) via the
+ # AVRO_MAX_COLLECTION_ITEMS environment variable.
+ DEFAULT_MAX_COLLECTION_ITEMS = 10_000_000 # Zero-byte element cap.
+ DEFAULT_MAX_COLLECTION_STRUCTURAL = 2147483639 # Integer.MAX_VALUE - 8.
+
+ def collection_limits
+ env = Integer(ENV['AVRO_MAX_COLLECTION_ITEMS'], exception: false)
+ if env && env >= 0
+ [env, env]
+ else
+ [DEFAULT_MAX_COLLECTION_ITEMS, DEFAULT_MAX_COLLECTION_STRUCTURAL]
+ end
+ end
+
+ def ensure_collection_available(decoder, total, count,
min_bytes_per_element)
+ # A negative count here is corrupt/malicious data (the read/skip
callers
+ # already normalized a legitimate negative block count to its absolute
+ # value); reject it explicitly.
+ raise CollectionSizeError, "Invalid negative collection block count:
#{count}" if count < 0
+
+ total += count
+ max_items, max_structural = collection_limits
+
+ # A structural cap covers all collections, including decoders that
cannot
+ # report their remaining bytes.
+ if total > max_structural
+ raise CollectionSizeError, "Collection size #{total} exceeds the
maximum allowed size of #{max_structural}"
+ end
+
+ if min_bytes_per_element <= 0
+ # Zero-byte elements (e.g. null) consume no input, so the
+ # bytes-remaining check cannot bound them; cap by item count.
+ if total > max_items
+ raise CollectionSizeError, "Collection of zero-byte elements
(#{total}) exceeds the maximum allowed size of #{max_items}"
+ end
+ elsif decoder.respond_to?(:bytes_remaining)
+ # A decoder that implements the read protocol but not
#bytes_remaining
+ # (e.g. a custom decoder) cannot report the remaining size; skip the
+ # byte check for it rather than raising NoMethodError.
+ remaining = decoder.bytes_remaining
+ # Compare via integer division rather than multiplying, so a huge
count
+ # does not create a large intermediate product.
+ if remaining && count > remaining / min_bytes_per_element
+ raise CollectionSizeError, "Collection claims #{count} elements
with at least #{min_bytes_per_element} bytes each, but only #{remaining} bytes
are available"
+ end
+ end
+
+ total
+ end
+
+ def skip_blocks(decoder, min_bytes = 0, &blk)
+ total = 0
block_count = decoder.read_long
while block_count != 0
if block_count < 0
+ # A negative count carries a byte size, so the whole block can be
+ # skipped directly without iterating.
decoder.skip(decoder.read_long)
else
+ total = ensure_collection_available(decoder, total, block_count,
min_bytes)
block_count.times(&blk)
end
Review Comment:
Fixed in 547dd67900: skip_blocks now normalizes a negative block count and
applies ensure_collection_available on the sized (negative) path too, so the
byte-sized block form can't bypass the caps; it also rejects a negative block
byte-size (which decoder.skip would otherwise seek backwards).
##########
lang/ruby/test/test_io.rb:
##########
@@ -42,6 +42,122 @@ def test_bytes
check_default('"bytes"', '"foo"', "foo")
end
+ # A bytes/string value declares a length prefix; a malicious or truncated
+ # input can declare far more bytes than actually exist. On a reader that can
+ # report its size, that is rejected before allocating for it.
+ def test_read_bytes_rejects_length_beyond_stream
+ writer = StringIO.new
+ declared = Avro::IO::BinaryDecoder::MAX_UNCHECKED_READ + 1
+ Avro::IO::BinaryEncoder.new(writer).write_long(declared)
+ reader = StringIO.new(writer.string)
+ decoder = Avro::IO::BinaryDecoder.new(reader)
+ assert_raise(Avro::AvroError) { decoder.read_bytes }
+ end
+
+ def test_read_bytes_within_stream_still_reads
+ payload = 'x' * (Avro::IO::BinaryDecoder::MAX_UNCHECKED_READ + 1)
+ writer = StringIO.new
+ Avro::IO::BinaryEncoder.new(writer).write_bytes(payload)
+ reader = StringIO.new(writer.string)
+ decoder = Avro::IO::BinaryDecoder.new(reader)
+ assert_equal(payload, decoder.read_bytes)
+ end
+
+ # An array/map block declares an element count; a malicious or truncated
input
+ # can declare far more elements than the remaining bytes could hold. The
count
+ # is validated against the bytes remaining before iterating, using the
minimum
+ # on-wire size of the element schema (so 0-byte elements like null are not
+ # falsely rejected).
+ def decode(schema_json, encoded)
+ schema = Avro::Schema.parse(schema_json)
+ reader = Avro::IO::DatumReader.new(schema)
+ reader.read(Avro::IO::BinaryDecoder.new(StringIO.new(encoded)))
+ end
+
+ def test_read_array_rejects_count_beyond_stream
+ writer = StringIO.new
+ Avro::IO::BinaryEncoder.new(writer).write_long(1_000_000)
+ assert_raise(Avro::IO::CollectionSizeError) do
+ decode('{"type":"array","items":"long"}', writer.string)
+ end
+ end
+
+ def test_read_map_rejects_count_beyond_stream
+ writer = StringIO.new
+ Avro::IO::BinaryEncoder.new(writer).write_long(1_000_000)
+ assert_raise(Avro::IO::CollectionSizeError) do
+ decode('{"type":"map","values":"long"}', writer.string)
+ end
+ end
+
+ def test_read_array_of_null_not_falsely_rejected
+ count = 100_000
+ writer = StringIO.new
+ encoder = Avro::IO::BinaryEncoder.new(writer)
+ encoder.write_long(count) # one block of `count` nulls (zero bytes each)
+ encoder.write_long(0) # end-of-array marker
+ result = decode('{"type":"array","items":"null"}', writer.string)
+ assert_equal(count, result.length)
+ assert_nil(result.first)
+ assert_nil(result.last)
+ end
+
+ def test_read_array_within_stream_still_reads
+ schema = Avro::Schema.parse('{"type":"array","items":"long"}')
+ writer = StringIO.new
+ Avro::IO::DatumWriter.new(schema).write([1, 2, 3],
Avro::IO::BinaryEncoder.new(writer))
+ assert_equal([1, 2, 3], decode('{"type":"array","items":"long"}',
writer.string))
+ end
+
+ # A zero-byte element type (null) consumes no input, so the bytes-remaining
+ # check cannot bound its block count; a tiny payload declaring a huge count
+ # must instead be rejected by the item cap before building the array.
+ def test_read_array_of_null_rejects_huge_count
+ writer = StringIO.new
+ Avro::IO::BinaryEncoder.new(writer).write_long(200_000_000) # ~4 byte
payload
+ assert_raise(Avro::IO::CollectionSizeError) do
+ decode('{"type":"array","items":"null"}', writer.string)
+ end
+ end
+
+ # INT64_MIN as a block count is the pathological negation case. Ruby integers
+ # do not overflow, so negating it yields 2**63, which the cap rejects.
INT64_MIN
+ # zig-zag encodes as the 10-byte varint below, followed by a block byte-size
(0)
+ # that the negative-block path reads.
+ def test_read_array_of_null_int64_min_block_count
+ payload = "\xFF\xFF\xFF\xFF\xFF\xFF\xFF\xFF\xFF\x01\x00".b
+ assert_raise(Avro::IO::CollectionSizeError) do
+ decode('{"type":"array","items":"null"}', payload)
+ end
+ end
+
+ def test_read_map_rejects_huge_count
+ writer = StringIO.new
+ Avro::IO::BinaryEncoder.new(writer).write_long(200_000_000)
+ assert_raise(Avro::IO::CollectionSizeError) do
+ decode('{"type":"map","values":"long"}', writer.string)
+ end
+ end
+
+ # The skip path (a writer field absent from the reader schema) must be
bounded
+ # too, so skipping a huge zero-byte block cannot loop endlessly.
+ def test_skip_array_of_null_rejects_huge_count
+ writers_schema = Avro::Schema.parse(<<-JSON)
+ {"type":"record","name":"Foo","fields":[
+ {"name":"arr","type":{"type":"array","items":"null"}},
+ {"name":"val","type":"int"}]}
+ JSON
+ readers_schema = Avro::Schema.parse(<<-JSON)
+ {"type":"record","name":"Foo","fields":[{"name":"val","type":"int"}]}
+ JSON
+ writer = StringIO.new
+ Avro::IO::BinaryEncoder.new(writer).write_long(200_000_000) # arr block
count
+ reader = Avro::IO::DatumReader.new(writers_schema, readers_schema)
+ assert_raise(Avro::IO::CollectionSizeError) do
+ reader.read(Avro::IO::BinaryDecoder.new(StringIO.new(writer.string)))
+ end
+ end
Review Comment:
Fixed in 547dd67900: added
test_skip_array_of_null_negative_block_rejects_huge_count, which skips a
record's array<null> field encoded with a huge negative block count and asserts
CollectionSizeError.
--
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]
