iemejia commented on code in PR #3863:
URL: https://github.com/apache/avro/pull/3863#discussion_r3566776735
##########
lang/php/lib/Datum/AvroIOBinaryDecoder.php:
##########
@@ -235,28 +273,39 @@ 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) {
Review Comment:
Fixed in 23f15e04f4: skipArray uses a non-strict comparison (0 !=
$blockCount), so a GMP numeric-string '0' terminates the loop on 32-bit builds.
##########
lang/php/lib/Datum/AvroIOBinaryDecoder.php:
##########
@@ -235,28 +273,39 @@ 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) {
if ($blockCount < 0) {
$decoder->skip($this->readLong());
- }
- for ($i = 0; $i < $blockCount; $i++) {
- AvroIODatumReader::skipData($writersSchema->items(), $decoder);
+ } else {
+ AvroIODatumReader::checkSkipCollectionCount($skipped,
$blockCount, $minBytes);
+ $skipped += $blockCount;
+ 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) {
Review Comment:
Fixed in 23f15e04f4: skipMap got the same non-strict comparison.
##########
lang/php/lib/Datum/AvroIODatumReader.php:
##########
@@ -538,4 +594,118 @@ private function readDecimal(string $bytes, int $scale):
string
return (string) ($scale > 0 ? ($int / (10 ** $scale)) : $int);
}
+
+ /**
+ * 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. It returns 0 for any schema that can encode to
+ * zero bytes: the null primitive, but also a record with no fields or
whose
+ * fields all encode to zero bytes. A zero return disables the collection
+ * check for that element type (so, e.g., an array of nulls is not falsely
+ * rejected). Types that cannot be resolved cheaply default to 1.
+ *
+ * @param array<int, bool> $visited
+ */
+ private static function minBytesPerElement(mixed $schema, array $visited =
[]): int
+ {
+ $type = $schema instanceof AvroSchema ? $schema->type() : $schema;
+ // Named/complex field types may nest a schema object; unwrap one
level.
+ if ($type instanceof AvroSchema) {
+ return self::minBytesPerElement($type, $visited);
+ }
+ if (!is_string($type)) {
+ return 1;
+ }
+ switch ($type) {
+ case AvroSchema::NULL_TYPE:
+ return 0;
+ case AvroSchema::FLOAT_TYPE:
+ return 4;
+ case AvroSchema::DOUBLE_TYPE:
+ return 8;
+ case AvroSchema::FIXED_SCHEMA:
+ return $schema instanceof AvroFixedSchema ? $schema->size() :
1;
+ case AvroSchema::RECORD_SCHEMA:
+ case AvroSchema::ERROR_SCHEMA:
+ if (!$schema instanceof AvroRecordSchema) {
+ return 1;
+ }
+ $id = spl_object_id($schema);
+ if (isset($visited[$id])) {
+ return 1; // self-referencing schema: safe lower bound of
1 byte
+ }
+ $visited[$id] = true;
+ $total = 0;
+ foreach ($schema->fields() as $field) {
+ $total += self::minBytesPerElement($field, $visited);
+ }
Review Comment:
Made explicit in 23f15e04f4 (now traverses via `$field->type()`). Note this
was already correct at runtime: AvroField extends AvroSchema, so
minBytesPerElement(field) used `$field->type()` internally and a record of null
fields returned 0 (verified) — but `$field->type()` is clearer.
##########
lang/php/lib/Datum/AvroIODatumReader.php:
##########
@@ -320,6 +346,8 @@ public function readMap(
$decoder->readLong();
}
+ // Map keys are strings (>= 1 byte length prefix) plus the value.
+ self::ensureCollectionAvailable($decoder, count($items),
$pair_count, $minBytes);
for ($i = 0; $i < $pair_count; $i++) {
Review Comment:
Fixed in 23f15e04f4: readMap now bounds against a separate cumulative
counter rather than count($items), so repeating a key can't shrink the count
and slip past the cap. Added a regression test.
##########
lang/php/lib/Datum/AvroIODatumReader.php:
##########
@@ -538,4 +594,118 @@ private function readDecimal(string $bytes, int $scale):
string
return (string) ($scale > 0 ? ($int / (10 ** $scale)) : $int);
}
+
+ /**
+ * 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. It returns 0 for any schema that can encode to
+ * zero bytes: the null primitive, but also a record with no fields or
whose
+ * fields all encode to zero bytes. A zero return disables the collection
+ * check for that element type (so, e.g., an array of nulls is not falsely
+ * rejected). Types that cannot be resolved cheaply default to 1.
+ *
+ * @param array<int, bool> $visited
+ */
+ private static function minBytesPerElement(mixed $schema, array $visited =
[]): int
+ {
+ $type = $schema instanceof AvroSchema ? $schema->type() : $schema;
+ // Named/complex field types may nest a schema object; unwrap one
level.
+ if ($type instanceof AvroSchema) {
+ return self::minBytesPerElement($type, $visited);
+ }
+ if (!is_string($type)) {
+ return 1;
+ }
+ switch ($type) {
+ case AvroSchema::NULL_TYPE:
+ return 0;
+ case AvroSchema::FLOAT_TYPE:
+ return 4;
+ case AvroSchema::DOUBLE_TYPE:
+ return 8;
+ case AvroSchema::FIXED_SCHEMA:
+ return $schema instanceof AvroFixedSchema ? $schema->size() :
1;
+ case AvroSchema::RECORD_SCHEMA:
+ case AvroSchema::ERROR_SCHEMA:
+ if (!$schema instanceof AvroRecordSchema) {
+ return 1;
+ }
+ $id = spl_object_id($schema);
+ if (isset($visited[$id])) {
+ return 1; // self-referencing schema: safe lower bound of
1 byte
+ }
+ $visited[$id] = true;
+ $total = 0;
+ foreach ($schema->fields() as $field) {
+ $total += self::minBytesPerElement($field, $visited);
+ }
+
+ return $total;
+ default:
+ // boolean, int, long, bytes, string, enum, union, array, map:
+ // all encode to at least one byte.
+ return 1;
+ }
+ }
+
+ /**
+ * Returns the configured collection limits as [zeroByteLimit,
structuralLimit].
+ * AVRO_MAX_COLLECTION_ITEMS, when a non-negative integer, caps both.
+ *
+ * @return array{int, int}
+ */
Review Comment:
Fixed in 23f15e04f4: reworded — the env var overrides both limits to the
given value (which may raise or lower them), not only cap them.
##########
lang/php/test/DatumIOTest.php:
##########
@@ -261,6 +262,225 @@ public function
test_invalid_bytes_logical_type_out_of_range(): void
$writer->write("10000", $encoder);
}
+ // A bytes/string value declares a length prefix; a malicious or truncated
+ // input can declare far more bytes than actually exist. On a reader that
+ // can report its size, that is rejected before allocating for it.
+ public function test_read_bytes_rejects_length_beyond_stream(): void
+ {
+ $io = new AvroStringIO();
+ (new AvroIOBinaryEncoder($io))->writeLong(100 * 1024 * 1024);
+ $io->seek(0);
+ $decoder = new AvroIOBinaryDecoder($io);
+
+ $this->expectException(AvroException::class);
+ $decoder->readBytes();
+ }
+
+ public function test_read_bytes_within_stream_still_reads(): void
+ {
+ $payload = str_repeat('x', 2 * 1024 * 1024);
+ $io = new AvroStringIO();
+ (new AvroIOBinaryEncoder($io))->writeBytes($payload);
+ $io->seek(0);
+ $decoder = new AvroIOBinaryDecoder($io);
+
+ $this->assertEquals($payload, $decoder->readBytes());
+ }
+
+ public function test_read_array_rejects_count_beyond_stream(): void
+ {
+ $io = new AvroStringIO();
+ (new AvroIOBinaryEncoder($io))->writeLong(1000000); // 1,000,000
longs, no data
+ $this->expectException(AvroException::class);
+ $this->decodeWith('{"type":"array","items":"long"}', $io);
+ }
+
+ public function test_read_map_rejects_count_beyond_stream(): void
+ {
+ $io = new AvroStringIO();
+ (new AvroIOBinaryEncoder($io))->writeLong(1000000);
+ $this->expectException(AvroException::class);
+ $this->decodeWith('{"type":"map","values":"long"}', $io);
+ }
+
+ public function test_read_array_of_null_not_falsely_rejected(): void
+ {
+ $count = 100000;
+ $io = new AvroStringIO();
+ $encoder = new AvroIOBinaryEncoder($io);
+ $encoder->writeLong($count); // one block of `count` nulls (zero bytes
each)
+ $encoder->writeLong(0); // end-of-array marker
+ $result = $this->decodeWith('{"type":"array","items":"null"}', $io);
+ $this->assertCount($count, $result);
+ }
+
+ public function test_read_array_within_stream_still_reads(): void
+ {
+ $schema = AvroSchema::parse('{"type":"array","items":"long"}');
+ $io = new AvroStringIO();
+ (new AvroIODatumWriter($schema))->write([1, 2, 3], new
AvroIOBinaryEncoder($io));
+ $this->assertEquals([1, 2, 3],
$this->decodeWith('{"type":"array","items":"long"}', $io));
+ }
+
+ public function test_array_of_null_exceeds_default_limit(): void
+ {
+ // The reported exploit: a ~6 byte payload declaring 200,000,000 nulls.
+ $this->expectException(AvroIOCollectionSizeException::class);
+ $this->decodeWith('{"type":"array","items":"null"}',
self::zeroByteBlock(200000000));
+ }
+
+ public function test_array_of_null_within_configured_limit_still_reads():
void
+ {
+ putenv('AVRO_MAX_COLLECTION_ITEMS=1000');
+
+ try {
+ $result = $this->decodeWith('{"type":"array","items":"null"}',
self::zeroByteBlock(1000));
+ $this->assertCount(1000, $result);
+ } finally {
+ putenv('AVRO_MAX_COLLECTION_ITEMS');
+ }
+ }
+
+ public function test_array_of_null_exceeds_configured_limit(): void
+ {
+ putenv('AVRO_MAX_COLLECTION_ITEMS=1000');
+
+ try {
+ $this->expectException(AvroException::class);
Review Comment:
Fixed in 23f15e04f4: now asserts AvroIOCollectionSizeException.
--
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]
