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

proposal: x/crypto/bcrypt: DefaultCost should be increased to 12 #54573

Open
mayrstefan opened this issue Aug 21, 2022 · 11 comments
Open

proposal: x/crypto/bcrypt: DefaultCost should be increased to 12 #54573

mayrstefan opened this issue Aug 21, 2022 · 11 comments
Labels
Proposal Proposal-Crypto Proposal related to crypto packages or other security issues
Milestone

Comments

@mayrstefan
Copy link

The bcrypt package uses a DefaultCost of 10 which has not changed since its initial commit 10 years ago.
https://github.com/golang/crypto/blob/bc19a97f63c84bfb02ed9bb14fb0f8f6bec9a964/bcrypt/bcrypt.go#L22-L24

Processing power has increased and the minimum default should be increased to 12 as recommended in Best practices for password hashing and storage.

Many code examples on the internet already use a cost of 12. The library defaults should be equally secure. Because the cost factor is exponential this increase is 4x more work.

@gopherbot gopherbot added this to the Proposal milestone Aug 21, 2022
@panjf2000
Copy link
Member

I'm not sure about this, it would be nice to have some references to other languages like C++, java, etc.

@seankhliao seankhliao added the Proposal-Crypto Proposal related to crypto packages or other security issues label Aug 22, 2022
@seankhliao
Copy link
Member

cc @golang/security

@mayrstefan
Copy link
Author

Still using cost of 10:

Already using cost of 12:

OpenBSD - where bcrypt comes from - can detect an automatic cost factor by system performance https://man.openbsd.org/OpenBSD-7.1/crypt_newhash.3

I don't know exactly why many examples or libraries have agreed to moved from 10 to 12 and no other value. But they did.

Somewhere in between is the OWASP Password Storage Cheat Sheet. While it still recommends a cost of 10 for bcrypt it also tells us to increase the work factor when systems become more powerful. This seems to align with the OpenBSD idea of not making it a static number.
Using a cost of 10 for a decade now it seems reasonable to increase the default cost value for the golang implementation. We should at least stay "as secure" as other implementations. Although from a security/cost perspective the OpenBSD approach looks even more attractive.

@panjf2000
Copy link
Member

Still using cost of 10:

Already using cost of 12:

OpenBSD - where bcrypt comes from - can detect an automatic cost factor by system performance https://man.openbsd.org/OpenBSD-7.1/crypt_newhash.3

I don't know exactly why many examples or libraries have agreed to moved from 10 to 12 and no other value. But they did.

Somewhere in between is the OWASP Password Storage Cheat Sheet. While it still recommends a cost of 10 for bcrypt it also tells us to increase the work factor when systems become more powerful. This seems to align with the OpenBSD idea of not making it a static number. Using a cost of 10 for a decade now it seems reasonable to increase the default cost value for the golang implementation. We should at least stay "as secure" as other implementations. Although from a security/cost perspective the OpenBSD approach looks even more attractive.

Thank you for your efforts, the OpenBSD approach seems to be a preference, if there is a easy way for Go to implement it. But since bcrypt doesn't prevent users changing the cost, I'm neutral on this proposal.

Let's collect more opinions here.

@earthboundkid
Copy link
Contributor

I would like a function that benchmarks your server and spits out a recommendation.

@julieqiu julieqiu removed this from Go Security Sep 8, 2022
@panjf2000 panjf2000 added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Sep 24, 2022
@gopherbot gopherbot removed the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Sep 24, 2022
@ianlancetaylor ianlancetaylor moved this to Incoming in Proposals Oct 5, 2022
@rsc
Copy link
Contributor

rsc commented Oct 12, 2022

Is bcrypt still something people should be using at all?

@earthboundkid
Copy link
Contributor

I don't think we want to add a warning like MD5 or keep a recommendation for a different crypto module up to date, so to me it makes sense to just tweak the DefaultCost.

@lzap
Copy link

lzap commented Jan 11, 2023

For the record, I proposed a calibration function which will can be used to calculate the cost exactly for given hardware/environment. You specify how much time you want the system to spend on hashing and the function gives you the cost.

https://go-review.googlesource.com/c/crypto/+/318710

@earthboundkid
Copy link
Contributor

That's great! I've thought about writing that before myself. But can it be changed to find a value that takes at least time.Duration instead of at most time.Duration?

@lzap
Copy link

lzap commented Jan 12, 2023

I actually closed that patchset, feel free to take it from here. I am bit busy these times, it totally needs more love. The constant string also smells, should have been a function parameter too.

In the review, I was told that adding a new function is an API change and needs a proper proposal, this is why I am dropping this. Just so you know, it would not get accepted just like that.

@rasky
Copy link
Member

rasky commented May 17, 2024

@golang/proposal-review seems like this proposal has stalled; can we get an update on this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Proposal Proposal-Crypto Proposal related to crypto packages or other security issues
Projects
Status: Incoming
Development

No branches or pull requests

8 participants