iemejia commented on code in PR #3864:
URL: https://github.com/apache/avro/pull/3864#discussion_r3566777007
##########
lang/perl/lib/Avro/BinaryDecoder.pm:
##########
@@ -124,18 +124,178 @@ sub skip_bytes {
my $class = shift;
my $reader = pop;
my $size = decode_long($class, undef, undef, $reader);
- $reader->seek($size, 0);
+ if ($size < 0) {
+ throw Avro::Schema::Error::Parse(
+ "Invalid negative bytes/string length: $size");
+ }
+ # Reject a declared length that exceeds the bytes actually remaining before
+ # skipping, mirroring decode_bytes: a seek past EOF silently succeeds on
+ # many handles, so without this a truncated input would later read zeros
+ # instead of failing.
+ _ensure_available($reader, $size);
+ # Skip forward by $size bytes relative to the current position (SEEK_CUR);
+ # whence 0 (SEEK_SET) would incorrectly seek to the absolute offset $size.
+ # A failed seek means the reader cannot skip and is treated as fatal.
+ unless ($reader->seek($size, 1)) {
+ throw Avro::Schema::Error::Parse("Failed to skip $size bytes");
+ }
return;
}
sub decode_bytes {
my $class = shift;
my $reader = pop;
my $size = decode_long($class, undef, undef, $reader);
- $reader->read(my $buf, $size);
+ if ($size < 0) {
+ throw Avro::Schema::Error::Parse(
+ "Invalid negative bytes/string length: $size");
+ }
+ _ensure_available($reader, $size);
+ my $nread = $reader->read(my $buf, $size);
+ if (!defined $nread || $nread != $size) {
+ # A short read means the input is truncated; failing here avoids
+ # returning an under-sized buffer and misaligning the decoder.
+ throw Avro::Schema::Error::Parse(
+ "Expected $size bytes but read " . (defined $nread ? $nread : 0));
+ }
return $buf;
}
+## 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. Only enforced for larger reads and when the reader can
+## report its size via seek/tell; smaller reads and non-seekable readers fall
+## through to a direct read.
+use constant _MAX_UNCHECKED_READ => 1024 * 1024;
+
+## Maximum number of zero-byte-encoded collection elements (e.g. an array of
+## nulls) to allocate from a single decode. Such elements consume no input, so
+## the bytes-remaining check cannot bound their count; without a cap a tiny
+## payload can declare a huge block count and exhaust memory. Overridable via
the
+## AVRO_MAX_COLLECTION_ITEMS environment variable, or by setting
+## $Avro::BinaryDecoder::MAX_COLLECTION_ITEMS directly.
+our $DEFAULT_MAX_COLLECTION_ITEMS = 10_000_000;
+## Structural cap on the number of elements in any array or map (an
+## overflow/defense-in-depth guard), matching the historical
Integer.MAX_VALUE-8
+## limit. Non-zero-byte elements are also bounded by the bytes remaining.
+our $DEFAULT_MAX_COLLECTION_STRUCTURAL = (2 ** 31) - 8;
+
+## AVRO_MAX_COLLECTION_ITEMS, when set, caps both limits; otherwise zero-byte
+## elements use the tighter default and all collections use the structural cap.
+## Both may also be overridden directly (e.g. in tests).
+our $MAX_COLLECTION_ITEMS;
+our $MAX_COLLECTION_STRUCTURAL;
+if ( defined $ENV{AVRO_MAX_COLLECTION_ITEMS} &&
$ENV{AVRO_MAX_COLLECTION_ITEMS} =~ /\A[0-9]+\z/ ) {
+ $MAX_COLLECTION_ITEMS = $MAX_COLLECTION_STRUCTURAL =
$ENV{AVRO_MAX_COLLECTION_ITEMS} + 0;
+}
+else {
+ $MAX_COLLECTION_ITEMS = $DEFAULT_MAX_COLLECTION_ITEMS;
+ $MAX_COLLECTION_STRUCTURAL = $DEFAULT_MAX_COLLECTION_STRUCTURAL;
+}
+
+## Number of bytes still available to read, or undef 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. Readers
+## that do not support seeking to the end (e.g. a streaming decompressor)
return
+## undef, so the check is simply skipped for them.
+sub _bytes_remaining {
+ my ($reader) = @_;
+ my $current = eval { $reader->tell };
+ return if !defined $current || $current < 0;
+
+ # Attempt to seek to the end. Readers that cannot (e.g. a streaming
+ # decompressor) leave the position unchanged; skip the check for them.
+ my $moved = eval { $reader->seek(0, 2) }; # SEEK_END
+ return if !$moved;
+
+ # We are now at the end. Record the end offset, then ALWAYS restore the
+ # original position. A restore failure leaves the reader corrupted for
+ # subsequent decoding, so treat it as fatal rather than continuing.
+ my $end = eval { $reader->tell };
+ unless (eval { $reader->seek($current, 0) }) { # SEEK_SET
+ throw Avro::Schema::Error::Parse(
+ "Failed to restore reader position after size check");
+ }
+ return if !defined $end || $end < 0;
+ return $end - $current;
+}
+
+sub _ensure_available {
+ my ($reader, $size) = @_;
+ return if $size <= _MAX_UNCHECKED_READ;
+ my $remaining = _bytes_remaining($reader);
+ return unless defined $remaining;
+ if ($size > $remaining) {
+ throw Avro::Schema::Error::Parse(
+ "Cannot read $size bytes, only $remaining remaining");
+ }
+ return;
+}
+
+## 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 collection check for it (so an array of nulls is not falsely
+## rejected).
+sub _min_bytes_per_element {
+ my ($schema, $visited) = @_;
+ $visited ||= {};
+ my $type = $schema->type;
+ return 0 if $type eq 'null';
+ return 4 if $type eq 'float';
+ return 8 if $type eq 'double';
+ return $schema->size if $type eq 'fixed';
+ if ($type eq 'record' || $type eq 'error') {
+ my $id = "$schema"; # stringified reference as identity
+ return 0 if $visited->{$id};
+ $visited->{$id} = 1;
+ my $total = 0;
+ for my $field (@{ $schema->fields }) {
+ $total += _min_bytes_per_element($field->{type}, $visited);
+ }
+ delete $visited->{$id};
+ return $total;
+ }
+ # boolean, int, long, bytes, string, enum, union, array, map: >= 1 byte
+ # (a union encodes at least a 1-byte branch index).
+ return 1;
+}
+
+## 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.
+sub _ensure_collection_available {
+ my ($reader, $existing, $count, $min_bytes) = @_;
+ return if $count <= 0;
+ if ($min_bytes > 0) {
+ my $remaining = _bytes_remaining($reader);
+ # Compare via integer division rather than multiplying, so $count *
$min_bytes
+ # cannot overflow/wrap (notably on 32-bit builds) and defeat the check.
+ if (defined $remaining && $count > int($remaining / $min_bytes)) {
+ throw Avro::Schema::Error::Parse(
+ "Collection claims $count elements with at least $min_bytes
bytes each, "
+ . "but only $remaining bytes are available");
+ }
+ # Structural / overflow guard, also covering non-seekable readers where
+ # the bytes check above cannot run.
+ if ($existing + $count > $MAX_COLLECTION_STRUCTURAL) {
+ throw Avro::BinaryDecoder::Error::CollectionSize(
+ "Cannot read a collection of more than
$MAX_COLLECTION_STRUCTURAL "
+ . "elements (declared @{[ $existing + $count ]})");
+ }
+ }
+ # Zero-byte elements (e.g. null) consume no input, so they cannot be
bounded
+ # by the bytes remaining; cap their cumulative count instead.
+ elsif ($existing + $count > $MAX_COLLECTION_ITEMS) {
+ throw Avro::BinaryDecoder::Error::CollectionSize(
+ "Cannot read a collection of more than $MAX_COLLECTION_ITEMS
zero-byte "
+ . "elements (declared @{[ $existing + $count ]}); raise "
+ . "\$Avro::BinaryDecoder::MAX_COLLECTION_ITEMS if this is
legitimate");
+ }
Review Comment:
Fixed in f46ed3d75d: _ensure_collection_available now applies the structural
cap to every collection (including zero-byte elements) before the tighter
zero-byte item cap, so raising MAX_COLLECTION_ITEMS above
MAX_COLLECTION_STRUCTURAL can't bypass the structural guard.
##########
lang/perl/lib/Avro/BinaryDecoder.pm:
##########
@@ -124,18 +124,178 @@ sub skip_bytes {
my $class = shift;
my $reader = pop;
my $size = decode_long($class, undef, undef, $reader);
- $reader->seek($size, 0);
+ if ($size < 0) {
+ throw Avro::Schema::Error::Parse(
+ "Invalid negative bytes/string length: $size");
+ }
+ # Reject a declared length that exceeds the bytes actually remaining before
+ # skipping, mirroring decode_bytes: a seek past EOF silently succeeds on
+ # many handles, so without this a truncated input would later read zeros
+ # instead of failing.
+ _ensure_available($reader, $size);
+ # Skip forward by $size bytes relative to the current position (SEEK_CUR);
+ # whence 0 (SEEK_SET) would incorrectly seek to the absolute offset $size.
+ # A failed seek means the reader cannot skip and is treated as fatal.
+ unless ($reader->seek($size, 1)) {
+ throw Avro::Schema::Error::Parse("Failed to skip $size bytes");
+ }
return;
}
sub decode_bytes {
my $class = shift;
my $reader = pop;
my $size = decode_long($class, undef, undef, $reader);
- $reader->read(my $buf, $size);
+ if ($size < 0) {
+ throw Avro::Schema::Error::Parse(
+ "Invalid negative bytes/string length: $size");
+ }
+ _ensure_available($reader, $size);
+ my $nread = $reader->read(my $buf, $size);
+ if (!defined $nread || $nread != $size) {
+ # A short read means the input is truncated; failing here avoids
+ # returning an under-sized buffer and misaligning the decoder.
+ throw Avro::Schema::Error::Parse(
+ "Expected $size bytes but read " . (defined $nread ? $nread : 0));
+ }
return $buf;
}
+## 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. Only enforced for larger reads and when the reader can
+## report its size via seek/tell; smaller reads and non-seekable readers fall
+## through to a direct read.
+use constant _MAX_UNCHECKED_READ => 1024 * 1024;
+
+## Maximum number of zero-byte-encoded collection elements (e.g. an array of
+## nulls) to allocate from a single decode. Such elements consume no input, so
+## the bytes-remaining check cannot bound their count; without a cap a tiny
+## payload can declare a huge block count and exhaust memory. Overridable via
the
+## AVRO_MAX_COLLECTION_ITEMS environment variable, or by setting
+## $Avro::BinaryDecoder::MAX_COLLECTION_ITEMS directly.
+our $DEFAULT_MAX_COLLECTION_ITEMS = 10_000_000;
+## Structural cap on the number of elements in any array or map (an
+## overflow/defense-in-depth guard), matching the historical
Integer.MAX_VALUE-8
+## limit. Non-zero-byte elements are also bounded by the bytes remaining.
+our $DEFAULT_MAX_COLLECTION_STRUCTURAL = (2 ** 31) - 8;
Review Comment:
Fixed in f46ed3d75d: DEFAULT_MAX_COLLECTION_STRUCTURAL is now (2 ** 31) - 1
- 8 = 2147483639 (Integer.MAX_VALUE - 8).
##########
lang/perl/lib/Avro/BinaryDecoder.pm:
##########
@@ -215,27 +375,63 @@ sub decode_enum {
}
sub skip_block {
- my $class = shift;
- my ($reader, $block_content) = @_;
- my $block_count = decode_long($class, undef, undef, $reader);
+ # Called as a plain function by skip_array/skip_map as
+ # skip_block($reader, $min_bytes, $coderef); it is not a method, so there
is
+ # no leading class argument. decode_long ignores its (shifted) first
argument
+ # and reads from the last, so pass __PACKAGE__ for it and $reader last.
+ my ($reader, $min_bytes, $block_content) = @_;
+ # Zero-byte elements loop with no per-item input, so bound them tightly;
+ # other elements consume bytes (bounded by EOF) so only the structural cap
+ # applies.
+ my $limit = $min_bytes > 0 ? $MAX_COLLECTION_STRUCTURAL :
$MAX_COLLECTION_ITEMS;
Review Comment:
Fixed in f46ed3d75d: skip_block now bounds zero-byte skips by the tighter of
the item and structural limits (the structural cap applies to every collection).
--
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]
