Welcome! Log In Create A New Profile

Advanced

[PHP-DEV] Re: [PHP-CVS] com php-src: Improved the fix for bug #67072, thanks Nikita: ext/standard/tests/serialize/005.phpt ext/standard/tests/serialize/bug67072.phpt ext/standard/var_unserializer.c ext/standard/var_unserializer.re

Posted by Stas Malyshev 
Hi!

>> Commit: c2acdbdd3deb6787329bf0aca8ab0c04ace2a50c
>> Author: Anatol Belski <[email protected]> Fri, 18 Apr 2014 15:13:32
>> +0200
>> Parents: 6e1e98d7b833492594aea9cf416905b42f8ee0f4
>> Branches: PHP-5.4 PHP-5.5 PHP-5.6 master
>>
>> Link:
>> http://git.php.net/?p=php-src.git;a=commitdiff;h=c2acdbdd3deb6787329bf0aca8ab0c04ace2a50c
>>
>> Log:
>> Improved the fix for bug #67072, thanks Nikita
>>
>> Bugs:
>> https://bugs.php.net/67072
> Just ran into an issue running Symfony unit tests with PHPUnit. The issue
> is this piece of code:
>
> // We have to use this dirty trick instead of
> ReflectionClass::newInstanceWithoutConstructor()
> // because of
> https://github.com/sebastianbergmann/phpunit-mock-objects/issues/154
> $object = unserialize(
> sprintf('O:%d:"%s":0:{}', strlen($className), $className)
> );
>
> This ends up using the O: serialization type on a Serializable class, thus
> causing a warning. Not sure what we should do about this.

This looks like a BC break, we can not have that in 5.4 and 5.5. We'll
have to fix or revert this.
--
Stanislav Malyshev, Software Architect
SugarCRM: http://www.sugarcrm.com/
(408)454-6900 ext. 227

--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php
Hello,

On Mon, May 5, 2014 22:21, Stas Malyshev wrote:
> Hi!
>
>
>>> Commit: c2acdbdd3deb6787329bf0aca8ab0c04ace2a50c
>>> Author: Anatol Belski <[email protected]> Fri, 18 Apr 2014
>>> 15:13:32
>>> +0200
>>> Parents: 6e1e98d7b833492594aea9cf416905b42f8ee0f4
>>> Branches: PHP-5.4 PHP-5.5 PHP-5.6 master
>>>
>>>
>>> Link:
>>> http://git.php.net/?p=php-src.git;a=commitdiff;h=c2acdbdd3deb6787329bf
>>> 0aca8ab0c04ace2a50c
>>>
>>>
>>> Log:
>>> Improved the fix for bug #67072, thanks Nikita
>>>
>>>
>>> Bugs:
>>> https://bugs.php.net/67072
>>>
>> Just ran into an issue running Symfony unit tests with PHPUnit. The
>> issue is this piece of code:
>>
>> // We have to use this dirty trick instead of
>> ReflectionClass::newInstanceWithoutConstructor()
>> // because of
>> https://github.com/sebastianbergmann/phpunit-mock-objects/issues/154
>> $object = unserialize(
>> sprintf('O:%d:"%s":0:{}', strlen($className), $className)
>> );
>>
>>
>> This ends up using the O: serialization type on a Serializable class,
>> thus causing a warning. Not sure what we should do about this.
>
> This looks like a BC break, we can not have that in 5.4 and 5.5. We'll
> have to fix or revert this. --
> Stanislav Malyshev, Software Architect
> SugarCRM: http://www.sugarcrm.com/
> (408)454-6900 ext. 227
>
>
> --
> PHP Internals - PHP Runtime Development Mailing List
> To unsubscribe, visit: http://www.php.net/unsub.php
>
>
>
The initial bug is that an unserialization string misuses "C:" vs "O:"
format which would call an incorrect method "unserialize" vs. "__wakeup".
And that leads to a crash with that particular ticket, due to an
incompletely initialized object. Formally this fix just brings the
functionality inline with the documentation,
http://de1.php.net/serializable . It says "Classes that implement this
interface no longer support __sleep() and __wakeup().". So that dirty
tricks should actually have been impossible, according to the doc. So per
fact, it breaks something which was already broken but users was relying
on it.

The patches fixes it globally preventing incomplete objects due to the
incorrect callback invocation. A fix for that particular ticket could be
to add __wakeup to SplFileObject and revert the patch to the unserialize
code. However that would open the rabbit hole again for any vulnerable
user constructed strings on any internal classes. Also, that would at most
shortly defer the issue because 5.6 is coming out in a few weeks, so the
bad user code will have to be fixed in the userspace soon. As conclusion -
IMHO it should stay fixed as it is, it's already documented in UPGRADING.
If you still think it's not worth it, so I can revert the global
unserializer fix and go with a local fix for the SplFileObject class. What
do you say, guys?

Cheers

Anatol


--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php
Hi!

> The initial bug is that an unserialization string misuses "C:" vs "O:"
> format which would call an incorrect method "unserialize" vs. "__wakeup".
> And that leads to a crash with that particular ticket, due to an
> incompletely initialized object. Formally this fix just brings the
> functionality inline with the documentation,
> http://de1.php.net/serializable . It says "Classes that implement this
> interface no longer support __sleep() and __wakeup().". So that dirty
> tricks should actually have been impossible, according to the doc. So per
> fact, it breaks something which was already broken but users was relying
> on it.

I'm not sure I understand this point. The manual says __sleep/__wakeup
won't be called, but that doesn't seem to me a problem here, the problem
here seems to be that unserialize throws a warning, not that __wakeup is
not called.

> The patches fixes it globally preventing incomplete objects due to the
> incorrect callback invocation. A fix for that particular ticket could be

This also breaks code that worked before, and does it in stable
versions. I do not think it is an acceptable solution for stable
versions. Can't we make the fix in a way that does not have BC breakage
for stable versions?

> shortly defer the issue because 5.6 is coming out in a few weeks, so the
> bad user code will have to be fixed in the userspace soon. As conclusion -

Only if that code is going to be run on 5.6.0 - which it won't be for
some time because nobody runs .0 versions in production next day they
are released. But even with 5.6, we are trying to minimize amount of
code we're breaking in minor releases, and here we're breaking phpunit,
which is one of the most used PHP tools. So I think we need to look for
a better solution. CCing also Sebastian, maybe he could explain more
about this hack and how it can be made to work more smoothly maybe.
--
Stanislav Malyshev, Software Architect
SugarCRM: http://www.sugarcrm.com/
(408)454-6900 ext. 227

--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php
Hi Stas,

On Tue, May 6, 2014 10:26, Stas Malyshev wrote:
> Hi!
>
>
>> The initial bug is that an unserialization string misuses "C:" vs "O:"
>> format which would call an incorrect method "unserialize" vs.
>> "__wakeup".
>> And that leads to a crash with that particular ticket, due to an
>> incompletely initialized object. Formally this fix just brings the
>> functionality inline with the documentation,
>> http://de1.php.net/serializable . It says "Classes that implement this
>> interface no longer support __sleep() and __wakeup().". So that dirty
>> tricks should actually have been impossible, according to the doc. So
>> per fact, it breaks something which was already broken but users was
>> relying on it.
>
> I'm not sure I understand this point. The manual says __sleep/__wakeup
> won't be called, but that doesn't seem to me a problem here, the problem
> here seems to be that unserialize throws a warning, not that __wakeup is
> not called.
>
Please take the case from #67072

- SplFileObject implements Serializable interface and serialization is
disabled by setting the callbacks to NULL or to the special Zend callback

- imagine the serialization were enabled on it, so the string would look like
'C:13:"SplFileObject":.......'

- when unserializing, the ->unserialize() method implementation should be
called (while it's disabled)

- the users wants to do the trick and constructs this
'O:13:"SplFileObject":1:{s:9:"*filename";s:15:"/home/flag/flag";}'

- the var_unserializer code thinks it's an object with possible __wakeup,
so no ->unserialize method is called, and no __wakeup as it's not
implemented. If it would call ->unserialize(), it would lead to a similar
error triggered from zend_class_unserialize_deny

Result - there's a C struct with unititialized members which would lead to
crashes when accessed. That will affect any internal class implementing
Serializable. The warning is just an indication that a class implementing
Serializable was passed with O: format and not with C:. That class per
definition shouldn't even have __wakeup as it implements Serializable,
while it's not disallowed.

>> The patches fixes it globally preventing incomplete objects due to the
>> incorrect callback invocation. A fix for that particular ticket could be
>>
>
> This also breaks code that worked before, and does it in stable
> versions. I do not think it is an acceptable solution for stable versions.
> Can't we make the fix in a way that does not have BC breakage
> for stable versions?
>
>> shortly defer the issue because 5.6 is coming out in a few weeks, so
>> the bad user code will have to be fixed in the userspace soon. As
>> conclusion -
>
> Only if that code is going to be run on 5.6.0 - which it won't be for
> some time because nobody runs .0 versions in production next day they are
> released. But even with 5.6, we are trying to minimize amount of code
> we're breaking in minor releases, and here we're breaking phpunit, which
> is one of the most used PHP tools. So I think we need to look for a better
> solution. CCing also Sebastian, maybe he could explain more about this
> hack and how it can be made to work more smoothly maybe. --

Yeah, Sebastian knows that at best. Generally, if no userspace fix is
possible, one could still revert and put __wakeup() to SplFileObject
class, while as mentioned it's discards the doc and brings possible
vulnerability back. At least 5.6.0 should have that fix IMHO, or master.

Cheers

Anatol

--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php
Sorry, only registered users may post in this forum.

Click here to login