golang / go

The Go programming language
https://go.dev
BSD 3-Clause "New" or "Revised" License
123.18k stars 17.57k forks source link

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

Open mayrstefan opened 2 years ago

mayrstefan commented 2 years ago

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.

panjf2000 commented 2 years ago

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

seankhliao commented 2 years ago

cc @golang/security

mayrstefan commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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

rsc commented 1 year ago

Is bcrypt still something people should be using at all?

earthboundkid commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 4 months ago

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