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

Changing password hashing algorithm from sha1 #68

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

C3realGuy
Copy link
Contributor

@C3realGuy C3realGuy commented May 21, 2017

to bcrypt using the password_hash() function.
We now transfer the password in plain from client to server, before we sha1 hashed it. So make sure
you have ssl activated for wedge. I completely rewrote the login authentication, the old code was full of compatibilty stuff which is not needed anymore in my opinion. Login stuff should be as short and simple as possible to make sure it's not buggy.
This is still WIP, currently you have to change the {db_prefix}members passwrd column to VARCHAR(255) to make this work properly.

Working:

  • Login
  • Updating old sha1 password hashes

Not working:

  • Changing password

to bcrypt using the password_hash() function.
We now transfer the password in plain from client
to server, before we sha1 hashed it. So make sure
you have ssl activated for wedge.
I completely rewrote the login authentication, the
old code was full of compatibilty stuff which is not
needed anymore in my opinion. Login stuff should be
as short and simple as possible to make sure it's
not buggy.
This is still WIP, currently you have to change the
{db_prefix}members passwrd column to VARCHAR(255) to
make this work properly.
@MissAllSunday
Copy link

May be better to move the cost to a $settings var or something similar, mainly because many crappy free hostings will choke on a 10 cost and also give a chance to those lucky ones to use a higher cost too.

Either on install or upgrade, use the function provided at php docs

As for setting the cookies, I wouldn't worry about hashing the password, instead add a new table with a token and a expiration value. The good thing about this is that the password isn't set and this works on a "per computer" basis, if the token gets compromised the password is still secured and you can easily destroy the compromised token from your own table by deleting the record, I have a working class and so far I haven't see any issues with it.

@C3realGuy
Copy link
Contributor Author

C3realGuy commented Nov 28, 2017

@MissAllSunday thanks for reviewing this (and sorry for me taking so much time to answer)

May be better to move the cost to a $settings var or something similar, mainly because many crappy free hostings will choke on a 10 cost and also give a chance to those lucky ones to use a higher cost too.

This was (or is) planned. Just don't know when to finish and how to test this thing, because it's a bit critical to change this thing :D Changing this to use a variable from Settings.php should be simple enough.

Either on install or upgrade, use the function provided at php docs

I'm using those methods, what do you mean? Maybe i didn't get your point :)

As for setting the cookies, I wouldn't worry about hashing the password, instead add a new table with a token and a expiration value. The good thing about this is that the password isn't set and this works on a "per computer" basis, if the token gets compromised the password is still secured and you can easily destroy the compromised token from your own table by deleting the record, I have a working class and so far I haven't see any issues with it.

The problem i had was that the session cookie was somehow getting generated on base of the hashed password (if i remember correctly). For me this behaviour sounds a bit weird and maybe we could just change that.

@Nao
Copy link
Member

Nao commented Nov 28, 2017

What's the latest news on this..? Are there any news actually..?

@C3realGuy
Copy link
Contributor Author

C3realGuy commented Nov 29, 2017

What's the latest news on this..? Are there any news actually..?

Nope, not any at all. If you find some time and think it's worth to replace sha1 with bcrypt, maybe this pull request is a good base. If i remember correctly most of the thinks worked. Except of maybe changing your password or something like that, but the login (and updating the old sha1 password with the new bcrypt thing if the old password hashes matched) worked.
As with the latest imgur hack disclosure in mind, it's maybe worth to harden your password hashes, because sha1 will not be something hard to crack, even with random per user salts as it is with wedge.

I would love to find some time to get this working... at some day i will pick this one up again ^^

@Nao
Copy link
Member

Nao commented Mar 9, 2018

Well, I don't know, I'm a bit rusty with the password-related codebase. (Mostly because Lestrades.com doesn't use password, instead getting tokens from the Steampowered.com website... Much easier to register that way.)

@Nao
Copy link
Member

Nao commented May 11, 2019

Technically, changing one's password matters a lot I'd say... ;)
I do appreciate though that you'd want to rewrite this, but can you make sure it all works?

Also: would it require a database update or everyone to log in again, if one simply updated their forum files with this..?

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.

3 participants