ergochat / ergo

A modern IRC server (daemon/ircd) written in Go.
https://ergo.chat/
MIT License
2.27k stars 180 forks source link

Increase bcrypt cost #2007

Closed yonas closed 1 year ago

yonas commented 1 year ago

MinCost is 4, which is very low. Even a cost of 10 is no longer recommended. Let's use 12.

bcrypt.DefaultCost is currently set to 12, so we can use as-is.

yonas commented 1 year ago

ffd3746 actually used bcrypt's default cost, not Ergo's. Bcrypt's default is currently 10, which is too low.

9d56e65 should fix this so the default cost is 12.

slingamn commented 1 year ago

See #1497 for context on the current value. Unlike a website, we don't have authentication cookies or bearer tokens that are cheap to check. One of our core objectives now is to reduce the latency of mobile reattach as much as possible, and increasing the bcrypt cost cuts into that directly.

slingamn commented 1 year ago

Oh, my bad --- your change only affects operator passwords, not user passwords.

Your change will not work as written because operator passwords are actually pure bcrypt, not the hybrid algorithm (#212) that is implemented in the passwd package and used for user passwords. (This is for backwards compatibility reasons: operator passwords are written into the config file, so we can't change algorithms and regenerate the stored hashes on the fly.)

I would potentially consider raising the cost parameter for operator passwords. However, I've always thought of this as a secondary issue because operators who are concerned about this threat model (ircd.yaml leaks and someone tries to crack the passwords) can just use stronger passwords. For example, my operator password hash on the testnet is public:

https://github.com/ergochat/testnet.ergo.chat/blob/36332ec2e07797f3c95d51a813e2d43ddcccbfcb/ircd.yaml#L591

but I'm not worried about it because the plaintext has 128 bits of entropy.

yonas commented 1 year ago

However, I've always thought of this as a secondary issue because operators who are concerned about this threat model (ircd.yaml leaks and someone tries to crack the passwords) can just use stronger passwords.

Well, if we're going to use bcrypt, claiming it's strong, but secretly knowing that the way it's being used makes it weak - giving a false sense of additional security - we might as well remove it. That would really motivate users to make sure their password has high entropy, such as 128 - 256 bits of entropy.

I would potentially consider raising the cost parameter for operator passwords.

Raising it to 12 would be a bare minimum. 15 might be more appropriate.

Making the cost configurable in the config file would be another option. If the hash includes the cost factor (I think Argon2's hash does), then users could change the cost and we could detect and auto-upgrade the hash.

slingamn commented 1 year ago

Well, if we're going to use bcrypt, claiming it's strong, but secretly knowing that the way it's being used makes it weak - giving a false sense of additional security - we might as well remove it.

Why remove it? Any cryptographically secure hashing function provides additional security here; as you saw above, my password hash for the testnet is public, but no one can recover the plaintext password.

slingamn commented 1 year ago

If the hash includes the cost factor (I think Argon2's hash does), then users could change the cost and we could detect and auto-upgrade the hash.

The bcrypt hash format does include the cost, however (as discussed above) we cannot auto-upgrade any hashes that are written into the config file.

yonas commented 1 year ago

... we cannot auto-upgrade any hashes that are written into the config file.

Why is this? I suppose you mean, if the user changes the hash in the config file and not just a config setting.

Let's give them a config setting and take on the responsibility of upgrading the hash.

slingamn commented 1 year ago

I'm not sure you understand the implications of this change. In particular, the bcrypt cost for user passwords (as opposed to operator passwords) is already configurable:

https://github.com/ergochat/ergo/blob/e20c983b5761adaf8bd1b4648131cd27ae4a4f51/default.yaml#L394

but if you asked me to raise the default/recommended value there, I would refuse, because of the mobile handshake issue described above.

So I'm open to raising the suggested cost parameter for operator passwords, or giving the operator more control (I think a command-line flag would be better than a config option, since the genpasswd subcommand doesn't currently depend on a config file), but what's the rationale? Is it just that some hardening is better than none? Is the config file more likely to leak than the database, because it might accidentally get versioned in git or similar?

If you are just upset about "false advertising" I am happy to delete the mention of bcrypt from the README also ;-)