bryant / argon2rs

The pure-Rust password hashing library running on Argon2.
MIT License
174 stars 19 forks source link

Generate a random, universally unique salt automatically #36

Open shssoichiro opened 6 years ago

shssoichiro commented 6 years ago

To be secure, a salt is required to be universally unique across all passwords everywhere. It is very easy for a developer to do this incorrectly, which is why password hashing libraries typically generate a salt internally to take away the risk of the caller doing it incorrectly. This library should follow this pattern and generate a universally unique salt for each password internally, rather than asking the caller to provide one.

burdges commented 6 years ago

There are no mechanisms for adding a salt specified in the argon2 specs, so maybe another crate should provide that functionality. And another crate would simplify users auditing their own code.

In fact, you could do this generically across some hash function interface, not unlike what the RustCrypto project does. It might look nicer if one waits for const generics though.

bryant commented 6 years ago

Inlined reply.

On Apr 28, 2018 10:17 PM, "Josh Holmer" notifications@github.com wrote:

To be secure, a salt is required to be universally unique across all passwords everywhere.

Could you explain this? Why is it insufficient for the salt generator to be difficult to predict? What steps are needed to ensure universal uniqueness?

It is very easy for a user to do this incorrectly, which is why password hashing libraries typically generate a salt internally to take away the risk of the user doing it incorrectly. This library should follow this pattern and generate a universally unique salt for each password internally, rather than asking the user to provide one.

Once we've decided that we really want this, a new API entry point could be allocated for it. Or we could adapt the *_simple calls to this. I don't see any reason to forgo the existing low-level functionality of Argon2::*.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub, or mute the thread.

burdges commented 6 years ago

There are applications for the low-level calls beyond simple password hashing, including PAKE schemes, PoW schemes, usage in low-entropy environments, etc., so they should definitely remain exposed. Also, salts might require std, unless the recent addition of jitter to rand avoids this.

bryant commented 6 years ago

Unless I've misunderstood, rand would not ensure universal uniqueness.

shssoichiro commented 6 years ago

The universal uniqueness rule comes from the idea that if a salt is shared between multiple passwords, an attacker can build tables to attack all of those passwords at once. In practice, the salt doesn't need to be universally unique (though it is preferable), but it should be difficult to predict and not shared between multiple passwords.

Allowing developers to generate their own salts is a footgun, because developers will commonly do things such as using the same salt for every password in their database, or basing the salt off of something easily guessable such as the username or an auto-incrementing user ID.

I agree with leaving the salt parameter for the low level calls, but the *_simple functions in particular should take only a password, to protect developers in the common use-case (e.g. "I want to hash a password to store in my database").

Also, this library already depends on std modules, so I don't think that is a concern in this case.

burdges commented 6 years ago

I think salts serve two purposes, first to reduce the entropy users must supply in their passwords, ala protection from tables, and second to prevent revealing the same password was used in multiple places. Associated data helps against the first, but not necessarily against the second. And "serious" deployments should record a "version" that encapsulates their particular parameter choices anyways, so they should store their salt that way probably.

You need two separate functions when managing the salt, so reusing the _simple postfix does not work so well. Analogs look more like:

struct Salted(pub [u8; 32]);  // 16 byte salt plus 16 byte result, not sure what these should be really
pub fn argon2i_salted(password: &str) -> Salted { .. }
pub fn argon2i_verify_salted(password: &str, salted: &Salted) -> bool { .. }