Backport Magento security Update 2.3.5-p2 from 20200728 APSB20-47

[theroch]

Description (*)

• Update _validateSecretKey in Mage_Adminhtml_Controller_Action to avoid time attacks related to CVE-2020-9690
• It is important to provide the user-supplied string as the second parameter, rather than the first.

Related Pull Requests

• none

Fixed Issues (if relevant)

• none

Manual testing scenarios (*)

1. Do several action in backend

Questions or comments

• I've reviewed the p2 patch, but I think the other fixed CVEs are realted to M2 only.

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]

Thank you a lot for checking this and contributing.
I added you to our internal advisories regarding this.

When you contact us over the recommended way (noted in https://github.com/OpenMage/magento-lts/blob/1.9.4.x/SECURITY.md ) we can handle this in private and better coordinate the release of security relevant information and releases.

Regarding the Patch, we use hash_equals() already on other places direct, so we do not need the fallback.
Also its a risk for itself to copy this and not using it via a composer dependency, as there may be related security issues in the future and we would not get an info about it this way.
(using such dependencies via composer is on our list)

[theroch]

@Flyingmana
Thx for your explanations and sorry for breaking the reporting process. It was the first time providing security related information for me. I will stick to it next time.

Should the PR to be closed now?

[Flyingmana]

No Problem, its also for us the first time to go through this process as a whole :)

[joshua-bn]

I implemented this locally:

    protected function _validateSecretKey(): bool
    {
        if (in_array($this->getRequest()->getActionName(), $this->_publicActions)) {
            return true;
        }

        $secretKey = (string) $this->getRequest()->getParam(Mage_Adminhtml_Model_Url::SECRET_KEY_PARAM_NAME, null);
        $adminSecretKey = (string) $this->getAdminUrlModel()->getSecretKey();

        return $secretKey && hash_equals($adminSecretKey, $secretKey);
    }

    protected function getAdminUrlModel(): Mage_Adminhtml_Model_Url
    {
        return Mage::getSingleton('adminhtml/url');
    }

[colinmollenhour]

I definitely like @joshua-bn solution better, far less code. hash-equals is supported since PHP 5.6 so no reason to have a helper and a library to replace one simple function.

[joshua-bn]

lol, as you said that I realized I made a mistake when I was cleaning it up.

edited it again because I use return type hints locally with PHP 7.4.

[Flyingmana]

This was resolved as part of the latest release (see security advisory and mentioned CVE in changelog)

[Flyingmana]

Fun fact, we already have a polyfill in our code for the hash_equals function, for the old php versions

[joshua-bn]

Thanks @Flyingmana https://github.com/OpenMage/magento-lts/blob/20.0/app/code/core/Mage/Adminhtml/Controller/Action.php#L397 (for anyone else interested)