Copilot commented on code in PR #3864:
URL: https://github.com/apache/avro/pull/3864#discussion_r3565112875
##########
lang/perl/lib/Avro/BinaryDecoder.pm:
##########
@@ -132,10 +132,95 @@ sub decode_bytes {
my $class = shift;
my $reader = pop;
my $size = decode_long($class, undef, undef, $reader);
+ _ensure_available($reader, $size);
$reader->read(my $buf, $size);
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;
+
+## 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;
+ return eval {
+ $reader->seek(0, 2) # SEEK_END
+ or die "seek to end failed\n";
+ my $end = $reader->tell;
+ $reader->seek($current, 0); # SEEK_SET, restore position
+ die "unknown end position\n" if !defined $end || $end < 0;
+ $end - $current;
+ };
+}
Review Comment:
`_bytes_remaining` uses a single `eval` around the seek/tell/restore
sequence. If `tell` after seeking to end dies, or if restoring the position
fails, the function returns undef and the decoder may continue with the reader
left at EOF, corrupting subsequent decoding. The size check should (1) always
attempt to restore the original position and (2) treat restore failure as a
hard error rather than silently skipping the check.
##########
lang/perl/lib/Avro/BinaryDecoder.pm:
##########
@@ -296,13 +383,16 @@ sub decode_map {
my $block_count = decode_long($class, @_);
my $writer_values = $writer_schema->values;
my $reader_values = $reader_schema->values;
+ # Map keys are strings (>= 1 byte length prefix) plus the value.
+ my $min_bytes = 1 + _min_bytes_per_element($writer_values);
Review Comment:
The map block size check currently assumes keys add only 1 byte (the length
prefix), but this decoder later rejects empty keys (`length $key` check). Given
that constraint, the minimum on-wire key encoding is at least 2 bytes (1-byte
length prefix + 1 byte data). Using 1 here makes the pre-check weaker than it
can be and may miss some truncated-map cases that could be rejected before
iterating.
##########
lang/perl/lib/Avro/BinaryDecoder.pm:
##########
@@ -132,10 +132,95 @@ sub decode_bytes {
my $class = shift;
my $reader = pop;
my $size = decode_long($class, undef, undef, $reader);
+ _ensure_available($reader, $size);
$reader->read(my $buf, $size);
Review Comment:
`decode_bytes` trusts the decoded length prefix and will pass negative sizes
through to `read`, which can cause incorrect behavior / misalignment. Avro
bytes/string lengths must be non-negative, so reject a negative decoded length
explicitly before continuing.
--
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]
