Copilot commented on code in PR #3863:
URL: https://github.com/apache/avro/pull/3863#discussion_r3567636982
##########
lang/php/lib/Datum/AvroIOBinaryDecoder.php:
##########
@@ -235,28 +279,74 @@ public function skipRecord(AvroRecordSchema
$writersSchema, AvroIOBinaryDecoder
public function skipArray(AvroArraySchema $writersSchema,
AvroIOBinaryDecoder $decoder): void
{
+ $minBytes =
AvroIODatumReader::collectionElementMinBytes($writersSchema->items());
+ $skipped = 0;
$blockCount = $decoder->readLong();
- while (0 !== $blockCount) {
+ while (0 != $blockCount) {
+ $blockSize = null;
if ($blockCount < 0) {
- $decoder->skip($this->readLong());
+ if (PHP_INT_MIN == $blockCount) {
+ throw new AvroException('Invalid array block count');
+ }
+ $blockCount = -$blockCount;
+ $blockSize = $decoder->readLong();
+ if ($blockSize < 0) {
+ throw new AvroException('Invalid negative array block
size');
+ }
}
- for ($i = 0; $i < $blockCount; $i++) {
- AvroIODatumReader::skipData($writersSchema->items(), $decoder);
+ // Bound the (normalized) count on both the sized and unsized
paths so
+ // a negative block count cannot bypass the skip limit.
+ AvroIODatumReader::checkSkipCollectionCount($skipped, $blockCount,
$minBytes);
+ $skipped += $blockCount;
+ if (null !== $blockSize) {
+ // seek() can move past EOF, so a truncated/oversized block
would
+ // otherwise be "skipped" silently, hiding truncation. Reject a
+ // block size larger than the bytes actually remaining.
+ if ($blockSize > $decoder->bytesRemaining()) {
+ throw new AvroException('Array block size exceeds the
remaining input');
+ }
+ $decoder->skip($blockSize);
+ } else {
Review Comment:
In the negative-block form (blockCount < 0), skipArray() trusts the declared
block byte-size and skips exactly that many bytes. If a malicious/truncated
input provides a blockSize that is *smaller* than the minimum possible encoding
for blockCount elements, the skip will under-skip and desynchronize the decoder
without raising (e.g., array<long> with count=5, blockSize=0). Add a
lower-bound check using minBytes so undersized blocks are rejected early.
##########
lang/php/lib/Datum/AvroIOBinaryDecoder.php:
##########
@@ -235,28 +279,74 @@ public function skipRecord(AvroRecordSchema
$writersSchema, AvroIOBinaryDecoder
public function skipArray(AvroArraySchema $writersSchema,
AvroIOBinaryDecoder $decoder): void
{
+ $minBytes =
AvroIODatumReader::collectionElementMinBytes($writersSchema->items());
+ $skipped = 0;
$blockCount = $decoder->readLong();
- while (0 !== $blockCount) {
+ while (0 != $blockCount) {
+ $blockSize = null;
if ($blockCount < 0) {
- $decoder->skip($this->readLong());
+ if (PHP_INT_MIN == $blockCount) {
+ throw new AvroException('Invalid array block count');
+ }
+ $blockCount = -$blockCount;
+ $blockSize = $decoder->readLong();
+ if ($blockSize < 0) {
+ throw new AvroException('Invalid negative array block
size');
+ }
}
- for ($i = 0; $i < $blockCount; $i++) {
- AvroIODatumReader::skipData($writersSchema->items(), $decoder);
+ // Bound the (normalized) count on both the sized and unsized
paths so
+ // a negative block count cannot bypass the skip limit.
+ AvroIODatumReader::checkSkipCollectionCount($skipped, $blockCount,
$minBytes);
+ $skipped += $blockCount;
+ if (null !== $blockSize) {
+ // seek() can move past EOF, so a truncated/oversized block
would
+ // otherwise be "skipped" silently, hiding truncation. Reject a
+ // block size larger than the bytes actually remaining.
+ if ($blockSize > $decoder->bytesRemaining()) {
+ throw new AvroException('Array block size exceeds the
remaining input');
+ }
+ $decoder->skip($blockSize);
+ } else {
+ for ($i = 0; $i < $blockCount; $i++) {
+ AvroIODatumReader::skipData($writersSchema->items(),
$decoder);
+ }
}
$blockCount = $decoder->readLong();
}
}
public function skipMap(AvroMapSchema $writersSchema, AvroIOBinaryDecoder
$decoder): void
{
+ // Map entries always carry a >= 1 byte key, so the minimum is
positive.
+ $minBytes = 1 +
AvroIODatumReader::collectionElementMinBytes($writersSchema->values());
+ $skipped = 0;
$blockCount = $decoder->readLong();
- while (0 !== $blockCount) {
+ while (0 != $blockCount) {
+ $blockSize = null;
if ($blockCount < 0) {
- $decoder->skip($this->readLong());
+ if (PHP_INT_MIN == $blockCount) {
+ throw new AvroException('Invalid map block count');
+ }
+ $blockCount = -$blockCount;
+ $blockSize = $decoder->readLong();
+ if ($blockSize < 0) {
+ throw new AvroException('Invalid negative map block size');
+ }
}
- for ($i = 0; $i < $blockCount; $i++) {
- $decoder->skipString();
- AvroIODatumReader::skipData($writersSchema->values(),
$decoder);
+ AvroIODatumReader::checkSkipCollectionCount($skipped, $blockCount,
$minBytes);
+ $skipped += $blockCount;
+ if (null !== $blockSize) {
+ // seek() can move past EOF; reject a block size larger than
the
+ // bytes remaining so a truncated block isn't silently skipped.
+ if ($blockSize > $decoder->bytesRemaining()) {
+ throw new AvroException('Map block size exceeds the
remaining input');
+ }
+ $decoder->skip($blockSize);
+ } else {
Review Comment:
In the negative-block form (blockCount < 0), skipMap() skips the declared
block byte-size without verifying it is large enough to contain the declared
number of entries. An undersized blockSize can cause under-skipping and leave
the decoder misaligned without throwing. Add a conservative lower-bound check
based on the minimum bytes per entry ($minBytes) before skipping.
--
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]
