rharel / webext-private-bookmarks

WebExtension that enables a special password-protected bookmark folder.
https://addons.mozilla.org/en-US/firefox/addon/webext-private-bookmarks/
Other
79 stars 15 forks source link

[Security Issue] Poor key derivation, switch to a KDF with salt #147

Open ghost opened 4 years ago

ghost commented 4 years ago

Hello,

Thank you for making this extension, it is very neat and useful.

I was looking at the code in sources/scripts/utilities/cryptography.js and it appears that your key derivation is not sufficient to prevent brute forcing in real-world use

When we use symmetric cryptography, we need a key. For AES-256-GCM we need a 256bit key. When we want to protect something using a password, this presents a challenge: basically no users will use a password/passphrase that is at least 256bits. In reality, most people will use mediocre at best passwords. We're lucky if they even made a unique password for this application.

You already know at least part of this as you have the digest() function using sha256 to get 256 bits from the password. You are in the right direction, but sha256 and similar primitives are not considered suitable for key derivation or password hashing. This is because sha256 is designed to be very fast, which means attackers can guess billions of passwords a second. We don't want attackers to guess fast. Powerful cracking rigs or ASICs can perform hundreds of billion hashes a second.

For the purpose of deriving a key, we have algorithms known as Key Derivation Functions. In 2020, Argon2id is regarded as the ideal choice with scrypt and PBKDF2 also being acceptable. (Assuming each is used with appropriate parameters)

Additionally, you do not use a salt which results in rainbow table attacks being feasible.

Under the current implementation I could precompute a massive table of all known leaked passwords or even the entire key space for a short-ish password and break the key using that.

The salt will likely be the hardest for you to implement, since it has to be stored in plaintext somewhere (but unique per user or instance).

To simplify this stuff for you, I suggest using a JavaScript libsodium binding/implementation or other high level library. subtlecrypto is too low level for most developers, but you can see the MDN docs for deriving a symmetric key https://developer.mozilla.org/en-US/docs/Web/API/SubtleCrypto/deriveKey

https://stackoverflow.com/a/38684378

Note: I am not a cryptographer, but I am a security engineer and have written code using cryptography before

rharel commented 2 years ago

Thanks for the notice, as you can tell by the delay of this reply I don't have a lot of time to work on this project anymore, only sporadically. I added salts to the latest push. I did not have time to look at key derivation though. Pull requests are welcome.