search

Consensys / eth-lightwallet

Lightweight JS Wallet for Node and the browser
MIT License
1.46k stars 501 forks source link

removes salt from serialize #184

Closed Moejoe90 closed 6 years ago

Moejoe90 commented 6 years ago

This makes the serialized vault more secure, from my point of view it makes no sense to store the salt in the wallets instance. as anyone with accesses to this has accesses to salt.

Additionally, if you provide salt manually this can open the door for more interesting scenarios for unlocking the vault, for example, 2fa or 2-way response.

Is there a reason why salt is part of the instance from what I see it should be stored separately or at least given an option to store in a more secure sense.

coder5876 commented 6 years ago

@moejoe90 This is not a good idea IMO, the reason for the salt is only to avoid rainbow attacks: namely that an attacker precomputes derived keys from common passwords, then is able to grab lots of encrypted wallets from malware infested computers and uses the precomputed derived keys to quickly test a lot of passwords.

It should always be possible to use only the password to decrypt a serialized keystore and that would not be possible with this suggested change.

Moejoe90 commented 6 years ago

I can understand your choice against rainbow attack, that is one way of looking into this. I still think storing salt in the instance is not the best option. in order to open the possibility of some 2fa without resorting to splitting the password and hashing it against some other 2fa text.

I will close this PR in that case. Thanks for the reply.