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

password cleanups / added bcrypt as secure password storage #227

Merged
merged 1 commit into from Mar 11, 2016

Conversation

lifeofguenter
Copy link
Contributor

I did some cleanups on the password handling on poweradmin

  • I am guessing if you wanted to change $password_encryption, you would have to change it first in "config-me.inc.php" - but during installation it would always only hash the admin password as md5 anyway (broken feature?). This is fixed now, e.g. if you change the value of $password_encryption prior installing, it will hash the admin password accordingly.
  • I took all "password generating functions" away from toolkit.inc.php and put it into the separate file password.inc.php so that I could include it easily into the installer without including the heavyweight of toolkit itself
  • I added bcrypt hashing (which is de-facto safest and standard beginning with PHP5.5. To activate enable following in the config-me.inc.php _prior_ to installing:
$password_encryption = 'bcrypt'; // md5, md5salt or bcrypt
$password_encryption_cost = 12; // needed for bcrypt
  • I added a timing attack safe string comparison for the legacy password storages (with bcrypt it is already built-in) via @ircmaxell
  • Replaced rand with mt_rand in generate_salt (which does not make it perfect, but hey better than nothing)

@stasic
Copy link
Contributor

stasic commented Mar 2, 2015

Hi,
that looks really nice!
I have some questions regarding your pull request:
Why do you use namespace ?
Are you sure that install/database-structure.inc.php is working with %s properly?
We need function needs_rehash!

Thanks
-arsen

@lifeofguenter
Copy link
Contributor Author

Thanks for the feedback!

Why do you use namespace ?

To artificially put up the requirements to a sane PHP version.

Are you sure that install/database-structure.inc.php is working with %s
properly?

Yes it gets sprintfted - I tested the code BTW ;)

We need function needs_rehash!

Definitely - but so far it looks here like dry land, so I would not put up
high hopes.

Thank you again for your feedback. Highly appreciated.

lifeofguenter added a commit to phpmyzone/phpmyzone that referenced this pull request Jun 28, 2015
@edmondas edmondas added this to the v2.1.8 milestone Jul 15, 2015
@edmondas edmondas self-assigned this Jul 15, 2015
@SebTM
Copy link
Contributor

SebTM commented Mar 9, 2016

@edmondas I had a look at it and think it's done good so we can merge?

@edmondas
Copy link
Contributor

@SebTM In general it looks fine, so I'm merging it

edmondas pushed a commit that referenced this pull request Mar 11, 2016
password cleanups / added bcrypt as secure password storage
@edmondas edmondas merged commit 5077d32 into poweradmin:master Mar 11, 2016
}

function needs_rehash($hash) {
// @todo
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lifeofguenter did you remember what should be implemented here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

None yet

5 participants