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

Stronger password encryption - bcrypt instead of SHA256 #25

Open
2 tasks
tiblu opened this issue Oct 31, 2018 · 12 comments
Open
2 tasks

Stronger password encryption - bcrypt instead of SHA256 #25

tiblu opened this issue Oct 31, 2018 · 12 comments
Labels
API API issue. Needs dev time. discussion Philosophical discussions.

Comments

@tiblu
Copy link
Member

tiblu commented Oct 31, 2018

Overview

SHA256-s time is over and a person with an access to Citizen OS database can figure out the actual password with a reasonable effort.

TODO

  • Use bcrypt OR something else. There is a whitepaper on the subject by RIA - https://www.ria.ee/sites/default/files/content-editors/publikatsioonid/cryptoreport2021.pdf
  • Figure out and implement a User friendly way to migrate the passwords from SHA256 to bcrypt.
    • IF there are not many Users that have used user/password authentication, is to just random generate new passwords for them which will force them to "Forgot password" flow and thus generating a new bcrypt in the DB. IF allowed, we can e-mail Users that we did it. The solution is the easisest and does not clutter the code.
    • Polite way, if many Users, is to swap the password on next login - when password is verified against SHA256 use bcrypt to hash the password and store it to DB. The problem with this solution is that we have to support parallel hashes for a while.
@tiblu tiblu added the security label Oct 31, 2018
@loorm
Copy link
Member

loorm commented Oct 21, 2019

Triage 13 - Mikk said there are things we need to discuss here.

@tiblu
Copy link
Member Author

tiblu commented Oct 21, 2019

@loorm
Yes, we need to discuss if we want to use any of my proposed solutions or we go for some other when it comes to migrating Users.

To give you an idea how many people MAY be affected, I counted the total Users in the DB that have passwords (non FB, Google... logins) and I counted 332.

@loorm loorm removed the security label Feb 28, 2020
@loorm loorm changed the title SECURITY: Stronger password encryption - bcrypt instead of SHA256 Stronger password encryption - bcrypt instead of SHA256 Mar 2, 2020
@loorm loorm added API API issue. Needs dev time. discussion Philosophical discussions. labels May 28, 2020
@loorm
Copy link
Member

loorm commented May 28, 2020

Assigning to Triage 23 on June 8.

@loorm
Copy link
Member

loorm commented Jun 8, 2020

Triage 23. This will not be the last time a crypto algorithm becomes out-dated and we need to migrate users. Therefore, let's create a new flow, where we ask users to update their password and explain why this is necessary. Sending to in prep.

@KatiVellak
Copy link

Legally reviewed, no additional comments.

@loorm
Copy link
Member

loorm commented Sep 8, 2021

First, I'm assuming this is still relevant. @tiblu, @ilmartyrk - is it?

If it is, then Mikk had this solution above:
"to just random generate new passwords for them which will force them to "Forgot password" flow and thus generating a new bcrypt in the DB. IF allowed, we can e-mail Users that we did it. The solution is the easisest and does not clutter the code."

Can we do this, minus the e-mail? Instead, notify these specific users with an in-app notification the next time they try to log in? Text could be: "Your previous password was less secure due to crypto algorithms developing over time. We have updated to more secure algorithms. Please click "Forgot password" to create a new, stronger password."

@tiblu
Copy link
Member Author

tiblu commented Sep 9, 2021

@loorm @ilmartyrk

First, I'm assuming this is still relevant. @tiblu, @ilmartyrk - is it?

If it is, then Mikk had this solution above:
"to just random generate new passwords for them which will force them to "Forgot password" flow and thus generating a new bcrypt in the DB. IF allowed, we can e-mail Users that we did it. The solution is the easisest and does not clutter the code."

Can we do this, minus the e-mail? Instead, notify these specific users with an in-app notification the next time they try to log in? Text could be: "Your previous password was less secure due to crypto algorithms developing over time. We have updated to more secure algorithms. Please click "Forgot password" to create a new, stronger password."

Yes, still relevant.

Technically we can do an in-app notification, BUT I would not AS Users MAY think of it as a suspicious behavior. Maybe they were hacked? Maybe my password has leaked? But it's preventative action, none of the cases stated.

IF at the time of implementation, we find that there are too many Users logging in with password, so many that we do not want them all go through the "Forgot password" flow, we can use a different migration to new hash:

  • For a transfer period, DB keeps both old and new password hash.
  • IF there is no new password hash, we check against the old password. IF old password is valid, we generate new bcrypt password. DESTROY (delete/randomize) old password hash.
  • IF there is a new bcrypt password hash, we ALWAYS check against the new password field.
  • All new accounts have new password hashes.
  • After a reasonable (tm) transfer time, like 1 year, we start checking ONLY new password (bcrypt) field in the API. We DROP old password hash column. Now, if someone logs in 1 year later, has no new password, they will go through forgot password sequence and I think it's acceptable.

I still prefer, if password using User numbers are acceptable, send everyone through forgot password flow.
OR at least have a short grace period, 3-6 months rather than 1 year.

@tiblu
Copy link
Member Author

tiblu commented Sep 28, 2021

Date: 28.09.2021:

prod::DATABASE=> SELECT COUNT(*) FROM "Users";
 count 
-------
  8386

prod::DATABASE=> SELECT COUNT(*) FROM "UserConnections" WHERE "connectionId" = 'citizenos';
 count 
-------
  2512

By the numbers on the 28th of Sept 2021 29.95% of Users use or have used password to log in to Citizen OS.

@loorm
Copy link
Member

loorm commented Sep 29, 2021

@tiblu

We both agree, that making the change and sending these 2512 people through the "forgot password" flow is the way to go.

I think we also both agree, that you cannot send someone to "Forgot password" flow, if they haven't really forgotten their password, without giving them an explanation.

You would prefer to send the explanation via e-mail.
I don't believe in e-mails and would prefer to receive the notification in app, at the moment when it is most relevant (i.e. when I have started to log in). This would not be a message visible to all users at all times, just pops up for users with uld passwords, who need to update them. The message text should make the reason very clear, such as: "Your password is quite old, please update it to stay secure."

Is this a correct understanding of things? If it is, what is your argument for e-mail over in-app message?

@anettlinno
Copy link
Collaborator

anettlinno commented Mar 29, 2022

Triage 63. Important update.

When user starts logging-in, we direct her to change her password through password-reset flow. There should also be explanatory text which explains that we are renewing our security systems and user has to change the password for any further action.

We inform users about the changes in notification/information bar in bottom of the page AND in password reset view as well, in case user enters via email invitation or link.

Est. dev. time 4 days. Sending this issue to To Do.

@ilmartyrk
Copy link
Member

Technical solution would be to update password encryption on API side, we delete all passwords that exist in our DB, then when users are logging in we can check if password exists and matches, exists, but doesn't match (display the current wrong password error) or doesn't exist and redirect them into reset/create password window in FE that has informative text displayed about what is going on.

@BeccaMelhuish
Copy link
Contributor

@ilmartyrk I'll assume this one's OK to go to 'later' as is an old dormant issue

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API API issue. Needs dev time. discussion Philosophical discussions.
Projects
Status: Backlog - Later
Development

No branches or pull requests

6 participants