paulgreg / UniquePasswordBuilder

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

Argon2 salt "looks dangerous" #16

Closed divVerent closed 5 years ago

divVerent commented 5 years ago

Preface: I do not intend to attack you and am actually likely to start using this; but one thing seems odd to me who has a cryptography and security background.

https://github.com/paulgreg/UniquePasswordBuilder/blob/67fc98394e6c7ce33bb4b3e87512e1b213bec632/src/passwordgeneration.js#L76

I know, the comment above it says it is randomly generated. But why is it there in the first place? What extra security does it bring beyond the master password? Can you prove that it was random?

Asking, as it simply looks bad. Like, if I wanted to make a trojaned password generator in C, I would probably hide some payload in a string like this and inject my bad behavior that way. How would it be involved? By a buffer overflow in the argon2 implementation of course. And of course I would claim that my exploit payload is just random data used as a salt...

Yes, I know exploits in JavaScript do not work this way. This approach works better in native apps. I do not know whether WebAssembly makes buffer overflow exploits possible though.

Or remember the Dual_EC_DRBG debacle? That was also caused by a "random looking" number... "what if there is a vulnerability in Argon2 triggered by this specific string in th salt, causing a backdoor?"

The common solution to this cryptographers use to cast away doubts like this when a parameter is needed but its value does not matter is using a "nothing up my sleeves" number. You know, something like 0x31415926535... (digits of pi) or similar.

Of course doing that would break existing generated passwords. But I would simply like to know:

Thank you!

divVerent commented 5 years ago

Actually, another option, maybe the nicest of all: add another hash type "Argon2 with alternate salt" that uses a nothing up my sleeves number instead here. I would suggest something like e.g. the first N digits of pi in base 64 (BTW: why not base 256? The salt needs not be printable), which will yield a string looking very much like your current one (could even have the same length), would very likely behave just as random as your current one even if flaws in Argon2 are discovered that affect not "random enough" salts, but everyone could verify how you generated it and that it is no obscure exploit.

I would BTW prefer pi over e simply because e is a bit too... simple. Of course other "similarly tricky" constants like gamma (Euler-M. constant), sin(1) come to mind as well and would of course be just as good. Just make sure the definition of the constant is not so complicated that you could have put something evil in there ;)

pmiossec commented 5 years ago

I think there is no problem with this string and I will explain you my point of view....

But why is it there in the first place?

This string is part of the salt used to generate the password

What extra security does it bring beyond the master password?

First, you should understand that the goal is not to generate a better password (argon2 algorithm is enough for that) but to protect your master password.

The only goal of the salt is to avoid attack by rainbow tables to prevent someone to get your master password.

From this point of view, the only 2 caracteristics that a good salt should have, to invalidate a maximum of rainbow tables (precalculate hashes) entries, is:

  1. being rather long
  2. being rather not to common

In UniquePasswordBuilder, the salt is generated by the concatenation of:

So it is important to notice that this string is only a part of the string passed as a salt to the argon2 algorithm and that it changes at each password generated. So as such, it's difficult to see it as a possible trojan or backdoor (and I am enough confident in the argon2 to not have a flaw in just processing the salt).

And the main goal of this string is to fulfill the 2 caracteristics of a good salt even in the case where the user decide to not fill the "user salt" part. So this string has been choosen to not be too short and with a good enough entropy (that's why I decided to generate is randomly). But, you should understand that, even if the entropy is not perfect, that's not a problem. The string should just be quite good to not be able to use rainbow tables.

The common solution to this cryptographers use to cast away doubts like this when a parameter is needed but its value does not matter is using a "nothing up my sleeves" number. You know, something like 0x31415926535... (digits of pi) or similar.

That could have been done but that would have made a less good salt in terms of the 2nd caracteristic (having a not too common salt). Perhaps someone has generated rainbow tables with Pi as salt. Generate a random string was simpler and more efficient!

We thought we did a good thing in introducing some random and I think it's doing a better job than what you propose.... That was not absolutely needed but better...

Of course doing that would break existing generated passwords.

Indeed, that's why I'm not fond of changing it... If you want to specify your salt, just fill the "user salt" field. Once again, this string is here to assure that minimum requirements of a good salt is filled.

Can this value be freely changed to whatever I like when I host my own instance?

yes, if you are deploying your own version, you could change the string value and put what you want... but once again, just specifying your user salt will have the same effect IMHO....

If it can, would it maybe be nice to have an option in the bookmarklet/app/settings to change this salt to another value (basically a second half of a master password)?

I don't see a great added value to that (the actual generated salt doing a good job), so I won't do it because that's a lot of work. But every conribution is encouraged and could be reviewed and if good, merged! 👍

If not, can you document explicitly in the source that changing this value is fine as long as one accepts breaking generated passwords? That probably would also cast away some doubt there. And maybe explicitly recommend changing this when making your own deployment, to use it as "per organization salt"...

yes, a little refactoring could easily be done with the string being put on top of the file to make it easily findable....

I hope it will cast away your doubts 😉

divVerent commented 5 years ago

I think there is no problem with this string and I will explain you my point of view....

But why is it there in the first place?

This string is part of the salt used to generate the password

What extra security does it bring beyond the master password?

First, you should understand that the goal is not to generate a better password (argon2 algorithm is enough for that) but to protect your master password.

The only goal of the salt is to avoid attack by rainbow tables to prevent someone to get your master password.

From this point of view, the only 2 caracteristics that a good salt should have, to invalidate a maximum of rainbow tables (precalculate hashes) entries, is:

  1. being rather long
  2. being rather not to common

In fact, it's supposed to be unique (or mostly unique - even a "stupid" per-user salt like "name and birthdate" will do) but is allowed to be public.

In UniquePasswordBuilder, the salt is generated by the concatenation of:

  • the website url for which we want to generate a password
  • a user salt that the user could specify
  • this "strange" string

So it is important to notice that this string is only a part of the string passed as a salt to the argon2 algorithm and that it changes at each password generated.

Until here I'm all with you.

So as such, it's difficult to see it as a possible trojan or backdoor (and I am enough confident in the argon2 to not have a flaw in just processing the salt).

Well... fans of the Underhanded C Code Contest could probably think of a few ways to hide some backdoor in the Argon2 implementation that can only be triggered by this "pattern".

And of course, conspiracy theorists may think that maybe only you know a weakness in Argon2 that this string exploits.

Looking at Argon2's definition, the only thing this salt is used for is for passing to Blake2b to generate an IV. So to exploit anything with this, one would probably actually need a weakness in the Blake2b implementation or in Blake2b itself - I don't think anything Argon-specific can be attacked with that, other than that maybe the concatenation part to feed Blake2b might be broken but that'd be pretty obvious.

And the main goal of this string is to fulfill the 2 caracteristics of a good salt even in the case where the user decide to not fill the "user salt" part. So this string has been choosen to not be too short and with a good enough entropy (that's why I decided to generate is randomly). But, you should understand that, even if the entropy is not perfect, that's not a problem. The string should just be quite good to not be able to use rainbow tables.

Of course.

The common solution to this cryptographers use to cast away doubts like this when a parameter is needed but its value does not matter is using a "nothing up my sleeves" number. You know, something like 0x31415926535... (digits of pi) or similar.

That could have been done but that would have made a less good salt in terms of the 2nd caracteristic (having a not too common salt). Perhaps someone has generated rainbow tables with Pi as salt. Generate a random string was simpler and more efficient!

True - however as said, doing so does cast some doubts.

We thought we did a good thing in introducing some random and I think it's doing a better job than what you propose.... That was not absolutely needed but better...

One other way cryptographers use for this BTW is something like here:

SHA-256 constants: https://tools.ietf.org/html/rfc4634#section-5 SHA-3 constants: https://crypto.stackexchange.com/questions/6444/why-are-the-constants-so-simple-in-keccak Blake2 uses a square root of primes construction: https://tools.ietf.org/html/rfc7693

Similarly, it wouldn't be too bad to e.g. take the date of an interesting event (even if it's just interesting to you), and then e.g. SHA256 it 42 times. The general idea is to make sure it doesn't have "too much" entropy so it can't have been specifically constructed to cause a flaw one wouldn't otherwise "randomly" encounter.

A fun choice for your specific software could have been using the scrypt mode of your own https://paulgreg.me/UniquePasswordBuilder/ at default settings with fixed "URL" UniquePasswordBuilder and master password UniquePasswordBuilder0, UniquePasswordBuilder1, ... until you have enough characters, yielding something like zE3B,zyUgZb(#FKthZ!r3X?8X?-36Sdp4__Zz:$9[G8;ao63=Eg0udqUR2oA&qqQU&Ee_r7mU(2t#V9;. Nobody's gonna have rainbow tables for that, and everyone could verify that it's truly arbitrary and that no way you could have done something evil in that construction.

Because that's the problem with randomness: you can never be sure. You can't prove to me that the string is really random and thus "harmless". Well, maybe if you had recorded the initial state of your browser's Math.random PRNG before running the password generator, that could be used to reproduce its construction; but given Chrome's PRNG has a 128 bit state, that could theoretically still have allowed for quite some evil (although not very much, so it'd probably be trustworthy enough).

Can this value be freely changed to whatever I like when I host my own instance?

yes, if you are deploying your own version, you could change the string value and put what you want... but once again, just specifying your user salt will have the same effect IMHO....

Can you simply just say in a comment above that line that - that the salt string in there can be safely freely changed if one wants to intentionally create an "incompatible" version? That alone would probably be already quite good at casting away doubts, as it'd make clear that you don't specifically insist on this string.

paulgreg commented 5 years ago

Here’s the PR explaining a little more that string : https://github.com/paulgreg/UniquePasswordBuilder/pull/19 What do you think ?

divVerent commented 5 years ago

Yeah, looks good. Thanks!