Welcome! Log In Create A New Profile

Advanced

[PHP-DEV] Problems with the fix for the BC break introduced in 5.4.29 and 5.5.13

Posted by Ferenc Kovacs 
Hi,

Could you check out my last mail about the unserialize stuff?:
http://news.php.net/php.internals/74947

After looking into the issue a bit more I think that our current fix isn't
what we want.
It still changes behavior compared to 5.4.27/5.5.12 while still allows
somebody to trigger the SplFileObject segfault for userland classes
extending SplFileObject:
http://3v4l.org/BDq6g

ps: Anatol is out of office for the next three weeks, so there is a chance
that he won't be able to read/reply to his mails.

--
Ferenc Kovács
@Tyr43l - http://tyrael.hu
Hi!

> Could you check out my last mail about the unserialize stuff?:
> http://news.php.net/php.internals/74947

Reading this message, I gather the situation is as follows:

1. Original fix banned O: unserialization of classes that have custom
serializers.
2. The following fix allowed this behavior for user classes.
3. The second fix means that if the user class descends from internal
class with serializer, we still have a problem.

My opinion is this:

1. Using unserialize() on anything that is not a result of serialize()
is a hack. As such, there are no support guarantees that it would work
in any particular manner, besides continuity of support for serialized
format itself. In particular, we have no guarantees that string that did
not come from serializer would behave in any particular way.

2. We should make reasonable effort to keep code that worked in version
x.y.z working in x.y.z+1. However, "reasonable" is important here, if
there's a behavior that is not documented and not wanted, we can break
it. We don't have to and will try not to, but we can.

3. In light of this, I think we could do with current patch in 5.4 and
5.5 (one that permits userland classes) but we should plug it completely
for 5.6. If somebody needs code that does whatever it did, it should be
in Reflection or someplace else, serializer should do serializing. But I
think for stable version it is a reasonable, if imperfect, compromise -
it would allow most common use cases (userland classes) to work and most
common crashes (internal classes) to not crash. Extending SplFileInfo
looks a bit more exotic so I think we can live with it not being fixed
till 5.6.

--
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
On Wed, Jun 18, 2014 at 12:53 AM, Stas Malyshev <[email protected]>
wrote:

> Hi!
>
> > Could you check out my last mail about the unserialize stuff?:
> > http://news.php.net/php.internals/74947
>
> Reading this message, I gather the situation is as follows:
>
> 1. Original fix banned O: unserialization of classes that have custom
> serializers.
>

Yeah, with
https://github.com/php/php-src/commit/757da1eed5ffbc1c5a0ae07e82ae91f7e431f1a2
we introduced the Serializable interface, which provides a different method
to serialize/unserialize an object.
Since then, serialize/unserialize uses a different codepath for objects
implementing this interface: __sleep()/__wakeup() is ignored, the
unserialized format will use C: instead of O, and the format of the payload
will be also a bit different:
http://3v4l.org/B57Kk

Even though we documented this from the userland point of view:
http://www.php.net/Serializable

Classes that implement this interface no longer support __sleep() and
> __wakeup(). The method serialize is called whenever an instance needs to be
> serialized.
> ...
> The above example will output something similar to:
> string(38) "C:3:"obj":23:{s:15:"My private data";}"


So while we never produced "O:" for objects implementing Serializable, we
also never enforced/validated that, which made it possible to unserialize a
Serializable via bypassing it's unserialize() method, and this also allowed
to unserialize classes which explicitly prohibits that
(zend_class_unserialize_deny).

Userland devs could have used Reflection to check if the class they are
trying to mock/whatever was implementing Serializable, but that would have
required a Reflection call plus with the O: format you can always use the
O:$classNameLength:"$className":0:{}
for every class, while with the C: format, you have to make sure that your
string passes the unserialize() call of that specific class, which makes it
unfeasible for general purpose libraries like phpunit-mock-objects or
doctrine2 entity manager.


> 2. The following fix allowed this behavior for user classes.
>

yes


> 3. The second fix means that if the user class descends from internal
> class with serializer, we still have a problem.
>

correct


>
> My opinion is this:
>
> 1. Using unserialize() on anything that is not a result of serialize()
> is a hack. As such, there are no support guarantees that it would work
> in any particular manner, besides continuity of support for serialized
> format itself. In particular, we have no guarantees that string that did
> not come from serializer would behave in any particular way.
>

strongly agree


>
> 2. We should make reasonable effort to keep code that worked in version
> x.y.z working in x.y.z+1. However, "reasonable" is important here, if
> there's a behavior that is not documented and not wanted, we can break
> it. We don't have to and will try not to, but we can.
>

+1, we don't really document the php serialize format (only show some
examples), so I would consider it an internal implementation detail, and we
indeed mentioned in the docs that classes implementing Serializable will
have a different output, so I think that it isn't anything wrong with
actually starting to enforce the documented behavior and validate and
reject malformed serialized strings.


>
> 3. In light of this, I think we could do with current patch in 5.4 and
> 5.5 (one that permits userland classes) but we should plug it completely
> for 5.6. If somebody needs code that does whatever it did, it should be
> in Reflection or someplace else, serializer should do serializing. But I
> think for stable version it is a reasonable, if imperfect, compromise -
> it would allow most common use cases (userland classes) to work and most
> common crashes (internal classes) to not crash. Extending SplFileInfo
> looks a bit more exotic so I think we can live with it not being fixed
> till 5.6.
>
>
I also agree that it is more important to make sure that internal objects
are properly instantiated and not subject to random segfaults and probably
other problems, than to allow an undocumented hack to be used for easier
instantiation of objects.

Back then when Sebastian
proposed ReflectionClass::newInstanceWithoutConstructor() for 5.4, he
himself introduced the only userland class restriction, because as he
mentioned some internal classes would crash if instantiated without a
constructor:
http://marc.info/?l=php-internals&m=131426563418067&w=2
And it properly considers userland classes extending internal classes as
internal, and prohibits the instantiation for such classes:
http://3v4l.org/Zp4YS

I agree that this would be big hit for some of those tools/libs, but other
than fixing every internal class to be properly initialized without their
constructors (which would be a nice thing anyways) I don't see any other
way to fix the segfaults while keeping the ability to instantiate any class
(be that an internal class, or a class implementing Serialize) with ease.

--
Ferenc Kovács
@Tyr43l - http://tyrael.hu
Hi!

> Back then when Sebastian
> proposed ReflectionClass::newInstanceWithoutConstructor() for 5.4, he
> himself introduced the only userland class restriction, because as he
> mentioned some internal classes would crash if instantiated without a
> constructor:
> http://marc.info/?l=php-internals&m=131426563418067&w=2
> And it properly considers userland classes extending internal classes as
> internal, and prohibits the instantiation for such classes:
> http://3v4l.org/Zp4YS

Maybe we should use the same thing for serializer, i.e.
ce->create_object check?

> without their constructors (which would be a nice thing anyways) I don't
> see any other way to fix the segfaults while keeping the ability to
> instantiate any class (be that an internal class, or a class
> implementing Serialize) with ease.

If we add ce->create_object check, wouldn't it solve most of the problems?
--
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
Le 18/06/2014 10:03, Ferenc Kovacs a écrit :

>> 2. The following fix allowed this behavior for user classes.
>>
>
> yes

Hmm... do you refer to
http://git.php.net/?p=php-src.git;a=patch;h=20568e502814fffc41d91a22edaf75ff5ae19d5c

I think this allow to unserialize "O:.." for "internal" classes.

As for user land classes, we have newInstanceWithoutConstructor.


Remi.


--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php
On Wed, Jun 18, 2014 at 10:15 AM, Remi Collet <[email protected]> wrote:

> Le 18/06/2014 10:03, Ferenc Kovacs a écrit :
>
> >> 2. The following fix allowed this behavior for user classes.
> >>
> >
> > yes
>
> Hmm... do you refer to
>
> http://git.php.net/?p=php-src.git;a=patch;h=20568e502814fffc41d91a22edaf75ff5ae19d5c
>
> I think this allow to unserialize "O:.." for "internal" classes.
>

allows unserialize "O:" for userland classes implementing Serializable(even
if that userland class extends an internal class):

[tyrael@ferencs-mbp-135 php-src.git (PHP-5.5 ✗)]$ ./sapi/cli/php -v
PHP 5.5.15-dev (cli) (built: Jun 17 2014 15:33:34) (DEBUG)
Copyright (c) 1997-2014 The PHP Group
Zend Engine v2.5.0, Copyright (c) 1998-2014 Zend Technologies
[tyrael@ferencs-mbp-135 php-src.git (PHP-5.5 ✗)]$ ./sapi/cli/php -r
"var_dump(unserialize('O:11:\"ArrayObject\":0:{}'));class MyArrayObject
extends ArrayObject{};var_dump(unserialize('O:13:\"MyArrayObject\":0:{}'));"

Warning: Erroneous data format for unserializing 'ArrayObject' in Command
line code on line 1
bool(false)
object(MyArrayObject)#1 (1) {
["storage":"ArrayObject":private]=>
array(0) {
}
}


>
> As for user land classes, we have newInstanceWithoutConstructor.
>

>
yes, but that also considers any class extending/implementing an internal
class/interface as internal.


--
Ferenc Kovács
@Tyr43l - http://tyrael.hu
Le 17/06/2014 16:05, Ferenc Kovacs a écrit :
> Hi,
>
> Could you check out my last mail about the unserialize stuff?:
> http://news.php.net/php.internals/74947

The initial commit try to fix a segfault for bug #67072.

https://bugs.php.net/67072

This case is really awful... something which should not exists.

Ok, a segfault is bad, but we have live with it since years.

My proposal, for a quick solution, trying to be pragmatic and
trying to make the most of php users happy.

Revert in 5.4.30 to the 5.4.28 code (only var_serialize.* + tests)
Revert in 5.5.14 to the 5.5.12 code

This should allow to release without new delay.

In 5.6+ (or master), keep the fix
(don't allow to unserialize things which don't have to be)

And allow newInstanceWithoutConstructor for internal classes.
(make this unserialize hack used by phpunit and doctrine unneeded)

And fix classes which need to be fixed.


Remi.


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

> My proposal, for a quick solution, trying to be pragmatic and
> trying to make the most of php users happy.

Is the solution of banning only the internal classes with create_object
and their descendants unsatisfactory?

> And allow newInstanceWithoutConstructor for internal classes.
> (make this unserialize hack used by phpunit and doctrine unneeded)

How we can safely make that? For internal classes I'm afraid making them
work safely without ctor would be a challenge - after all, all the code
expects ctor to run. For user classes at least the engine would throw a
fatal error at worst, but for internal classes we'd get segfaults all
over the place. I'm not sure how this can be done safely.
--
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
Am 19.06.2014 00:22, schrieb Stas Malyshev:
> How we can safely make that? For internal classes I'm afraid making them
> work safely without ctor would be a challenge - after all, all the code
> expects ctor to run. For user classes at least the engine would throw a
> fatal error at worst, but for internal classes we'd get segfaults all
> over the place. I'm not sure how this can be done safely.

The use case I am interested is test doubles. When I create a stub
or mock of a class then I do not wants its original functionality to
be executed. I just want to have an object that looks like an object
of the original class. When the original functionality is not executed,
though, how can we run into a segfault when the constructor of an
internal class is not executed?

--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php
On Thu, Jun 19, 2014 at 8:07 AM, Sebastian Bergmann <[email protected]> wrote:
> Am 19.06.2014 00:22, schrieb Stas Malyshev:
>> How we can safely make that? For internal classes I'm afraid making them
>> work safely without ctor would be a challenge - after all, all the code
>> expects ctor to run. For user classes at least the engine would throw a
>> fatal error at worst, but for internal classes we'd get segfaults all
>> over the place. I'm not sure how this can be done safely.
>
> The use case I am interested is test doubles. When I create a stub
> or mock of a class then I do not wants its original functionality to
> be executed. I just want to have an object that looks like an object
> of the original class. When the original functionality is not executed,
> though, how can we run into a segfault when the constructor of an
> internal class is not executed?

Because you'll need an object, so you'll need to create it.
Internal classe based objects may have some more logic that just
executing their userland's __construct() , that's the case for
SplFileObject and many other ones (date, xml ...).

Internal class based objects have an internal constructor which is
expected to be called (the create_object() handler).
It has never been planned for this internal constructor not to be called.

Julien.P

--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php
On Thu, Jun 19, 2014 at 12:22 AM, Stas Malyshev <[email protected]> wrote:
> Hi!
>
>> My proposal, for a quick solution, trying to be pragmatic and
>> trying to make the most of php users happy.
>
> Is the solution of banning only the internal classes with create_object
> and their descendants unsatisfactory?

For me, it's a safe solution, however, it breaks BC, I think it's a
no-go for 5.4 and 5.5.
This however could be a 5.6+ solution.

For 5.4 and 5.5, there doesn't seem to be easy fix on the go.
I suggest following what Remi suggests :

- Revert the BC break in 5.5 and 5.4
- Keep the segfault, we've been living with it for ages
- Patch the manual to clearly show one should never try to unserialize
hand-made strings : we just do not support such behavior (thus, it
could lead to segfaults)
- What if tomorrow we replace our serializer by igbinary or so ? Users
should not be aware of internal details, serialized string is
definetly one

Julien

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

> The use case I am interested is test doubles. When I create a stub
> or mock of a class then I do not wants its original functionality to
> be executed. I just want to have an object that looks like an object
> of the original class. When the original functionality is not executed,
> though, how can we run into a segfault when the constructor of an
> internal class is not executed?

Dtors would be one thing. But another thing is that we can not ensure no
method of original class would be ever called - even with mocking, etc.,
as most mocks do not override every method of the class, and even if
they do there are handlers that can not be overridden from userspace, so
if those are set and expect certain values be there they can be
activated by third-party code which has no idea it's dealing with the
mock. So we'd have a ticking bomb on our hands.

So I'm not sure there is such thing as creating an internal object
without calling its ctor or doing something else that initializes it.

I understand why people may want to do it, but I don't think it can
safely be done for internal objects. Unless somebody has some bright
ideas that I have missed.
--
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!

> For me, it's a safe solution, however, it breaks BC, I think it's a
> no-go for 5.4 and 5.5.

It breaks BC in something that never was part of the official API, never
was promised to work and works only by accident for internal classes.
Each such object can segfault at smallest provocation, so keeping them
is essentially requiring that we keep segfault compatibility. I don't
think we should promise that.

> - Revert the BC break in 5.5 and 5.4
> - Keep the segfault, we've been living with it for ages

That's not the reason not to fix segfaults. Virtually every bug we're
fixing in the code we have been "living with for ages". That's not the
reason to keep them now that we know it segfaults.

> - Patch the manual to clearly show one should never try to unserialize
> hand-made strings : we just do not support such behavior (thus, it
> could lead to segfaults)

Well, here we contradict ourselves then - if we do not support such
behavior, why we are taking so much effort to enable it? Because
declaring that changing something even in small part is inacceptable
even if the price is known crashes in the application (and I wonder what
security people can make of it - uninitialized objects might be way
worse than just null pointer deref...) - that looks a lot like
supporting to me. So in what meaning we don't support it then?
--
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
Travis is now running PHP 5.4.29, tests are failing all over the place right now*.
When these kind of things happen, just take down the release while you work out the details please.


(* most people in the wild don’t always use latests PHPUnit as it has a tendency to break stuff badly, just look to the now dead Zeta Components for a example of bad PHPunit breaks)


On 19 Jun 2014, at 11:13 , Stas Malyshev <[email protected]> wrote:

> Hi!
>
>> For me, it's a safe solution, however, it breaks BC, I think it's a
>> no-go for 5.4 and 5.5.
>
> It breaks BC in something that never was part of the official API, never
> was promised to work and works only by accident for internal classes.
> Each such object can segfault at smallest provocation, so keeping them
> is essentially requiring that we keep segfault compatibility. I don't
> think we should promise that.
>
>> - Revert the BC break in 5.5 and 5.4
>> - Keep the segfault, we've been living with it for ages
>
> That's not the reason not to fix segfaults. Virtually every bug we're
> fixing in the code we have been "living with for ages". That's not the
> reason to keep them now that we know it segfaults.
>
>> - Patch the manual to clearly show one should never try to unserialize
>> hand-made strings : we just do not support such behavior (thus, it
>> could lead to segfaults)
>
> Well, here we contradict ourselves then - if we do not support such
> behavior, why we are taking so much effort to enable it? Because
> declaring that changing something even in small part is inacceptable
> even if the price is known crashes in the application (and I wonder what
> security people can make of it - uninitialized objects might be way
> worse than just null pointer deref...) - that looks a lot like
> supporting to me. So in what meaning we don't support it then?
> --
> 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
>


--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php
On Thu, Jun 19, 2014 at 2:04 PM, André Rømcke <[email protected]> wrote:

> Travis is now running PHP 5.4.29, tests are failing all over the place
> right now*.
> When these kind of things happen, just take down the release while you
> work out the details please.
>
>
> (* most people in the wild don’t always use latests PHPUnit as it has a
> tendency to break stuff badly, just look to the now dead Zeta Components
> for a example of bad PHPunit breaks)
>
>
>
we can't really "take down" a release after it was announced(it is not a
technical limitation, but a logical one, the cat is out of the bag already
at that point), our only option is to quickly release another one, but that
should be used as a last-resort action, as we have many precedence that
releases like that (made under time pressure) are more likely to don't
properly fix the problem at hand, or that it introduces another one.
as you can see from the discussion above, we are still not sure about
whether or not we want to "fix" the behavior change in 5.4.29/5.5.13 so I
think it was a good thing that we didn't took down the release and rushed
another release, which reverts the behavior when there is a chance that we
decide to keep the original fix anyways.

(* most people testing their php lib/app on travis probably use the phpunit
pre-installed on the travis box, the same as they do with the php install)

ps: please don't top post on this list.

--
Ferenc Kovács
@Tyr43l - http://tyrael.hu
On Thu, Jun 19, 2014 at 11:13 AM, Stas Malyshev <[email protected]>
wrote:

> Hi!
>
> > For me, it's a safe solution, however, it breaks BC, I think it's a
> > no-go for 5.4 and 5.5.
>
> It breaks BC in something that never was part of the official API, never
> was promised to work and works only by accident for internal classes.
> Each such object can segfault at smallest provocation, so keeping them
> is essentially requiring that we keep segfault compatibility. I don't
> think we should promise that.
>
> > - Revert the BC break in 5.5 and 5.4
> > - Keep the segfault, we've been living with it for ages
>
> That's not the reason not to fix segfaults. Virtually every bug we're
> fixing in the code we have been "living with for ages". That's not the
> reason to keep them now that we know it segfaults.
>
> > - Patch the manual to clearly show one should never try to unserialize
> > hand-made strings : we just do not support such behavior (thus, it
> > could lead to segfaults)
>
> Well, here we contradict ourselves then - if we do not support such
> behavior, why we are taking so much effort to enable it? Because
> declaring that changing something even in small part is inacceptable
> even if the price is known crashes in the application (and I wonder what
> security people can make of it - uninitialized objects might be way
> worse than just null pointer deref...) - that looks a lot like
> supporting to me. So in what meaning we don't support it then?
> --
> 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
>
>
I've linked the discussion and provide a really compact summary (I think it
is too compact) at https://bugs.php.net/bug.php?id=67072
I'm a bit indecisive about the current situation, because I think that we
should fix the root cause of these problems, but I'm also wanna make sure
that we don't totally criple phpunit-mock-objects and doctrine with a micro
release.
I think it would be safe to prohibit the unserialization of classes
implementing the Serializable interface with the "O:" format, userland
should use the "C:" format for these classes, we don't have that many
classes outright denying the unserialization, and if a class validates the
incoming data in the unserialize method, then the userland should make sure
to provide appropriate data for the validation to pass. (Ofc. we should
also make sure that we don't require more data, than necessary, see
https://bugs.php.net/bug.php?id=67453&edit=1)

Another topic would be to that internal classes (not implementing
Serializable) can still be instantiated without the constructor call
through the unserialize method (using the "O:" format), and we can't
prohibit that, as it is/can be perfectly normal to unserialize an internal
class previously properly instantiated and serialize, but we don't have the
knowledge in the class to tell what is a properly serialized string, and
what isn't for that given class.
(Which means that allowing the internal classes to be instantiated via
ReflectionClass::newInstanceWithoutConstructor() doesn't really introduces
a new set of problems, it would just allow to shut yourself with reflection
instead of an obscure unserialize trick. (Not saying we should do this,
just mentioning that the problem already exists).
I think that it would be a nice if we could add more validation for the
internal classes __wakeup()/unserialize() methods for data which presence
is mandatory for the class to be able to work, which would ofc. bother the
life of the phpunit/doctrine users/devs, but it would only prevent the
mocking those classes which would be unstable/dangerous previously
when instantiated
without the constructor call.

--
Ferenc Kovács
@Tyr43l - http://tyrael.hu
On 20 June 2014 02:30, Ferenc Kovacs <[email protected]> wrote:

> I've linked the discussion and provide a really compact summary (I think it
> is too compact) at https://bugs.php.net/bug.php?id=67072
> I'm a bit indecisive about the current situation, because I think that we
> should fix the root cause of these problems, but I'm also wanna make sure
> that we don't totally criple phpunit-mock-objects and doctrine with a micro
> release.
>


At the end of the day, what we need is just instantiating an object without
invoking its constructor.

Wouldn't it be much easier to just revert for 5.4 and 5.5 and then patching
`ReflectionClass#newInstanceWithoutConstructor()` to just handle internal
PHP classes as well?
Is there a technical limitation for this?
Are there security problems related to the patch that broke
phpunit/doctrine? If not, can the patch simply be delayed and only applied
to 5.6?

If we get a stable API to instantiate objects in our way, we'd simply
gradually move away from the reflection "hack" in the next point releases
of doctrine/phpunit, while using `unserialize()` only for older versions of
PHP.

Marco Pivetta

http://twitter.com/Ocramius

http://ocramius.github.com/
Hi!

> Another topic would be to that internal classes (not implementing
> Serializable) can still be instantiated without the constructor call
> through the unserialize method (using the "O:" format), and we can't
> prohibit that, as it is/can be perfectly normal to unserialize an
> internal class previously properly instantiated and serialize, but we
> don't have the knowledge in the class to tell what is a properly
> serialized string, and what isn't for that given class.

Ah, I was missing this part. I thought those objects go through C, but
now I see they can go through O also. This means we can't really ban
internal classes in O:. But can we see if the serializer is
zend_user_serialize (in which case it's probably OK to allow internal
class or user class) or some other function (in which case allowing
internal class is probably not a good idea)?
Do we still have problems or segfaults in this case?

> I think that it would be a nice if we could add more validation for the
> internal classes __wakeup()/unserialize() methods for data which
> presence is mandatory for the class to be able to work, which would ofc.

Some classes just don't allow being serialized/unserialized at all, and
use serialize handlers to do that. Trying to run them through
unserialize in a roundabout way is very dangerous since they would
assume nobody does that.

> bother the life of the phpunit/doctrine users/devs, but it would only
> prevent the mocking those classes which would be unstable/dangerous
> previously when instantiated without the constructor call.

I guess this is where I'm trying to get - banning unserializing of the
classes which are unsafe (i.e. O: in internal class while having
serialize handler) while not banning cases that we can allow. So I
wonder here if additional check for zend_user_serialize would help?
--
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!

> Wouldn't it be much easier to just revert for 5.4 and 5.5 and then
> patching `ReflectionClass#newInstanceWithoutConstructor()` to just
> handle internal PHP classes as well?

We can not just handle internal PHP classes without calling their ctor.
That's the whole problem - for internal class, if ctor is not called,
you can get segfault on anything you do with the class.

> Is there a technical limitation for this?
> Are there security problems related to the patch that broke
> phpunit/doctrine? If not, can the patch simply be delayed and only
> applied to 5.6?

Right now we know this thing can produce segfaults. But running code on
unititialized data may be worse than that.

--
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
On Fri, Jun 20, 2014 at 2:58 AM, Stas Malyshev <[email protected]>
wrote:

> Hi!
>
> > Wouldn't it be much easier to just revert for 5.4 and 5.5 and then
> > patching `ReflectionClass#newInstanceWithoutConstructor()` to just
> > handle internal PHP classes as well?
>
> We can not just handle internal PHP classes without calling their ctor.
> That's the whole problem - for internal class, if ctor is not called,
> you can get segfault on anything you do with the class.
>
> > Is there a technical limitation for this?
> > Are there security problems related to the patch that broke
> > phpunit/doctrine? If not, can the patch simply be delayed and only
> > applied to 5.6?
>
> Right now we know this thing can produce segfaults. But running code on
> unititialized data may be worse than that.
>
> --
> Stanislav Malyshev, Software Architect
> SugarCRM: http://www.sugarcrm.com/
> (408)454-6900 ext. 227
>

I have a longer draft reply about the technical parts, but I wanted to send
a quick headsup:
The original SplFileObject segfault can be achived without any unserialize
trick, one can simply extend an internal class depending on it's
constructor for providing the required initial state, and simply not call
the parent::__construct() from the child:
http://3v4l.org/fqFC6

And I also think that we can't fix/remove the O:X:"Y":0:{} trick without
providing some usable alternatives for phpunit/doctrine(not providing it
would really hurt the adoption imo).
So at the moment I see two options:

1. Revert back to the original behavior for 5.4 and 5.5, and for 5.6
introduce the original clean fix (classes implementing Serializable can
only be unserialized with the "C:" format) and remove the restriction from
ReflectionClass::newInstanceWithoutConstructor about internal classes (and
make sure that we put up a warning in the docs about the potential
instability of internal classes instanitated this way.
2. Do the same as suggested for 5.6 in 1) but also for 5.4 and 5.5.

I think the first one is better.
If we would keep the current status, it would mean for the userland project
that:

- for 5.6
- they should use ReflectionClass::newInstanceWithoutConstructor for
non-internal classes(using clunky checks like in
https://github.com/sebastianbergmann/phpunit-mock-objects/commit/4a338e464a94d3e5a48ae28292e98e9b4e3ac898
)
- they should need to use the unserialize O:X:"Y":0:{} trick for
internal(including non-internal classes extending internal
classes) classes
not implementing Serializable
- they should use unserialize C:X:"Y":0:{} for internal classes
implementing Serializable and because there are classes denying the
unserialization or requiring specific unserialize payload (which we can't
expect the userland to keep track of) they would have to check if the
unserialize fails and throw and exception, making the
instanitation of some
classes outright impossible.
- for 5.4 and 5.5
- they should use ReflectionClass::newInstanceWithoutConstructor for
non-internal classes(using clunky checks like in
https://github.com/sebastianbergmann/phpunit-mock-objects/commit/4a338e464a94d3e5a48ae28292e98e9b4e3ac898
)
- they should need to use the unserialize O:X:"Y":0:{} trick for the
internal classes not implementing Serializable and for
non-internal classes
extending internal classes implementing Serializable
- they should use unserialize C:X:"Y":0:{} for internal classes
implementing Serializable and because there are classes denying the
unserialization or requiring specific unserialize payload (which we can't
expect the userland to keep track of) they would have to check if the
unserialize fails and throw and exception, making the
instanitation of some
classes outright impossible.

(and both phpunit and doctrine 2 supports 5.3 where
ReflectionClass::newInstanceWithoutConstructor isn't available)

--
Ferenc Kovács
@Tyr43l - http://tyrael.hu
Hi!

> The original SplFileObject segfault can be achived without any
> unserialize trick, one can simply extend an internal class depending on
> it's constructor for providing the required initial state, and simply
> not call the parent::__construct() from the child:
> http://3v4l.org/fqFC6

Unserialize is a problem because unserialize sometimes deals with
external data, so if you can trick unserialize to crash, you may be able
to cause at least DoS, maybe RCE. Just some crafted code is less a
problem is this context.

So I'd like very much to find a solution which allows to eliminate
crashes in unserialize(). The question is can we do this without messing
up phpunit and so too badly.
--
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
Stas Malyshev
[PHP-DEV] Bug 67072 resolution for 5.4/5.5
June 22, 2014 06:10AM
Hi!

We're getting pretty short on time for resolving 67072 one way or
another, since we want to release next week (we've accumulated quite a
lot of security issues). I still think that:

1. We can not leave unserialize() segfault in the code in 5.4
undisturbed. It's DoS opening as a minimum, and RCE potential, if
suitable internal class can be found, for every app that accepts
user-controlled serialized data. And there are a lot of such apps out there.

2. We do not want to break phpunit/doctrine, at least as much as
possible without hurting #1.

3. Current code does not fix the segfault problem entirely, e.g.:
class MySplFileObject extends SplFileObject {}
echo
unserialize('O:15:"MySplFileObject":1:{s:9:"*filename";s:15:"/home/flag/flag";}');

still segfaults for me.

4. There are other ways to cause segfaults with internal objects,
unfortunately, but those require deliberate coding and I'm less worried
about it since those have no external exploitation potential.

Given this, I am proposing to check the check in var_unserializer to this:

if (ce->serialize == NULL || ce->unserialize == zend_user_unserialize ||
(ZEND_INTERNAL_CLASS != ce->type && ce->create_object == NULL)) {
object_init_ex(*rval, ce);
}

This would allow O: to work with the following:

1. Regular user classes
2. User classes implementing Serializable - the result will be
semantically broken but no segfault should be produced since it is still
all within PHP engine, so the worst is that some PHP variables will be
left null instead of their real values.
3. Internal classes that do not have their own serializer

The following will still not work with O::
1. Internal classes with their own serializer
2. User classes extending classes in 1.

I've checked the Horde/phpunit test by Remi, and it works fine, but I'd
like more feedback on this. Please note we need some solution one way or
another in 3 days, so if we need more discussion on this I'd advise
maybe set up some session on IRC to hash it out.

P.S. For 5.6, I'd just remove the usage of O: hack for any class that
produces C: in serialize. If serializer does not produce it, it should
not accept it.
--
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
Ferenc Kovacs
[PHP-DEV] Re: Bug 67072 resolution for 5.4/5.5
June 23, 2014 01:20AM
On Sun, Jun 22, 2014 at 6:03 AM, Stas Malyshev <[email protected]>
wrote:

> Hi!
>
> We're getting pretty short on time for resolving 67072 one way or
> another, since we want to release next week (we've accumulated quite a
> lot of security issues). I still think that:
>
> 1. We can not leave unserialize() segfault in the code in 5.4
> undisturbed. It's DoS opening as a minimum, and RCE potential, if
> suitable internal class can be found, for every app that accepts
> user-controlled serialized data. And there are a lot of such apps out
> there.
>

for the issue to materialize you need to feed hand-crafted input to
unserialize, anybody doing that with user controlled data already asking
for problems, but I agree that we should fix if, and even fix it in a micro
version if we won't totally cripple the the userland tools unfortunately
depending on this trick.


>
> 2. We do not want to break phpunit/doctrine, at least as much as
> possible without hurting #1.
>
>
+1


> 3. Current code does not fix the segfault problem entirely, e.g.:
> class MySplFileObject extends SplFileObject {}
> echo
>
> unserialize('O:15:"MySplFileObject":1:{s:9:"*filename";s:15:"/home/flag/flag";}');
>
> still segfaults for me.
>

yeah, this is what I also noticed and reported.


>
> 4. There are other ways to cause segfaults with internal objects,
> unfortunately, but those require deliberate coding and I'm less worried
> about it since those have no external exploitation potential.
>

I would say that this also a deliberate attempt, maybe a bit more obscure.


>
> Given this, I am proposing to check the check in var_unserializer to this:
>
> if (ce->serialize == NULL || ce->unserialize == zend_user_unserialize ||
> (ZEND_INTERNAL_CLASS != ce->type && ce->create_object == NULL)) {
> object_init_ex(*rval, ce);
> }
>
> This would allow O: to work with the following:
>
> 1. Regular user classes
> 2. User classes implementing Serializable - the result will be
> semantically broken but no segfault should be produced since it is still
> all within PHP engine, so the worst is that some PHP variables will be
> left null instead of their real values.
> 3. Internal classes that do not have their own serializer
>
> The following will still not work with O::
> 1. Internal classes with their own serializer
> 2. User classes extending classes in 1.
>
>
I prefer this over what we have in 5.4/5.5 and given how few classes does
1, actually mean, I think it would be an acceptable compromise, but let's
hear what others think.


> I've checked the Horde/phpunit test by Remi, and it works fine, but I'd
> like more feedback on this. Please note we need some solution one way or
> another in 3 days, so if we need more discussion on this I'd advise
> maybe set up some session on IRC to hash it out.
>

I think #php.pecl on efnet would be the best place, as some of us already
lurks there.


>
> P.S. For 5.6, I'd just remove the usage of O: hack for any class that
> produces C: in serialize. If serializer does not produce it, it should
> not accept it.
>

agree, but as I mentioned I would like to provide some alternative for
creating those classes (eg. removing the restriction on internal classes
for ReflectionClass::newInstanceWithoutConstructor)

ps: I've seen that you created a pull request with the patch, if somebody
don't wanna copypaste the patch from the mail, here it is:
https://github.com/php/php-src/pull/701

--
Ferenc Kovács
@Tyr43l - http://tyrael.hu
Stas Malyshev
[PHP-DEV] Re: Bug 67072 resolution for 5.4/5.5
June 23, 2014 02:30AM
Hi!

> for the issue to materialize you need to feed hand-crafted input to
> unserialize,

True.

> anybody doing that with user controlled data already asking
> for problems,

True in theory, in practice this is widely and commonly done.

> I prefer this over what we have in 5.4/5.5 and given how few classes
> does 1, actually mean, I think it would be an acceptable compromise, but
> let's hear what others think.

Cool, waiting for others to chime in.

> ps: I've seen that you created a pull request with the patch, if
> somebody don't wanna copypaste the patch from the mail, here it is:
> https://github.com/php/php-src/pull/701

Yes, thanks for quoting it, it seems to be green on Travis and phpunit
also seems to work fine with it. I also added a unit tests with a couple
of cases to see how it's supposed to work.

--
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
Julien Pauli
[PHP-DEV] Re: Bug 67072 resolution for 5.4/5.5
June 23, 2014 10:00AM
On Mon, Jun 23, 2014 at 2:20 AM, Stas Malyshev <[email protected]> wrote:
> Hi!
>
>> for the issue to materialize you need to feed hand-crafted input to
>> unserialize,
>
> True.
>
>> anybody doing that with user controlled data already asking
>> for problems,
>
> True in theory, in practice this is widely and commonly done.
>
>> I prefer this over what we have in 5.4/5.5 and given how few classes
>> does 1, actually mean, I think it would be an acceptable compromise, but
>> let's hear what others think.
>
> Cool, waiting for others to chime in.
>
>> ps: I've seen that you created a pull request with the patch, if
>> somebody don't wanna copypaste the patch from the mail, here it is:
>> https://github.com/php/php-src/pull/701
>
> Yes, thanks for quoting it, it seems to be green on Travis and phpunit
> also seems to work fine with it. I also added a unit tests with a couple
> of cases to see how it's supposed to work.
>
> --
> Stanislav Malyshev, Software Architect
> SugarCRM: http://www.sugarcrm.com/
> (408)454-6900 ext. 227


Hello,

I find the compromise nice.
The goal is to have something barely working in most use cases for 5.4
and 5.5, and prepare something nicer and stronger for 5.6.

So, the proposed patch ( Stas' ) is nice for this, as comon tools still work.

I'm also ok for the 5.6 statements :
- Disalow O: for classes with custom serializer
- Unlock newInstanceArgWithoutConstructor() for internal classes

Note that unlocking newInstanceArgWithoutConstructor() for internal
classes may require lot of work.
Remi already tried to patch some extensions for them to work AFAIR.

Julien

--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php
Ferenc Kovacs
[PHP-DEV] Re: Bug 67072 resolution for 5.4/5.5
June 23, 2014 10:10AM
On Mon, Jun 23, 2014 at 9:54 AM, Julien Pauli <[email protected]> wrote:

> On Mon, Jun 23, 2014 at 2:20 AM, Stas Malyshev <[email protected]>
> wrote:
> > Hi!
> >
> >> for the issue to materialize you need to feed hand-crafted input to
> >> unserialize,
> >
> > True.
> >
> >> anybody doing that with user controlled data already asking
> >> for problems,
> >
> > True in theory, in practice this is widely and commonly done.
> >
> >> I prefer this over what we have in 5.4/5.5 and given how few classes
> >> does 1, actually mean, I think it would be an acceptable compromise, but
> >> let's hear what others think.
> >
> > Cool, waiting for others to chime in.
> >
> >> ps: I've seen that you created a pull request with the patch, if
> >> somebody don't wanna copypaste the patch from the mail, here it is:
> >> https://github.com/php/php-src/pull/701
> >
> > Yes, thanks for quoting it, it seems to be green on Travis and phpunit
> > also seems to work fine with it. I also added a unit tests with a couple
> > of cases to see how it's supposed to work.
> >
> > --
> > Stanislav Malyshev, Software Architect
> > SugarCRM: http://www.sugarcrm.com/
> > (408)454-6900 ext. 227
>
>
> Hello,
>
> I find the compromise nice.
> The goal is to have something barely working in most use cases for 5.4
> and 5.5, and prepare something nicer and stronger for 5.6.
>
> So, the proposed patch ( Stas' ) is nice for this, as comon tools still
> work.
>
> I'm also ok for the 5.6 statements :
> - Disalow O: for classes with custom serializer
> - Unlock newInstanceArgWithoutConstructor() for internal classes
>
> Note that unlocking newInstanceArgWithoutConstructor() for internal
> classes may require lot of work.
> Remi already tried to patch some extensions for them to work AFAIR.
>

and maybe not even possible to fix all those cases, yet we already have the
same problem with:
MyClass extends InternalClassDependingOnConstructor {
public function __construct(){
//not calling parent::__construct
}
}

so that shouldn't be a blocker for enabling internal classes
for newInstanceWithoutConstructor
but I would discuss this separately/later, as the 5.4/5.5 decision/fix is a
bit more urgent.

--
Ferenc Kovács
@Tyr43l - http://tyrael.hu
Julien Pauli
[PHP-DEV] Re: Bug 67072 resolution for 5.4/5.5
June 23, 2014 11:00AM
On Mon, Jun 23, 2014 at 10:07 AM, Ferenc Kovacs <[email protected]> wrote:
>
>
>
> On Mon, Jun 23, 2014 at 9:54 AM, Julien Pauli <[email protected]> wrote:
>>
>> On Mon, Jun 23, 2014 at 2:20 AM, Stas Malyshev <[email protected]>
>> wrote:
>> > Hi!
>> >
>> >> for the issue to materialize you need to feed hand-crafted input to
>> >> unserialize,
>> >
>> > True.
>> >
>> >> anybody doing that with user controlled data already asking
>> >> for problems,
>> >
>> > True in theory, in practice this is widely and commonly done.
>> >
>> >> I prefer this over what we have in 5.4/5.5 and given how few classes
>> >> does 1, actually mean, I think it would be an acceptable compromise,
>> >> but
>> >> let's hear what others think.
>> >
>> > Cool, waiting for others to chime in.
>> >
>> >> ps: I've seen that you created a pull request with the patch, if
>> >> somebody don't wanna copypaste the patch from the mail, here it is:
>> >> https://github.com/php/php-src/pull/701
>> >
>> > Yes, thanks for quoting it, it seems to be green on Travis and phpunit
>> > also seems to work fine with it. I also added a unit tests with a couple
>> > of cases to see how it's supposed to work.
>> >
>> > --
>> > Stanislav Malyshev, Software Architect
>> > SugarCRM: http://www.sugarcrm.com/
>> > (408)454-6900 ext. 227
>>
>>
>> Hello,
>>
>> I find the compromise nice.
>> The goal is to have something barely working in most use cases for 5.4
>> and 5.5, and prepare something nicer and stronger for 5.6.
>>
>> So, the proposed patch ( Stas' ) is nice for this, as comon tools still
>> work.
>>
>> I'm also ok for the 5.6 statements :
>> - Disalow O: for classes with custom serializer
>> - Unlock newInstanceArgWithoutConstructor() for internal classes
>>
>> Note that unlocking newInstanceArgWithoutConstructor() for internal
>> classes may require lot of work.
>> Remi already tried to patch some extensions for them to work AFAIR.
>
>
> and maybe not even possible to fix all those cases, yet we already have the
> same problem with:
> MyClass extends InternalClassDependingOnConstructor {
> public function __construct(){
> //not calling parent::__construct
> }
> }
>
> so that shouldn't be a blocker for enabling internal classes for
> newInstanceWithoutConstructor
> but I would discuss this separately/later, as the 5.4/5.5 decision/fix is a
> bit more urgent.

Yes, for 5.4 and 5.5 , Stas' patch looks right to me.

Julien

--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php
Matteo Beccati
Re: [PHP-DEV] Re: Bug 67072 resolution for 5.4/5.5
June 23, 2014 11:20AM
Hi Julien,

On 23/06/2014 09:54, Julien Pauli wrote:
> I'm also ok for the 5.6 statements :
> - Disalow O: for classes with custom serializer
> - Unlock newInstanceArgWithoutConstructor() for internal classes
>
> Note that unlocking newInstanceArgWithoutConstructor() for internal
> classes may require lot of work.
> Remi already tried to patch some extensions for them to work AFAIR.

With 5.6.0 being RC1 already, would someone have time and be willing to
commit doing all the work required without delaying the stable release?
I'm not entirely sure, but I certainly hope I'm wrong.


Cheers
--
Matteo Beccati

Development & Consulting - http://www.beccati.com/

--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php
Ferenc Kovacs
Re: [PHP-DEV] Re: Bug 67072 resolution for 5.4/5.5
June 23, 2014 11:30AM
On Mon, Jun 23, 2014 at 11:16 AM, Matteo Beccati <[email protected]> wrote:

> Hi Julien,
>
> On 23/06/2014 09:54, Julien Pauli wrote:
> > I'm also ok for the 5.6 statements :
> > - Disalow O: for classes with custom serializer
> > - Unlock newInstanceArgWithoutConstructor() for internal classes
> >
> > Note that unlocking newInstanceArgWithoutConstructor() for internal
> > classes may require lot of work.
> > Remi already tried to patch some extensions for them to work AFAIR.
>
> With 5.6.0 being RC1 already, would someone have time and be willing to
> commit doing all the work required without delaying the stable release?
> I'm not entirely sure, but I certainly hope I'm wrong.
>
>
What do you mean?
I can promise that the 5.6 RMs (Julien and Me) will make sure that we have
this sorted out for 5.6 before we have the final release if that is what
you are asking for.

--
Ferenc Kovács
@Tyr43l - http://tyrael.hu
Julien Pauli
Re: [PHP-DEV] Re: Bug 67072 resolution for 5.4/5.5
June 23, 2014 11:50AM
On Mon, Jun 23, 2014 at 11:19 AM, Ferenc Kovacs <[email protected]> wrote:
>
>
>
> On Mon, Jun 23, 2014 at 11:16 AM, Matteo Beccati <[email protected]> wrote:
>>
>> Hi Julien,
>>
>> On 23/06/2014 09:54, Julien Pauli wrote:
>> > I'm also ok for the 5.6 statements :
>> > - Disalow O: for classes with custom serializer
>> > - Unlock newInstanceArgWithoutConstructor() for internal classes
>> >
>> > Note that unlocking newInstanceArgWithoutConstructor() for internal
>> > classes may require lot of work.
>> > Remi already tried to patch some extensions for them to work AFAIR.
>>
>> With 5.6.0 being RC1 already, would someone have time and be willing to
>> commit doing all the work required without delaying the stable release?
>> I'm not entirely sure, but I certainly hope I'm wrong.
>>
>
> What do you mean?
> I can promise that the 5.6 RMs (Julien and Me) will make sure that we have
> this sorted out for 5.6 before we have the final release if that is what you
> are asking for.

Sure. We can delay. We may delay.
If the quality of the project is involved, we will delay.

5.4 has been delayed, with 8 RCs before final, because of unresolvable
bug at this time.
However, for 5.6, we are already late on the schedule. Last RC of 5.4
was taggued in February , our 1st RC for 5.6 has been taggued in June.

If we go for checking all the internal classes shipped in the
distribution, then we should start the task ASAP.
We cant delay too much, and I think it is reasonable to have a release
during the summer.

Julien

--
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