[PS-2216] Feature/scrypt kdf

[quexten]

Type of change

- [ ] Bug fix
- [x] New feature development
- [ ] Tech debt (refactoring, code cleanup, dependency upgrades, etc)
- [ ] Build/deploy pipeline (DevOps)
- [ ] Other

Objective

This pull request implements the scrypt kdf as another choice for the vault's pbkdf. The idea with scrypt is that through a large internal state, it is not possible to use a GPU to parallelize the computation, and thus is more resistant against bruteforce attacks.

Edit @djsmith85: Community forum thread: https://community.bitwarden.com/t/scrypt-kdf-support/48148/1

Code changes

There is also a pull request for the mobile implementation here: bitwarden/mobile/pull/2285

• messages.json, change-kdf-component.ts, change-kdf.component.html: Implement the UI for changing the kdf type. Since scrypt does not use iterations, but a work factor (which needs to be a power of 2), a drop-down menu made more sense for selection
• webCryptoFunction.service.spec.ts: Add unit tests for the scrypt function. The test strings were generated with GHCQ CyberChef
• kdfType.ts: Add the scrypt type to the enum, and the default work factor and work factor options (since other defaults seemed to also be located in this file)
• crypto.service.ts: Implement the case for the scrypt kdf type. The default scrypt work factor is 2^16, the minimum is 2^15. This and the other parameters are taken from this post: https://words.filippo.io/the-scrypt-parameters/
• node-crypto-function-service.ts, webCryptoFunction.service.ts, cryptoFunction.service.ts: Implement the function calls for scrypt
• package.json: Add the noble hashes library for the scrypt implementation, since the cryptographic libraries bitwarden was already dependent on, do not implement scrypt.

Screenshots

[bitwarden-bot]

Thank you for your contribution! We've added this to our internal Community PR board for review.
ID: PS-2216

[MGibson1]

The lib you're using also has a maximum work factor of 2^22

[quexten]

Ok, added a check/error for this. Also I had to increase the maximum amount of memory that scrypt can use, which is required for 2^21, 2^22. Should we somehow display an estimate to the user, of how much memory will be used since we can directly calculate this?

[kspearrin]

@quexten Thanks for your PRs on this. They look pretty good. A few high level thoughts, since I know you are working on an Argon2 solution as well:

1. We probably need to update the server APIs to accept more KDF options than just an iteration count int, especially with Argon2, which I believe will have more parameters that we need to expose to the user.
2. We should probably work toward standardizing on a KDF library that supports both scrypt and argon2, like libsodium bindings. For example, https://github.com/jedisct1/libsodium.js

[quexten]

Thanks for the feedback. In that case, should we use libsodium bindings for the mobile hashes as well? For scrypt, bouncycastle can be used, but argon2 is not implemented in bouncycastle.

Edit: OK i had a look at libsodium.js, and it seems scrypt is only exposed on a high level. The work factor N cannot be picked directly, but only an "opslimit" from which N is derrived. So instead of exposing N to the user, we could expose the opsLimit (and memoryLimit) and let libsodium pick the N.

Edit2: Libsodium has a low level api exposing the actual scrypt api, but I assumed n was not available since the typescript type definitions are incorrectly named.

Much worse though, there seems to be a bug in the compilation somewhere. When calling the scrypt module with WASM disabled, the results are incorrect. When WASM is enabled, the results are correct. So for now, libsodium is unusable for scrypt when WASM is disabled. I have filed an issue with libsodium.js for this: jedisct1/libsodium.js#310

Edit3: And as for argon2 in libsodium: The high level function exposed here only allows a fixed salt length of 16, so user emails would have to be hashed to 16 bytes to use this. Not sure this is the best solution, any thoughts?

[quexten]

@quexten Thanks for your PRs on this. They look pretty good. A few high level thoughts, since I know you are working on an Argon2 solution as well:

1. We probably need to update the server APIs to accept more KDF options than just an iteration count int, especially with Argon2, which I believe will have more parameters that we need to expose to the user.

2. We should probably work toward standardizing on a KDF library that supports both scrypt and argon2, like libsodium bindings. For example, https://github.com/jedisct1/libsodium.js

Created a pull request for bitwarden server implementing an API change for this.
bitwarden/server#2583

[kspearrin]

Thanks for the feedback. In that case, should we use libsodium bindings for the mobile hashes as well? For scrypt, bouncycastle can be used, but argon2 is not implemented in bouncycastle.

I think that would be good path to standardize on.

Edit: OK i had a look at libsodium.js, and it seems scrypt is only exposed on a high level. The work factor N cannot be picked directly, but only an "opslimit" from which N is derrived. So instead of exposing N to the user, we could expose the opsLimit (and memoryLimit) and let libsodium pick the N.

Edit2: Libsodium has a low level api exposing the actual scrypt api, but I assumed n was not available since the typescript type definitions are incorrectly named.

Much worse though, there seems to be a bug in the compilation somewhere. When calling the scrypt module with WASM disabled, the results are incorrect. When WASM is enabled, the results are correct. So for now, libsodium is unusable for scrypt when WASM is disabled. I have filed an issue with libsodium.js for this: jedisct1/libsodium.js#310

The more I think about this, I am not sure we need to support scrypt at all. Why would we recommend using scrypt if we are adding support for Argon2 as well? We would have PBKDF2 for FIPS compliance and Argon2id for non-FIPS deployments.

Edit3: And as for argon2 in libsodium: The high level function exposed here only allows a fixed salt length of 16, so user emails would have to be hashed to 16 bytes to use this. Not sure this is the best solution, any thoughts?

Hashing the email would work fine, though I am not sure the ideal hash algorithm for 16 bytes here. I can only think of MD5 or taking half of a SHA256 hash.

[kspearrin]

I am playing around with libsodium a bit and noticed that the only input parameters are memLimit (memory) and opsLimit (iterations). It doesn't appear that parallelism is even an available parameter. Is this your understanding as well @quexten ?

[quexten]

Thanks for the feedback. In that case, should we use libsodium bindings for the mobile hashes as well? For scrypt, bouncycastle can be used, but argon2 is not implemented in bouncycastle.

I think that would be good path to standardize on.

Edit: OK i had a look at libsodium.js, and it seems scrypt is only exposed on a high level. The work factor N cannot be picked directly, but only an "opslimit" from which N is derrived. So instead of exposing N to the user, we could expose the opsLimit (and memoryLimit) and let libsodium pick the N.
Edit2: Libsodium has a low level api exposing the actual scrypt api, but I assumed n was not available since the typescript type definitions are incorrectly named.
Much worse though, there seems to be a bug in the compilation somewhere. When calling the scrypt module with WASM disabled, the results are incorrect. When WASM is enabled, the results are correct. So for now, libsodium is unusable for scrypt when WASM is disabled. I have filed an issue with libsodium.js for this: jedisct1/libsodium.js#310

The more I think about this, I am not sure we need to support scrypt at all. Why would we recommend using scrypt if we are adding support for Argon2 as well? We would have PBKDF2 for FIPS compliance and Argon2id for non-FIPS deployments.

Edit3: And as for argon2 in libsodium: The high level function exposed here only allows a fixed salt length of 16, so user emails would have to be hashed to 16 bytes to use this. Not sure this is the best solution, any thoughts?

Hashing the email would work fine, though I am not sure the ideal hash algorithm for 16 bytes here. I can only think of MD5 or taking half of a SHA256 hash.

Yeah, I agree we might not need scrypt support. Honestly I worked on scrypt first as I thought adding argon2 would be more difficult, but I'm happy to forgo scrypt support if we have argon2.

I am playing around with libsodium a bit and noticed that the only input parameters are memLimit (memory) and opsLimit (iterations). It doesn't appear that parallelism is even an available parameter. Is this your understanding as well @quexten ?

Yes, there are some issues about this on the libsodium repo. They talked about adding parallelism, but the maintainer said they might add it if other hashing functions use it too. Might not be too much of an issue though. Also they claim that threads are not that significant of a setting anyways (w.r.t hashing speed): jedisct1/libsodium#986 .

By the way, we could just change the libsodium.js bindings to allow calls to raw argon2id, instead of opslimit/memlimit, so that we can directly set argon2's iterations and memorycost instead of having them derrived from "opslimit" and "memorylimit". That would also allow us to forgo hashing the email, as the salt can by any length.

Edit: Actually, the raw argon2 API supports parallelism, but in the implementation it is not parallel. So while it allows verifying hashes with parallelism from other implementations, it does not make much sense to use for generating hashes.

Alternatively we just use the high level pwhash api and let the user configure opslimit and memlimit, but then we should clearly document how these parameters relate to argon2 parameters.

[kspearrin]

@quexten

Looking at these libraries a bit more, I see that https://github.com/antelle/argon2-browser is just a wrapper around the argon2 upstream c library, which makes me feel a bit better about it.

Are all of these Argon2 libraries operating this way? If so, we might not need to standardize on the same one.

[quexten]

Hashing the email would work fine, though I am not sure the ideal hash algorithm for 16 bytes here. I can only think of MD5 or taking half of a SHA256 hash.

Libsodium actually has a high level API for hashing, which uses blakeb2 under the hood: https://libsodium.gitbook.io/doc/hashing/generic_hashing
It allows defining an output size. So we could just hash the email using this, with 16 bytes output size.

[quexten]

@quexten

Looking at these libraries a bit more, I see that https://github.com/antelle/argon2-browser is just a wrapper around the argon2 upstream c library, which makes me feel a bit better about it.

Are all of these Argon2 libraries operating this way? If so, we might not need to standardize on the same one.

As far as I know most js wrappers for argon2 operate in this way. The ones I used on the typescript clients definetly do. There is one (very slow) pure js implementation available, but all others that I saw just compile the C implementation and bind to it. The implementation for the mobile pull request does not, as it is a pure C# implementation. But I'm happy to switch to an implementation that just binds the upstream c library.

[kspearrin]

@quexten
Looking at these libraries a bit more, I see that https://github.com/antelle/argon2-browser is just a wrapper around the argon2 upstream c library, which makes me feel a bit better about it.
Are all of these Argon2 libraries operating this way? If so, we might not need to standardize on the same one.

As far as I know most js wrappers for argon2 operate in this way. There is one (very slow) pure js implementation available, but all others that I saw just compile the C implementation and bind to it. The implementation for the mobile pull request does not, as it is a pure C# implementation. But I'm happy to switch to an implementation that just binds the upstream c library.

Let's look for reputable, minimal implementations that just wrap the upstream c library.

Should we close these scrypt PRs and move discussion to the argon2 PR?

[quexten]

@quexten
Looking at these libraries a bit more, I see that https://github.com/antelle/argon2-browser is just a wrapper around the argon2 upstream c library, which makes me feel a bit better about it.
Are all of these Argon2 libraries operating this way? If so, we might not need to standardize on the same one.

As far as I know most js wrappers for argon2 operate in this way. There is one (very slow) pure js implementation available, but all others that I saw just compile the C implementation and bind to it. The implementation for the mobile pull request does not, as it is a pure C# implementation. But I'm happy to switch to an implementation that just binds the upstream c library.

Let's look for reputable, minimal implementations that just wrap the upstream c library.

Should we close these scrypt PRs and move discussion to the argon2 PR?

Alright.