Closed micaksica2 closed 7 years ago
This is great, thank you! Just one small thing, could you move the constants to the top level of the module?
@micaksica2 really good call! Thanks for the PR. I second @daffl's changes, if you'd be so kind. š
I noticed another thing... from what I understand it looks like this will only increase the work rate whenever a new password is hashed. Correct me if I'm wrong there.
If so, can we also take the same approach mentioned in the article from team Clio - where we re-hash an existing password after a successful login if the salt cost factor is less than the one used to originally hash the password? This would ensure that so long as people are logging in again their passwords will be continually updated to stay more secure.
I think the algorithm looks like this:
We can check the cost factor used for a hash using bcrypt.getRounds
.
The code to check and rehash should probably all go right in here.
Let me know what ya'll think @daffl @marshallswain @micaksica2
@ekryski Correct, this will only change passwords when they are new. It will not increase password security of existing hashes.
Your algorithm is correct. If you have a successful login, you check to see if the current cost in the bcrypt hash is equivalent to the current cost, and then re-hash if stored_cost < current_cost to the current_cost value.
I didn't do this because 1) I am still too unfamiliar with the Feathers codebase to feel comfortable dropping all of this in without having had some conversations with maintainers, and 2) it opens up the entire password DB to a downgrade attack in which everyone can be reverted to BCRYPT_WORK_FACTOR_BASE
in an environment in which the system clock is set backward.
If I have control of NTP for the server, or the system clock directly, everyone that logs in in this period ends up downgraded, not just the users that have set new passwords. I don't think it's worth worrying about, and bcrypt 12 is still plenty good at this stage, so I'd say the risk is minimal in this case, but it's still a risk. A lot of other stuff is probably gonna go haywire with the clock that far off (SSL cert verification is my first expectation if this is years into the future, as many certs will likely have been issued later than that malicious date.)
@micaksica2 thanks! I appreciate the thought. I definitely agree. Had not thought about being able to downgrade. I think this is good to š¢.
@daffl @marshallswain I think we can just bring this is and do a patch release. Nothing breaking here, unless I'm missing something....
Sounds good agreed. Making the release now.
Released as v0.4.4
Cost factor 12 appears to be generally accepted as standard in many bcrypt libraries as default in the present-day. When SecureDrop used
bcrypt
, they believed 12 was acceptable for their extreme threat model as of 2013.Summary
This pull request is a proof-of-concept that increases password hash security for
bcrypt
as used by Feathers.js from work factor 10 to cost factor 12, and self-optimizes its cost factor in the future. I found this idea in this Medium post and find it especially useful for frameworks, in which the maintainers may not be able to get their code updated, but instead can increase overall security.Every 1.5 years from 1/1/17, the cost factor will increase by 1 (doubling the work required) to a maximum of 19. This is meant to roughly follow Moore's Law as it has traditionally progressed. See https://stackoverflow.com/questions/4443476/optimal-bcrypt-work-factor for an answer talking about this.
Note some arbitrary decisions have been made in this application, such as a max cost cap at 19. This is such that if there is a calculation error due to a system clock issue on the Node.js server, we don't end up going to the the bcrypt max and causing a strange DoS vector.
This passes the default mocha tests; I did not write tests for system clock changes, etc. Such tests are useful as
BCRYPT_CURRENT_DATE
is controllable from outside the scope of the application. To mitigate against issues, in a case in which an adversary can control the system clock and set it pre-1/1/17 to gain a floored value that is negative, the increase of 0 will be chosen, therefore the attack is limited to the bcrypt cost factorBCRYPT_WORK_FACTOR_BASE
only.Please do not merge this without testing/review. I am adding it to start a discussion on cost factor changes and scalability within the framework.