Salts are used to combat Rainbow Table attacks, where an attacker pre-computes a table of inputs in a specific hash function, and stores the results for reuse. This way the attacker only needs access to large amounts of resources once, which he then reuse across multiple attack sessions.
Salts are supposed to have enough entropy to force an attacker to generate an infeasible amount of tables. E.g. he would need to create a table for each permutation (salt) of the hash function like this:
Salt = a, table 1: sha1("password" + "a")
Salt = b, table 1: sha1("password" + "b")
Now, only using a single byte of salt is not wise as the attacker would only need to generate 255 tables, which is feasible today using GPU generated pre-computed Rainbow Tables. The usual recommendation is to use at least 32 bits (4 bytes) to make it infeasible.
The salt in RndPhrase is calculated as self.seed + '$' + self.uri, where seed is user supplied and has no restriction on length or complexity. 'uri' is also supplied by the user and also has no requirements, but is supposed to represent the URL, for which the user is generating a password.
The issue arise in the edge case where both the seed and uri is set to nothing or less than what can be considered secure. Due to the static '$' in the salt, the minimum length in the case where 'seed' and 'uri' is not set is 1, which is not good enough.
Suggested fix
I'd be better to enforce a strict minimum on seed and uri to make sure this edge case can't happen for whatever reason. However, another solution is to use a so called 'application salt' (or pepper in popular terminology), which sets the lower limit on salts to a pseudo random string, which then also guarantees a minimum amount of entropy.
Note that this code does not work as we append a string with a byte array. It is only to illustrate how it is supposed to work. Note that the combination of factors in the seed should actually be done on a byte array basis to not be vulnerable to type confusion. I recommend you convert 'seed' and 'uri' to uint8Array (using proper encoding, see #25) and concatenate them together instead.
rlindsgaard
commented
7 years ago
Salts are used to combat Rainbow Table attacks, where an attacker pre-computes a table of inputs in a specific hash function, and stores the results for reuse. This way the attacker only needs access to large amounts of resources once, which he then reuse across multiple attack sessions.
Salts are supposed to have enough entropy to force an attacker to generate an infeasible amount of tables. E.g. he would need to create a table for each permutation (salt) of the hash function like this:
Salt = a, table 1: sha1("password" + "a") Salt = b, table 1: sha1("password" + "b")
Now, only using a single byte of salt is not wise as the attacker would only need to generate 255 tables, which is feasible today using GPU generated pre-computed Rainbow Tables. The usual recommendation is to use at least 32 bits (4 bytes) to make it infeasible.
The salt in RndPhrase is calculated as
self.seed + '$' + self.uri, where seed is user supplied and has no restriction on length or complexity. 'uri' is also supplied by the user and also has no requirements, but is supposed to represent the URL, for which the user is generating a password.The issue arise in the edge case where both the seed and uri is set to nothing or less than what can be considered secure. Due to the static '$' in the salt, the minimum length in the case where 'seed' and 'uri' is not set is 1, which is not good enough.
Suggested fix
I'd be better to enforce a strict minimum on seed and uri to make sure this edge case can't happen for whatever reason. However, another solution is to use a so called 'application salt' (or pepper in popular terminology), which sets the lower limit on salts to a pseudo random string, which then also guarantees a minimum amount of entropy.
It is as simple as doing this:
Note that this code does not work as we append a string with a byte array. It is only to illustrate how it is supposed to work. Note that the combination of factors in the seed should actually be done on a byte array basis to not be vulnerable to type confusion. I recommend you convert 'seed' and 'uri' to uint8Array (using proper encoding, see #25) and concatenate them together instead.