Skip to content

Conversation

@gantunesr
Copy link
Member

@gantunesr gantunesr commented Mar 17, 2025

Description

This change increases the number of iteration required for the key derivation from 5_000 to 600_000.

Related issues

Fixes: https://github.com/MetaMask/accounts-planning/issues/100

Manual testing steps

Test case 1: Upgrade the client from the current version to feature branch
  1. Install the current version of MM mobile or the last commit in the main branch
  2. Create or import a wallet and verify it is correctly created
  3. Lock the wallet
  4. Upgrade the app to this feature branch
  5. Unlock the wallet, it should unlock the wallet
Test case 2: Lock and unlock the wallet
  1. Create or import a wallet and verify it is correctly created
  2. Lock the wallet
  3. Unlock the wallet, it should unlock the wallet
Test case 3: Reveal SRP and private key
  1. Create or import a wallet and verify it is correctly created
  2. Go to Settings > Security & Privacy
  3. Reveal the SRP, it should show the correct SRP
  4. Go back to Security & Privacy
  5. Reveal the private key of the current account
Test case 4: Change password
Test case 5: Unlock with biometrics

Screenshots/Recordings

Not applicable

Pre-merge author checklist

Pre-merge reviewer checklist

  • I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed).
  • I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots.

@metamaskbot metamaskbot added the team-accounts-framework Accounts Framework team label Mar 17, 2025
@github-actions

This comment was marked as outdated.

@gantunesr gantunesr marked this pull request as ready for review March 18, 2025 12:54
@gantunesr gantunesr requested review from a team as code owners March 18, 2025 12:54
@gantunesr gantunesr requested a review from a team March 18, 2025 12:54
@gantunesr gantunesr requested a review from a team as a code owner March 18, 2025 12:54
@github-actions

This comment was marked as outdated.

@github-actions

This comment was marked as outdated.

@github-actions

This comment was marked as outdated.

@gantunesr
Copy link
Member Author

Blocked by #14304

owencraston
owencraston previously approved these changes Apr 10, 2025
Copy link
Contributor

@owencraston owencraston left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did not perform manual testing but the code looks good.

@gantunesr
Copy link
Member Author

gantunesr commented Apr 10, 2025

Basic performance tests (execution time in ms)

Using 5.000 iterations

Method keyFromPassword decryptWithDetail loginVaultCreation
Test 1 4.37 5,83 142.43
Test 2 4.17 7.81 125.67
Test 3 3.34 4.70 66.35
Test 4 3.37 4.78 69.05
Test 5 3.48 4.45 59.35
Average 3.74 5.51 92.57

Using 600.000 iterations

Method keyFromPassword decryptWithDetail loginVaultCreation
Test 1 308.91 309.99 429.29
Test 2 291.42 305.66 565.25
Test 3 348.70 350.17 503.98
Test 4 367.32 368.66 519.08
Test 5 380.14 381.65 562.84
Average 339.30 343.23 516.09

Execution time increases from deriving the encryption key from 5,000 iterations to 600,000 iterations:

  • keyFromPassword: 9,072%
  • decryptWithDetail: 6,230%
  • loginVaultCreation: 557%

edit:

Using 600.000 iterations and react-native-quick-crypto

Method keyFromPassword decryptWithDetail loginVaultCreation
447.359 449.803 637.011
598.649 601.918 789.760
554.537 558.304 719.538
527.388 530.147 681.279
554.744 557.730 713.822
Average 536.5354 539.5804 708.282

@github-actions

This comment was marked as outdated.

@github-actions

This comment was marked as outdated.

NicholasEllul
NicholasEllul previously approved these changes Apr 14, 2025
Copy link
Contributor

@NicholasEllul NicholasEllul left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Proposed changes look good from a security perspective. While we see that the time to decrypt has increased by ~100x, this also means that each attempt an attacker makes trying to crack the vault will cost ~100x more than it does today.

The security properties of PBKDF2 come from the amount of time it takes to perform. Increasing the iteration count is expected to proportionally increase the time the operation takes. This allows for it to be more resistant to brute forcing.

We can look into making future optimizations to minimize the occurrences where performing PBKDF2 is necessary; however, we should expect to see the impact of these changes when users are logging in via password to perform a vault decryption.

An additional note to consider:

The security of the encrypted vault is a combination of two factors:

  • The number of iterations performed on the user's password
  • The complexity of the user's password

A long and highly complex password can get away with less iterations; however, a weak and predictable password will require a higher iteration count. We can use both levers to strengthen the security of the vault.

@gantunesr gantunesr dismissed stale reviews from NicholasEllul and owencraston via f9109d7 May 6, 2025 20:25
@github-actions

This comment was marked as outdated.

@github-actions
Copy link
Contributor

github-actions bot commented May 9, 2025

https://bitrise.io/ Bitrise

❌❌❌ pr_smoke_e2e_pipeline failed on Bitrise! ❌❌❌

Commit hash: 7348f41
Build link: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/7ebce034-0e6d-4151-a6ef-e66791dafec7

Note

  • You can kick off another pr_smoke_e2e_pipeline on Bitrise by removing and re-applying the Run Smoke E2E label on the pull request

Tip

  • Check the documentation if you have any doubts on how to understand the failure on bitrise

@sonarqubecloud
Copy link

sonarqubecloud bot commented May 9, 2025

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

team-accounts-framework Accounts Framework team

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants