Hi!
I'd like to put to vote my proposal about the filtered unserialize():
https://wiki.php.net/rfc/secure_unserialize
It was discussed a number of times before and I think it is time to have
a decision on it. If you need any clarifications on the proposal, please
do not hesitate to ask.
Stanislav Malyshev, Software Architect
SugarCRM: http://www.sugarcrm.com/
I'd like to put to vote my proposal about the filtered
unserialize():
Hi,
Coming late to the discussion. Was there any discussion to make the
new argument a callback instead? Pass it the fully-qualified class
name, have it return true (the class should be loaded) or false (the
class should not be loaded). Deprecate the unserialize_callback_func
mechanism at the same time.
Damien
Hi!
Coming late to the discussion. Was there any discussion to make the
new argument a callback instead? Pass it the fully-qualified class
name, have it return true (the class should be loaded) or false (the
class should not be loaded). Deprecate theunserialize_callback_func
mechanism at the same time.
That was not discussed. It can be made this way, though it would be more
complicated and have more moving parts, but that's not part of the
present RFC.
--
Stanislav Malyshev, Software Architect
SugarCRM: http://www.sugarcrm.com/
Coming late to the discussion. Was there any discussion to make the
new argument a callback instead? Pass it the fully-qualified class
name, have it return true (the class should be loaded) or false (the
class should not be loaded). Deprecate theunserialize_callback_func
mechanism at the same time.That was not discussed. It can be made this way, though it would be more
complicated and have more moving parts, but that's not part of the
present RFC.
The way the RFC does it doesn’t preclude using a callback, so this could
always be done later.
Andrea Faulds
http://ajf.me/
Hi!
I'd like to put to vote my proposal about the filtered
unserialize():https://wiki.php.net/rfc/secure_unserialize
It was discussed a number of times before and I think it is time to have
a decision on it. If you need any clarifications on the proposal, please
do not hesitate to ask.
I wonder how often the final parameter will simply be get_declared_classes()
Instead of true/false/array, maybe we could go for int/array and have
constants for "allow anything", "disallow everything", "allow declared
only".
Hi!
I wonder how often the final parameter will simply be
get_declared_classes()
That would be quite dangerous as there are a lot of classes in the
extensions, not all of them are your friends. Also, in a big projects,
you don't always know which classes may be currently loaded.
On Mon, Nov 3, 2014 at 10:10 PM, Stas Malyshev smalyshev@sugarcrm.com
wrote:
Hi!
I'd like to put to vote my proposal about the filtered
unserialize():https://wiki.php.net/rfc/secure_unserialize
It was discussed a number of times before and I think it is time to have
a decision on it. If you need any clarifications on the proposal, please
do not hesitate to ask.
I'm -1 on this RFC, because I think this only further encourages
ill-advised usages of unserialize() on user-provided strings. I don't think
adding an allowed classes lists makes unserialize() safe, because it also
supports some other weird things. For example, our serialization format
allows you to create references. I'd imagine that you can easily use this
to cause a DOS condition if the code processing the unserialize output uses
any kind of recursion.
Furthermore I dislike some details of the particular implementation: The
ability to use false as a synonym for [] seems unnecessary. Directly using
an extra argument will be inconvenient for future additions, e.g. if you
really wanted to more this secure, you'd probably also want to have options
to disable references and to limit the cumulative number of array elements
(hashdos). I'd prefer using an options array for this.
Nikita
Hi!
I'm -1 on this RFC, because I think this only further encourages
ill-advised usages ofunserialize()on user-provided strings. I don't
I guess that's where we disagree. I think that security is a layered
approach (see more here:
http://php100.wordpress.com/2014/11/03/unserialize-and-being-practical/). Some
people think that if somebody deviates from the best practice, however
good are the reasons, there should be no support whatsoever in securing
the alternative approach since it "encourages" departing from best
practices. I think this approach is wrong.
format allows you to create references. I'd imagine that you can easily
use this to cause a DOS condition if the code processing the unserialize
output uses any kind of recursion.
I'm not sure what you mean here. I'm not aware of any way to cause any
recursion or DoS when parsing serialized data, as for user-side data
processing, of course I do not attempt to cure the world and solve the
halting problem, I only give you a tool to filter data. If you store
recursive data structures in your data and process it, this RFC does not
provide recursion protection, we can not do everything in one RFC :)
Furthermore I dislike some details of the particular implementation: The
ability to use false as a synonym for [] seems unnecessary. Directly
I could make it produce an error on false, but I don't see what use case
it would help. If you have such use case, please describe it.
using an extra argument will be inconvenient for future additions, e.g.
if you really wanted to more this secure, you'd probably also want to
have options to disable references and to limit the cumulative number of
array elements (hashdos). I'd prefer using an options array for this.
I sill have hope named parameters happen some time, which would
eliminate the need option arrays. Since right now we have only one
option, I think option array is redundant here.
Stanislav Malyshev, Software Architect
SugarCRM: http://www.sugarcrm.com/
On Tue, Nov 4, 2014 at 7:45 PM, Stas Malyshev smalyshev@sugarcrm.com
wrote:
Hi!
I'm -1 on this RFC, because I think this only further encourages
ill-advised usages ofunserialize()on user-provided strings. I don'tI guess that's where we disagree. I think that security is a layered
approach (see more here:
http://php100.wordpress.com/2014/11/03/unserialize-and-being-practical/).
Some
people think that if somebody deviates from the best practice, however
good are the reasons, there should be no support whatsoever in securing
the alternative approach since it "encourages" departing from best
practices. I think this approach is wrong.
To clarify: I don't think it makes sense to add an additional security
option, if we cannot say that unserialize() is to our knowledge fully
secure if this option is used. Adding this makes sense if you can say "if
you use this option, you can safely use unserialize() on user-provided
input, nothing bad can happen, ignore anything we told you previously". I
don't think that adding an allowed classes list quite gets us to that
point. The two issues I see are the ability to create references and to
exploit hashdos (the latter also applies to json). Maybe there are others,
this would need closer review.
Furthermore I dislike some details of the particular implementation: The
ability to use false as a synonym for [] seems unnecessary. DirectlyI could make it produce an error on false, but I don't see what use case
it would help. If you have such use case, please describe it.
Just looking at your implementation again, it looks like "false" is not a
special value and you actually accept anything, regardless of type. So if I
pass the string "foo" as the second parameter, it will simply do nothing.
Silently discarding values for security features sounds dubious to me.
Nikita
Hi!
To clarify: I don't think it makes sense to add an additional security
option, if we cannot say thatunserialize()is to our knowledge fully
That's where we disagree. I think security is a spectrum, and you can
make it better. It looks like you think it's binary - either it is
fully airtight secure, or there's no point even bothering. I think
there is a point.
Just looking at your implementation again, it looks like "false" is not
a special value and you actually accept anything, regardless of type. So
The RFC is not about specific pull, it's about the design. If there are
bugs in the pull, it can be fixed.
Stanislav Malyshev, Software Architect
SugarCRM: http://www.sugarcrm.com/
I agree with Nikita.
Adding an extra argument for one particular security related case looks
weird.
Will we add more arguments for others?
I would also prefer options array and [] instead of 'false'.
unserialize($string, array('allowed_classes'=>[]));
It's also more readable :)
Thanks. Dmitry.
On Tue, Nov 4, 2014 at 9:45 PM, Stas Malyshev smalyshev@sugarcrm.com
wrote:
Hi!
I'm -1 on this RFC, because I think this only further encourages
ill-advised usages ofunserialize()on user-provided strings. I don'tI guess that's where we disagree. I think that security is a layered
approach (see more here:
http://php100.wordpress.com/2014/11/03/unserialize-and-being-practical/).
Some
people think that if somebody deviates from the best practice, however
good are the reasons, there should be no support whatsoever in securing
the alternative approach since it "encourages" departing from best
practices. I think this approach is wrong.format allows you to create references. I'd imagine that you can easily
use this to cause a DOS condition if the code processing the unserialize
output uses any kind of recursion.I'm not sure what you mean here. I'm not aware of any way to cause any
recursion or DoS when parsing serialized data, as for user-side data
processing, of course I do not attempt to cure the world and solve the
halting problem, I only give you a tool to filter data. If you store
recursive data structures in your data and process it, this RFC does not
provide recursion protection, we can not do everything in one RFC :)Furthermore I dislike some details of the particular implementation: The
ability to use false as a synonym for [] seems unnecessary. DirectlyI could make it produce an error on false, but I don't see what use case
it would help. If you have such use case, please describe it.using an extra argument will be inconvenient for future additions, e.g.
if you really wanted to more this secure, you'd probably also want to
have options to disable references and to limit the cumulative number of
array elements (hashdos). I'd prefer using an options array for this.I sill have hope named parameters happen some time, which would
eliminate the need option arrays. Since right now we have only one
option, I think option array is redundant here.Stanislav Malyshev, Software Architect
SugarCRM: http://www.sugarcrm.com/
2014-11-04 20:48 GMT+01:00 Dmitry Stogov dmitry@zend.com:
I agree with Nikita.
Adding an extra argument for one particular security related case looks
weird.
Same opinion here.
Unfortunately, I can't propose something more robust instead, but I
have the feeling that this RFC tries to solve the symptoms of some
deeper problems with a short-term vision only.
What if I want to unserialize an object of class A which has a
reference to class B? Should "B" be part of the filter? And what if B
has, in turn, some other class references?
I'm +1 for addressing the issue that this RFC tries to solve, but not
in the current state.
Regards,
Patrick
I'd like to put to vote my proposal about the filtered
unserialize():
Hi,
After discussing this RFC with a few other people, we seem to agree that
allowing some level of security-related filtering when unserializing is
a nice idea -- so, we would be +1 on the principle.
Some of us think raising a notice -- or, better, throwing an exception
-- in case of a not-allowed class might be helpful, especially when it
comes to detecting problems and unsafe input data. Still, we understand
__PHP_Incomplete_Class must have been chosen to remain close to the way
unserialize() now behaves.
--
Pascal MARTIN, AFUP - French UG
http://php-internals.afup.org/