Pbkdf2 iterations fix #14523

supermassive · 2022-08-08T11:52:26Z

Increased pbkdf2 iteration count to 310K. Added migration which happens on wallet unlock or wallet restore.
Removed some migration code https://github.dev/brave/brave-core/pull/9674/files which was here for a year.

Resolves https://github.com/brave/internal/issues/889
Resolves brave/brave-browser#24581

Submitter Checklist:

I confirm that no security/privacy review is needed, or that I have requested one
There is a ticket for my issue
Used Github auto-closing keywords in the PR description above
Wrote a good PR/commit description
Squashed any review feedback or "fixup" commits before merge, so that history is a record of what happened in the repo, not your PR
Added appropriate labels (QA/Yes or QA/No; release-notes/include or release-notes/exclude; OS/...) to the associated issue
Checked the PR locally: npm run test -- brave_browser_tests, npm run test -- brave_unit_tests, npm run lint, npm run gn_check, npm run tslint
Ran git rebase master (if needed)

Reviewer Checklist:

A security review is not needed, or a link to one is included in the PR description
New files have MPL-2.0 license header
Adequate test coverage exists to prevent regressions
Major classes, functions and non-trivial code blocks are well-commented
Changes in component dependencies are properly reflected in gn
Code follows the style guide
Test plan is specified in PR before merging

After-merge Checklist:

Test Plan:

Increased pbkdf2 iteration count could be measured by increased lag after clicking 'Unlock' button.
Upgrade scenarios should be carefully tested here. Migration happens when user successfully unlocks wallet or successfully restores wallet with right mnemonic and password pair.

brave://prefs-internals/ could be used to aid testing
keyring_encryption_keys_migrated - changes from false to true after migration or after creating new wallet
for default, filecoin, filecoin_testnet, solana in wallet.keyring object:
encrypted_mnemonic, password_encryptor_nonce and password_encryptor_salt are updated after migration
each encrypted_private_key whitin imported_accounts also updated

kdenhartog

Spotted a few crypto bugs in here we want to fix around nonce reuse. Let's get those fixed first and then I can approve.

Overall the design and approach taken here looks like a good way of doing it and thanks for doing the refactoring as well!

kdenhartog · 2022-08-09T10:03:00Z

components/brave_wallet/browser/keyring_service.cc

+
+    SetPrefInBytesForKeyring(
+        prefs_, kEncryptedMnemonic,
+        encryptor->Encrypt(base::make_span(*mnemonic), *nonce), keyring_id);


We need to be generating a new nonce used to encrypt here rather than reusing the one that was used previously to encrypt the keyring. Otherwise there's a crypto bug in this that would allow an attacker to combine the old ciphertext with the new ciphertext in a way that could reveal the XOR'd plaintext of the two messages.

++ this is the only place in this PR we need to use a new nonce for. See this thread for details: https://github.com/brave/brave-core/pull/14523/files#r941156147

generating new nonce here
bd0e5d5

components/brave_wallet/browser/keyring_service.cc

kdenhartog · 2022-08-09T10:11:31Z

components/brave_wallet/browser/keyring_service.cc

                                              base::span<const uint8_t> bytes,
                                              const std::string& id) {
  const std::string encoded = base::Base64Encode(bytes);
-  SetPrefForKeyring(prefs_, key, base::Value(encoded), id);
+  SetPrefForKeyring(prefs, key, base::Value(encoded), id);
 }

 std::vector<uint8_t> KeyringService::GetOrCreateNonceForKeyring(


If we want to reuse this function for nonce generation for different plaintext (e.g. each mnemonic and each imported account) which is what this PR is doing, we should update the API to also pass in the constant of the pref we'll be looking up/setting it into. Otherwise, we'll introduce a nonce reuse attack this way as well.

It looks like we're already doing this properly for each keyring, but we also want to do this for each imported account. Otherwise when calling this function during import account after already having created the keyring for the coin type in use we'll end up reusing the nonce and an attacker could take the two ciphertexts of the mnemonic and the private key and end up with the XOR(mnemonic, private_key).

E.g

Nonce 1 -> defaultKeyring, Mnemonic
Nonce 2 -> defaultKeyring, imported account 1
Nonce 3 -> defaultKeyring, imported account 2
Nonce N-1 -> defaultKeyring, imported account N

Nonce 1 -> SolanaKeyring, Mnemonic
Nonce 2 -> SolanaKeyring, imported account 1
Nonce 3 -> SolanaKeyring, imported account 2
Nonce N-1 -> SolanaKeyring, imported account N

You might want to check this security review first.
Sharing nonce between mnemonic and imported private keys is not introduced with this PR.

EDITED this comment to make it more on topic for others briefly reviewing this thread

What I was concerned about originally was the nonce reuse in the MaybeMigratePBKDF2Iterations function as a whole.

Thanks for pointing that out @darkdh that's good context as to the rationale behind the decisions of the current work.

On the topic of reusing the nonce between the imported keys and the mnemonic like we do by reusing the nonce from line 1729 and line 1761 the CFRG RFC for AES-GCM-SIV says the following:

We stress that nonce misuse-resistant schemes guarantee that if a
nonce repeats, then the only security loss is that identical
plaintexts will produce identical ciphertexts. Since this can also
be a concern (as the fact that the same plaintext has been encrypted
twice is revealed), we do not recommend using a fixed nonce as a
policy. In addition, as we show below, better-than-birthday bounds
are achieved by AES-GCM-SIV when the nonce repetition rate is low.
Finally, as shown in [BHT18], there is a great security benefit in
the multiuser/multikey setting when each particular nonce is reused
by a small number of users only. We stress that the nonce misuse-
resistance property is not intended to be coupled with intentional
nonce reuse; rather, such schemes provide the best possible security
in the event of nonce reuse. Due to all of the above, it is
RECOMMENDED that AES-GCM-SIV nonces be randomly generated.

A simple way that we could fix this is to add an API parameter to GetOrCreateNonceForKeyring such that it would include the id of the pref stored as well. In the case of the imported accounts we could probably make this the name of the imported account, or even just a counter with a state parameter of the current count such that the nonce doesn't need to be reused.

With that said, I don't see it as absolutely necessary to switch the nonces for the imported accounts each time and would be ok if we reused the nonce across the mnemonic and the imported keys if we find it to be a simpler model so we don't have to store as much state.

TL;DR we'll address this part in a separate issue.

On the topic of reviewing the security of nonce reuse across migration

I spent some time reviewing this and convinced myself that nonce reuse here is a concern, but not like I originally thought with the same message being used. The reason for this is because the way in which we're generating the keys is via the PBKDF2 function and we're securely generating different keys for this because we're changing the number of iterations used.

Essentially what we're doing is AES-CTR with the same nonce (GCM-SIV is just doing a combo of hashing of the message and combining that with the nonce to produce a random nonce in a secure way and then using AES-CTR for the rest starting from the nonce value) and same message, but because we're securely generating random keys generated by PBKDF2 the result of XOR encrypted_mnemonic1 (C1) and encrypted_mnemonic2 (C2) is the output of PBKDF2 w/ 100k iterations (K1) XORed with the PBKDF2 w/ 310k iterations (K2). This means assuming the attacker was able to brute force the K1 key (the same security level as we have today and it should be noted this isn't easy) they'd be able to then XOR the K2 key and decrypt C2.

Essentially what this means is that by reusing the same nonce value and same mnemonic the weakest link remains the ability to brute force the K1 key effectively making all migrated mnemonics have the same security level as we currently do because they can still brute force the old K1 value to reveal the new K2 value (assuming they have access to the old encrypted_mnemonic ciphertext which is unplausible)

However, if we generate a new nonce value here we eliminate this slight cryptographic security downgrade that the migration is intended to be fixing.

TL;DR - Essentially, we just need to make sure we're not reusing the same nonce that's pulled out on line 1703 and used to decrypt on line 1718 can't be the same nonce as the one used to re-encrypt with the new encrypt call on line 1729 and we don't encounter a downgrade to the current security.

I agree that we should use different nonce in migration function but my point is if we want to do different nonce for import accounts, it should be in different PR.

@darkdh and I spoke on Slack and his perspective on doing this nonce rotation for the imported accounts separately is a much cleaner approach all things considered because it makes this easier to revert changes if needed and makes it much easier for others to assess what's happening in this PR if they weren't involved. Let's keep this PR focused only on updating the nonces for the re-encrypted mnemonics to keep this PR strictly focused on the original issue.

I'll go through and resolve the comments that aren't necessary for nonce updates and open a second issue for the imported_keys nonce rotation.

Agree that we should store encrypted ciphertext and nonce as pair for each piece of data we encrypt. But this is a subject of another issue.

Added salt and nonce rotation here
bd0e5d5

components/brave_wallet/browser/keyring_service.cc

kdenhartog · 2022-08-09T10:42:22Z

components/brave_wallet/browser/keyring_service.cc

-  }
+  // TODO(apaymyshev): move this call(and other ones in this file) to
+  // background thread.
+  auto encryptor = PasswordEncryptor::DeriveKeyFromPasswordUsingPbkdf2(


If a user has already setup a keyring will this properly decrypt? Seems like we'd need to be migrating before decrypting with the new 310k iterations.

We should check if migration is happened or not. If it is migrated then use new iterations, otherwise use old iterations here. This API is simply used to validate current password regardless wallet 's lock state and it is often called before UI calling Unlock(). So we should not do the migration here.

We need a test case to validate password before migration and after migration.

++ to this ^. I was thinking this was unlocking the wallet originally as well if the password validated, but @darkdh approach is a cleaner way to handle this migration

Well, password validation was supposed to be done for already unlocked keyring, so no migration here needed. Added some checks and tests for that in
bd0e5d5

components/brave_wallet/browser/password_encryptor.cc

components/brave_wallet/browser/pref_names.h

components/brave_wallet/browser/keyring_service.cc

darkdh · 2022-08-09T18:00:32Z

components/brave_wallet/browser/keyring_service.cc

-  }
+  // TODO(apaymyshev): move this call(and other ones in this file) to
+  // background thread.
+  auto encryptor = PasswordEncryptor::DeriveKeyFromPasswordUsingPbkdf2(


We should check if migration is happened or not. If it is migrated then use new iterations, otherwise use old iterations here. This API is simply used to validate current password regardless wallet 's lock state and it is often called before UI calling Unlock(). So we should not do the migration here.

We need a test case to validate password before migration and after migration.

browser/brave_wallet/keyring_service_unittest.cc

kdenhartog

Updating the comments left here to make it easier for @supermassive to figure out what actually needs to be addressed and resolved the ones I don't think are necessary anymore

kdenhartog · 2022-08-10T19:02:44Z

components/brave_wallet/browser/keyring_service.cc

+        mojom::kFilecoinTestnetKeyringId, mojom::kSolanaKeyringId}) {
+    auto legacy_encrypted_mnemonic =
+        GetPrefInBytesForKeyring(prefs_, kEncryptedMnemonic, keyring_id);
+    auto nonce =


nit: can we rename this to legacy_nonce?

components/brave_wallet/browser/keyring_service.cc

kdenhartog · 2022-08-10T19:05:30Z

components/brave_wallet/browser/keyring_service.cc

+
+    SetPrefInBytesForKeyring(
+        prefs_, kEncryptedMnemonic,
+        encryptor->Encrypt(base::make_span(*mnemonic), *nonce), keyring_id);


++ this is the only place in this PR we need to use a new nonce for. See this thread for details: https://github.com/brave/brave-core/pull/14523/files#r941156147

darkdh · 2022-08-10T20:49:34Z

btw you also need a brave-browser issue as a placeholder for the security issue, otherwise we won't be able to set milestone and tracking release correctly. ex. brave/brave-browser#24415

kdenhartog · 2022-08-11T03:16:33Z

btw you also need a brave-browser issue as a placeholder for the security issue, otherwise we won't be able to set milestone and tracking release correctly. ex. brave/brave-browser#24415

Thanks for calling that out, done here: brave/brave-browser#24581

Also, for a test plan is it possible for us to inspect the prefs on the filesystem? If so some additional potential options for testing this is we could see that the ciphertext is different between the creation event and the final event. For a new creation event we could chose a known password and mnemonic and make sure we can manually decrypt it as well to make sure the number of iterations are correct. Happy to help with this if QA wants to use either of these options instead.

supermassive · 2022-08-11T09:59:52Z

@kdenhartog @darkdh PTAL

browser/brave_wallet/keyring_service_unittest.cc

darkdh · 2022-08-11T23:06:08Z

Also, for a test plan is it possible for us to inspect the prefs on the filesystem? If so some additional potential options for testing this is we could see that the ciphertext is different between the creation event and the final event. For a new creation event we could chose a known password and mnemonic and make sure we can manually decrypt it as well to make sure the number of iterations are correct. Happy to help with this if QA wants to use either of these options instead.

Please update the test plan to tell QA what to inspect with path of the prefs in brave://prefs-internals/
ex. wallet.keyrings.default.encrypted_mnemonic

supermassive · 2022-08-12T06:37:52Z

Also, for a test plan is it possible for us to inspect the prefs on the filesystem? If so some additional potential options for testing this is we could see that the ciphertext is different between the creation event and the final event. For a new creation event we could chose a known password and mnemonic and make sure we can manually decrypt it as well to make sure the number of iterations are correct. Happy to help with this if QA wants to use either of these options instead.

Please update the test plan to tell QA what to inspect with path of the prefs in brave://prefs-internals/ ex. wallet.keyrings.default.encrypted_mnemonic

updated testplan, thanks!

darkdh

lgtm now

kdenhartog

Looks good to me, thanks for the fixes

kdenhartog · 2022-08-17T14:19:26Z

Here's the separate issue for changing the nonce for the imported private keys later.

brave/brave-browser#24742

supermassive added 3 commits August 8, 2022 18:51

remove deprecated migration code

f79564a

refactored pref utility methods for keyring service

f737986

Refactor PasswordEncryptor and Base64Decode usage

8e9e29c

supermassive requested a review from a team as a code owner August 8, 2022 11:52

github-actions bot assigned supermassive Aug 8, 2022

darkdh self-requested a review August 8, 2022 23:34

Migrate pbkdf2 iterations

9e0ec70

supermassive force-pushed the pbkdf2_iterations_fix branch from 0c962cc to 9e0ec70 Compare August 9, 2022 04:24

kdenhartog requested changes Aug 9, 2022

View reviewed changes

darkdh reviewed Aug 9, 2022

View reviewed changes

kdenhartog requested changes Aug 10, 2022

View reviewed changes

fixes per review

bd0e5d5

supermassive requested a review from kdenhartog August 11, 2022 10:00

darkdh reviewed Aug 11, 2022

View reviewed changes

supermassive added 2 commits August 12, 2022 13:38

fixes per review

af40a88

undo ValidatePassword changes

e48add2

darkdh approved these changes Aug 16, 2022

View reviewed changes

kdenhartog approved these changes Aug 17, 2022

View reviewed changes

supermassive merged commit 707d4a0 into master Aug 18, 2022

supermassive deleted the pbkdf2_iterations_fix branch August 18, 2022 05:45

github-actions bot added this to the 1.44.x - Nightly milestone Aug 18, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pbkdf2 iterations fix #14523

Pbkdf2 iterations fix #14523

supermassive commented Aug 8, 2022 •

edited

Loading

kdenhartog left a comment

kdenhartog Aug 9, 2022

kdenhartog Aug 10, 2022

supermassive Aug 11, 2022

kdenhartog Aug 9, 2022

darkdh Aug 9, 2022

kdenhartog Aug 10, 2022 •

edited

Loading

darkdh Aug 10, 2022

kdenhartog Aug 10, 2022 •

edited

Loading

supermassive Aug 11, 2022

kdenhartog Aug 9, 2022

darkdh Aug 9, 2022

kdenhartog Aug 10, 2022

supermassive Aug 11, 2022 •

edited

Loading

darkdh Aug 9, 2022

kdenhartog left a comment

kdenhartog Aug 10, 2022

supermassive Aug 11, 2022

kdenhartog Aug 10, 2022

darkdh commented Aug 10, 2022

kdenhartog commented Aug 11, 2022

supermassive commented Aug 11, 2022

darkdh commented Aug 11, 2022

supermassive commented Aug 12, 2022

darkdh left a comment

kdenhartog left a comment

kdenhartog commented Aug 17, 2022

Pbkdf2 iterations fix #14523

Pbkdf2 iterations fix #14523

Conversation

supermassive commented Aug 8, 2022 • edited Loading

Submitter Checklist:

Reviewer Checklist:

After-merge Checklist:

Test Plan:

kdenhartog left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kdenhartog Aug 10, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kdenhartog Aug 10, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

supermassive Aug 11, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kdenhartog left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

darkdh commented Aug 10, 2022

kdenhartog commented Aug 11, 2022

supermassive commented Aug 11, 2022

darkdh commented Aug 11, 2022

supermassive commented Aug 12, 2022

darkdh left a comment

Choose a reason for hiding this comment

kdenhartog left a comment

Choose a reason for hiding this comment

kdenhartog commented Aug 17, 2022

supermassive commented Aug 8, 2022 •

edited

Loading

kdenhartog Aug 10, 2022 •

edited

Loading

kdenhartog Aug 10, 2022 •

edited

Loading

supermassive Aug 11, 2022 •

edited

Loading