Argon2 KDF Support

Quexten · January 24, 2023, 5:34pm

PR = Pull request, I.e someone writes code and suggests to (in this case) Bitwarden: “Hey, there is this code I wrote, would you like to include (pull) it?”

trparky · January 24, 2023, 5:40pm

Oh, ok. So, considering that the core teams are involved in this, you think it won’t be long. Good.

grb · January 24, 2023, 10:41pm

@Quexten

It seems that the password-protected JSON exports are hardcoded to use PBKDF2:

github.com

bitwarden/clients/blob/feb10128f0963bf6a9dabf23d1a01d8ae8ea117c/libs/common/src/services/export.service.ts#L62


      
          async getPasswordProtectedExport(password: string, organizationId?: string): Promise<string> {
            const clearText = organizationId
              ? await this.getOrganizationExport(organizationId, "json")
              : await this.getExport("json");
          
          
  const salt = Utils.fromBufferToB64(await this.cryptoFunctionService.randomBytes(16));
            const kdfIterations = DEFAULT_KDF_ITERATIONS;
            const key = await this.cryptoService.makePinKey(
              password,
              salt,
              KdfType.PBKDF2_SHA256,
              kdfIterations
            );
          
          
  const encKeyValidation = await this.cryptoService.encrypt(Utils.newGuid(), key);
            const encText = await this.cryptoService.encrypt(clearText, key);
          
          
  const jsonDoc: any = {
              encrypted: true,
              passwordProtected: true,
              salt: salt,

Perhaps this requires a separate PR, but it would be nice if when a user sets the hash function option to Argon2 for their vault, this setting would also be carried over to exports (and Sends).

Quexten · January 24, 2023, 10:45pm

I already have a PR for this open

github.com/bitwarden/clients

[PS-2264] Fix encrypted export ignoring user's KDF settings

bitwarden:master ← quexten:fix/encrypted-export-kdf

opened 12:58PM - 15 Jan 23 UTC

quexten

+18 -11

## Type of change ``` - [x] Bug fix - [ ] New feature development - [ …] Tech debt (refactoring, code cleanup, dependency upgrades, etc) - [ ] Build/deploy pipeline (DevOps) - [ ] Other ``` ## Objective The encrypted export format contains fields for the KDF type and iteration count, but it was only statically set to PBKDF2 and 100.000 iterations. As a user I would expect it to follow my vault's KDF settings, should I choose to configure them with higher iteration counts/other kdf functions. This pull request changes the export and import to remove the hardcording, such that they work with different iteration counts and different KDF types. ## Code changes We just inject the stateservice into the export service to get the KDF type and iterations, and write them into the exported json/use them to encrypt.

By the way, Sends (which I don’t really use) also have 100K fixed pbkdf2. But they don’t even store the kdf / iterations in the database, so changing it would require another database migration / backend change which I didn’t really feel like taking on considering how low the risk for a send is anyways.

Quexten · January 25, 2023, 2:04pm

Mobile and Server support has been merged to master , only clients (Web/Desktop/Browser/CLI) is still in-progress.

deviator · January 26, 2023, 3:44pm

Good stuff! I have one comment regarding the Argon2id default parametes as I noticed that you have chosen extremely conservative default values:

export const DEFAULT_ARGON2_MEMORY = 19;
export const DEFAULT_ARGON2_PARALLELISM = 1;
export const DEFAULT_ARGON2_ITERATIONS = 2;

It seems that the default values are based on the OWASP recommendation which I believe applies to server side operations where you may have a significant number of hashing/key derivation operations at the same time. This is not the case with Bitwarden as the key derivation is done on the client side. KeePass, for example, recommends following method to determine the memory parameter:

Find out the RAM size of each of your devices on which you want to open your database file. Let M be the minimum of these sizes. Set the memory parameter to min(M /2, 1 GB) (i.e. use the half of M , if it is less than 1 GB, otherwise use 1 GB).

Quexten · January 26, 2023, 3:45pm

I agree, they are based on OWASP. The defaults should run on all devices, so 1GiB is too large as a default. But I have suggested higher defaults on the configuration pull requests, let’s see what the bitwarden team thinks on this.

grb · January 26, 2023, 4:08pm

OWASP recommendations are apparently cribbed from Steve Thomas, who explains the rationale for his recommendations in the Info section at the bottom of his Minimum Password Settings page. The settings are designed to throttle an attacker’s hash speeds to 10kH/s/GPU. Of note is that this is the recommendation when using passwords for authentication purposes only. As I’ve noted in another post, when the password is used for encryption, hash rates should ideally be much slower (<1kH/s/GPU).

Quexten · January 26, 2023, 4:09pm

Thanks for finding this! I’ll note this on the GitHub thread aswell.

tomtom · January 26, 2023, 4:14pm

Thanks for the hard work you‘re doing. Really appreciate it.

Have a nice day

Quexten · January 28, 2023, 2:40am

Updated defaults now to Iterations = 3, Memory = 64MiB, Parallelism = 4 from RFC9106’s recommendation on “safe defaults”. If you have significantly worse system specs, then you can go lower. If you have higher specs on your devices you can go higher.

grb · January 28, 2023, 3:19am

The recommended defaults in RFC9106 also specify “128-bit salt, and 256-bit tag size” — are these specs also implemented in your PR?

Quexten · January 28, 2023, 3:33am

The salt is the email, so we don’t directly influence how many bit’s it has. Since it has to be at least 8 bytes, I had to update it to be the SHA256 hash of the email such that even very short emails will work correctly. So it is 32 Byte i.e 256 bit.

Tag size = hash size. This is 32 Byte (or 256), same as for PBKDF2.

Quexten · January 30, 2023, 4:36pm

The last 2 Pull-requests have now been merged!

mkucharski · February 13, 2023, 8:09pm

I saw comments about that WASM support is only affecting old browsers, in this thread and other KDF related threads.

Please be aware, there are modern web browser setups where WASM is disabled by default. Reliance on WASM is not that an optimal solution and I imagine it will break some clients.

Quexten · February 13, 2023, 8:23pm

By default it is enabled in (nearly) all popular, modern browsers (WebAssembly | Can I use... Support tables for HTML5, CSS3, etc). Though I agree there are browsers where it is disabled (someone mentioned iOS in lockdown mode, possibly also the Tor browser?). Do you have as specific modern browser in mind?

There is not much that can be done about this unless web browsers natively implement argon2. If that happens, switching out the wasm implementation is only a few lines of code.
Users who have such a browser setup should stick with pbkdf2 for the time being.

mkucharski · February 13, 2023, 9:07pm

Do you have as specific modern browser in mind?

Chromium on OpenBSD by default has WASM disabled.

Quexten · February 13, 2023, 9:23pm

Ah, I see. Well as I said, can’t do much about that unless browsers implement argon2 natively after it getting added to WebCrypto. My advice would be either to continue using pbkdf2 or to enable WebAssembly in chromium.

bw-admin · February 14, 2023, 10:45am

Hey @Quexten we’re switching over to Github discussions to keep the PR chats closer to the code. all new threads here are locked, but replies will still function for the time being.

Feel free to resume discussion on Github:

Quexten · February 14, 2023, 10:47am

Yeah, I saw that, I just responded to @mkucharski’s question This PR (set of PR’s) is completed and merged anyways, so I don’t see a need to re-open it on GitHub.