SMF 2.0.11: safe_unserialize in Sources/Subs.php can be made safer

Started by qc, May 02, 2016, 11:15:35 AM

Previous topic - Next topic

qc

The function safe_unserialize in Sources/Subs.php does not check for objects implementing the Serializable interface which might become a security concern once such a class has been loaded at runtime or is available for autoloading.

While objects are generally serialized as e.g. O:3:"obj":{...}, an object implementing the Serializable interface is serialized as C:3:"obj":{...}. The regular expression used by safe_unserialize recognizes the first but not the second case. Therefore I recommend making a simple modification to safe_unserialize ([OC]: instead of O:):


function safe_unserialize($data)
{
// There's no reason input should contain an object,
// user is up to no good...
if (preg_match('/(^|;|{|})[OC]:([0-9]|\+|\-)+/', $data) === 0)
return @unserialize($data);
}
Playing quizduell? Having quizduell questions? Our german quizduell forum quizcommunity.de is looking for quiz freaks to come and play quizduell with us :)

Arantor

Wait, what? An object instance that happens to implement a specific interface is now registered as a specific and different serialised type (as opposed to just being an object, and how this is no different to an object that happens to implement any of the serialise-related magic methods, e.g. __wake)? WTF, PHP devs? WTF?!

That said, the expression should actually cover a third case and one that the original 'safe' version didn't cover as IPB learned, where occasionally O+ would be used as the serialise type. So it should really be [OC]\+?

qc

Well, looking at that regex again I would say ditch it and come up with something more robust, probably based on white-listing the allowed types?

Unfortunately the current regex also matches e.g. the completely valid string s:4:";O:3". Might cause some hard-to-trace errors...

@Arantor:  At least PHP 5.5.9 doesn't recognize the "O+" type and unserialize() reports an error. Was this possible on older versions?
Playing quizduell? Having quizduell questions? Our german quizduell forum quizcommunity.de is looking for quiz freaks to come and play quizduell with us :)

Arantor

Valid point :) I seem to recall this was done mostly to quickly fix it in 2.0 while 2.1 does away with serialising in general and uses JSON - it wasn't meant to be a great fix, just enough to get by.

Thing is, in almost every case in core SMF where it does this, it's actually under SMF control building an array and serialising it; there's only a few places where user input is taken in a serialised form (predominantly search throws around the list of parameters in a serialised form, albeit base64 encoded) which is why this was introduced... though actually doing anything with it is hard because very little of SMF uses OOP and even less can be relied upon to be loaded. I think I pushed for this to be introduced once modders started using autoloaders, which could make this viable as an exploit vector.

qc

I discovered the IPB exploit you mentioned, it was not the type "O+" but the length "+3" that was the culprit, e.g. "O:+3:..." could be used to circumvent an early version of the validation regex. It seems SMF uses the same regex as IPB did but with an additional scan for "+" and "-" in the length.

Quote from: Arantor on May 02, 2016, 11:46:02 AMI think I pushed for this to be introduced once modders started using autoloaders, which could make this viable as an exploit vector.
Just from a quick glance it seems there has been an exploit for that unserialize issue already, so including that fix was probably for the best.
Playing quizduell? Having quizduell questions? Our german quizduell forum quizcommunity.de is looking for quiz freaks to come and play quizduell with us :)

Arantor

Huh, so it was. Been a while since I looked at it, was going off my memory. Fun fact, IPB added the same function as safe_unserialise and they got it from phpBB originally (as did SMF, I think) and added the +/- check because it was vulnerable.

In base SMF you're largely restricted to whatever you can statically call from the standard library, though that's not completely safe as demonstrated (and fixed properly in 2.1 to not use serialise in the first place)

Advertisement: