Disable class unserialization where it is not needed.

[colinmollenhour]

The unserialize() method by default will unserialize a payload with any serialized classes making it vulnerable to poisonous payloads. This PR makes use of the second parameter added in PHP 7.0 which allows you to specify specific classes that may be allowed or to disallow all classes (effectively only allowing scalar values and arrays). It may or may not be patching any specific known vulnerabilities but it is definitely trying to be proactive in preventing future vulnerabilities by going ahead and disallowing classes anywhere there is not a known use case for them.

Please review thoroughly as I am not an expert on every area of Magento and it is possible that Magento serializes classes legitimately in cases I am not aware of, but I did a quick review of every line I changed and it seems Magento already did a good job of generally serializing only arrays (e.g. no usages of __sleep and __wakeup in Mage namespace, mostly arrays passed to Varien_Object constructors)

Description (*)

Adds ['allowed_classes' => false] as second parameter of unserialize() to prevent possible vulnerabilities exploits that rely on the default behavior of unserialize to instantiate any class.

Manual testing scenarios (*)

All areas where code was modified should ideally be tested..

Questions or comments

This is completely untested so far...

It would be great if we could somehow enforce the use of this second parameter (prevent use of unserialize with only the first parameter) in the future.. I don't know a great way to do this, though.

Contribution checklist (*)

• Pull request has a meaningful description of its purpose
• All commits are accompanied by meaningful commit messages
• All automated tests passed successfully (all builds are green)

[Flyingmana]

looks good from the general approach.
I can put this on one of the demo instances on the weekend for testing purposes

[joshua-bn]

@colinmollenhour do you know of anywhere that we need classes to be unserialized?

Shouldn't these use Zend_Serializer?

Then we can override the PhpSerialize adapter

    /**
     * Unserialize
     *
     * @todo   Allow integration with unserialize_callback_func
     * @param  string $serialized
     * @param  array $opts
     * @return mixed
     * @throws Zend_Serializer_Exception on unserialize error
     */
    public function unserialize($serialized, array $opts = array())
    {
        // TODO: @see php.ini directive "unserialize_callback_func"
        $ret = @unserialize($serialized);
        if ($ret === false && $serialized !== self::$_serializedFalse) {
            $lastErr = error_get_last();
            #require_once 'Zend/Serializer/Exception.php';
            throw new Zend_Serializer_Exception($lastErr['message']);
        }
        return $ret;
    }

So $opts = array_merge(['allowed_classes' => false], $opts); and then $ret = @unserialize($serialized, $opts['allowed_classes']);

[colinmollenhour]

@colinmollenhour do you know of anywhere that we need classes to be unserialized?

Not offhand, that's why this needs much more review.

Shouldn't these use Zend_Serializer?

I think we should get away from using these Zend classes unless there are really good reasons. I think it is likely entirely obsolete now that PHP 7 added 'allow_classes' functionality so if anything I'd actually go back and remove uses of both Zend_Serializer and also the Mage 'unserializeArray' helper in favor of using this second option and not using a wrapper.

Then we can override the PhpSerialize adapter...
So $opts = array_merge(['allowed_classes' => false], $opts); and then $ret = @unserialize($serialized, $opts['allowed_classes']);

Good suggestions!

[joshua-bn]

I think we should get away from using these Zend classes unless there are really good reasons.

I tend to agree. In this case PHP doesn't throw a normal exception so we would need to catch Throwable to get rid of the Zend_Serializer class.

[colinmollenhour]

What about replacing all the found usages of raw unserialize() with Mage::helper('core')->unserializeArray() which looks like this:

    /**
     * @param string $str
     * @return mixed
     * @throws Exception
     */
    public function unserializeArray($str)
    {
        try {
            $result = unserialize($str, ['allowed_classes' => false]);
            if ($result === false && $str !== serialize(false)) {
                throw new Exception('Error unserializing data.');
            }
            return $result;
        } catch (Error $e) {
            throw new Exception('Error unserializing data: '.$e->getMessage(), 0, $e);
        }
    }

This way all parse errors are converted to Exception and it also correctly handles unserializing "false" without counting it as an error.

[joshua-bn]

Changing the Mage_Core_Helper_UnserializeArray class (core/unserializeArray) or adding that new method?

I don't know what the purpose of that class is/was. So it doesn't unserialize objects? This should just replace that whole thing.

The name bugs me. "unserializeArray" implies it's unserializing an array. It can be anything.

I'd make another parameter array $allowedClasses =[]. Then $allowedClasses = $allowedClasses ?: false; So if the array is empty, it is false. If it is not empty, it is only those classes. There would be no "true" option that allows all classes - which is ridiculously dangerous.

Is there a better exception to use than \Exception? Mage_Core_Exception even? An UnserializeException would be best. That could extend \RuntimeException

[colinmollenhour]

All good ideas, @joshua-bn, please let me know if you'd like to tackle the issue, I don't think I will be able to any time really soon..

[joshua-bn]

@colinmollenhour not sure if/when I will be able to work on it. If I get time, I'll give it a shot.

I don't have a copy of OpenMage running. I just copy the changes from here to my highly modified implementation.

[joshua-bn]

It might not be a perfect implementation, but it does a lot for security now and that's better than perfection later.

[convenient]

I don't know of a single used instance where magento serialized a class and expected it to be unserialized. It was always arrays of data in my experience.

Maybe @pocallaghan has thoughts? To me this seems a change that should have been made years ago.

[colinmollenhour]

In my opinion this is ready to merge. I tested with a fresh installation by creating a product and category, adding to cart, completing checkout, creating an account, adding to wishlist, updating wishlist comment, etc.

[joshua-bn]

I agree. It looks good.

[leganzaro]

Possible issues with unserialize (second parameter) in php 7.1, 7.2 & 7.3:

Warning: unserialize() expects exactly 1 parameter, 2 given in ../app/code/core/Mage/Core/Model/Cache.php on line 503
Warning: unserialize() expects exactly 1 parameter, 2 given in ../lib/Varien/Db/Adapter/Pdo/Mysql.php on line 1567
Warning: unserialize() expects exactly 1 parameter, 2 given in ../app/code/core/Mage/Core/Model/Cache.php on line 614
...

[Flyingmana]

as of https://www.php.net/manual/en/function.unserialize.php the second parameter should be supported since 7.0

[arthurpro]

yesterday I merged openmage into our codebase and this change broke several legitimate objects saved in the database resulting in an error The script tried to execute a method or access a property of an incomplete object.

this needs some changes for backward compatibility instead of forcing it to ['allowed_classes' => ['Varien_Object']]

[tmotyl]

@arthurpro can you please create an issue about it? Please include details about your use cases, how this can be reproduced. Thanks