Re: Bug 67072 resolution for 5.4/5.5

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: > > 1. We can not leave unserialize() segfault in the code in 5.4 > undisturbed. It's DoS opening as a minimum, and RCE potential, if > suitable internal class can be found, for every app that accepts > user-controlled serialized data. And there are a lot of such apps out > there. > for the issue to materialize you need to feed hand-crafted input to unserialize, anybody doing that with user controlled data already asking for problems, but I agree that we should fix if, and even fix it in a micro version if we won't totally cripple the the userland tools unfortunately depending on this trick. > > 2. We do not want to break phpunit/doctrine, at least as much as > possible without hurting #1. > > +1 > 3. Current code does not fix the segfault problem entirely, e.g.: > class MySplFileObject extends SplFileObject {} > echo > > > unserialize('O:15:"MySplFileObject":1:{s:9:"*filename";s:15:"/home/flag/flag";}'); > > still segfaults for me. > yeah, this is what I also noticed and reported. > > 4. There are other ways to cause segfaults with internal objects, > unfortunately, but those require deliberate coding and I'm less worried > about it since those have no external exploitation potential. > I would say that this also a deliberate attempt, maybe a bit more obscure. > > Given this, I am proposing to check the check in var_unserializer to this: > > if (ce->serialize == NULL || ce->unserialize == zend_user_unserialize || > (ZEND_INTERNAL_CLASS != ce->type && ce->create_object == NULL)) { > object_init_ex(*rval, ce); > } > > This would allow O: to work with the following: > > 1. Regular user classes > 2. User classes implementing Serializable - the result will be > semantically broken but no segfault should be produced since it is still > all within PHP engine, so the worst is that some PHP variables will be > left null instead of their real values. > 3. Internal classes that do not have their own serializer > > The following will still not work with O:: > 1. Internal classes with their own serializer > 2. User classes extending classes in 1. > > I prefer this over what we have in 5.4/5.5 and given how few classes does 1, actually mean, I think it would be an acceptable compromise, but let's hear what others think. > I've checked the Horde/phpunit test by Remi, and it works fine, but I'd > like more feedback on this. Please note we need some solution one way or > another in 3 days, so if we need more discussion on this I'd advise > maybe set up some session on IRC to hash it out. > I think #php.pecl on efnet would be the best place, as some of us already lurks there. > > P.S. For 5.6, I'd just remove the usage of O: hack for any class that > produces C: in serialize. If serializer does not produce it, it should > not accept it. > agree, but as I mentioned I would like to provide some alternative for creating those classes (eg. removing the restriction on internal classes for ReflectionClass::newInstanceWithoutConstructor) ps: I've seen that you created a pull request with the patch, if somebody don't wanna copypaste the patch from the mail, here it is: https://github.com/php/php-src/pull/701 -- Ferenc Kovács @Tyr43l - http://tyrael.hu