Everything
php
Learn Community Releases The PHP Project
Home » Project » Newsgroups » php.internals » [RFC] [VOTE] Filtered unserialize() »

[RFC] [VOTE] Filtered unserialize()

php.internals

Stas Malyshev

11 years ago
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/

Damien Tournoud

11 years ago
On Mon, Nov 3, 2014 at 10:10 PM, Stas Malyshev <smalyshev@sugarcrm.com> wrote:
> I'd like to put to vote my proposal about the filtered unserialize(): > > https://wiki.php.net/rfc/secure_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

Stas Malyshev

11 years ago
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.
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/

Andrew Faulds

11 years ago
> On 3 Nov 2014, at 21:33, Stas Malyshev <smalyshev@sugarcrm.com> wrote: > >> 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. > > 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/

Leigh

11 years ago
On 3 November 2014 21:10, 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 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".

Stas Malyshev

11 years ago
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.

Nikita Popov

11 years ago
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

Stas Malyshev

11 years ago
Hi!
> 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
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/

Nikita Popov

11 years ago
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 of unserialize() 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. >
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. 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. >
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

Stas Malyshev

11 years ago
Hi!
> 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*
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/

Dmitry Stogov

11 years ago
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:

Patrick ALLAERT

11 years ago
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

Pascal MARTIN

11 years ago
On 03/11/2014 22:10, Stas Malyshev wrote:
> 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/