Copilot commented on code in PR #3864:
URL: https://github.com/apache/avro/pull/3864#discussion_r3566902063
##########
lang/perl/lib/Avro/BinaryDecoder.pm:
##########
@@ -124,18 +124,179 @@ 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) - 1 - 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;
Review Comment:
_ensure_available() skips the bytes-remaining check for sizes <=
_MAX_UNCHECKED_READ. That’s fine for decode_bytes (it verifies short reads),
but it leaves skip paths that use seek() (skip_bytes/skip_fixed/skip_block
negative blocks) still able to seek past EOF for small sizes and continue
decoding (unsigned_varint() does not check read() return values, so EOF can be
interpreted as 0 bytes). Add an optional flag so skip paths can force the
availability check regardless of size, without changing decode_bytes behavior
for small reads.
##########
lang/perl/lib/Avro/BinaryDecoder.pm:
##########
@@ -124,18 +124,179 @@ 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);
Review Comment:
skip_bytes() relies on _ensure_available(), but _ensure_available currently
skips checks for <=1MiB. Because skip_bytes uses seek(), small truncated inputs
can still be skipped past EOF and later decoded as zeros. After making
_ensure_available support a forced check, pass the force flag here so
skip_bytes always rejects a declared length that exceeds remaining bytes when
the reader can report its size.
##########
lang/perl/lib/Avro/BinaryDecoder.pm:
##########
@@ -215,27 +376,67 @@ 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. Every collection is capped by the structural limit, so for
+ # zero-byte elements use whichever of the two limits is tighter.
+ my $limit = $MAX_COLLECTION_STRUCTURAL;
+ if ($min_bytes <= 0 && $MAX_COLLECTION_ITEMS < $limit) {
+ $limit = $MAX_COLLECTION_ITEMS;
+ }
+ my $block_count = decode_long(__PACKAGE__, undef, undef, $reader);
+ my $skipped = 0;
while ($block_count) {
if ($block_count < 0) {
- $reader->seek($block_count, 0);
- next;
+ # A negative count is followed by a long block size in bytes, which
+ # lets the whole block be skipped without decoding each item. Skip
+ # forward by that many bytes relative to the current position
+ # (SEEK_CUR); whence 0 (SEEK_SET) would seek to an absolute (here
+ # nonsensical) offset. A failed seek is treated as fatal.
+ my $block_size = decode_long(__PACKAGE__, undef, undef, $reader);
+ if ($block_size < 0) {
+ # A negative block size would seek the reader backwards,
+ # risking an infinite loop or mis-decoding of a corrupt input.
+ throw Avro::Schema::Error::Parse(
+ "Invalid negative block size: $block_size");
+ }
+ # Reject a block size that exceeds the bytes actually remaining
+ # before skipping, so a truncated input fails instead of seeking
+ # past EOF.
+ _ensure_available($reader, $block_size);
Review Comment:
In skip_block()’s negative-count path, _ensure_available() is called before
seeking over $block_size, but _ensure_available currently skips checks for
<=1MiB. Since this branch uses seek(), small truncated blocks can still be
skipped past EOF and subsequent decode_long() calls may interpret EOF as zeros.
After adding a force flag to _ensure_available, pass it here to always validate
remaining bytes for the skip.
##########
lang/perl/lib/Avro/BinaryDecoder.pm:
##########
@@ -350,7 +559,18 @@ sub decode_union {
sub skip_fixed {
my $class = shift;
my ($schema, $reader) = @_;
- $reader->seek($schema->size, 0);
+ # Reject a fixed size that exceeds the bytes actually remaining before
+ # skipping, mirroring skip_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, $schema->size);
Review Comment:
skip_fixed() calls _ensure_available() before seeking over the fixed
payload, but _ensure_available currently skips checks for <=1MiB. Since
skip_fixed uses seek(), small truncated fixed payloads can still be skipped
past EOF and later decoded as zeros. After adding a force flag to
_ensure_available, pass it here so the skip always validates remaining bytes
when possible.
--
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]
