iemejia commented on code in PR #3862:
URL: https://github.com/apache/avro/pull/3862#discussion_r3565199971
##########
lang/ruby/lib/avro/io.rb:
##########
@@ -103,10 +110,33 @@ def read_string
end
def read(len)
- # Read n bytes
+ # Read n bytes. Reject a declared length that exceeds the bytes
+ # actually remaining before allocating for it, to guard against an
+ # out-of-memory attack from a malicious or truncated input. The check
+ # is only applied to larger reads; smaller reads and stream readers
that
+ # cannot report their size fall back to reading directly.
+ if len < 0
+ # A negative length would make IO#read return the rest of the stream,
+ # which bypasses the size check and can allocate without bound.
+ raise AvroError, "Cannot read a negative number of bytes: #{len}"
+ end
+ if len > MAX_UNCHECKED_READ
+ remaining = bytes_remaining
+ if remaining && len > remaining
+ raise AvroError, "Cannot read #{len} bytes, only #{remaining}
remaining"
+ end
+ end
@reader.read(len)
end
+ # Number of bytes still available to read, or nil when the reader cannot
+ # report its size. Used to reject a declared length or collection block
+ # count that exceeds the data actually available before allocating for
it.
+ def bytes_remaining
+ return nil unless @reader.respond_to?(:size) &&
@reader.respond_to?(:tell)
+ @reader.size - @reader.tell
+ end
Review Comment:
Fixed in 34859b4: bytes_remaining now checks that #size/#tell return
Integers and rescues IOError/SystemCallError/NotImplementedError, returning nil
when the size cannot be reported.
##########
lang/ruby/lib/avro/io.rb:
##########
@@ -496,6 +529,43 @@ def skip_record(writers_schema, decoder)
end
private
+ # 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 whose declared element count
+ # could not be backed by the bytes actually remaining, before iterating.
+ # Skipped when the per-element minimum is zero or when the reader cannot
+ # report how many bytes remain.
+ def ensure_collection_available(decoder, count, min_bytes_per_element)
+ return if count <= 0 || min_bytes_per_element <= 0
+ remaining = decoder.bytes_remaining
+ if remaining && count * min_bytes_per_element > remaining
+ raise AvroError, "Collection claims #{count} elements with at least
#{min_bytes_per_element} bytes each, but only #{remaining} bytes are available"
+ end
Review Comment:
Fixed in 34859b4: ensure_collection_available compares count against
remaining / min_bytes_per_element instead of multiplying, avoiding a large
intermediate Integer.
--
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]
