-
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 15 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(); | ||
|
|
@@ -78,6 +116,12 @@ public function readLong(): string|int | |
| $byte = ord($this->nextByte()); | ||
| $bytes = [$byte]; | ||
| while (0 != ($byte & 0x80)) { | ||
| // A 64-bit value uses at most 10 bytes; reject an overlong varint | ||
| // rather than reading an unbounded continuation chain and silently | ||
| // corrupting the value. Bounds both the native and GMP decode paths. | ||
| if (count($bytes) >= 10) { | ||
| throw new AvroException('Varint is too long'); | ||
| } | ||
| $byte = ord($this->nextByte()); | ||
| $bytes[] = $byte; | ||
| } | ||
|
|
@@ -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 { | ||
|
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 ( |
||
| 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 { | ||
|
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. |
||
| 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) | ||
| ); | ||
| } | ||
| } |
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().