-
Notifications
You must be signed in to change notification settings - Fork 1.8k
AVRO-4298: [php] Bound allocation when decoding length-prefixed values and collections #3863
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 4 commits
92e0e5d
5e997eb
f3a4764
bd6bb27
3d7f736
695a4b2
23f15e0
063699d
542afad
2db3fd1
0d02b5b
8d2b1cb
f6ffd01
f448112
33c9885
c5a8634
c5e93a3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -38,6 +38,14 @@ | |
| */ | ||
| class AvroIOBinaryDecoder | ||
| { | ||
| /** | ||
| * Reads with a declared length above this many bytes are validated against | ||
| * the number of bytes actually remaining before allocating, to guard | ||
| * against an out-of-memory attack from a malicious or truncated input. | ||
| * Smaller reads skip the check to avoid per-value overhead. | ||
| */ | ||
| private const MAX_UNCHECKED_READ = 1048576; // 1 MiB | ||
|
|
||
| /** | ||
| * @param AvroIO $io object from which to read. | ||
| */ | ||
|
|
@@ -65,9 +73,39 @@ public function readBoolean(): bool | |
| */ | ||
| public function read(int $len): string | ||
| { | ||
| if ($len < 0) { | ||
| // AvroStringIO::read() accepts a negative length and moves the | ||
| // pointer backwards; reject it before delegating. | ||
| throw new AvroException("Cannot read a negative number of bytes: {$len}"); | ||
| } | ||
| if ($len > self::MAX_UNCHECKED_READ) { | ||
| $remaining = $this->bytesRemaining(); | ||
| if ($len > $remaining) { | ||
| throw new AvroException("Cannot read {$len} bytes, only {$remaining} remaining."); | ||
| } | ||
| } | ||
|
|
||
| return $this->io->read($len); | ||
| } | ||
|
|
||
| /** | ||
| * Number of bytes still available to read, determined by seeking to the end | ||
| * and restoring the position (which both the string and file IO | ||
| * implementations support). Used to reject a declared length or collection | ||
| * block count that exceeds the data actually available before allocating. | ||
| */ | ||
| public function bytesRemaining(): int | ||
| { | ||
| $current = $this->io->tell(); | ||
| $this->io->seek(0, AvroIO::SEEK_END); | ||
| $end = $this->io->tell(); | ||
| $this->io->seek($current, AvroIO::SEEK_SET); | ||
|
|
||
| // Clamp to 0: AvroStringIO::seek() allows seeking past EOF, which would | ||
| // otherwise yield a confusing negative "remaining" count. | ||
| return max(0, $end - $current); | ||
| } | ||
|
Comment on lines
+111
to
+119
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed in f3a4764: bytesRemaining() clamps to 0 via max(0, ...), so a position past EOF no longer yields a negative remaining count. |
||
|
|
||
| public function readInt(): int | ||
| { | ||
| return (int) $this->readLong(); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -285,11 +285,12 @@ public function readArray( | |
| ): array { | ||
| $items = []; | ||
| $blockCount = $decoder->readLong(); | ||
| while (0 !== $blockCount) { | ||
| while (0 != $blockCount) { | ||
| if ($blockCount < 0) { | ||
| $blockCount = -$blockCount; | ||
| $decoder->readLong(); // Read (and ignore) block size | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed in 2db3fd1. The array block-size long is now validated and a negative value is rejected with |
||
| } | ||
| self::ensureCollectionAvailable($decoder, $blockCount, self::minBytesPerElement($writersSchema->items())); | ||
| for ($i = 0; $i < $blockCount; $i++) { | ||
|
Comment on lines
329
to
331
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed in 5e997eb: readArray now uses a non-strict comparison (0 != blockCount), matching readMap, so a GMP numeric-string block count of '0' terminates the loop on 32-bit builds. |
||
| $items[] = $this->readData( | ||
| $writersSchema->items(), | ||
|
|
@@ -320,6 +321,8 @@ public function readMap( | |
| $decoder->readLong(); | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed in 2db3fd1. The map block-size long is now validated and rejected if negative, consistent with skipMap(). |
||
| } | ||
|
|
||
| // Map keys are strings (>= 1 byte length prefix) plus the value. | ||
| self::ensureCollectionAvailable($decoder, $pair_count, 1 + self::minBytesPerElement($writersSchema->values())); | ||
| for ($i = 0; $i < $pair_count; $i++) { | ||
|
Comment on lines
+370
to
376
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed in 23f15e0: 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. |
||
| $key = $decoder->readString(); | ||
| $items[$key] = $this->readData( | ||
|
|
@@ -538,4 +541,81 @@ 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; | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed — minBytesPerElement now returns |
||
| 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 | ||
| } | ||
|
Comment on lines
+672
to
+675
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed in f3a4764: minBytesPerElement() returns 1 (not 0) on a self-referencing cycle, keeping the guard conservative for arrays/maps of recursive records. |
||
| $visited[$id] = true; | ||
| $total = 0; | ||
| foreach ($schema->fields() as $field) { | ||
| $total += self::minBytesPerElement($field, $visited); | ||
| } | ||
|
Comment on lines
+678
to
+680
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Made explicit in 23f15e0 (now traverses via |
||
|
|
||
| return $total; | ||
| default: | ||
| // boolean, int, long, bytes, string, enum, union, array, map: | ||
| // all encode to at least one byte. | ||
| return 1; | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Rejects 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 (e.g. an array of nulls). | ||
| * | ||
| * @throws AvroException if the declared count exceeds the bytes remaining | ||
| */ | ||
| private static function ensureCollectionAvailable( | ||
| AvroIOBinaryDecoder $decoder, | ||
| int $count, | ||
| int $minBytesPerElement | ||
| ): void { | ||
| if ($count <= 0 || $minBytesPerElement <= 0) { | ||
| return; | ||
| } | ||
| $remaining = $decoder->bytesRemaining(); | ||
| if ($count > intdiv($remaining, $minBytesPerElement)) { | ||
| throw new AvroException( | ||
| "Collection claims {$count} elements with at least {$minBytesPerElement} " | ||
| ."bytes each, but only {$remaining} bytes are available." | ||
| ); | ||
| } | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in 5e997eb: read(int) now rejects a negative length before delegating to AvroIO::read().