paulgreg / UniquePasswordBuilder

A bookmarklet to generate strong password per site
https://paulgreg.me/UniquePasswordBuilder/
MIT License
8 stars 4 forks source link

UPB uses argon2i | hex | argon2d rather than argon2id #21

Open divVerent opened 5 years ago

divVerent commented 5 years ago

For some reason, UPB runs Argon2i on the password and salt, and then Argon2d on the hex-encoded result. This isn't the same as the usually recommended Argon2id.

This is only a minor flaw, as n-rounds Argon2i + n-iterations Argon2d should be at least as good as n+1-iterations Argon2id, and the added binary to hex encoding shouldn't do any harm.

However, it takes about twice as long for not substantially increased GPU/ASIC resistance.

I'd suggest that a future version should probably support Argon2id directly, increasing performance by factor 2 (or, allowing twice as much safety against GPU/ASIC hashing at the same execution time).

BTW, I have created a direct port of the JS code to a shell script to verify that the UPB code generates correct results:

https://gist.github.com/divVerent/e5301731864f61af9cccb3e772d235a9

It yields the same results as UPB, thereby verifying that it doesn't have any accidental bugs in e.g. the JS library implementations or their use.

paulgreg commented 5 years ago

Thanks for that analysis. You’re right, it would be better to do it on one pass instead of 2.

divVerent commented 5 years ago

Yeah... and now I'm even having doubts myself.

Your approach has a higher resistance against side channel attacks (see Spectre, Meltdown etc.), as it perturbs the data a lot more with Argon2i before running the data-dependent Argon2d.

IETF recommendation is still Argon2id, but still, I can't really say anymore that what UPB does is wrong or worse, but it is certainly nonstandard.

Maybe explain rationale behind this in a comment?

paulgreg commented 5 years ago

I think it’s better to comply to standard and not do fancy stuff by ourselve (history has shown that dev « inventing » crypto is never a good approach). I may update UPB some day, to respect standard (but it should be compatible with current way).

divVerent commented 5 years ago

On Wed, Jan 16, 2019, 02:04 Grégory Paul <notifications@github.com wrote:

I think it’s better to comply to standard and not do fancy stuff by ourselve (history has shown that dev « inventing » crypto is never a good approach). I may update UPB some day, to respect standard (but it should be compatible with current way).

Yes, probably the proper solution here is to keep the old mode and, like in a few months or quarters, add a new mode that is more standard. Probably together with other fixes.

You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/paulgreg/UniquePasswordBuilder/issues/21#issuecomment-454674097, or mute the thread https://github.com/notifications/unsubscribe-auth/AAPWsERmgne27nk3e86bNVWaqq33bv59ks5vDs7tgaJpZM4Z2VCj .

pmiossec commented 5 years ago

For some reason, UPB runs Argon2i on the password and salt, and then Argon2d on the hex-encoded result. This isn't the same as the usually recommended Argon2id.

and

I'd suggest that a future version should probably support Argon2id directly

It's what I wanted at the beginning but the only reason it is not what have been made is because the npm library doesn't support argon2id :cry:

That's why I opt for 2i and after 2d when I read :

Argon2id is a hybrid version. It follows the Argon2i approach for the first pass over memory and the Argon2d approach for subsequent passes

So I think that what have been done should be OK....

Ps: sorry for the delay, I just saw the issue, now...