Use native PHP Password API #992

Closed
airbone42 opened this Issue Jan 21, 2015 · 28 comments

Comments

Projects
@airbone42
Member

airbone42 commented Jan 21, 2015

PHP 5.5 introduced a new password API natively to PHP. http://php.net/manual/en/book.password.php

As using BCRYPT for the default hashing algorithm it's not only more secure than the current implementaiton of md5 and sha256. But will even be automatically maintained with newer PHP versions and does not depend on any maintenance or upgrades by Magento.

So my suggestion is to replace the current hashing implementation in the Encryptor with using native password_hash and password_verify. Especially for an e-commerce system security should have a very high priority.

So rumors tell that Magento 2 will soon raise min. requirements to PHP 5.5, so that would be the best point to integrate this. Anyway if that min. version update might not come there's also a backward compatibility library available at https://github.com/ircmaxell/password_compat which could be used for PHP <5.5.

If Magento needs MD5 and SHA256 for b/c to Magento 1 hashes, I would suggest to move that into a separate module, so new Magento 2 shops without old data don't need to bother about this old hasing algorithms and the code coming with it. Even shops with older Magento 1 data could remove that b/c module after all customers have updated their password over time or by enforcing them after the first login. This reduces amount of code and complexity, buy having these Mage1 b/c modules and migrations optional.

Anyway, would Magento be interested in porting the Encryptor into that way? If it will get accepted I would definitely dig into this and try to create a PR to speed up development.

@davidalger

This comment has been minimized.

Show comment
Hide comment
@davidalger

davidalger Jan 21, 2015

Member

If I remember correctly, @ericthehacker mentioned to me the other day that he was planning on porting/integrating his M1 module (https://github.com/ericthehacker/magento-phpnativepasswords) for supporting this to M2 and submitting a PR. Don't know if he's started yet or not. I think the idea is a good move in the right direction.

For supporting the old hashes, it should be pretty trivial to support multiple hash types, but I think supporting them on an ongoing basis is a must and IMO it wouldn't make sense to split different hash types out into separate modules.

Member

davidalger commented Jan 21, 2015

If I remember correctly, @ericthehacker mentioned to me the other day that he was planning on porting/integrating his M1 module (https://github.com/ericthehacker/magento-phpnativepasswords) for supporting this to M2 and submitting a PR. Don't know if he's started yet or not. I think the idea is a good move in the right direction.

For supporting the old hashes, it should be pretty trivial to support multiple hash types, but I think supporting them on an ongoing basis is a must and IMO it wouldn't make sense to split different hash types out into separate modules.

@ericthehacker

This comment has been minimized.

Show comment
Hide comment
@ericthehacker

ericthehacker Jan 21, 2015

Contributor

I can confirm that I am planning to port my M1 module to M2 and issue a pull request.

Contributor

ericthehacker commented Jan 21, 2015

I can confirm that I am planning to port my M1 module to M2 and issue a pull request.

@ericthehacker

This comment has been minimized.

Show comment
Hide comment
@ericthehacker

This comment has been minimized.

Show comment
Hide comment
@ericthehacker

ericthehacker Jan 22, 2015

Contributor

@airbone42 @davidalger
The implementation is complete, with unit tests, on https://github.com/ericthehacker/magento2/compare/feature/native-password-hashing .

The only remaining issue is to determine the best way to include the compat library. Although it is composer friendly, it doesn't use any particular class naming standard -- it simply defines the collection of four global functions which comprise the native PHP password hashing API -- so I can't quickly think of a good way to ensure it's available without relying on the autoloader.

I'll probably take a look at this tomorrow, but any ideas on how to import this compat library on PHP < 5.5.0 are welcome in the meantime.

Contributor

ericthehacker commented Jan 22, 2015

@airbone42 @davidalger
The implementation is complete, with unit tests, on https://github.com/ericthehacker/magento2/compare/feature/native-password-hashing .

The only remaining issue is to determine the best way to include the compat library. Although it is composer friendly, it doesn't use any particular class naming standard -- it simply defines the collection of four global functions which comprise the native PHP password hashing API -- so I can't quickly think of a good way to ensure it's available without relying on the autoloader.

I'll probably take a look at this tomorrow, but any ideas on how to import this compat library on PHP < 5.5.0 are welcome in the meantime.

@ericthehacker

This comment has been minimized.

Show comment
Hide comment
@ericthehacker

ericthehacker Jan 23, 2015

Contributor

It's still not clear to me how to import the compatibility library since it doesn't contain a class which the autoloader will pick up on.

Obviously, I could follow the M1 approach of using require_once in the model which uses it, and this of course works perfectly: https://gist.github.com/ericthehacker/09ed7e8f1ac4d567c90c .

However, in the M2 core, I can't find a clear precedent for arbitrarily requiring external libraries, nor can I find a similar example of including a vendor function-only library.

Of course, if bumping the minimum PHP requirement to 5.5.0 is an option (which I would support), this issue may be irrelevant.

Any ideas?

Contributor

ericthehacker commented Jan 23, 2015

It's still not clear to me how to import the compatibility library since it doesn't contain a class which the autoloader will pick up on.

Obviously, I could follow the M1 approach of using require_once in the model which uses it, and this of course works perfectly: https://gist.github.com/ericthehacker/09ed7e8f1ac4d567c90c .

However, in the M2 core, I can't find a clear precedent for arbitrarily requiring external libraries, nor can I find a similar example of including a vendor function-only library.

Of course, if bumping the minimum PHP requirement to 5.5.0 is an option (which I would support), this issue may be irrelevant.

Any ideas?

@buskamuza

This comment has been minimized.

Show comment
Hide comment
@buskamuza

buskamuza Jan 23, 2015

Contributor

@ericthehacker , we're going to stop supporting PHP 5.4 soon. The work is already in progress.
So maybe you just don't need compatibility library then?

Contributor

buskamuza commented Jan 23, 2015

@ericthehacker , we're going to stop supporting PHP 5.4 soon. The work is already in progress.
So maybe you just don't need compatibility library then?

@alankent

This comment has been minimized.

Show comment
Hide comment
@alankent

alankent Jan 23, 2015

I have not looked at any code, but just for clarity a requirement is going to be to upgrade customers from M1 with currently encrypted passwords. So while using a better hash for new passwords sounds great (1) we need to migrate old passwords during an upgrade, and (2) we would need to schedule an internal security audit of the new code as a safeguard.

I have not looked at any code, but just for clarity a requirement is going to be to upgrade customers from M1 with currently encrypted passwords. So while using a better hash for new passwords sounds great (1) we need to migrate old passwords during an upgrade, and (2) we would need to schedule an internal security audit of the new code as a safeguard.

@ericthehacker

This comment has been minimized.

Show comment
Hide comment
@ericthehacker

ericthehacker Jan 23, 2015

Contributor

@buskamuza
Thanks for the clarification of regarding PHP version support.

@alankent
Upgrading previously hashed customer passwords shouldn't be a problem -- I've already implemented this in my M1 module but wasn't sure if it was necessary for M2.

Contributor

ericthehacker commented Jan 23, 2015

@buskamuza
Thanks for the clarification of regarding PHP version support.

@alankent
Upgrading previously hashed customer passwords shouldn't be a problem -- I've already implemented this in my M1 module but wasn't sure if it was necessary for M2.

@avoelkl

This comment has been minimized.

Show comment
Hide comment
@avoelkl

avoelkl Jan 23, 2015

Contributor

Good thing to use the standard PHP password functions from PHP >= 5.5! As PHP 5.4 is in extended support with security fixes already and will be at end of live at the time when M2 is released, this is the chance to do so (as @buskamuza already confirmed).
password_hash/password_verify are available from PHP 5 >= 5.5.0 on so I don't see a need for the compatibility library then.

Regarding migration: I like the way @airbone42 suggested by putting the old encryption functionality into a seperate module which is optional. This keeps the old hashing from M2 away.
So there won't be a need to migrate old passwords all at once during an upgrade as this extra module could just check the password at login with the old hashing functions and create a new password with the new PHP password function.

Contributor

avoelkl commented Jan 23, 2015

Good thing to use the standard PHP password functions from PHP >= 5.5! As PHP 5.4 is in extended support with security fixes already and will be at end of live at the time when M2 is released, this is the chance to do so (as @buskamuza already confirmed).
password_hash/password_verify are available from PHP 5 >= 5.5.0 on so I don't see a need for the compatibility library then.

Regarding migration: I like the way @airbone42 suggested by putting the old encryption functionality into a seperate module which is optional. This keeps the old hashing from M2 away.
So there won't be a need to migrate old passwords all at once during an upgrade as this extra module could just check the password at login with the old hashing functions and create a new password with the new PHP password function.

@airbone42

This comment has been minimized.

Show comment
Hide comment
@airbone42

airbone42 Jan 24, 2015

Member

Anyway it's technically not possible to migrate the old hashes without bruteforcing them automatically. The only way would be to ask customers to specify a new password after the first login.

Member

airbone42 commented Jan 24, 2015

Anyway it's technically not possible to migrate the old hashes without bruteforcing them automatically. The only way would be to ask customers to specify a new password after the first login.

@ericthehacker

This comment has been minimized.

Show comment
Hide comment
@ericthehacker

ericthehacker Jan 26, 2015

Contributor

The only reasonable way to upgrade the hash security for old passwords would be to catch them during login, and if the passwords validates (using the old hash algorithm), rehash using the native API.

Contributor

ericthehacker commented Jan 26, 2015

The only reasonable way to upgrade the hash security for old passwords would be to catch them during login, and if the passwords validates (using the old hash algorithm), rehash using the native API.

@alankent

This comment has been minimized.

Show comment
Hide comment
@alankent

alankent Jan 26, 2015

Normal approaches (some mentioned earlier in this thread) are

  1. Keep track of version of hash function with password, so checking code knows which hash function to use (e.g. prefix password hash with version number)
  2. Try multiple hash functions and accept password if any hash function works

The first is more secure. Forcing customers to re-enter a new password is not acceptable.

Normal approaches (some mentioned earlier in this thread) are

  1. Keep track of version of hash function with password, so checking code knows which hash function to use (e.g. prefix password hash with version number)
  2. Try multiple hash functions and accept password if any hash function works

The first is more secure. Forcing customers to re-enter a new password is not acceptable.

@airbone42

This comment has been minimized.

Show comment
Hide comment
@airbone42

airbone42 Jan 27, 2015

Member

The problem with tracking the version and not migrating the passwords is that the list will get longer and longer over time. Right now we have MD5 and SHA256 just for backwards compatibility in Magento 2. And if that approach continues they'll still be there in Magento 3 if there's no cut or a migration at some point. So newer passwords will be more secure, but there might still be a lot of old MD5 passwords especially in the bigger shop systems which have their customer base for a long time.

This issue has to be about getting rid of unsecure hashing methods, not only about adding a new one (which is also important, but just the first step).

Member

airbone42 commented Jan 27, 2015

The problem with tracking the version and not migrating the passwords is that the list will get longer and longer over time. Right now we have MD5 and SHA256 just for backwards compatibility in Magento 2. And if that approach continues they'll still be there in Magento 3 if there's no cut or a migration at some point. So newer passwords will be more secure, but there might still be a lot of old MD5 passwords especially in the bigger shop systems which have their customer base for a long time.

This issue has to be about getting rid of unsecure hashing methods, not only about adding a new one (which is also important, but just the first step).

@alankent

This comment has been minimized.

Show comment
Hide comment
@alankent

alankent Jan 27, 2015

I will make sure the product team are aware of this thread. But be aware that an influence is whether all merchants are happy to be FORCED to reset all customer passwords rather than give them an option to CHOOSE whether to take advantage of new password hashing (as part of a M2 upgrade.)

I will make sure the product team are aware of this thread. But be aware that an influence is whether all merchants are happy to be FORCED to reset all customer passwords rather than give them an option to CHOOSE whether to take advantage of new password hashing (as part of a M2 upgrade.)

@davidalger

This comment has been minimized.

Show comment
Hide comment
@davidalger

davidalger Jan 27, 2015

Member

I see three possible solutions, which I'll list in order of a (security minded) preference:

  1. System only contains support for the PHP 5.5 hashing API. Users which have been imported and/or migrated in from Magento 1 may use (new functionality) to "reset" their password and regain access to their account. This option is best from a long-term security standpoint, but hardest on users, particularly if the UI/UX is not really thought through. This also requires the greatest development effort.
  2. System contains support for the PHP 5.5 hashing API and uses this hashing API exclusively by default. There is a system preference added to allow those migrating from old systems (may or may not be Magento 1) to enable support for older and less secure hashing algorithms with note (on the setting) indicating that upon login the hash will be upgraded to the modern and more secure PCI compliant hashing algorithm. This requires extra development work (to add the config setting) but allows the more security minded to prohibit the use of old hash data. If the setting is added, I would expect to see it off by default, requiring those desiring hash upgrades to enable the setting for the hash upgrades to occur.
  3. System contains support for the PHP 5.5 hashing API as well as the sha/md5 hashes previous versions of Magento CE/EE have used, automatically re-hashing the password to use the current standard upon customer login. This requires minimal extra development and is currently the behavior of EE when taking in data from an existing CE installation.

My personal preference would be option no 2.

Member

davidalger commented Jan 27, 2015

I see three possible solutions, which I'll list in order of a (security minded) preference:

  1. System only contains support for the PHP 5.5 hashing API. Users which have been imported and/or migrated in from Magento 1 may use (new functionality) to "reset" their password and regain access to their account. This option is best from a long-term security standpoint, but hardest on users, particularly if the UI/UX is not really thought through. This also requires the greatest development effort.
  2. System contains support for the PHP 5.5 hashing API and uses this hashing API exclusively by default. There is a system preference added to allow those migrating from old systems (may or may not be Magento 1) to enable support for older and less secure hashing algorithms with note (on the setting) indicating that upon login the hash will be upgraded to the modern and more secure PCI compliant hashing algorithm. This requires extra development work (to add the config setting) but allows the more security minded to prohibit the use of old hash data. If the setting is added, I would expect to see it off by default, requiring those desiring hash upgrades to enable the setting for the hash upgrades to occur.
  3. System contains support for the PHP 5.5 hashing API as well as the sha/md5 hashes previous versions of Magento CE/EE have used, automatically re-hashing the password to use the current standard upon customer login. This requires minimal extra development and is currently the behavior of EE when taking in data from an existing CE installation.

My personal preference would be option no 2.

@pronto2000

This comment has been minimized.

Show comment
Hide comment
@pronto2000

pronto2000 Jan 27, 2015

You know, David, I like the third option a lot. In fact I have a project underway that involves importing customers from custom built system and I'm probably going to implement something like that. It's completely invisible from customer perspective and yet relatively simple to do. All that is needed is either to add an attribute for the hash type or make an educated guess based on hash length (first method being more safe, but requires more work). Also, this makes really easy to change hash method should a vulnerability found from the existing one.

You know, David, I like the third option a lot. In fact I have a project underway that involves importing customers from custom built system and I'm probably going to implement something like that. It's completely invisible from customer perspective and yet relatively simple to do. All that is needed is either to add an attribute for the hash type or make an educated guess based on hash length (first method being more safe, but requires more work). Also, this makes really easy to change hash method should a vulnerability found from the existing one.

@verklov

This comment has been minimized.

Show comment
Hide comment
@verklov

verklov Jan 29, 2015

Contributor

@davidalger, agree with @pronto2000. Looks like option 3 is easier to implement and eliminates the use of less secure hashing algorithm as soon as customer logs in to the system. Why do you personally prefer option 2?

Contributor

verklov commented Jan 29, 2015

@davidalger, agree with @pronto2000. Looks like option 3 is easier to implement and eliminates the use of less secure hashing algorithm as soon as customer logs in to the system. Why do you personally prefer option 2?

@davidalger

This comment has been minimized.

Show comment
Hide comment
@davidalger

davidalger Feb 2, 2015

Member

@verklov Simple. I prefer option 2 because it gives us both the easy route which some want, and the control that others would like with minimal effort. Essentially option 2 is exactly as Magento 1 currently behaves, but with a system configuration flag which allows implementors or site admins to control the behavior. I.e. determine whether or not the system supports only one hash system or will auto-upgrade older hashes on successful login. Option 3 strictly reflects current Magento Enterprise v1 behavior as it pertains to consuming md5 hashes in sites upgraded to Magento Enterprise from Magento Community

Member

davidalger commented Feb 2, 2015

@verklov Simple. I prefer option 2 because it gives us both the easy route which some want, and the control that others would like with minimal effort. Essentially option 2 is exactly as Magento 1 currently behaves, but with a system configuration flag which allows implementors or site admins to control the behavior. I.e. determine whether or not the system supports only one hash system or will auto-upgrade older hashes on successful login. Option 3 strictly reflects current Magento Enterprise v1 behavior as it pertains to consuming md5 hashes in sites upgraded to Magento Enterprise from Magento Community

@pronto2000

This comment has been minimized.

Show comment
Hide comment
@pronto2000

pronto2000 Feb 2, 2015

Hmm ... to me the main difference between option2 and option3 is that option3 automatically upgrades password security once customer logs on no matter what. Option2 basically makes higher security level optional. I see no reason whatsoever for that except perhaps because it provides an opportunity to downgrade.

Hmm ... to me the main difference between option2 and option3 is that option3 automatically upgrades password security once customer logs on no matter what. Option2 basically makes higher security level optional. I see no reason whatsoever for that except perhaps because it provides an opportunity to downgrade.

@davidalger

This comment has been minimized.

Show comment
Hide comment
@davidalger

davidalger Feb 2, 2015

Member

Option 2 (as I intended to write it) would not make higher security optional. It would make supporting less secure hashes optional. I.e. the system would support and use the more secure hashing algorithm by default. If you enabled it, it would also digest and auto-upgrade the older (less secure) hashes upon successful login. If left disabled, the system would not even attempt to validate agains the older hashes.

Member

davidalger commented Feb 2, 2015

Option 2 (as I intended to write it) would not make higher security optional. It would make supporting less secure hashes optional. I.e. the system would support and use the more secure hashing algorithm by default. If you enabled it, it would also digest and auto-upgrade the older (less secure) hashes upon successful login. If left disabled, the system would not even attempt to validate agains the older hashes.

@ericthehacker

This comment has been minimized.

Show comment
Hide comment
@ericthehacker

ericthehacker Mar 29, 2015

Contributor

@alankent
Was a decision on legacy password hashes ever made?

I've updated the feature branch to be compatible with develop at this point (specifically, by moving the unit tests to their new location after RC2).

Contributor

ericthehacker commented Mar 29, 2015

@alankent
Was a decision on legacy password hashes ever made?

I've updated the feature branch to be compatible with develop at this point (specifically, by moving the unit tests to their new location after RC2).

@antonkril

This comment has been minimized.

Show comment
Hide comment
@antonkril

antonkril Mar 30, 2015

Contributor

@ericthehacker we decided to go with solution similar to described in http://crypto.stackexchange.com/questions/2945/is-this-password-migration-strategy-secure

This will allow instant security for all hashes.

Contributor

antonkril commented Mar 30, 2015

@ericthehacker we decided to go with solution similar to described in http://crypto.stackexchange.com/questions/2945/is-this-password-migration-strategy-secure

This will allow instant security for all hashes.

@piotrekkaminski

This comment has been minimized.

Show comment
Hide comment
@piotrekkaminski

piotrekkaminski Apr 9, 2015

Contributor

@antonkril can you point to internal Jira ID with this change? There is a conflicting change planned

Contributor

piotrekkaminski commented Apr 9, 2015

@antonkril can you point to internal Jira ID with this change? There is a conflicting change planned

@ericthehacker

This comment has been minimized.

Show comment
Hide comment
@ericthehacker

ericthehacker May 5, 2015

Contributor

@piotrekkaminski
Was this conflict ever resolved?

Contributor

ericthehacker commented May 5, 2015

@piotrekkaminski
Was this conflict ever resolved?

@vpelipenko

This comment has been minimized.

Show comment
Hide comment
@vpelipenko

vpelipenko Jul 7, 2015

Contributor

Internal ticket: MAGETWO-35013

Contributor

vpelipenko commented Jul 7, 2015

Internal ticket: MAGETWO-35013

@tkn98

This comment has been minimized.

Show comment
Hide comment
@tkn98

tkn98 Dec 15, 2015

Contributor

Any traktion on this one? +1 to have support for password_hash(). Also +1 for an option to auto-upgrade on first login to have backwards compat build in for a user-migration phase.

Contributor

tkn98 commented Dec 15, 2015

Any traktion on this one? +1 to have support for password_hash(). Also +1 for an option to auto-upgrade on first login to have backwards compat build in for a user-migration phase.

@vkorotun vkorotun removed the PS label Aug 4, 2016

@piotrekkaminski

This comment has been minimized.

Show comment
Hide comment
@piotrekkaminski

piotrekkaminski Aug 30, 2016

Contributor

Thank you for your submission.

We recently made some changes to the way we process GitHub submissions to more quickly identify and respond to core code issues.

Feature Requests and Improvements should now be submitted to the new Magento 2 Feature Requests and Improvements forum (see details here).

We are closing this GitHub ticket and have moved your request to the new forum.

Contributor

piotrekkaminski commented Aug 30, 2016

Thank you for your submission.

We recently made some changes to the way we process GitHub submissions to more quickly identify and respond to core code issues.

Feature Requests and Improvements should now be submitted to the new Magento 2 Feature Requests and Improvements forum (see details here).

We are closing this GitHub ticket and have moved your request to the new forum.

@danslo

This comment has been minimized.

Show comment
Hide comment
@danslo

danslo Dec 16, 2016

Contributor

I've written a module that implements this, while preserving backward's compatibility with Magento hashing mechanism. It could be useful for some people: https://bitbucket.org/creaminternet/module-secure-passwords/

Contributor

danslo commented Dec 16, 2016

I've written a module that implements this, while preserving backward's compatibility with Magento hashing mechanism. It could be useful for some people: https://bitbucket.org/creaminternet/module-secure-passwords/

magento-team pushed a commit that referenced this issue Apr 5, 2017

Merge pull request #992 from magento-jackalopes/pr
- MAGETWO-66190 Remove usage of serialize/unserialize in tests
- MAGETWO-65623 Remove serialize/unserialize usages in sample data
- MAGETWO-65185 Improve upgrade performance

@magento-engcom-team magento-engcom-team moved this from TODO to Done in branch [2.3-develop] Sep 20, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment