Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Upstream merge 1.9.4.5 #944

Merged
merged 3 commits into from
May 5, 2020
Merged

Upstream merge 1.9.4.5 #944

merged 3 commits into from
May 5, 2020

Conversation

Flyingmana
Copy link
Contributor

No description provided.

@colinmollenhour colinmollenhour merged commit 16dc8f7 into 1.9.4.x May 5, 2020
@sreichel sreichel added the patch label May 9, 2020
@joniklas
Copy link

joniklas commented May 14, 2020

I don't know if this is the right place to ask:

But is there any background information why the password upgrade process is now checking the hash version against Mage_Core_Model_Encryption::HASH_VERSION_SHA256 and not against Mage_Core_Model_Encryption::HASH_VERSION_LATEST anymore?

Isn't this reducing security? Thanks for pointing to some insights.

@seansan
Copy link
Contributor

seansan commented May 14, 2020

Is this The V2 version of the patch? There seems to be a critical hack in v1 version https://twitter.com/colinodell/status/1260683841452363776

@colinmollenhour
Copy link
Member

@seansan Version 2 was applied, the $authResult check already exists. Thanks for checking, though!

@joniklas
Copy link

joniklas commented May 15, 2020

@colinmollenhour And why is it checking hash version against Mage_Core_Model_Encryption::HASH_VERSION_SHA256 and NOT Mage_Core_Model_Encryption::HASH_VERSION_LATEST ? Don't we want to use bcrypt implementation from PHP if available?

@colinmollenhour
Copy link
Member

There are two methods, getHash and getHashPassword. This update upgrades the former from MD5 to SHA256 by default and the latter remains using bcrypt as long as the password_hash function exists.

@joniklas
Copy link

joniklas commented May 15, 2020

@colinmollenhour Also this upgrade changes some occurrences of getHashPassword to getHash. (in Mage_Admin_Model_User::_getEncodedPassword, Mage_Api_Model_User::_getEncodedApiKey and Mage_Customer_Model_Customer::hashPassword)
Aren't these the places where we want bcrypt!?

@colinmollenhour
Copy link
Member

You are probably right, but I don't know the reasoning behind why the Magento team made those changes.. If you want to submit a working and tested PR that changes it we would be happy to review and consider changing it.

@sreichel sreichel deleted the upstream-merge-1.9.4.5 branch June 5, 2020 02:19
@sreichel sreichel added this to the Release 19.4.3 milestone Jun 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants