Copilot commented on code in PR #3856:
URL: https://github.com/apache/avro/pull/3856#discussion_r3564612118
##########
lang/php/lib/DataFile/AvroDataIOReader.php:
##########
@@ -220,16 +234,62 @@ private function readBlockHeader(): string|int
return $this->decoder->readLong();
}
+ /**
+ * The maximum number of bytes a single block is allowed to decompress to.
+ */
+ private static function maxDecompressLength(): int
+ {
+ $value = getenv(self::MAX_DECOMPRESS_LENGTH_ENV);
+ if (false !== $value && ctype_digit($value) && (int) $value > 0) {
+ return (int) $value;
+ }
+
+ return self::DEFAULT_MAX_DECOMPRESS_LENGTH;
+ }
+
+ /**
+ * @throws AvroDataIODecompressionSizeException if the length exceeds the
limit
+ */
+ private static function checkDecompressLength(int $length, int
$maxLength): void
+ {
+ if ($length > $maxLength) {
+ throw new AvroDataIODecompressionSizeException($maxLength);
+ }
+ }
+
/**
* @throws AvroException
*/
private function gzUncompress(string $compressed): string
{
- $datum = gzinflate($compressed);
+ $maxLength = self::maxDecompressLength();
+ $context = inflate_init(ZLIB_ENCODING_RAW);
+ if (false === $context) {
+ throw new AvroException('gzip uncompression failed.');
+ }
- if (false === $datum) {
+ // Inflate in chunks and check the running length after each step so an
+ // over-large (or malicious) block is rejected without materializing
the
+ // full output, while genuine decompression errors (inflate_add ===
false)
+ // are reported distinctly.
+ $datum = '';
+ $length = strlen($compressed);
+ for ($offset = 0; $offset < $length; $offset +=
self::INFLATE_CHUNK_SIZE) {
+ $piece = substr($compressed, $offset, self::INFLATE_CHUNK_SIZE);
+ $out = @inflate_add($context, $piece);
+ if (false === $out) {
+ throw new AvroException('gzip uncompression failed.');
+ }
+ $datum .= $out;
+ self::checkDecompressLength(strlen($datum), $maxLength);
+ }
+
+ $out = @inflate_add($context, '', ZLIB_FINISH);
+ if (false === $out) {
throw new AvroException('gzip uncompression failed.');
}
+ $datum .= $out;
+ self::checkDecompressLength(strlen($datum), $maxLength);
return $datum;
}
Review Comment:
gzUncompress() appends to $datum in a tight loop ("$datum .= $out"), which
repeatedly copies the whole accumulated string. With a large (up to 200 MiB)
decompressed block, this can become quadratic time/memory churn. Consider
accumulating chunks and tracking the decompressed length incrementally, then
implode once at the end.
##########
lang/php/lib/DataFile/AvroDataIOReader.php:
##########
@@ -276,17 +340,54 @@ private function snappyUncompress(string $compressed):
string
if (!extension_loaded('snappy')) {
throw new AvroException('Please install ext-snappy to use snappy
compression.');
}
+ $maxLength = self::maxDecompressLength();
+ // The Snappy block header declares the uncompressed length as a
varint;
+ // reject an over-large block before allocating for it. Parsed with an
+ // early cap so it stays correct even if a 32-bit int would overflow.
+ self::ensureSnappyWithinLimit(substr((string) $compressed, 0, -4),
$maxLength);
$crc32 = unpack('N', substr((string) $compressed, -4))[1];
$datum = snappy_uncompress(substr((string) $compressed, 0, -4));
if (false === $datum) {
throw new AvroException('snappy uncompression failed.');
}
+ self::checkDecompressLength(strlen($datum), $maxLength);
+
if ($crc32 !== crc32($datum)) {
throw new AvroException('snappy uncompression failed - crc32
mismatch.');
}
return $datum;
}
+
+ /**
+ * Reject a Snappy block whose declared uncompressed length (a
little-endian
+ * base-128 varint at the start of the block) exceeds $maxLength, before
+ * allocating for it. The running length is compared against the cap after
+ * every group, and any wrap to a negative value (32-bit int overflow) is
+ * treated as over the limit, so the guard holds on 32-bit builds too.
+ *
+ * @throws AvroDataIODecompressionSizeException if the declared length
exceeds the limit
+ */
+ private static function ensureSnappyWithinLimit(string $data, int
$maxLength): void
+ {
+ $result = 0;
+ $shift = 0;
+ $length = strlen($data);
+ for ($i = 0; $i < $length; $i++) {
+ $byte = ord($data[$i]);
+ $result += ($byte & 0x7F) << $shift;
+ if ($result < 0 || $result > $maxLength) {
+ throw new AvroDataIODecompressionSizeException($maxLength);
+ }
+ if (0 === ($byte & 0x80)) {
+ return; // declared length is within the limit
+ }
+ $shift += 7;
+ if ($shift > 28) {
+ return; // more than 5 bytes: malformed; the post-decompress
check will catch it
+ }
+ }
+ }
Review Comment:
ensureSnappyWithinLimit() returns early when the length varint is longer
than 5 bytes (or never terminates), which can bypass the declared-length cap
and allow snappy_uncompress() to allocate based on a huge/malformed length. For
a decompression-bomb guard, malformed/overlong varints should be rejected
before calling into the extension.
--
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]
