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:
- Original fix banned O: unserialization of classes that have custom
serializers. - The following fix allowed this behavior for user classes.
- The second fix means that if the user class descends from internal
class with serializer, we still have a problem.
My opinion is this:
-
Using
unserialize()on anything that is not a result ofserialize()
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. -
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. -
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
On Wed, Jun 18, 2014 at 12:53 AM, Stas Malyshev smalyshev@sugarcrm.com
wrote:
Hi!
Could you check out my last mail about the unserialize stuff?:
http://news.php.net/php.internals/74947Reading this message, I gather the situation is as follows:
- 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.
- The following fix allowed this behavior for user classes.
yes
- 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:
- Using
unserialize()on anything that is not a result ofserialize()
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
- 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.
- 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
Le 18/06/2014 10:03, Ferenc Kovacs a écrit :
- 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.
Le 18/06/2014 10:03, Ferenc Kovacs a écrit :
- 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.
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
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?
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
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
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
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
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)
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
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 smalyshev@sugarcrm.com
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--
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
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
Hi!
Wouldn't it be much easier to just revert for 5.4 and 5.5 and then
patchingReflectionClass#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
On Fri, Jun 20, 2014 at 2:58 AM, Stas Malyshev smalyshev@sugarcrm.com
wrote:
Hi!
Wouldn't it be much easier to just revert for 5.4 and 5.5 and then
patchingReflectionClass#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:
- 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. - 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.
- they should use ReflectionClass::newInstanceWithoutConstructor for
- 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.
- they should use ReflectionClass::newInstanceWithoutConstructor for
(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
Just a follow-up on this: it looks like the most common use-cases affected
by the breakage are about classes extending ArrayObject, which is
reasonable.
Creating a new instance without invoking their constructor is now not
possible in 5.4.30, 5.5.14 and 5.6.0-rc.1 because of the "invalid
serialization format" error.
The only workaround I found is creating a new class at runtime, overriding
the constructor and instantiating it, which has the horrible side-effect of
get_class() obviously returning something different from expectations,
and is going to break other things in codebases that highly rely on
reflection.
Can anybody suggest a workaround for this problem?
Should ReflectionClass#newInstanceWithoutConstructor() be enabled for
those classes?
If so, in 5.4/5.5?
Is this just a dead end and are we supposed to just disallow mocking on
those particular classes?
My other solution would be to build a map (case-by-case) of internal PHP
classes and custom ways of instantiating/serializing them.
Something like following example, which is horror: http://3v4l.org/oSPvF
An additional problem is when a userland class extends an internal class
AND implements the Serializable interface on its own.
In such cases, knowing the serialization format is impossible for us, and
ReflectionClass#newInstanceWithoutConstructor() still cannot be used.
Any direction here would be useful. I'm considering going down the
"exception thrown" way, as suggested by Ferenc, but for 50600 it would be
neat to just have ReflectionClass doing the trick.
Marco Pivetta
Hi!
Can anybody suggest a workaround for this problem?
ShouldReflectionClass#newInstanceWithoutConstructor()be enabled for
those classes?
If so, in 5.4/5.5?
I think we should move away from the practice of using serializer for
something it was never made for, namely a weird way of instantiating
classes. Serializer should be working only with serialized data.
Now, the question is can we instantiate the internal class without
calling its ctor, and the answer here would probably be "no", at least
not safely. While in the case of user class the engine can be reasonable
sure even if you don't call the ctor the basic structures are
initialized properly, in the case of the internal class all bets are
off. I'm not sure yet which use cases require ctor not to be called, but
I'm not sure how we can deliver on internal classes here.
An additional problem is when a userland class extends an internal class
AND implements theSerializableinterface on its own.
In such cases, knowing the serialization format is impossible for us,
andReflectionClass#newInstanceWithoutConstructor()still cannot be used.
Again, I would like to strongly suggest not using serialization format
for hacks. It's just not what it's for, and we already suffering the
consequences. We need some other solution here. Let's start with this:
what gives us the guarantee that internal class which is extended by
userland class will be found in proper state without calling the ctor?
--
Stanislav Malyshev, Software Architect
SugarCRM: http://www.sugarcrm.com/
2014.06.30. 9:10, "Stas Malyshev" smalyshev@sugarcrm.com ezt írta:
Hi!
Can anybody suggest a workaround for this problem?
ShouldReflectionClass#newInstanceWithoutConstructor()be enabled for
those classes?
If so, in 5.4/5.5?I think we should move away from the practice of using serializer for
something it was never made for, namely a weird way of instantiating
classes. Serializer should be working only with serialized data.Now, the question is can we instantiate the internal class without
calling its ctor, and the answer here would probably be "no", at least
not safely. While in the case of user class the engine can be reasonable
sure even if you don't call the ctor the basic structures are
initialized properly, in the case of the internal class all bets are
off. I'm not sure yet which use cases require ctor not to be called, but
I'm not sure how we can deliver on internal classes here.An additional problem is when a userland class extends an internal class
AND implements theSerializableinterface on its own.
In such cases, knowing the serialization format is impossible for us,
andReflectionClass#newInstanceWithoutConstructor()still cannot be
used.Again, I would like to strongly suggest not using serialization format
for hacks. It's just not what it's for, and we already suffering the
consequences. We need some other solution here. Let's start with this:
what gives us the guarantee that internal class which is extended by
userland class will be found in proper state without calling the ctor?
currently nothing.
we could
1, make sure that every internal class is fine without the constructor
(lazy initiation for example). this would be probably the most work.
2, we could make it mandatory or allow internal classes to opt-in to
mandatory that the base constructor is always called.
3, we could turn the mandatory contructors final.
1, would allow us to keep instantiating the internal classes without
constructors, but it would be error prune imo.
2/3 would could defend against the segfaults/etc but would be a pita to the
userland.
I think that the mid/long range solution should be to provide a way to
dynamically create objects which doesn't inherit code from the original
class/interface but the signature should be compatible
(Sebastian opened a separate thread about using anonymous classes for
this.), but I don't think we have time for that in 5.6.
But we should provide an upgrade path, so the current status in 5.6 is a
no-go imo.
2014.06.30. 9:10, "Stas Malyshev" smalyshev@sugarcrm.com ezt írta:
Hi!
Can anybody suggest a workaround for this problem?
ShouldReflectionClass#newInstanceWithoutConstructor()be enabled for
those classes?
If so, in 5.4/5.5?I think we should move away from the practice of using serializer for
something it was never made for, namely a weird way of instantiating
classes. Serializer should be working only with serialized data.Now, the question is can we instantiate the internal class without
calling its ctor, and the answer here would probably be "no", at least
not safely. While in the case of user class the engine can be reasonable
sure even if you don't call the ctor the basic structures are
initialized properly, in the case of the internal class all bets are
off. I'm not sure yet which use cases require ctor not to be called, but
I'm not sure how we can deliver on internal classes here.An additional problem is when a userland class extends an internal class
AND implements theSerializableinterface on its own.
In such cases, knowing the serialization format is impossible for us,
andReflectionClass#newInstanceWithoutConstructor()still cannot be
used.Again, I would like to strongly suggest not using serialization format
for hacks. It's just not what it's for, and we already suffering the
consequences. We need some other solution here. Let's start with this:
what gives us the guarantee that internal class which is extended by
userland class will be found in proper state without calling the ctor?currently nothing.
we could
1, make sure that every internal class is fine without the constructor (lazy
initiation for example). this would be probably the most work.
2, we could make it mandatory or allow internal classes to opt-in to
mandatory that the base constructor is always called.
3, we could turn the mandatory contructors final.1, would allow us to keep instantiating the internal classes without
constructors, but it would be error prune imo.
2/3 would could defend against the segfaults/etc but would be a pita to the
userland.I think that the mid/long range solution should be to provide a way to
dynamically create objects which doesn't inherit code from the original
class/interface but the signature should be compatible
(Sebastian opened a separate thread about using anonymous classes for
this.), but I don't think we have time for that in 5.6.
But we should provide an upgrade path, so the current status in 5.6 is a
no-go imo.
I agree that the problem seem to have no solution.
An object has a constructor => you can't have an object without
calling its constructor.
Passing the theoretical concepts, Stas just proved that the practical
are just right : we can't assume internal class will work without
calling their constructor as they usually initialize internal
structures and states.
Lazy init also is not a viable option , we can't put INIT_CHECK()
macros everywhere.
Perhaps anonymous classes ?
Or just to know , why can't you create a class that extends the one
you want, and write an empty constructor into it ?
That way you'd be able to create an object using "new" , and redefine
the methods you want to make them return what you want. am I wrong ?
Julien
2014.06.30. 9:10, "Stas Malyshev" smalyshev@sugarcrm.com ezt írta:
Hi!
Can anybody suggest a workaround for this problem?
ShouldReflectionClass#newInstanceWithoutConstructor()be enabled
for
those classes?
If so, in 5.4/5.5?I think we should move away from the practice of using serializer for
something it was never made for, namely a weird way of instantiating
classes. Serializer should be working only with serialized data.Now, the question is can we instantiate the internal class without
calling its ctor, and the answer here would probably be "no", at least
not safely. While in the case of user class the engine can be reasonable
sure even if you don't call the ctor the basic structures are
initialized properly, in the case of the internal class all bets are
off. I'm not sure yet which use cases require ctor not to be called, but
I'm not sure how we can deliver on internal classes here.An additional problem is when a userland class extends an internal
class
AND implements theSerializableinterface on its own.
In such cases, knowing the serialization format is impossible for us,
andReflectionClass#newInstanceWithoutConstructor()still cannot be
used.Again, I would like to strongly suggest not using serialization format
for hacks. It's just not what it's for, and we already suffering the
consequences. We need some other solution here. Let's start with this:
what gives us the guarantee that internal class which is extended by
userland class will be found in proper state without calling the ctor?currently nothing.
we could
1, make sure that every internal class is fine without the constructor
(lazy
initiation for example). this would be probably the most work.
2, we could make it mandatory or allow internal classes to opt-in to
mandatory that the base constructor is always called.
3, we could turn the mandatory contructors final.1, would allow us to keep instantiating the internal classes without
constructors, but it would be error prune imo.
2/3 would could defend against the segfaults/etc but would be a pita to
the
userland.I think that the mid/long range solution should be to provide a way to
dynamically create objects which doesn't inherit code from the original
class/interface but the signature should be compatible
(Sebastian opened a separate thread about using anonymous classes for
this.), but I don't think we have time for that in 5.6.
But we should provide an upgrade path, so the current status in 5.6 is a
no-go imo.I agree that the problem seem to have no solution.
An object has a constructor => you can't have an object without
calling its constructor.
currently it is allowed to extend the class and override the __construct()
to not call the parents.
Passing the theoretical concepts, Stas just proved that the practical
are just right : we can't assume internal class will work without
calling their constructor as they usually initialize internal
structures and states.
more than that, Internal classes can initialize and depend on properties
that you can't reproduce from an userland method (without calling the
original __construct()).
Lazy init also is not a viable option , we can't put INIT_CHECK()
macros everywhere.
and some of those macros would do what __construct() would do anyways, so
if you are instantiating without a constructor so that there is no code
execution, you wouldn't want lazy init also.
Perhaps anonymous classes ?
yeah, that is one of the proposed solution.
Or just to know , why can't you create a class that extends the one
you want, and write an empty constructor into it ?
some mocking frameworks uses that concept for instanitating objects:
http://news.php.net/php.internals/75125
That way you'd be able to create an object using "new" , and redefine
the methods you want to make them return what you want. am I wrong ?
but this will still produce incomplete/unstable objects which are exposed
to the dos/security problems.
so I think that supporting the creation of mocks/doubles natively(through
Reflection)
I think it would make sense to create a wiki page (rfc maybe) and summarize
the topic, because I got the feeling that there are some ideas/suggestions
which are brought up multiple times as if they were not mentioned before. I
will try to write this up today or tomorrow and then send a mail about it
so you guys can review and extend on it.
--
Ferenc Kovács
@Tyr43l - http://tyrael.hu
Perhaps anonymous classes ?
yeah, that is one of the proposed solution.
I can't really understand how would they help with the internal classes?
Would they call constructor or not? If not - the problem is still there
isn't it?
That way you'd be able to create an object using "new" , and redefine
the methods you want to make them return what you want. am I wrong ?but this will still produce incomplete/unstable objects which are exposed
to the dos/security problems.
so I think that supporting the creation of mocks/doubles natively(through
Reflection)
Natively. But again, how can we do this generically? I mean not touching
definition of the classes itself. You will still need to instantiate an
object with the internal class entry, or am I wrong here?
If it comes only to the problem of mocking the solution could also be to
allow usage of classes as an interfaces. So for example if we will be able
to write something like this: class MockedSplFileInfo implements
\SplFileInfo { public function getBaseName(){ /* some mocking logic */ }
... and so on ... }
I don't see any way to keep the ability to instantiate internal class
without constructor and to guarantee its stability at the same time. The
only way is to not instantiate it. (assuming we don't ask classes to make
init checks on every possible call)
Perhaps anonymous classes ?
Or just to know , why can't you create a class that extends the one
you want, and write an empty constructor into it ?
That way you'd be able to create an object using "new" , and redefine
the methods you want to make them return what you want. am I wrong ?
That's what I've been doing so far, but the method doesn't work with:
- final constructors
- APIs that rely on reflection-ish logic (
get_class())
Marco Pivetta
I think we should move away from the practice of using serializer for
something it was never made for, namely a weird way of instantiating
classes. Serializer should be working only with serialized data.
Now, the question is can we instantiate the internal class without
calling its ctor, and the answer here would probably be "no", at least
not safely. While in the case of user class the engine can be reasonable
sure even if you don't call the ctor the basic structures are
initialized properly, in the case of the internal class all bets are
off. I'm not sure yet which use cases require ctor not to be called, but
I'm not sure how we can deliver on internal classes here.
Sorry, I’m a bit late to discussion and might be missing something obvious.
As far as I remember, internal classes have 2 constructors.
- implementation of __construct() function
- create_object hook
It should be possible to keep unsafe stuff in create_object which is called unconditionally and leave safe initialisation in __construct.
so, in case when __construct is not called object will have properties initialised with nulls, empty strings, etc.
I understand that is a lot of work on case-by-case basis but it is doable.
--
Alexey Zakhlestin
CTO at Grids.by/you
https://github.com/indeyets
PGP key: http://indeyets.ru/alexey.zakhlestin.pgp.asc
On Mon, Jun 30, 2014 at 8:10 PM, Alexey Zakhlestin indeyets@gmail.com
wrote:
I think we should move away from the practice of using serializer for
something it was never made for, namely a weird way of instantiating
classes. Serializer should be working only with serialized data.Now, the question is can we instantiate the internal class without
calling its ctor, and the answer here would probably be "no", at least
not safely. While in the case of user class the engine can be reasonable
sure even if you don't call the ctor the basic structures are
initialized properly, in the case of the internal class all bets are
off. I'm not sure yet which use cases require ctor not to be called, but
I'm not sure how we can deliver on internal classes here.Sorry, I’m a bit late to discussion and might be missing something obvious.
As far as I remember, internal classes have 2 constructors.
- implementation of __construct() function
- create_object hook
It should be possible to keep unsafe stuff in create_object which is
called unconditionally and leave safe initialisation in __construct.so, in case when __construct is not called object will have properties
initialised with nulls, empty strings, etc.I understand that is a lot of work on case-by-case basis but it is doable.
sorry for the late reply.
you are right and after looking into the implementation I think classes
having custom object storage (eg. create_object) shouldn't assume that
their __construct will be called, but either do the initialization in the
create_object hook or validate in the other methods that the object was
properly initialized.
given that this lack of initialization problem is already present(you can
extend such a class and have a __construct() in the subclass which doesn't
call the parent constructor), and we want to keep the unserialize trick
fixed (as that exposes this problems to the remote attacker when some code
accepts arbitrary serialized data from the client) I see no reason to keep
the limitation in the ReflectionClass::newInstanceWithoutConstructor() and
allowing the instantiation of internal classes will provide a clean upgrade
path to doctrine and co.
ofc. we have to fix the internal classes misusing the constructor for
proper initialization one by one, but that can happen after the initial
5.6.0 release (ofc it would be wonderful if people could lend me a hand for
fixing as much as we can before the release), but we have to fix those
anyways.
I consider this problem the biggest impediment for releasing the 5.6.0
final, so I would really like to hear what do you guys think about my
proposed solution?
Ferenc Kovács
@Tyr43l - http://tyrael.hu
sorry for the late reply.
you are right and after looking into the implementation I think classes
having custom object storage (eg. create_object) shouldn't assume that
their __construct will be called, but either do the initialization in the
create_object hook or validate in the other methods that the object was
properly initialized.
given that this lack of initialization problem is already present(you can
extend such a class and have a __construct() in the subclass which doesn't
call the parent constructor), and we want to keep the unserialize trick
fixed (as that exposes this problems to the remote attacker when some code
accepts arbitrary serialized data from the client) I see no reason to keep
the limitation in the ReflectionClass::newInstanceWithoutConstructor() and
allowing the instantiation of internal classes will provide a clean upgrade
path to doctrine and co.
ofc. we have to fix the internal classes misusing the constructor for
proper initialization one by one, but that can happen after the initial
5.6.0 release (ofc it would be wonderful if people could lend me a hand for
fixing as much as we can before the release), but we have to fix those
anyways.
This sounds reasonable to me. newInstanceWithoutConstructor does not have
the same remote exploitation concerns as serialize, so allowing crashes
here that can also be caused by other means seems okay to me (especially if
we're planning to fix them lateron). Only additional restriction I'd add is
to disallow calling it on an internal + final class. For those the
__construct argument does not apply. For them the
entity-extending-internal-class usecase doesn't apply either, so that
shouldn't be a problem.
Nikita
sorry for the late reply.
you are right and after looking into the implementation I think classes
having custom object storage (eg. create_object) shouldn't assume that
their __construct will be called, but either do the initialization in the
create_object hook or validate in the other methods that the object was
properly initialized.
given that this lack of initialization problem is already present(you can
extend such a class and have a __construct() in the subclass which doesn't
call the parent constructor), and we want to keep the unserialize trick
fixed (as that exposes this problems to the remote attacker when some code
accepts arbitrary serialized data from the client) I see no reason to keep
the limitation in the ReflectionClass::newInstanceWithoutConstructor() and
allowing the instantiation of internal classes will provide a clean
upgrade
path to doctrine and co.
ofc. we have to fix the internal classes misusing the constructor for
proper initialization one by one, but that can happen after the initial
5.6.0 release (ofc it would be wonderful if people could lend me a hand
for
fixing as much as we can before the release), but we have to fix those
anyways.This sounds reasonable to me. newInstanceWithoutConstructor does not have
the same remote exploitation concerns as serialize, so allowing crashes
here that can also be caused by other means seems okay to me (especially if
we're planning to fix them lateron). Only additional restriction I'd add is
to disallow calling it on an internal + final class. For those the
__construct argument does not apply. For them the
entity-extending-internal-class usecase doesn't apply either, so that
shouldn't be a problem.Nikita
Thanks for the prompt reply!
I was considering mentioning the final constructors, but as we previously
didn't checked that and from a quick look it seems that we are mostly using
it as an easy/cheap way to make sure that the object is initialized
properly (which could also happen when a subclass calls
parent::__construct() from it's constructor) which isn't exactly the best
usecase for final.
But I don't really mind having that check.
--
Ferenc Kovács
@Tyr43l - http://tyrael.hu
sorry for the late reply.
you are right and after looking into the implementation I think classes
having custom object storage (eg. create_object) shouldn't assume that
their __construct will be called, but either do the initialization in the
create_object hook or validate in the other methods that the object was
properly initialized.
given that this lack of initialization problem is already present(you can
extend such a class and have a __construct() in the subclass which
doesn't
call the parent constructor), and we want to keep the unserialize trick
fixed (as that exposes this problems to the remote attacker when some
code
accepts arbitrary serialized data from the client) I see no reason to
keep
the limitation in the ReflectionClass::newInstanceWithoutConstructor()
and
allowing the instantiation of internal classes will provide a clean
upgrade
path to doctrine and co.
ofc. we have to fix the internal classes misusing the constructor for
proper initialization one by one, but that can happen after the initial
5.6.0 release (ofc it would be wonderful if people could lend me a hand
for
fixing as much as we can before the release), but we have to fix those
anyways.This sounds reasonable to me. newInstanceWithoutConstructor does not have
the same remote exploitation concerns as serialize, so allowing crashes here
that can also be caused by other means seems okay to me (especially if we're
planning to fix them lateron). Only additional restriction I'd add is to
disallow calling it on an internal + final class. For those the __construct
argument does not apply. For them the entity-extending-internal-class
usecase doesn't apply either, so that shouldn't be a problem.Nikita
Thanks for the prompt reply!
I was considering mentioning the final constructors, but as we previously
didn't checked that and from a quick look it seems that we are mostly using
it as an easy/cheap way to make sure that the object is initialized properly
(which could also happen when a subclass calls parent::__construct() from
it's constructor) which isn't exactly the best usecase for final.
But I don't really mind having that check.
I'm +1 also with the idea.
Just take care to have a clone_handler defined as well, as the default
clone handler doesn't call create_object.
http://lxr.php.net/xref/PHP_5_5/Zend/zend_objects.c#218
Julien
On Tue, Jul 22, 2014 at 1:48 PM, Nikita Popov nikita.ppv@gmail.com
wrote:On Tue, Jul 22, 2014 at 12:27 PM, Ferenc Kovacs tyra3l@gmail.com
wrote:sorry for the late reply.
you are right and after looking into the implementation I think classes
having custom object storage (eg. create_object) shouldn't assume that
their __construct will be called, but either do the initialization in
the
create_object hook or validate in the other methods that the object was
properly initialized.
given that this lack of initialization problem is already present(you
can
extend such a class and have a __construct() in the subclass which
doesn't
call the parent constructor), and we want to keep the unserialize trick
fixed (as that exposes this problems to the remote attacker when some
code
accepts arbitrary serialized data from the client) I see no reason to
keep
the limitation in the ReflectionClass::newInstanceWithoutConstructor()
and
allowing the instantiation of internal classes will provide a clean
upgrade
path to doctrine and co.
ofc. we have to fix the internal classes misusing the constructor for
proper initialization one by one, but that can happen after the initial
5.6.0 release (ofc it would be wonderful if people could lend me a hand
for
fixing as much as we can before the release), but we have to fix those
anyways.This sounds reasonable to me. newInstanceWithoutConstructor does not
have
the same remote exploitation concerns as serialize, so allowing crashes
here
that can also be caused by other means seems okay to me (especially if
we're
planning to fix them lateron). Only additional restriction I'd add is to
disallow calling it on an internal + final class. For those the
__construct
argument does not apply. For them the entity-extending-internal-class
usecase doesn't apply either, so that shouldn't be a problem.Nikita
Thanks for the prompt reply!
I was considering mentioning the final constructors, but as we previously
didn't checked that and from a quick look it seems that we are mostly
using
it as an easy/cheap way to make sure that the object is initialized
properly
(which could also happen when a subclass calls parent::__construct() from
it's constructor) which isn't exactly the best usecase for final.
But I don't really mind having that check.I'm +1 also with the idea.
Just take care to have a clone_handler defined as well, as the default
clone handler doesn't call create_object.
http://lxr.php.net/xref/PHP_5_5/Zend/zend_objects.c#218Julien
thanks, I will keep that in mind when we start moving the initialization
from the constructors into the create_object functions.
I've also went ahead and created a pull request for the proposed changes:
https://github.com/php/php-src/pull/733
as you can see I've taken Nikita's advice and internal classes with final
constructors are still not allowed to be instantiated.
--
Ferenc Kovács
@Tyr43l - http://tyrael.hu
On Tue, Jul 22, 2014 at 1:48 PM, Nikita Popov nikita.ppv@gmail.com
wrote:On Tue, Jul 22, 2014 at 12:27 PM, Ferenc Kovacs tyra3l@gmail.com
wrote:sorry for the late reply.
you are right and after looking into the implementation I think
classes
having custom object storage (eg. create_object) shouldn't assume that
their __construct will be called, but either do the initialization in
the
create_object hook or validate in the other methods that the object
was
properly initialized.
given that this lack of initialization problem is already present(you
can
extend such a class and have a __construct() in the subclass which
doesn't
call the parent constructor), and we want to keep the unserialize
trick
fixed (as that exposes this problems to the remote attacker when some
code
accepts arbitrary serialized data from the client) I see no reason to
keep
the limitation in the ReflectionClass::newInstanceWithoutConstructor()
and
allowing the instantiation of internal classes will provide a clean
upgrade
path to doctrine and co.
ofc. we have to fix the internal classes misusing the constructor for
proper initialization one by one, but that can happen after the
initial
5.6.0 release (ofc it would be wonderful if people could lend me a
hand
for
fixing as much as we can before the release), but we have to fix those
anyways.This sounds reasonable to me. newInstanceWithoutConstructor does not
have
the same remote exploitation concerns as serialize, so allowing crashes
here
that can also be caused by other means seems okay to me (especially if
we're
planning to fix them lateron). Only additional restriction I'd add is
to
disallow calling it on an internal + final class. For those the
__construct
argument does not apply. For them the entity-extending-internal-class
usecase doesn't apply either, so that shouldn't be a problem.Nikita
Thanks for the prompt reply!
I was considering mentioning the final constructors, but as we
previously
didn't checked that and from a quick look it seems that we are mostly
using
it as an easy/cheap way to make sure that the object is initialized
properly
(which could also happen when a subclass calls parent::__construct()
from
it's constructor) which isn't exactly the best usecase for final.
But I don't really mind having that check.I'm +1 also with the idea.
Just take care to have a clone_handler defined as well, as the default
clone handler doesn't call create_object.
http://lxr.php.net/xref/PHP_5_5/Zend/zend_objects.c#218Julien
thanks, I will keep that in mind when we start moving the initialization
from the constructors into the create_object functions.
I've also went ahead and created a pull request for the proposed changes:
https://github.com/php/php-src/pull/733
as you can see I've taken Nikita's advice and internal classes with final
constructors are still not allowed to be instantiated.
When should we start patching those ?
I guess asap ?
Julien
On Tue, Jul 22, 2014 at 2:04 PM, Ferenc Kovacs tyra3l@gmail.com
wrote:On Tue, Jul 22, 2014 at 1:48 PM, Nikita Popov nikita.ppv@gmail.com
wrote:On Tue, Jul 22, 2014 at 12:27 PM, Ferenc Kovacs tyra3l@gmail.com
wrote:sorry for the late reply.
you are right and after looking into the implementation I think
classes
having custom object storage (eg. create_object) shouldn't assume
that
their __construct will be called, but either do the initialization
in
the
create_object hook or validate in the other methods that the object
was
properly initialized.
given that this lack of initialization problem is already
present(you
can
extend such a class and have a __construct() in the subclass which
doesn't
call the parent constructor), and we want to keep the unserialize
trick
fixed (as that exposes this problems to the remote attacker when
some
code
accepts arbitrary serialized data from the client) I see no reason
to
keep
the limitation in the
ReflectionClass::newInstanceWithoutConstructor()
and
allowing the instantiation of internal classes will provide a clean
upgrade
path to doctrine and co.
ofc. we have to fix the internal classes misusing the constructor
for
proper initialization one by one, but that can happen after the
initial
5.6.0 release (ofc it would be wonderful if people could lend me a
hand
for
fixing as much as we can before the release), but we have to fix
those
anyways.This sounds reasonable to me. newInstanceWithoutConstructor does not
have
the same remote exploitation concerns as serialize, so allowing
crashes
here
that can also be caused by other means seems okay to me (especially
if
we're
planning to fix them lateron). Only additional restriction I'd add is
to
disallow calling it on an internal + final class. For those the
__construct
argument does not apply. For them the entity-extending-internal-class
usecase doesn't apply either, so that shouldn't be a problem.Nikita
Thanks for the prompt reply!
I was considering mentioning the final constructors, but as we
previously
didn't checked that and from a quick look it seems that we are mostly
using
it as an easy/cheap way to make sure that the object is initialized
properly
(which could also happen when a subclass calls parent::__construct()
from
it's constructor) which isn't exactly the best usecase for final.
But I don't really mind having that check.I'm +1 also with the idea.
Just take care to have a clone_handler defined as well, as the default
clone handler doesn't call create_object.
http://lxr.php.net/xref/PHP_5_5/Zend/zend_objects.c#218Julien
thanks, I will keep that in mind when we start moving the initialization
from the constructors into the create_object functions.
I've also went ahead and created a pull request for the proposed changes:
https://github.com/php/php-src/pull/733
as you can see I've taken Nikita's advice and internal classes with final
constructors are still not allowed to be instantiated.When should we start patching those ?
I guess asap ?Julien
Not sure, on one hand, I would be glad seeing these fixed, but on the other
hand I'm a little bit worried about introducing destabilizing changes this
close to the release, and these problems existed for years (triggerable
either through instantiating via the unserialize O: trick or through a
subclass) without any reports before https://bugs.php.net/bug.php?id=67072
--
Ferenc Kovács
@Tyr43l - http://tyrael.hu
On Tue, Jul 22, 2014 at 2:04 PM, Ferenc Kovacs tyra3l@gmail.com
wrote:On Tue, Jul 22, 2014 at 1:48 PM, Nikita Popov nikita.ppv@gmail.com
wrote:On Tue, Jul 22, 2014 at 12:27 PM, Ferenc Kovacs tyra3l@gmail.com
wrote:sorry for the late reply.
you are right and after looking into the implementation I think
classes
having custom object storage (eg. create_object) shouldn't assume
that
their __construct will be called, but either do the initialization
in
the
create_object hook or validate in the other methods that the object
was
properly initialized.
given that this lack of initialization problem is already
present(you
can
extend such a class and have a __construct() in the subclass which
doesn't
call the parent constructor), and we want to keep the unserialize
trick
fixed (as that exposes this problems to the remote attacker when
some
code
accepts arbitrary serialized data from the client) I see no reason
to
keep
the limitation in the
ReflectionClass::newInstanceWithoutConstructor()
and
allowing the instantiation of internal classes will provide a clean
upgrade
path to doctrine and co.
ofc. we have to fix the internal classes misusing the constructor
for
proper initialization one by one, but that can happen after the
initial
5.6.0 release (ofc it would be wonderful if people could lend me a
hand
for
fixing as much as we can before the release), but we have to fix
those
anyways.This sounds reasonable to me. newInstanceWithoutConstructor does not
have
the same remote exploitation concerns as serialize, so allowing
crashes
here
that can also be caused by other means seems okay to me (especially
if
we're
planning to fix them lateron). Only additional restriction I'd add
is
to
disallow calling it on an internal + final class. For those the
__construct
argument does not apply. For them the
entity-extending-internal-class
usecase doesn't apply either, so that shouldn't be a problem.Nikita
Thanks for the prompt reply!
I was considering mentioning the final constructors, but as we
previously
didn't checked that and from a quick look it seems that we are mostly
using
it as an easy/cheap way to make sure that the object is initialized
properly
(which could also happen when a subclass calls parent::__construct()
from
it's constructor) which isn't exactly the best usecase for final.
But I don't really mind having that check.I'm +1 also with the idea.
Just take care to have a clone_handler defined as well, as the default
clone handler doesn't call create_object.
http://lxr.php.net/xref/PHP_5_5/Zend/zend_objects.c#218Julien
thanks, I will keep that in mind when we start moving the initialization
from the constructors into the create_object functions.
I've also went ahead and created a pull request for the proposed
changes:
https://github.com/php/php-src/pull/733
as you can see I've taken Nikita's advice and internal classes with
final
constructors are still not allowed to be instantiated.When should we start patching those ?
I guess asap ?Julien
Not sure, on one hand, I would be glad seeing these fixed, but on the other
hand I'm a little bit worried about introducing destabilizing changes this
close to the release, and these problems existed for years (triggerable
either through instantiating via the unserialize O: trick or through a
subclass) without any reports before https://bugs.php.net/bug.php?id=67072
Agree.
We could start on a case by case basis, knowing we still got at least
one RC to try that.
Then anyway we're gonna fix them into next 5.6 revisions, so ...
Julien.P
On Tue, Jul 22, 2014 at 2:04 PM, Ferenc Kovacs tyra3l@gmail.com
wrote:On Tue, Jul 22, 2014 at 1:48 PM, Nikita Popov nikita.ppv@gmail.com
wrote:On Tue, Jul 22, 2014 at 12:27 PM, Ferenc Kovacs tyra3l@gmail.com
wrote:sorry for the late reply.
you are right and after looking into the implementation I think
classes
having custom object storage (eg. create_object) shouldn't assume
that
their __construct will be called, but either do the initialization
in
the
create_object hook or validate in the other methods that the object
was
properly initialized.
given that this lack of initialization problem is already
present(you
can
extend such a class and have a __construct() in the subclass which
doesn't
call the parent constructor), and we want to keep the unserialize
trick
fixed (as that exposes this problems to the remote attacker when
some
code
accepts arbitrary serialized data from the client) I see no reason
to
keep
the limitation in the
ReflectionClass::newInstanceWithoutConstructor()
and
allowing the instantiation of internal classes will provide a clean
upgrade
path to doctrine and co.
ofc. we have to fix the internal classes misusing the constructor
for
proper initialization one by one, but that can happen after the
initial
5.6.0 release (ofc it would be wonderful if people could lend me a
hand
for
fixing as much as we can before the release), but we have to fix
those
anyways.This sounds reasonable to me. newInstanceWithoutConstructor does not
have
the same remote exploitation concerns as serialize, so allowing
crashes
here
that can also be caused by other means seems okay to me (especially
if
we're
planning to fix them lateron). Only additional restriction I'd add
is
to
disallow calling it on an internal + final class. For those the
__construct
argument does not apply. For them the
entity-extending-internal-class
usecase doesn't apply either, so that shouldn't be a problem.Nikita
Thanks for the prompt reply!
I was considering mentioning the final constructors, but as we
previously
didn't checked that and from a quick look it seems that we are mostly
using
it as an easy/cheap way to make sure that the object is initialized
properly
(which could also happen when a subclass calls parent::__construct()
from
it's constructor) which isn't exactly the best usecase for final.
But I don't really mind having that check.I'm +1 also with the idea.
Just take care to have a clone_handler defined as well, as the default
clone handler doesn't call create_object.
http://lxr.php.net/xref/PHP_5_5/Zend/zend_objects.c#218Julien
thanks, I will keep that in mind when we start moving the initialization
from the constructors into the create_object functions.
I've also went ahead and created a pull request for the proposed
changes:
https://github.com/php/php-src/pull/733
as you can see I've taken Nikita's advice and internal classes with
final
constructors are still not allowed to be instantiated.When should we start patching those ?
I guess asap ?Julien
Not sure, on one hand, I would be glad seeing these fixed, but on the other
hand I'm a little bit worried about introducing destabilizing changes this
close to the release, and these problems existed for years (triggerable
either through instantiating via the unserialize O: trick or through a
subclass) without any reports before https://bugs.php.net/bug.php?id=67072Agree.
We could start on a case by case basis, knowing we still got at least
one RC to try that.
Then anyway we're gonna fix them into next 5.6 revisions, so ...Julien.P
I spent some time today reviewing our internal bundled extensions
searching the ones that overwrite create_object and have a dangerous
__construct constructing internal object data.
What I found not safe, throwing unwanted warnings hitting an
undesired behavior, or even segfaulting, and thus needing patch :
- Dom
- Mysqli
- sqlite3 (sqlite3stmt class segfaults)
What I found safe, with checkers that check object is properly
initialized, so not needing patch, but throwing exceptions or warnings
when used bad constructed :
- Date
- fileinfo
- Intl
- Pdo
- Reflection
- SimpleXML* (not faulting, but could need a better implementation of
the checks and the thrown error messages) - SPL
Any extension not present in one of the two above lists is safe.
The method I used is rather simple : I looked for classes having an
overriden create_object and a __construct().
I passed the class to NewInstanceWithoutConstructor() (patched to
allow them) , and I then called some methods on the instance.
Not perfect, but that's a first pass. It still took me several hours
to perform, so we can resonnably assume it is right.
I'll try to patch the extensions needing care in the next few days.
Julien.P
Hi!
What I found not safe, throwing unwanted warnings hitting an
undesired behavior, or even segfaulting, and thus needing patch :
- Dom
- Mysqli
- sqlite3 (sqlite3stmt class segfaults)
I'm testing a patch for sqlite3 right now and will commit it shortly,
but I could not reproduce anything weird with DOM. What class/method
crashes for you there?
--
Stanislav Malyshev, Software Architect
SugarCRM: http://www.sugarcrm.com/
Hi!
What I found safe, with checkers that check object is properly
initialized, so not needing patch, but throwing exceptions or warnings
when used bad constructed :
- SPL
SPL unfortunately is not safe at all - a lot of iterator classes
segfault on no-ctor initialization. I'll make a patch for them.
--
Stanislav Malyshev, Software Architect
SugarCRM: http://www.sugarcrm.com/
hi,
Hi!
What I found safe, with checkers that check object is properly
initialized, so not needing patch, but throwing exceptions or warnings
when used bad constructed :
- SPL
SPL unfortunately is not safe at all - a lot of iterator classes
segfault on no-ctor initialization. I'll make a patch for them.
It would be good to have a section in UPGRADING.INTERNALS explaining
in details what should be done, very important for non core extensions
(pecl or other repositories).
Cheers,
Pierre
@pierrejoye | http://www.libgd.org
Hi!
It would be good to have a section in UPGRADING.INTERNALS explaining
in details what should be done, very important for non core extensions
(pecl or other repositories).
Probably a good idea but I'm not sure what exactly to write there,
besides "initialize everything, check everything" :) There are many
places (esp. in SPL, but not only) where checks are present in some
methods, but not others, or some values are checked while others do not,
there are also places where some checks produce errors and some
exceptions, sometimes even within one class. We don't need to clean that
up right now but it's a good idea to keep in mind we need to.
There's also a more tricky problem since many clone functions don't do
the right thing too. E.g. this script:
class MyPDO extends PDO { function __construct() {} }
$pdo = new MyPDO();
$pdo2 = clone $pdo;
$pdo2->query("123");
Produces a segfault, even though without clone it works fine. I wonder
if one overrides __clone wouldn't more things break.
I wrote a simple fuzzer to test classes, here:
https://gist.github.com/smalyshev/f4d0c1502fa2411478ff
Note: it runs random functions with random arguments, so don't run it
anywhere it could hurt real data.
Stanislav Malyshev, Software Architect
SugarCRM: http://www.sugarcrm.com/
Hi!
What I found not safe, throwing unwanted warnings hitting an
undesired behavior, or even segfaulting, and thus needing patch :
- Dom
- Mysqli
- sqlite3 (sqlite3stmt class segfaults)
I've fixed all the instances of problematic behavior with no ctor that I
could catch now. I could not reproduce problems in DOM or mysqli, so
it'd be nice to have reproductions there. Otherwise, I think the core
modules are fine in my tests.
Stanislav Malyshev, Software Architect
SugarCRM: http://www.sugarcrm.com/
On Tue, Jul 29, 2014 at 9:48 AM, Stas Malyshev smalyshev@sugarcrm.com
wrote:
Hi!
What I found not safe, throwing unwanted warnings hitting an
undesired behavior, or even segfaulting, and thus needing patch :
- Dom
- Mysqli
- sqlite3 (sqlite3stmt class segfaults)
I've fixed all the instances of problematic behavior with no ctor that I
could catch now. I could not reproduce problems in DOM or mysqli, so
it'd be nice to have reproductions there. Otherwise, I think the core
modules are fine in my tests.
Mmmm, I can't reproduce segfaults.
I can only hit the check messages that check the instance is correctly
constructed.
When sqlite and SPL is patched, can we say we are safe about this issue ?
For SPL, I have no deep knowledge about it, I couldn't find any bad
behavior from my tests, all seemed to check the state, but I did not
perform full 100% coverage tests.
Thanks.
Julien
Hi!
client) I see no reason to keep the limitation in
the ReflectionClass::newInstanceWithoutConstructor() and allowing the
instantiation of internal classes will provide a clean upgrade path to
doctrine and co.
I think we should ensure the objects will be in safe (i.e. no-crashing)
state following create_object even if ctor was not called. The question
is if we can ensure that and what would be the effort.
As for upgrade path for doctrine, etc. - if we're talking about
something like test mocking, wouldn't something like:
class Mock_Stub_FooClass extends FooClass {
function __construct() {}
}
$foo = new Mock_Stub_FooClass();
solve the problem? I understand it's not argument against
newInstanceWithoutConstructor as essentially it does the same, just
saying newInstanceWithoutConstructor strictly speaking would not be
necessary?
I think an approach for this would be to take pull 733 and make a script
(or a test) that takes all the classes we define internally (may be a
bit hard since some extensions need external libs, but we can check
those manually) and instantiate them using this (and all other
non-standard) method and try to call random methods and see if we get
any crashes. But we should be reasonably sure at least most common ones
would not crash if we enable this.
Another question would be if objects like COM would react to this
properly. But if not it would be possible to make them final too.
Stanislav Malyshev, Software Architect
SugarCRM: http://www.sugarcrm.com/
On Sat, Jul 26, 2014 at 12:29 AM, Stas Malyshev smalyshev@sugarcrm.com
wrote:
Hi!
client) I see no reason to keep the limitation in
the ReflectionClass::newInstanceWithoutConstructor() and allowing the
instantiation of internal classes will provide a clean upgrade path to
doctrine and co.I think we should ensure the objects will be in safe (i.e. no-crashing)
state following create_object even if ctor was not called. The question
is if we can ensure that and what would be the effort.
we already discussed this via moving the init logic to the create/clone
handlers from __construct for such classes.
As for upgrade path for doctrine, etc. - if we're talking about
something like test mocking, wouldn't something like:class Mock_Stub_FooClass extends FooClass {
function __construct() {}
}
$foo = new Mock_Stub_FooClass();
yeah, that would work ofc, but as these libs seems to have instanitate
arbitrary classes, that would require either generating files on the fly
and including them or simply evaling them, but of those are a bit dirtier
than using Reflection for the same job.
solve the problem? I understand it's not argument against
newInstanceWithoutConstructor as essentially it does the same, just
saying newInstanceWithoutConstructor strictly speaking would not be
necessary?
true, but it can also be used to argue for loosening the restriction, why
restrict something from Reflection, which is already possible from simple
class extension.
I think an approach for this would be to take pull 733 and make a script
(or a test) that takes all the classes we define internally (may be a
bit hard since some extensions need external libs, but we can check
those manually) and instantiate them using this (and all other
non-standard) method and try to call random methods and see if we get
any crashes.
Julien already went over the potential classes and did the same thing
(calling methods over them), the list of affected classes is just the
previous one in this thread.
But we should be reasonably sure at least most common ones
would not crash if we enable this.
this change wouldn't introduce any crash, which isn't already possible
through a subclass not calling parent::__construct, ofc. if we can fix the
crashes before the final release that's nice, but I wouldn't say it is
mandatory, how it was hidden for years(and even so that the remote trigger
from unserialize is fixed).
Another question would be if objects like COM would react to this
properly. But if not it would be possible to make them final too.
yeah, but I would rather fix them than move to final, as I mentioned
before, it seems that some/most of the internal classes with final
constructor are final so that this problem can't occur, but this doesn't a
nice thing from OOP POV and also will cause problems if/when we introduce a
reflection method removing final from classes/methods (this was already
proposed not that long ago with a working patch but was turned down because
other reasons).
--
Ferenc Kovács
@Tyr43l - http://tyrael.hu
Hi!
yeah, that would work ofc, but as these libs seems to have instanitate
arbitrary classes, that would require either generating files on the fly
and including them or simply evaling them, but of those are a bit
dirtier than using Reflection for the same job.
True but that's what phpunit, etc. are doing for mocks anyway, aren't they?
true, but it can also be used to argue for loosening the restriction,
why restrict something from Reflection, which is already possible from
simple class extension.
I agree, probably makes sense to allow it if you can do it anyway.
Reflection is not something you can trigger without explicit codding
(unlike O: thing) so it's fine with me.
a nice thing from OOP POV and also will cause problems if/when we
introduce a reflection method removing final from classes/methods (this
was already proposed not that long ago with a working patch but was
turned down because other reasons).
That probably wouldn't be a good idea, especially for internal classes.
--
Stanislav Malyshev, Software Architect
SugarCRM: http://www.sugarcrm.com/
On Sat, Jul 26, 2014 at 1:26 AM, Stas Malyshev smalyshev@sugarcrm.com
wrote:
Hi!
yeah, that would work ofc, but as these libs seems to have instanitate
arbitrary classes, that would require either generating files on the fly
and including them or simply evaling them, but of those are a bit
dirtier than using Reflection for the same job.True but that's what phpunit, etc. are doing for mocks anyway, aren't they?
true, but it can also be used to argue for loosening the restriction,
why restrict something from Reflection, which is already possible from
simple class extension.I agree, probably makes sense to allow it if you can do it anyway.
Reflection is not something you can trigger without explicit codding
(unlike O: thing) so it's fine with me.a nice thing from OOP POV and also will cause problems if/when we
introduce a reflection method removing final from classes/methods (this
was already proposed not that long ago with a working patch but was
turned down because other reasons).That probably wouldn't be a good idea, especially for internal classes.
Like I said in a previous mail, only Dom, Mysqli and sqlite3 need patch to
be able to work with only create_object invoked.
I'm not sure about COM as I can't setup a test environment for it.
I recently started patching mysqli.
I think it is feasable to have all of them patched so that
newInstanceWithoutConstructor() may be safely "opened" to internal classes,
for 5.6.0.
A quick word as well to say I'm against giving the opportunity to userland
to remove the final attribute, as this will lead to lots of bugs, some of
them not even fixable I think.
Julien.Pauli
On Sat, Jul 26, 2014 at 1:26 AM, Stas Malyshev smalyshev@sugarcrm.com
wrote:Hi!
yeah, that would work ofc, but as these libs seems to have instanitate
arbitrary classes, that would require either generating files on the fly
and including them or simply evaling them, but of those are a bit
dirtier than using Reflection for the same job.True but that's what phpunit, etc. are doing for mocks anyway, aren't
they?true, but it can also be used to argue for loosening the restriction,
why restrict something from Reflection, which is already possible from
simple class extension.I agree, probably makes sense to allow it if you can do it anyway.
Reflection is not something you can trigger without explicit codding
(unlike O: thing) so it's fine with me.a nice thing from OOP POV and also will cause problems if/when we
introduce a reflection method removing final from classes/methods (this
was already proposed not that long ago with a working patch but was
turned down because other reasons).That probably wouldn't be a good idea, especially for internal classes.
Like I said in a previous mail, only Dom, Mysqli and sqlite3 need patch
to be able to work with only create_object invoked.
I'm not sure about COM as I can't setup a test environment for it.I recently started patching mysqli.
I think it is feasable to have all of them patched so that
newInstanceWithoutConstructor() may be safely "opened" to internal classes,
for 5.6.0.
that would be nice.
A quick word as well to say I'm against giving the opportunity to userland
to remove the final attribute, as this will lead to lots of bugs, some of
them not even fixable I think.
just to clarify: I didn't suggested to introduce this method (it was
proposed by somebody else in the past), just mentioned that I don't think
that turning classes/constructors into final only to be safe from these
kind of problems is a bad idea imo, and one of my arguments was that there
is a chance that we would like to allow userland to override final, and it
would cause problems anyways.
--
Ferenc Kovács
@Tyr43l - http://tyrael.hu
On Sat, Jul 26, 2014 at 1:26 AM, Stas Malyshev smalyshev@sugarcrm.com
wrote:
Hi!
yeah, that would work ofc, but as these libs seems to have instanitate
arbitrary classes, that would require either generating files on the fly
and including them or simply evaling them, but of those are a bit
dirtier than using Reflection for the same job.True but that's what phpunit, etc. are doing for mocks anyway, aren't they?
nope, some mock frameworks do that (afair atoum for example) but
phpunit(phpunit-mock-objects to be more precise) was using the serialize
trick and newInstanceWithoutConstructor() (
https://github.com/sebastianbergmann/phpunit-mock-objects/blob/42f29d650744fd7ce785e7a4dd3598a09b0bfd2b/src/Framework/MockObject/Generator.php#L273),
and now they switched to Instantiator from Ocramius(he also participated in
this thread) which also uses the unserialize trick and
newInstanceWithoutConstructor() (
https://github.com/Ocramius/Instantiator/blob/86e562799ceb87316fcb70e151551f47dd215a0f/src/Instantiator/Instantiator.php#L80
).
true, but it can also be used to argue for loosening the restriction,
why restrict something from Reflection, which is already possible from
simple class extension.I agree, probably makes sense to allow it if you can do it anyway.
Reflection is not something you can trigger without explicit codding
(unlike O: thing) so it's fine with me.
yeah, exactly.
a nice thing from OOP POV and also will cause problems if/when we
introduce a reflection method removing final from classes/methods (this
was already proposed not that long ago with a working patch but was
turned down because other reasons).That probably wouldn't be a good idea, especially for internal classes.
I should have left out this argument, because that has it's own can of
worms, but if anybody interested in the original proposal:
http://grokbase.com/t/php/php-internals/121gmxkz1c/php-dev-reflection-to-remove-final
I wasn't proposing merging this in my mail, just argued that if this
proposal ever merged, that would either require fixing up the finals
anyways or it would be possible to trigger similar segfaults and stuff.
--
Ferenc Kovács
@Tyr43l - http://tyrael.hu
Hi!
nope, some mock frameworks do that (afair atoum for example) but
phpunit(phpunit-mock-objects to be more precise) was using the serialize
trick and newInstanceWithoutConstructor()
(https://github.com/sebastianbergmann/phpunit-mock-objects/blob/42f29d650744fd7ce785e7a4dd3598a09b0bfd2b/src/Framework/MockObject/Generator.php#L273),
It does that too. But at least in version 3 it was also generating and
eval'ing classes, in PHPUnit_Framework_MockObject_Generator. Looking here:
https://github.com/sebastianbergmann/phpunit-mock-objects/tree/master/src/Framework/MockObject
It is still doing class generation and evals. Not sure why it doesn't
also override the ctor, maybe it was for some implementation consideration.
--
Stanislav Malyshev, Software Architect
SugarCRM: http://www.sugarcrm.com/
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:
-
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. -
We do not want to break phpunit/doctrine, at least as much as
possible without hurting #1. -
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.
- 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:
- Regular user classes
- 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. - Internal classes that do not have their own serializer
The following will still not work with O::
- Internal classes with their own serializer
- 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
On Sun, Jun 22, 2014 at 6:03 AM, Stas Malyshev smalyshev@sugarcrm.com
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:
- 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.
- We do not want to break phpunit/doctrine, at least as much as
possible without hurting #1.
+1
- Current code does not fix the segfault problem entirely, e.g.:
class MySplFileObject extends SplFileObject {}
echounserialize('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.
- 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:
- Regular user classes
- 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.- Internal classes that do not have their own serializer
The following will still not work with O::
- Internal classes with their own serializer
- 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
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
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/701Yes, 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
On Mon, Jun 23, 2014 at 2:20 AM, Stas Malyshev smalyshev@sugarcrm.com
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/701Yes, 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. 227Hello,
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
On Mon, Jun 23, 2014 at 2:20 AM, Stas Malyshev smalyshev@sugarcrm.com
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/701Yes, 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. 227Hello,
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
Hi Julien,
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/
Hi Julien,
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
Hi Julien,
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
On Mon, Jun 23, 2014 at 11:16 AM, Matteo Beccati php@beccati.com
wrote:Hi Julien,
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
I'm not proposing to wait with the 5.6 final release until we fixed every
internal class so it is safe to instantiate them without calling their
constructors(as we discussed before that could even turn out impossible).
What I meant that I can promise that we will have a solution for 5.6, which
provides a feasible migration path for apps/libs currently depending on the
unserialize O: hack to mock/thaw objects.
--
Ferenc Kovács
@Tyr43l - http://tyrael.hu
Le 23/06/2014 01:10, Ferenc Kovacs a écrit :
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
Tested with symfony 2.5.0 test suite.
Symfony\Component\Form\Tests\Extension\Core\Type\FileTypeTest::testSetData
Erroneous data format for unserializing 'Mock_UploadedFile_c913ac22'
/dev/shm/extras/BUILD/symfony-59365832a09a1d914d14cbca6a21a7f572760c3b/src/Symfony/Component/Form/Tests/Extension/Core/Type/FileTypeTest.php:82
/dev/shm/extras/BUILD/symfony-59365832a09a1d914d14cbca6a21a7f572760c3b/src/Symfony/Component/Form/Tests/Extension/Core/Type/FileTypeTest.php:20
Minimal reproducer:
<?php
class FooFile extends SplFileInfo {
}
$str = 'O:7:"FooFile":0:{}';
var_dump(unserialize($str));
?>
:(
Remi
P.S. "ce->create_object == NULL" is the cause.
Hi!
Minimal reproducer:
<?php
class FooFile extends SplFileInfo {
}
$str = 'O:7:"FooFile":0:{}';
var_dump(unserialize($str));
?>
I'm afraid here we can't do much - SplFileInfo is one of the classes
that it is unsafe to instantiate this way. It's what original 67072 was
about and I don't think it's safe to leave it this way, since at best it
can crash any code that uses unserialize() on external data, at worst
you got RCE.
--
Stanislav Malyshev, Software Architect
SugarCRM: http://www.sugarcrm.com/
(408)454-6900 ext. 227
Le 23/06/2014 19:41, Stas Malyshev a écrit :
Hi!
Minimal reproducer:
<?php
class FooFile extends SplFileInfo {
}
$str = 'O:7:"FooFile":0:{}';
var_dump(unserialize($str));
?>I'm afraid here we can't do much - SplFileInfo is one of the classes
that it is unsafe to instantiate this way. It's what original 67072 was
about and I don't think it's safe to leave it this way, since at best it
can crash any code that usesunserialize()on external data, at worst
you got RCE.
I think the segfault have to be fixed in spl.
And if we plan to allow newInstanceArgWithoutConstructor() for internal
classes this is mandatory.
See attached patch (quickly written, just for test)
So we can allow "O:.." (perhaps only for empty data used in the
phpunit/doctrine hack, => strlen(*p)<=1)
Running:
echo
unserialize('O:13:"SplFileObject":1:{s:9:"*filename";s:15:"/home/flag/flag";}');
echo unserialize('O:13:"SplFileObject":0:{}');
Warning: Erroneous data format for unserializing 'SplFileObject' ...
Fatal error: SplFileObject::__toString(): Object not initialized ...
Remi.
Hi!
I think the segfault have to be fixed in spl.
This can be done, however can we ensure all classes in PHP and
extensions would run properly when unserialized despite explicit
prohibition from the class to serialize/unserialize it?
Doing O: trick on such class is just not right. Note that crashes is
just the start of the problem - what if circumventing prohibited
unserialization puts the class into state that allows remote attacker to
trick it into doing something it's not supposed to do? Remember the
__dtor issue? That one worked on PHP level, this one would work on C
level, which would be much worse.
And if we plan to allow newInstanceArgWithoutConstructor() for internal
classes this is mandatory.
I'm not sure this is safe either, by the way. But at least here we don't
allow remote data to control our class' content and inject any data into
it without any controls whatsoever. So here might be a better way out of
this. But we need to be very careful with it.
So we can allow "O:.." (perhaps only for empty data used in the
phpunit/doctrine hack, => strlen(*p)<=1)
Right now we can not - it leads to remote-triggerable crash in any app
that unserializes outside data, and there are lots of these - just
search on github for unserialize($_POST or unserialize($_COOKIE. And I'm
not sure we can safely allow this in general. I'm sorry that this would
make a neat hack unavailable, but I think security of PHP apps is more
important than preserving this hack which was never documented and never
supposed to work in the first place.
I understand that this creates a need that we do not cover of how to
mock such objects, and I welcome suggestions - including how to make
newInstanceArgWithoutConstructor safe. But currently I do not see how we
can leave the unserialize hack in for classes like SplFileObject -
unless somebody points me to a way to make it safe.
--
Stanislav Malyshev, Software Architect
SugarCRM: http://www.sugarcrm.com/
(408)454-6900 ext. 227
Hi!
I think the segfault have to be fixed in spl.
This can be done, however can we ensure all classes in PHP and
extensions would run properly when unserialized despite explicit
prohibition from the class to serialize/unserialize it?Doing O: trick on such class is just not right. Note that crashes is
just the start of the problem - what if circumventing prohibited
unserialization puts the class into state that allows remote attacker to
trick it into doing something it's not supposed to do? Remember the
__dtor issue? That one worked on PHP level, this one would work on C
level, which would be much worse.
+1.
- Can't we just deny any unserilization with "O:" if the class has a
custom serializer ? - Can't we throw an exception on any attempt to unserialize ("O:" or
"C:") any class that uses zend_unserialize_deny ?
I mean, we won't be able to find a solution which is
- 100% garantied no BC break
- 100% garantied no segfault
So, I suggest we use the safe path and stop hacking, discovering a
nasty bug about the hack, then hack it again, just for libraries
relying on unsupported tricks about the serialize format ?
And if we plan to allow newInstanceArgWithoutConstructor() for internal
classes this is mandatory.I'm not sure this is safe either, by the way. But at least here we don't
allow remote data to control our class' content and inject any data into
it without any controls whatsoever. So here might be a better way out of
this. But we need to be very careful with it.So we can allow "O:.." (perhaps only for empty data used in the
phpunit/doctrine hack, => strlen(*p)<=1)Right now we can not - it leads to remote-triggerable crash in any app
that unserializes outside data, and there are lots of these - just
search on github for unserialize($_POST or unserialize($_COOKIE. And I'm
not sure we can safely allow this in general. I'm sorry that this would
make a neat hack unavailable, but I think security of PHP apps is more
important than preserving this hack which was never documented and never
supposed to work in the first place.I understand that this creates a need that we do not cover of how to
mock such objects, and I welcome suggestions - including how to make
newInstanceArgWithoutConstructor safe. But currently I do not see how we
can leave the unserialize hack in for classes like SplFileObject -
unless somebody points me to a way to make it safe.
Yes, we should bring sane, safe solution for such needs. But sane and
safe, which is all but rushing.
We'll bring solutions to 5.6 , but for 5.4 and 5.5 , we just won't be
able to satisfy everybody.
Julien
On Tue, 24 Jun 2014 11:58:11 +0400, Stas Malyshev smalyshev@sugarcrm.com
wrote:
Hi!
I think the segfault have to be fixed in spl.
This can be done, however can we ensure all classes in PHP and
extensions would run properly when unserialized despite explicit
prohibition from the class to serialize/unserialize it?Doing O: trick on such class is just not right. Note that crashes is
just the start of the problem - what if circumventing prohibited
unserialization puts the class into state that allows remote attacker to
trick it into doing something it's not supposed to do? Remember the
__dtor issue? That one worked on PHP level, this one would work on C
level, which would be much worse.And if we plan to allow newInstanceArgWithoutConstructor() for internal
classes this is mandatory.I'm not sure this is safe either, by the way. But at least here we don't
allow remote data to control our class' content and inject any data into
it without any controls whatsoever. So here might be a better way out of
this. But we need to be very careful with it.So we can allow "O:.." (perhaps only for empty data used in the
phpunit/doctrine hack, => strlen(*p)<=1)Right now we can not - it leads to remote-triggerable crash in any app
that unserializes outside data, and there are lots of these - just
search on github for unserialize($_POST or unserialize($_COOKIE. And I'm
not sure we can safely allow this in general. I'm sorry that this would
make a neat hack unavailable, but I think security of PHP apps is more
important than preserving this hack which was never documented and never
supposed to work in the first place.I understand that this creates a need that we do not cover of how to
mock such objects, and I welcome suggestions - including how to make
newInstanceArgWithoutConstructor safe. But currently I do not see how we
can leave the unserialize hack in for classes like SplFileObject -
unless somebody points me to a way to make it safe.
Hi Guys,
I don't see any problem in the fact that you can skip actual instantiation
of the object. Even if there is a problem with that - a lot of user-land
code already uses this trick and it will be a pretty big BC break (as we
already see it in the case of Symfony's test suite) and it was told a
million times already. Using input data blindly is user's mistake. I think
there are already cases of PHP trying to 'secure' users from themselves
(hello magic quotes). And I think all we have to do is to prevent
possibility of segfault, not the possibility of instantiation of the
object (without calling internal constructor) itself, because skipping
constructor (for whatever type classes - internal or user-land) has its
use cases.
Don't you think that it would be better, instead of preventing creation of
the objects without calling internal constructor, to prevent calling any
internal methods on those objects (apart from ctor of course) if they were
instantiated the 'wrong' way. I mean we would have to add a check in
zend_call_function (maybe in some other places as well) like
if (fci.object_ptr && EX(function_state).function->type ==
ZEND_INTERNAL_FUNCTION && ! was_properly_instantiated(fci.object_ptr)) {
...then throw an error... }
But as I understand right now we don't have any generic means to get to
know whether internal constructor was called or not, so that would be a
bit problematic - we would need to add some field in the zend_object
structure which could be used as a flag to indicate some things, including
this one - whether internal constructor was called or not (it shouldn't be
very hard to implement). The only issue here is increasing of memory usage
by objects by 32 bits...
What do you think?
Hi!
I don't see any problem in the fact that you can skip actual instantiation
of the object. Even if there is a problem with that - a lot of user-land
The problem is remotely-triggerable DoS and potentially RCE. Not
sounding bad enough?
code already uses this trick and it will be a pretty big BC break (as we
The mere fact O: works for custom serializer objects is a clear bug. It
should have never worked, it was never documented as working and nobody
ever promised anybody it would work. It is unfortunate that userland
code relies on a bug to be working, but this particular bug is too
dangerous to keep in. Yes, BC is important. Not having
remotely-triggerable memory corruption is more important.
million times already. Using input data blindly is user's mistake. I think
Sorry, you can not bursh it off in this case as "user error". The case
where unserializer crashes on wrong data is a bad problem - and there's
absolutely no way for the user to filter the data to ensure it does not
happen, except for implementing completely custom unserializer. The only
way is to completely abandon usage of serialize(). This is not a good
solution to propose to people with existing apps.
there are already cases of PHP trying to 'secure' users from themselves
(hello magic quotes). And I think all we have to do is to prevent
How magic quotes have anything to do with it? MC failed because it
didn't work and was applied in proper place, not because it didn't allow
you to trigger crashes and UMRs whenever you'd like. We give people
plenty of the rope, but there should be some limit. I think
remotely-triggerable crashes go beyond the limit.
possibility of segfault, not the possibility of instantiation of the
object (without calling internal constructor) itself, because skipping
constructor (for whatever type classes - internal or user-land) has its
use cases.
I'd like to question this - the whole meaning of ctor is to initialize
the object and if you need to routinely skip it the problem is probably
elsewhere. But this is a topic for another discussion.
Don't you think that it would be better, instead of preventing creation of
the objects without calling internal constructor, to prevent calling any
We're not preventing this right now. We're preventing using serializer
for doing this, because it does not work. If there's another way of
doing this without causing breakage, then fine, do it. Let's have an RFC
about it and discuss it.
Stanislav Malyshev, Software Architect
SugarCRM: http://www.sugarcrm.com/
(408)454-6900 ext. 227
Hi!
I don't see any problem in the fact that you can skip actual
instantiation
of the object. Even if there is a problem with that - a lot of
user-landThe problem is remotely-triggerable DoS and potentially RCE. Not
sounding bad enough?
Hey Stas,
You keep mentioning these two, but don't they assume that the serialized
data is user-provided?
I don't think anybody sane would/should do that in first place, as it would
be already possible to cause RCE just with any class implementing the
Serializable interface.
Yes, laravel did this via super-closure, but there was a security fix for
it recently.
Could you clarify on this particular point?
I see RCEs anywhere user input is used to dynamically instantiate anything
as well, I just think that it is not the case here, as it would be the
developer's fault for granting access to serialized data to the user.
Marco Pivetta
Hi!
You keep mentioning these two, but don't they assume that the serialized
data is user-provided?
Yes, they do.
I don't think anybody sane would/should do that in first place, as it
Simple search on github suggests otherwise.
would be already possible to cause RCE just with any class implementing
theSerializableinterface.
Not sure how you could do that, could you please explain how would you
cause that?
I see RCEs anywhere user input is used to dynamically instantiate
anything as well, I just think that it is not the case here, as it would
be the developer's fault for granting access to serialized data to the user.
I'm not sure what is the problem with simple instantiation - of course,
this runs the ctor and unserialize methods, but those are supposed to be
able to handle external data. I was, of course, speaking of running
arbitrary code on C level, not just PHP methods purposed to handle the
data by the developers.
Stanislav Malyshev, Software Architect
SugarCRM: http://www.sugarcrm.com/
Hi!
You keep mentioning these two, but don't they assume that the serialized
data is user-provided?Yes, they do.
I don't think anybody sane would/should do that in first place, as it
Simple search on github suggests otherwise.
sane doesn't mean everyone.
Allowing un-serializing data coming from user input is as bad as eval(),
and trying to defend from it is also quite useless.
would be already possible to cause RCE just with any class implementing
theSerializableinterface.Not sure how you could do that, could you please explain how would you
cause that?
Assuming this exists in the user's codebase:
class Prank implements Serializable
{
public function serialize() {}
public function unserialize() { exec('rm -rf /'); }
}
Then send a serialized prank over the internets.
Other interesting security issues are related to this as well in my
opinion, but I'd have to do research on the problem first.
I was, of course, speaking of running
arbitrary code on C level, not just PHP methods purposed to handle the
data by the developers.
That was my misunderstanding. Thanks for clarifying.
Marco Pivetta
Hi!
sane doesn't mean everyone.
Allowing un-serializing data coming from user input is as bad as
eval(), and trying to defend from it is also quite useless.
I would like to hear some justification for this claim.
Assuming this exists in the user's codebase:
class Prank implements Serializable
{
public functionserialize(){}
public functionunserialize(){ exec('rm -rf /'); }
}
That one fat assumption. Who would put such code in the codebase? With
the same argument you can claim HTTP protocol has a RCE built it, of
course "assuming" your http server has exec('rm -rf /'); in it ready to
be called. That's not what RCE means. RCE means code execution without
specially crafted code that is actually written on the server in order
to facilitate the exact problem.
Other interesting security issues are related to this as well in my
opinion, but I'd have to do research on the problem first.
If you can demonstrate a real RCE or any other problem using
unserialize() (besides the __dtor issue which is widely known along with
its mitigation) please share it with me or security@php.net. Those
happen, as any other bugs, but claim that unserialize() is the same as
eval() seems to be over-reaching.
--
Stanislav Malyshev, Software Architect
SugarCRM: http://www.sugarcrm.com/
Le 24/06/2014 09:58, Stas Malyshev a écrit :
I understand that this creates a need that we do not cover of how to
mock such objects, and I welcome suggestions - including how to make
newInstanceArgWithoutConstructor safe. But currently I do not see how we
can leave the unserialize hack in for classes like SplFileObject -
unless somebody points me to a way to make it safe.
Proposal, hawfull hack attached.
Obviously, this doesn't make newInstanceWithoutConstructor safe ;)
But this answer the need.
And we can state in documentation.
"force : allow to instantiate internal classes. Never use this !"
And nobody could complain if this cause segfault.
Remi.
Le 24/06/2014 09:58, Stas Malyshev a écrit :
I understand that this creates a need that we do not cover of how to
mock such objects, and I welcome suggestions - including how to make
newInstanceArgWithoutConstructor safe. But currently I do not see how we
can leave the unserialize hack in for classes like SplFileObject -
unless somebody points me to a way to make it safe.
Proposal, hawfull hack attached.
Obviously, this doesn't make newInstanceWithoutConstructor safe ;)
But this answer the need.
And we can state in documentation.
"force : allow to instantiate internal classes. Never use this !"
And nobody could complain if this cause segfault.
Remi.
Le 23/06/2014 19:41, Stas Malyshev a écrit :
Hi!
Minimal reproducer:
<?php
class FooFile extends SplFileInfo {
}
$str = 'O:7:"FooFile":0:{}';
var_dump(unserialize($str));
?>I'm afraid here we can't do much - SplFileInfo is one of the classes
that it is unsafe to instantiate this way. It's what original 67072 was
about and I don't think it's safe to leave it this way, since at best it
can crash any code that usesunserialize()on external data, at worst
you got RCE.I think the segfault have to be fixed in spl.
We should fix those issues, sure, but if we consider
unserialize($arbitraryUserInput) a relatively common thing, then continuing
to allow the Serializable Internal classes to be unserialized with the O
format will allow remote DOS or worse until we "fixed" all of those classes
to support instanitation without a constructor call.
I don't think we can wait for that with the 5.4/5.5 releases (and there are
already other sec related fixes waiting to be released).
And if we plan to allow newInstanceArgWithoutConstructor() for internal
classes this is mandatory.
I'm not so sure about that it is mandatory, as I mentioned the problem also
exists with MyClass extends SomeInternalClass {public function
__construct(){}} but what makes the difference between
newInstanceArgWithoutConstructor/extending the class is that it requires
deliberate action, explicitly written code, vs the arbitrary unserialize
call.
See attached patch (quickly written, just for test)
So we can allow "O:.." (perhaps only for empty data used in the
phpunit/doctrine hack, => strlen(*p)<=1)Running:
echounserialize('O:13:"SplFileObject":1:{s:9:"*filename";s:15:"/home/flag/flag";}');
echo unserialize('O:13:"SplFileObject":0:{}');Warning: Erroneous data format for unserializing 'SplFileObject' ...
Fatal error: SplFileObject::__toString(): Object not initialized ...
Would be nice if others could also comment, because I think we are starting
to argue in circles.
--
Ferenc Kovács
@Tyr43l - http://tyrael.hu
<?php
class FooFile extends SplFileInfo {
}
$str = 'O:7:"FooFile":0:{}';
var_dump(unserialize($str));
?>
I'm afraid here we can't do much - SplFileInfo is one of the classes
that it is unsafe to instantiate this way. It's what original 67072 was
about and I don't think it's safe to leave it this way, since at best it
can crash any code that usesunserialize()on external data, at worst
you got RCE.
As I see it, the main problem here is trying to cheat when creating a
restored process? Rather than correctly calling the class and
instantiating it via the normal process, some third party process
creates a fake saved version of the object and tries to restart it
bypassing the proper construction process?
That many objects are not designed to be constructed in this way is the
problem, and PHP5.4 and 5.5 are not the platform to be making changes to
that process? Personally I think that bypassing the 'constructor' can be
viewed two ways. Originally PHP did not even have the need for it, as it
has been a more recent addition, so that it has unforeseen side effects
is to be expected? NOW changing things back so that it is not necessary
seems to be counter intuitive? There were good reasons for insisting on
the constructor stage, and seeding an object with a state stored by the
serialize process needs a cross check that the object is reseeded
correctly when unserialized?
That an inherent crash path needs to be plugged is a given, but all I am
seeing in the proposed patches are more bandaids on the whole process?
If the constructor can now be skipped in some instances then why have it
at all? Go back to the situation where one manually called a
'constructor' when it was needed or fix the unserialize process on
objects to include a suitable constructor? If an object has been
'created' but not 'initialized' then a process needs to be run to make
the object actually usable?
That some users of PHP have a problem with the recent changes is more
because they are using undocumented processes so while allowing them to
continue using the hacks short term may be correct - reverting the
changes - the correct path going forward is perhaps to document a
correct process and block any problem paths?
--
Lester Caine - G8HFL
Contact - http://lsces.co.uk/wiki/?page=contact
L.S.Caine Electronic Services - http://lsces.co.uk
EnquirySolve - http://enquirysolve.com/
Model Engineers Digital Workshop - http://medw.co.uk
Rainbow Digital Media - http://rainbowdigitalmedia.co.uk
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