use pbkdf2 at instantiation

[garbados]

Much like #3 this PR uses pbkdf2 to derive a cryptographic key from a password. Unlike that PR, this is only done once when Crypt is initialized. As a result, there is not a significant performance difference between this version and the master branch, which does not use pbkdf2.

[coveralls]

Pull Request Test Coverage Report for Build 1080153385

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

• For more information on this, see Tracking coverage changes with pull request builds.
• To avoid this issue with future PRs, see these Recommended CI Configurations.
• For a quick fix, rebase this PR at GitHub. Your next report should be accurate.

Details

• 36 of 36 (100.0%) changed or added relevant lines in 1 file are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage increased (+3.8%) to 100.0%

---

Totals
Change from base Build 818894132:	3.8%
Covered Lines:	57
Relevant Lines:	57

---

💛 - Coveralls

[jcoglan]

I would strongly prefer this approach compared to #3. Deriving a new key for every encrypt() call is not necessary and as you say it just makes things unacceptably slow. The key thing is to make the encryption key unguessable to an attacker that has access to the encrypted data, and deriving the key once per Crypt instance is sufficient for that. You're already using a different nonce for each encrypt() call to make sure each input will encrypt to a different output; using a different key for each input is not required.

I did have a few questions about the design. First, salt is an optional constructor param and if omitted you generate a random salt. Since the salt is not included in the ciphertext, or stored anywhere else, it will only be possible to decrypt data using the same Crypt instance you used to encrypt it. This might be useful, but not especially so for data in persistent storage, where you want to decrypt data many times and across many processes.

Putting the salt into the ciphertext isn't a good solution either because that then requires you re-derive the key for every decrypt() operation, equivalent to #3. We should either make salt a mandatory parameter, or default it to some stable value, e.g. a hard-coded GUID. I don't think it's critical that the salt is secret, as long as the password remains secret. Indeed, storing the salt either in the ciphertexts or in a config record somewhere would expose it to the attacker, and as far as I'm aware, having the salt would not make it easier for the attacker to retrieve the actual encryption key.

Some other little questions about the implementation: this change in the use of slice() -- will this result in the first KEY_LENGTH bytes being dropped from the buffer, rather than kept?

-    this._key = hash(decodeUTF8(password)).slice(0, secretbox.keyLength)
+    this._pass = hash(decodeUTF8(password)).slice(KEY_LENGTH)

You set SALT_LENGTH = secretbox.nonceLength, but the PBKDF2 salt becomes the input data for a HMAC operation and its size is unrelated to the nonce size for XSalsa20 (192 bits / 24 bytes). Per my above comments, the random salt should maybe be removed, but I still curious about this choice rather than picking some size appropriate to the hashing function, e.g. 256 bits.

Finally, the number of iterations should probably be configurable through the constructor. 1000 isn't a particularly large value but is probably reasonable for running in the browser.

[jcoglan]

In #3 you were also concerned about the bundle size resulting from importing pbkdf2. I agree pulling in 300kb of additional code is big downside, I was wondering whether using WebCrypto is a viable solution to this, or if it's still not widely available enough (IE/Edge and Node.js < 16 don't have it). It does have PBKDF2-HMAC-SHA512 in the API, but if you want a 256-bit key for XSalsa20-Poly1305 you have to lie and say you want the key for some other cipher that WebCrypto knows and has 256-bit keys, e.g. AES-CBC:

  let key = await crypto.subtle.deriveKey(
    { name: 'PBKDF2', hash: 'SHA-512', salt, iterations },
    await crypto.subtle.importKey('raw', password, 'PBKDF2', false, ['deriveKey']),
    { name: 'AES-CBC', length: 256 },
    true,
    ['encrypt']
  );

  // returns an ArrayBuffer
  let extracted = await crypto.subtle.exportKey('raw', key);

Maybe we could use this in the browser, use crypto.pbkdf2() in Node, and provide a shim for runtimes without WebCrypto? Or figure out a way to import just the things we need, rather than all the stuff the pbkdf2 package is pulling in. In vault-cipher I use AES-CBC, HMAC-SHA{1,256}, and PBKDF2 and it bundles to ~70kb, so it must be possible to get the size down.

[garbados]

I would strongly prefer this approach compared to #3. Deriving a new key for every encrypt() call is not necessary and as you say it just makes things unacceptably slow. The key thing is to make the encryption key unguessable to an attacker that has access to the encrypted data, and deriving the key once per Crypt instance is sufficient for that. You're already using a different nonce for each encrypt() call to make sure each input will encrypt to a different output; using a different key for each input is not required.

Great, glad to get this level of logical review.

I did have a few questions about the design. First, salt is an optional constructor param and if omitted you generate a random salt. Since the salt is not included in the ciphertext, or stored anywhere else, it will only be possible to decrypt data using the same Crypt instance you used to encrypt it. This might be useful, but not especially so for data in persistent storage, where you want to decrypt data many times and across many processes.

Naturally. I'd like to expose an .export() and .import() method that allow a Crypt instant to expose and ingest a string representation of its internal cryptographic values -- that is, the password and salt, along with other options -- while also encrypting this value for transport. Thus app developers will be able to provide users with a way of safely and simply transporting credentials across devices without having to remember a salt.

Some other little questions about the implementation: this change in the use of slice() -- will this result in the first KEY_LENGTH bytes being dropped from the buffer, rather than kept?

Yes, this is a mistake. Thanks for spotting it.

You set SALT_LENGTH = secretbox.nonceLength, but the PBKDF2 salt becomes the input data for a HMAC operation and its size is unrelated to the nonce size for XSalsa20 (192 bits / 24 bytes). Per my above comments, the random salt should maybe be removed, but I still curious about this choice rather than picking some size appropriate to the hashing function, e.g. 256 bits.

It seemed like a sensible default but you're right that it's confusing to use such a value.

Finally, the number of iterations should probably be configurable through the constructor. 1000 isn't a particularly large value but is probably reasonable for running in the browser.

I agree. I'll update the constructor with an opts parameter so these things can be set.

[garbados]

Maybe we could use this in the browser, use crypto.pbkdf2() in Node, and provide a shim for runtimes without WebCrypto?

I tried this in #1 (comment) but bundling gets really complicated for the user so I'm keen to avoid require('crypto') as much as possible.

I was just clued into https://github.com/Daninet/hash-wasm which seems exactly like what we want for this.

[garbados]

I've added a configurable options parameter to the constructor and deriveKey function, as well as import and export methods for transporting credentials. The README has been accordingly updated. Also the password-slicing bug has been fixed by, uh, not slicing the hash at all. The new lib we use for pbkdf2 does the slicing and dicing internally.

Speaking of the new lib, the bundled size of this version is 59.5 KB! Not bad for crypto, IMO.

[jcoglan]

This looks really good -- that bundle size is much more in line with what I'd expect for something like this and avoids us having to deal with cross-platform stuff ourselves if tweetnacl and hash-wasm deal with it for us.

I've also tried out hash-wasm myself and verified that it outputs the same PBKDF2 results as other crypto libraries, which is important for not breaking people's passwords should you ever decide to replace your PBKDF2 implementation.

[jcoglan]

I particularly like the opaque portable string method of transporting the internal state; it means users can get the default choices and not have to worry about internal configuration beyond setting a password. Such a value could easily be stored in a local doc in crypto-pouch, for example.

[garbados]

Great! I'm going to go ahead and merge this then and cut a new release :)

[coveralls]

Pull Request Test Coverage Report for Build 1080194719

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

• For more information on this, see Tracking coverage changes with pull request builds.
• To avoid this issue with future PRs, see these Recommended CI Configurations.
• For a quick fix, rebase this PR at GitHub. Your next report should be accurate.

Details

• 36 of 36 (100.0%) changed or added relevant lines in 1 file are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage increased (+3.8%) to 100.0%

---

Totals
Change from base Build 818894132:	3.8%
Covered Lines:	57
Relevant Lines:	57

---

💛 - Coveralls

[coveralls]

Pull Request Test Coverage Report for Build 1080153385

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

• For more information on this, see Tracking coverage changes with pull request builds.
• To avoid this issue with future PRs, see these Recommended CI Configurations.
• For a quick fix, rebase this PR at GitHub. Your next report should be accurate.

Details

• 36 of 36 (100.0%) changed or added relevant lines in 1 file are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage increased (+3.8%) to 100.0%

---

Totals
Change from base Build 818894132:	3.8%
Covered Lines:	57
Relevant Lines:	57

---

💛 - Coveralls