-
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 5 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(); | ||
|
|
@@ -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) { | ||
|
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: skipArray uses a non-strict comparison (0 != $blockCount), so a GMP numeric-string '0' terminates the loop on 32-bit builds. |
||
| if ($blockCount < 0) { | ||
| $decoder->skip($this->readLong()); | ||
| } | ||
| for ($i = 0; $i < $blockCount; $i++) { | ||
| AvroIODatumReader::skipData($writersSchema->items(), $decoder); | ||
| } else { | ||
|
Comment on lines
+313
to
+324
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 — skipArray now rejects a block byte-size larger than
Comment on lines
+317
to
+324
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 — skipArray now also rejects a block size too small to hold blockCount elements at their minimum on-wire size ( |
||
| 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) { | ||
|
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: skipMap got the same non-strict comparison. |
||
| if ($blockCount < 0) { | ||
| $decoder->skip($this->readLong()); | ||
| } | ||
| for ($i = 0; $i < $blockCount; $i++) { | ||
| $decoder->skipString(); | ||
| AvroIODatumReader::skipData($writersSchema->values(), $decoder); | ||
| } else { | ||
|
Comment on lines
+353
to
+363
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 — skipMap likewise validates the block byte-size against
Comment on lines
+356
to
+363
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 — skipMap has the same block-size-vs-count check before skipping. |
||
| AvroIODatumReader::checkSkipCollectionCount($skipped, $blockCount, $minBytes); | ||
| $skipped += $blockCount; | ||
| for ($i = 0; $i < $blockCount; $i++) { | ||
| $decoder->skipString(); | ||
| AvroIODatumReader::skipData($writersSchema->values(), $decoder); | ||
| } | ||
| } | ||
| $blockCount = $decoder->readLong(); | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,43 @@ | ||
| <?php | ||
|
|
||
| /** | ||
| * Licensed to the Apache Software Foundation (ASF) under one | ||
| * or more contributor license agreements. See the NOTICE file | ||
| * distributed with this work for additional information | ||
| * regarding copyright ownership. The ASF licenses this file | ||
| * to you under the Apache License, Version 2.0 (the | ||
| * "License"); you may not use this file except in compliance | ||
| * with the License. You may obtain a copy of the License at | ||
| * | ||
| * https://www.apache.org/licenses/LICENSE-2.0 | ||
| * | ||
| * Unless required by applicable law or agreed to in writing, software | ||
| * distributed under the License is distributed on an "AS IS" BASIS, | ||
| * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| * See the License for the specific language governing permissions and | ||
| * limitations under the License. | ||
| */ | ||
|
|
||
| declare(strict_types=1); | ||
|
|
||
| namespace Apache\Avro\Datum; | ||
|
|
||
| use Apache\Avro\AvroException; | ||
|
|
||
| /** | ||
| * Raised when an array or map declares more items than the configured maximum. | ||
| * | ||
| * The block count of an array or map is read from the (potentially untrusted or | ||
| * truncated) input and drives allocation of the resulting collection. This | ||
| * exception guards against unbounded memory allocation from a very large or | ||
| * malformed block count. | ||
| */ | ||
| class AvroIOCollectionSizeException extends AvroException | ||
| { | ||
| public function __construct(int $maxItems) | ||
| { | ||
| parent::__construct( | ||
| sprintf('Cannot read collections larger than %d items.', $maxItems) | ||
| ); | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -43,6 +43,29 @@ | |
| */ | ||
| class AvroIODatumReader | ||
| { | ||
| /** | ||
| * Name of the environment variable overriding the maximum number of | ||
| * zero-byte-encoded collection elements (e.g. an array of nulls) to | ||
| * allocate from a single decode. | ||
| */ | ||
| public const MAX_COLLECTION_ITEMS_ENV = 'AVRO_MAX_COLLECTION_ITEMS'; | ||
|
|
||
| /** | ||
| * Default maximum number of zero-byte-encoded collection elements to | ||
| * allocate. 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 with the | ||
| * {@see self::MAX_COLLECTION_ITEMS_ENV} environment variable. | ||
| */ | ||
| public const DEFAULT_MAX_COLLECTION_ITEMS = 10000000; | ||
|
|
||
| /** | ||
| * 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. | ||
| */ | ||
| public const DEFAULT_MAX_COLLECTION_STRUCTURAL = 2147483639; | ||
|
|
||
| public function __construct( | ||
| private ?AvroSchema $writersSchema = null, | ||
| private ?AvroSchema $readersSchema = null | ||
|
|
@@ -284,12 +307,14 @@ public function readArray( | |
| AvroIOBinaryDecoder $decoder | ||
| ): array { | ||
| $items = []; | ||
| $minBytes = self::minBytesPerElement($writersSchema->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, count($items), $blockCount, $minBytes); | ||
| 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(), | ||
|
|
@@ -312,6 +337,7 @@ public function readMap( | |
| AvroIOBinaryDecoder $decoder | ||
| ): array { | ||
| $items = []; | ||
| $minBytes = 1 + self::minBytesPerElement($writersSchema->values()); | ||
| $pair_count = $decoder->readLong(); | ||
| while (0 != $pair_count) { | ||
| if ($pair_count < 0) { | ||
|
|
@@ -320,6 +346,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, count($items), $pair_count, $minBytes); | ||
| 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( | ||
|
|
@@ -530,6 +558,34 @@ public function readDefaultValue(AvroSchema $fieldSchema, mixed $defaultValue): | |
| } | ||
| } | ||
|
|
||
| /** | ||
| * Minimum on-wire size of a collection element schema. Exposed for the skip | ||
| * path, which lives in the decoder. | ||
| */ | ||
| public static function collectionElementMinBytes(AvroSchema $schema): int | ||
| { | ||
| return self::minBytesPerElement($schema); | ||
| } | ||
|
|
||
| /** | ||
| * Bounds the cumulative number of elements skipped in an array or map, so a | ||
| * huge block of zero-byte elements cannot loop unboundedly (a CPU | ||
| * exhaustion) even though skipping allocates nothing. | ||
| * | ||
| * @throws AvroIOCollectionSizeException if the limit is exceeded | ||
| */ | ||
| public static function checkSkipCollectionCount(int $existing, int $count, int $minBytes): void | ||
| { | ||
| if ($count <= 0) { | ||
| return; | ||
| } | ||
| [$zeroByteLimit, $structuralLimit] = self::collectionLimits(); | ||
| $limit = $minBytes > 0 ? $structuralLimit : $zeroByteLimit; | ||
| if ($count > $limit || $existing > $limit - $count) { | ||
| throw new AvroIOCollectionSizeException($limit); | ||
| } | ||
| } | ||
|
|
||
| private function readDecimal(string $bytes, int $scale): string | ||
| { | ||
| $mostSignificantBit = ord($bytes[0]) & 0x80; | ||
|
|
@@ -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; | ||
|
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; | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Returns the configured collection limits as [zeroByteLimit, structuralLimit]. | ||
| * AVRO_MAX_COLLECTION_ITEMS, when a non-negative integer, caps both. | ||
| * | ||
| * @return array{int, int} | ||
| */ | ||
|
Comment on lines
+690
to
+696
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: reworded — the env var overrides both limits to the given value (which may raise or lower them), not only cap them. |
||
| private static function collectionLimits(): array | ||
| { | ||
| $value = getenv(self::MAX_COLLECTION_ITEMS_ENV); | ||
| if (false !== $value && '' !== $value && preg_match('/^-?\d+$/', $value)) { | ||
| $parsed = (int) $value; | ||
| if ($parsed >= 0) { | ||
| return [$parsed, $parsed]; | ||
| } | ||
| } | ||
|
|
||
| return [self::DEFAULT_MAX_COLLECTION_ITEMS, self::DEFAULT_MAX_COLLECTION_STRUCTURAL]; | ||
| } | ||
|
|
||
| /** | ||
| * Rejects a collection (array or map) block that could not be backed by the | ||
| * input, before iterating. | ||
| * | ||
| * For elements with a positive minimum on-wire size, the declared count is | ||
| * checked against the bytes remaining and a structural cap. For zero-byte | ||
| * elements (e.g. an array of nulls), which consume no input and so cannot be | ||
| * bounded by the bytes remaining, the cumulative count is checked against the | ||
| * tighter zero-byte limit. | ||
| * | ||
| * @throws AvroException if the bytes-remaining check fails | ||
| * @throws AvroIOCollectionSizeException if a size limit is exceeded | ||
| */ | ||
| private static function ensureCollectionAvailable( | ||
| AvroIOBinaryDecoder $decoder, | ||
| int $existing, | ||
| int $count, | ||
| int $minBytesPerElement | ||
| ): void { | ||
| if ($count <= 0) { | ||
| return; | ||
| } | ||
| [$zeroByteLimit, $structuralLimit] = self::collectionLimits(); | ||
| if ($minBytesPerElement > 0) { | ||
| $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." | ||
| ); | ||
| } | ||
| if ($count > $structuralLimit || $existing > $structuralLimit - $count) { | ||
| throw new AvroIOCollectionSizeException($structuralLimit); | ||
| } | ||
|
|
||
| return; | ||
| } | ||
| if ($count > $zeroByteLimit || $existing > $zeroByteLimit - $count) { | ||
| throw new AvroIOCollectionSizeException($zeroByteLimit); | ||
| } | ||
| } | ||
| } | ||
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().