Magento-lts: 1.9.4.5 (SUPEE-11314) downgrades password hashes to SHA256 (from "latest" Bcrypt)

[joniklas]

The patched password hash upgrade functionality changes password hashes if the hash version is NOT SHA256. This means is also changes password hashes if the "latest" hashing type Bcrypt is used! So it is a kind of downgrade.

This affects customers, admin users and api users.

How to reproduce:

• Check in database the password_hash for an excisting customer/admin is bcrypt (starts with $2y$...)
• Login with a customer/admin
• Check in database the password_hash is now SHA256

Code References:
Customer:
https://github.com/OpenMage/magento-lts/commit/16dc8f76f1f6bbbaf18005e88ae08df218d60aac#r39249967

if (Mage_Core_Model_Encryption::HASH_VERSION_SHA256 !== $currentVersionHash) {
$model->changePassword($password, false);
}

https://github.com/OpenMage/magento-lts/commit/16dc8f76f1f6bbbaf18005e88ae08df218d60aac#r39249998

Enterprise Editon:
In EE the end result of the password hashes is okay and stays with bcrypt (because of differing default values). But in EE the password hash gets changed on each and every login since it also compares against HASH_VERSION_SHA256 instead of HASH_VERSION_LATEST.

I expect OpenMage Community Edition also wants to stay with bcrypt for password hashing?

[colinmollenhour]

Thanks for your clear bug report, @joniklas. Can you submit a PR with a proposed fix? I think we all will agree that bcrypt is the preferred algorithm where available but I can't say when any of us will have time to create a patch ourselves.

[jfksk]

That really looks like Adobe did this for M1 -> M2 migrations.
M2 uses Argon2id via Sodium, if available, and SHA256 as fallback
Here is the M2 Encryptor

As, to my understanding, Argon2id is superior to bcrypt for storing passwords, I'd suggest to do the same. If compatibility is not a goal, bcrypt, being superior to SHA256, could be used as Fallback.

Quiet a while back, I wrote a tiny module to get rid of MD5. I kept it (and kept it compatible to M2), because the M1 core-code made me wanna cry plus I didn't want to break possible migrations. I didn't upgrade it yet, but I will add Argon2id - bcrypt support does not make a ton of sense to me - having Agron2id.

[joniklas]

I agree @jfksk, this encryptor code is not optimal. So the question is how to get the Bcrypt hashing back for all systems supporting it:

Options:
1.) Rewrite or extend the encryptor completely
2.) Basically revert the changes made in this version/patch.

• Use the formerly used dynamic \Mage_Core_Helper_Data::getVersionHash instead of the const HASH_VERSION_SHA256 in the comparison
• Use \Mage_Core_Helper_Data::getHashPassword instead of getHash

Questions:

• Does anybody see a security issue when using the old (dynamic) \Mage_Core_Helper_Data::getVersionHash? (Is any of the security issues in this security patch related to the usage of this method?)
• Is reverting (many of the) changes an acceptable patch for OpenMage?

[jfksk]

@joniklas, That's not my day. I needed to double-double check whats going on.

• Option 1: Requires quiet a bit of rewriting/migration logic.
• Option 2: That would leave you with no hash-upgrade-path, if I see that correctly. Is that right? Because that would be also really bad and should be fixed.

To break it down: the M2 way would be flexible but requires quiet a few changes. The M1 status quo seems to be fundamentally broken and desperatly needs an upgrade mechanism: MD5 and SHA512 to password_hash and password_hash to whatever password_hash spits out. I am unsure what would be better...

Besides that: I wondered, if MD5 is still good enough for everything else. Is that a concern for form keys and the admin URL key?

[jfksk]

Okay, some more thoughts on that:

Scenario 1:

• use M2 way of storing hashes. That means they would have the format {hash}:{salt}:{version}.
• use a hash version (SHA256) for getHash (form-keys ...).
• use Argon2id13 (if available) or SHA512 for passwords.

Dropping bcrypt would simplify that a little bit. Would that be acceptable, in hindsight, that sodium (Argon2id13) is bundled with php 7.2+?
All password hashes would be upgraded/sidegraded or even downgraded (bcrypt -> sha512) on login. That is an soft requirement because M1 is not storing the used algo. for creating the hash.
Argon2id13 hashes could be compatible with M2. SHA512 is not supported by M2. Furthermore M2 uses hash_hmac while M1 prepends the salt to the data.

Scenario 2:

• use a hash version (SHA256) for getHash (formkeys ...).
• use password_* for passwords but not with PASSWORD_DEFAULT as it still defaults to bcrypt. [1] Argon2 would be used if available. Or is there a good reason for the default?

SHA512/MD5 is dropped (upgraded on login/ replaced with SHA256)
All password hashes would be upgraded on login.
There will be an performance impact on login w/ Argon2id hashing.
Hashes will not be compatible to M2.
IMHO the easiest way to default to the strongest available algo.

[1] https://github.com/php/php-src/blob/bb6f374048bc0b4203e4fec7fd4e887519f663d6/ext/standard/password.c#L424

Scenario 3:

• completely revert the change.

There would would still be MD5 for getHash and only bcrypt for passwords.
AFAIK there wouldn't be a hash upgrade. That's IMHO seriously bad.
Hashes will not be compatible to M2 (bcrypt/hash_hmac vs prepending the salt).

Scenario 4:

• partially revert the change.

There would still be MD5 for getHash and only bcrypt for passwords.
Hashes will not be compatible to M2 (bcrypt/hash_hmac vs prepending the salt).
All password hashes would be upgraded on login (as it is, that's only to bcrypt)

To me, scenario 2 seems to be the way to go. Any thoughts on this? Other options/variations?

[colinmollenhour]

I think I'm fine with Scenario 2 but also think bcrypt is secure enough currently. I propose you go ahead and submit a PR for it that we can inspect and discuss further.

[jfksk]

I added PR #998 for discussion. That's basically scenario 2.

[joniklas]

@jfksk PR #998 looks nice! Very straight forward.
Dropping pre PHP 5.5 support should be fine!?

[jfksk]

@joniklas thx.
I mean even PHP 5.6 /7.0 are not receiving any support from the PHP core team since ~ 1/2019 and the project readme states 7.0+ (which should be perfectly fine with the PR) as requirement. IMHO, PHP 5 shouldn't be a concern anymore.

[jfksk]

Small update to whom it might concern: I pushed a small module that does exactly what the PR does, as I'm atm not quiet sure if I can do anything more to get it merged.
As I did so, I kindly like to ask to contact me personally if there is any security problem with it - TYIA!
I'm also on Discord and open for an informal discussion, if that helps...