technion / ruby-argon2

A Ruby gem offering bindings for Argon2 password hashing
MIT License
229 stars 30 forks source link

:salt_do_not_supply option renamed #54

Closed jeremyevans closed 2 years ago

jeremyevans commented 2 years ago

This was apparently deliberate in 0ef1e33b7d9ec5d8a1fc59b0a092e236d59109ca, but I think you should reconsider as there are valid use cases.

Rodauth (https://github.com/jeremyevans/rodauth) uses this option because it separates the salt from the password hash to prevent attackers from being able to brute force password hashes (attackers cannot brute force passwords if they only have the salt and not the hash). By renaming the option, you broke Rodauth's use of argon2, and if Rodauth doesn't work around this issue by detecting the argon2 version and using the new option name, it would not be able to use argon2 as securely as it uses bcrypt. Please recognize there are valid reasons for providing a salt when creating an argon2 password, and provide a supported way to do so that will not change in minor versions of argon2.

joshbuker commented 2 years ago

@jeremyevans I could be off-base, but it sounds like you may be confusing the purpose of the salt and the pepper.

The salt, from what I understand, is intended purely to protect against rainbow table attacks. There is no issue with an attacker having access to both the hash and the salt, as the salt has already done its job in protecting against pre-computation attacks.

Having a secret stored separately from the hash is the duty of the pepper, which this library does support natively within the public API. What you've described using the salt for within Rodauth, should instead be the pepper which is kept entirely separate from the database storing password hashes/salts.

This is why the salt is plaintext (b64 encoded) and included in the final digest Argon2 produces; it's not actually important for it to be kept separate from the hash for it to fulfill its purpose.

(Again, grain of salt. I could totally be wrong on this. This is just the understanding I've built up while working with Sorcery)

jeremyevans commented 2 years ago

@athix You are off base :). With a password hash, an attacker can try to bruteforce the password. Without a password hash, an attacker cannot. With Rodauth's design, because an attacker doesn't have access to the hash itself, the only way they can bruteforce a password is to issue a database query for each check (candidate password hashes are checked using a security definer database function that doesn't provide an attacker access to the actual password hash), which is going to be much slower than attempting to bruteforce the password on an FPGA/ASIC.

Let's say you are an attacker and you have access to a password hash. It doesn't matter if the password is peppered or not, you can still try to bruteforce it. Additionally, if an attacker has compromised your environment to such an extent that they have access to password hashes, they likely have access to the pepper as well, in which case the cost of bruteforcing the hash is the same as having no pepper at all.

FWIW, Rodauth has support for password peppering as well. Peppering can be useful when you cannot use Rodauth's more secure default approach of checking passwords using a database function.

technion commented 2 years ago

Breaking changes are not on. I'm reverting this immediately. It's not like something named salt_do_not_supply was confusing to users.

joshbuker commented 2 years ago

@jeremyevans

Let's say you are an attacker and you have access to a password hash. It doesn't matter if the password is peppered or not, you can still try to bruteforce it.

From what I've researched so far, this is exactly the purpose of the pepper, per Argon2 README:

The secret parameter, which is used for keyed hashing. This allows a secret key to be input at hashing time (from some external location) and be folded into the value of the hash. This means that even if your salts and hashes are compromised, an attacker cannot brute-force to find the password without the key.

That said, this library is @technion rodeo, and it looks like this is being reverted either way. Problem solved :tada: