On Tue, Nov 4, 2014 at 7:45 PM, Stas Malyshev <smalys...@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
