[
https://issues.apache.org/jira/browse/THRIFT-6261?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Jens Geyer updated THRIFT-6261:
-------------------------------
Description:
{{lib/php/lib/Protocol/TBinaryProtocol.php}} rejects a container size below
zero in {{readMapBegin()}}, {{readListBegin()}} and {{readSetBegin()}}:
{code:php}
if ($size < 0) {
throw new TProtocolException('Negative size',
TProtocolException::NEGATIVE_SIZE);
}
{code}
{{lib/php/lib/Protocol/TCompactProtocol.php}} has no such check.
{{readMapBegin()}} ({{:519}}) takes its size straight from {{readVarint()}},
and {{readCollectionBegin()}} ({{:504}}), which serves both {{readListBegin()}}
and {{readSetBegin()}}, does the same for the long form:
{code:php}
$size = $sizeType >> 4;
$type = $this->getTType($sizeType);
if ($size == 15) {
$result += $this->readVarint($size);
}
{code}
A negative value is reachable, because {{readVarint()}} ({{:152}}) accumulates
into a PHP {{int}}:
{code:php}
while ($idx < self::MAX_VARINT_BYTES) { // 10
...
$result |= ($byte & 0x7f) << $shift;
if (($byte >> 7) === 0) { return $idx; }
$shift += 7;
}
{code}
On the tenth byte {{$shift}} is 63, so the sign bit of the 64-bit {{int}} is
set. Measured on PHP 8.1: the ten-byte varint {{FF FF FF FF FF FF FF FF FF 01}}
decodes to {{-1}}, and a generated {{for ($i = 0; $i < $size; $i++)}} loop then
runs **zero** iterations.
So the same input that makes {{TBinaryProtocol}} raise {{NEGATIVE_SIZE}} makes
{{TCompactProtocol}} return an empty container and carry on. The two protocols
should answer it the same way.
h2. Suggested fix
Add the {{NEGATIVE_SIZE}} check to {{readMapBegin()}} and
{{readCollectionBegin()}}, worded as in {{TBinaryProtocol}}.
_Drafted with AI assistance (Claude Opus 5); reviewed and filed by Jens Geyer._
was:
{{lib/php/lib/Protocol/TBinaryProtocol.php}} rejects a container size below
zero in {{readMapBegin()}}, {{readListBegin()}} and {{readSetBegin()}}:
{code:php}
if ($size < 0) {
throw new TProtocolException('Negative size',
TProtocolException::NEGATIVE_SIZE);
}
{code}
{{lib/php/lib/Protocol/TCompactProtocol.php}} has no such check.
{{readMapBegin()}} ({{:519}}) takes its size straight from {{readVarint()}},
and {{readCollectionBegin()}} ({{:504}}), which serves both {{readListBegin()}}
and {{readSetBegin()}}, does the same for the long form:
{code:php}
$size = $sizeType >> 4;
$type = $this->getTType($sizeType);
if ($size == 15) {
$result += $this->readVarint($size);
}
{code}
The size then reaches the generated {{read()}} loops, which run {{for ($i = 0;
$i < $size; ...)}} -- so a negative value silently yields an empty container
instead of the protocol error the binary protocol raises for the same input.
The two protocols should answer the same input the same way.
h2. Suggested fix
Add the {{NEGATIVE_SIZE}} check to {{readMapBegin()}} and
{{readCollectionBegin()}}, worded as in {{TBinaryProtocol}}.
_Drafted with AI assistance (Claude Opus 5); reviewed and filed by Jens Geyer._
> PHP: TCompactProtocol accepts a negative container size where TBinaryProtocol
> rejects it
> ----------------------------------------------------------------------------------------
>
> Key: THRIFT-6261
> URL: https://issues.apache.org/jira/browse/THRIFT-6261
> Project: Thrift
> Issue Type: Bug
> Components: PHP - Library
> Reporter: Jens Geyer
> Priority: Major
>
> {{lib/php/lib/Protocol/TBinaryProtocol.php}} rejects a container size below
> zero in {{readMapBegin()}}, {{readListBegin()}} and {{readSetBegin()}}:
> {code:php}
> if ($size < 0) {
> throw new TProtocolException('Negative size',
> TProtocolException::NEGATIVE_SIZE);
> }
> {code}
> {{lib/php/lib/Protocol/TCompactProtocol.php}} has no such check.
> {{readMapBegin()}} ({{:519}}) takes its size straight from {{readVarint()}},
> and {{readCollectionBegin()}} ({{:504}}), which serves both
> {{readListBegin()}} and {{readSetBegin()}}, does the same for the long form:
> {code:php}
> $size = $sizeType >> 4;
> $type = $this->getTType($sizeType);
> if ($size == 15) {
> $result += $this->readVarint($size);
> }
> {code}
> A negative value is reachable, because {{readVarint()}} ({{:152}})
> accumulates into a PHP {{int}}:
> {code:php}
> while ($idx < self::MAX_VARINT_BYTES) { // 10
> ...
> $result |= ($byte & 0x7f) << $shift;
> if (($byte >> 7) === 0) { return $idx; }
> $shift += 7;
> }
> {code}
> On the tenth byte {{$shift}} is 63, so the sign bit of the 64-bit {{int}} is
> set. Measured on PHP 8.1: the ten-byte varint {{FF FF FF FF FF FF FF FF FF
> 01}} decodes to {{-1}}, and a generated {{for ($i = 0; $i < $size; $i++)}}
> loop then runs **zero** iterations.
> So the same input that makes {{TBinaryProtocol}} raise {{NEGATIVE_SIZE}}
> makes {{TCompactProtocol}} return an empty container and carry on. The two
> protocols should answer it the same way.
> h2. Suggested fix
> Add the {{NEGATIVE_SIZE}} check to {{readMapBegin()}} and
> {{readCollectionBegin()}}, worded as in {{TBinaryProtocol}}.
> _Drafted with AI assistance (Claude Opus 5); reviewed and filed by Jens
> Geyer._
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
