UnserializeArray in Mage_Sales_Model_Quote_Item::compare causes exception and password leak

[Hanmac]

Related to this: #4352 we should be careful what kind of strings we are trying to unserialize

Preconditions (*)

1. Older Stacktrace, but Magento 20.3.0 should be affected too
2. Have a Product with a custom Option using a Text-Input Field
3. Have this Code piece:

magento-lts/app/code/core/Mage/Sales/Model/Quote/Item.php

Lines 519 to 524 in c294b09

	/** @var Unserialize_Parser $parser */
	$parser = Mage::helper('core/unserializeArray');
	$_itemOptionValue =
	is_numeric($itemOptionValue) ? $itemOptionValue : $parser->unserialize($itemOptionValue);
	$_optionValue = is_numeric($optionValue) ? $optionValue : $parser->unserialize($optionValue);

Steps to reproduce (*)

1. Login
2. Put Product into Cart with option Text "abc"
3. Logout
4. Put Product into Cart with option Text "def"
5. Login again, how it tries to Merge the Quotes

Expected result (*)

1. What should happen is two quote items

Actual result (*)

1. [Screenshots, logs or description]
2. This Exception in the Logs

Exception: Notice: unserialize(): Error at offset 0 of 4 bytes  in app/code/core/Mage/Core/Helper/UnserializeArray.php on line 44 in app/code/core/Mage/Core/functions.php:207
Stack trace:
#0 [internal function]: mageCoreErrorHandler(8, 'unserialize(): ...', '/var/www/riesae...', 44, Array)
#1 app/code/core/Mage/Core/Helper/UnserializeArray.php(44): unserialize('def', Array)
#2 app/code/core/Mage/Sales/Model/Quote/Item.php(540): Mage_Core_Helper_UnserializeArray->unserialize('stud')
#3 app/code/core/Mage/Sales/Model/Quote.php(1889): Mage_Sales_Model_Quote_Item->compare(Object(Mage_Sales_Model_Quote_Item))
#4 app/code/core/Mage/Checkout/Model/Session.php(303): Mage_Sales_Model_Quote->merge(Object(Mage_Sales_Model_Quote))
#5 app/code/core/Mage/Checkout/Model/Observer.php(44): Mage_Checkout_Model_Session->loadCustomerQuote()
#6 app/code/core/Mage/Core/Model/App.php(1410): Mage_Checkout_Model_Observer->loadCustomerQuote(Object(Varien_Event_Observer))
#7 app/code/core/Mage/Core/Model/App.php(1389): Mage_Core_Model_App->_callObserverMethod(Object(Mage_Checkout_Model_Observer), 'loadCustomerQuo...', Object(Varien_Event_Observer))
#8 app/Mage.php(517): Mage_Core_Model_App->dispatchEvent('customer_login', Array)
#9 app/code/core/Mage/Customer/Model/Session.php(262): Mage::dispatchEvent('customer_login', Array)
#10 app/code/core/Mage/Customer/Model/Session.php(247): Mage_Customer_Model_Session->setCustomerAsLoggedIn(Object(Mage_Customer_Model_Customer))
#11 app/code/core/Mage/Customer/controllers/AccountController.php(159): Mage_Customer_Model_Session->login('tester@testshop...', 'test123') ##### <<<< Leaked PW
#12 app/code/core/Mage/Core/Controller/Varien/Action.php(437): Mage_Customer_AccountController->loginPostAction()
#13 app/code/core/Mage/Core/Controller/Varien/Router/Standard.php(262): Mage_Core_Controller_Varien_Action->dispatch('loginPost')
#14 app/code/core/Mage/Core/Controller/Varien/Front.php(192): Mage_Core_Controller_Varien_Router_Standard->match(Object(Mage_Core_Controller_Request_Http))
#15 app/code/core/Mage/Core/Model/App.php(381): Mage_Core_Controller_Varien_Front->dispatch()
#16 app/Mage.php(752): Mage_Core_Model_App->run(Array)
#17 index.php(78): Mage::run('', 'store')
#18 {main}

Because the Exception happens while logging in, the stacktrace does leak the username and password.
But this might be a different can of worms.

Solution

I would test if the string does look like a serialized array before calling unserialize

// only ever try to unserialize if it looks like a serialized array
$_itemOptionValue = str_starts_with($itemOptionValue, 'a:') ? $parser->unserialize($itemOptionValue) : $itemOptionValue;
$_optionValue = str_starts_with($optionValue, 'a:') ? $parser->unserialize($optionValue) : $optionValue;

I'm going to make a PR soon

[Hanmac]

@sreichel about the password leak, might be worth it to look into #[\SensitiveParameter] ?

[fballiano]

@Hanmac If you're interested MahoCommerce/maho@891b27d

[sreichel]

@sreichel about the password leak, might be worth it to look into #[\SensitiveParameter] ?

It does not work with php7. There is PR reday for it. I'll publish later.

[sreichel]

Mhhh, i cannot reproduce it on recent OM.

At the end i have both items in cart w/o any logged exception.

Please check your code ... "in app/code/core/Mage/Core/Helper/UnserializeArray.php on line 44" looks strange. Method unserialize() should be called on line 30.

[Hanmac]

@sreichel found the reason, it was an old exception happened because of old version of UnserializeArray still using Unserialize_Parser

for password leak, also see this commit:
#386 (comment)