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

Fix password hashing #1

merged 19 commits into from Feb 2, 2019


None yet
2 participants
Copy link

sexwarrior commented Jan 24, 2019

Eliminate old SHA1 password hashing (except to allow for users to transition as they log in)

SexWarrior and others added some commits Jan 24, 2019

Remove client-side sha1 hashing from blanko-fes
There may have been some merits to doing this back when HTTP was the default way of accessing a web forum, but it is the current year.
Get rid of client-side sha1 altogether
All password hashing should now be handled server-side, regardless of theme. Huzzah.
Eliminate super-legacy encryption login checks
There should not affect us, and we don't really need to check each incorrect password against 500 outdated hashes.
First attempt at accepting both password hash formats
This *should* now accept both password_hash() outputs and SMF's old SHA1 hashes. Let's see if it does.
Admin session validation should also work with new hashes
I don't think we need to worry about SHA1 here - the three users this will affect will most likely have logged in before they get caught by this
Fix login cookie when changing password
Isn't it great that the same thing has a different name in literally every file?
Actually let's still check sha1 in admin validation
This reverts the decision made in a2131f8 to not check sha1 here. There's no major reason not to, and this way is consistent with other parts of the application.

@sexwarrior sexwarrior requested a review from pardisfla Jan 24, 2019

Copy link

pardisfla left a comment

This looks good to me based on what you have changed, although I haven't looked at what you haven't changed to see if there's anything missed.

I'll sync mobiquo with this later and then merge the whole thing as one PR, with the reservation we already discussed regarding plaintext passwords potentially not being cleared from memory. I'm going to look into that further but it doesn't seem like a likely enough attack vector to justify blocking this change.

Show resolved Hide resolved Sources/Profile.php Outdated
Show resolved Hide resolved Sources/Profile-Modify.php Outdated

sexwarrior and others added some commits Jan 28, 2019

$_POST['passwrd2'] is no longer relevant when reloading user settings
At this stage the password has already been verified and changed - checking it again is meaningless, we just want to set up a working cookie

This comment has been minimized.

Copy link

pardisfla commented Feb 2, 2019

We're just removing mobiquo because it's a piece of crap, nobody uses it, and it's wasted work to maintain.

@sexwarrior sexwarrior added the e label Feb 2, 2019

@pardisfla pardisfla merged commit c63fba7 into master Feb 2, 2019

@pardisfla pardisfla deleted the fix-password-hashing branch Feb 2, 2019

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