feat: reduce user storage encryption iteration count

Prithpal-Sooriya commented 2 weeks ago

Description

Balance between security and UX/speed. We perform many Encryption/Decryption transactions for notifications and speed really matters! The content we are saving in user storage does not need to be so secure that it compromises UX.

Manual testing steps

N/A, this is testable once the entire Notifications Feature has been merged.

Screenshots/Recordings

Before

After

Pre-merge author checklist

[ ] I’ve followed MetaMask Coding Standards.
[ ] I've completed the PR template to the best of my ability
[ ] I’ve included tests if applicable
[ ] I’ve documented my code using JSDoc format if applicable
[ ] I’ve applied the right labels on the PR (see labeling guidelines). Not required for external contributors.

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.

github-actions[bot] commented 2 weeks ago

CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes.

legobeat commented 2 weeks ago

This number seems rather low. How was it chosen? On mobile, we are bumping the default to 900,000 iterations, and I think it's already the case in the extension.

@Prithpal-Sooriya How was the exact number 32767 determined here?

Noting that:

If we end up needing to fix/replace/alter the encryption or derivation code, the payload saved as UserStorage will become unusable.

So any new production value will effectively "wipe" all past storage using the old value, so we should expect it to be sticky. Migration will be needed if this is to be seamless for users. So if reducing, we should still go as high as we can tolerate, which is the general recommendation.

There was also some conversation in the linked previous PR about considering a different KDF - is this still on the table?

The content we are saving in user storage does not need to be so secure

You may think that about the current usage but does the assumption hold for all users, and future use of the same derived key?

Prithpal-Sooriya commented 2 weeks ago

@legobeat

Yeah this PR was mostly a hotfix from the notification team due to high iteration count compromising UX speed (we encrypt and decrypt a few times in a row). Maybe its better to move this back into draft, and choose a better solution after discussion 😄

How was the exact number 32767 determined here?

Yeah this is magic number-y, its 2**15 - 1 - a number we are using on Portfolio. TBH we can definitely go higher, just that 900_000 was tremendously slow.

So any new production value will effectively "wipe" all past storage using the old value, so we should expect it to be sticky. Migration will be needed if this is to be seamless for users. So if reducing, we should still go as high as we can tolerate, which is the general recommendation.

We store this iteration count with the payload, so when decrypting we can use iteration count from here. This allows us to increase the iteration count if we need to. But you are right, we also do versioning, so can support "migrations" when we need it.

There was also some conversation in the linked previous PR about considering a different KDF - is this still on the table?

Yep 100%, I did a little playing around and got @noble/hashes/scrypt working, we could also use @noble/hashes/argon2 (not tested this, and this has not been audited in the @noble repo).

Please correct me if wrong, but these also have "levers"/fields which we can up the work factor, but still need to balance UX 😄

Update:

Here is a draft/temp PR to demo using scrypt instead of pbkdf2 https://github.com/MetaMask/metamask-extension/pull/24541

Prithpal-Sooriya commented 1 week ago

Closing this since this is not a solution we will be implementing.

MetaMask / metamask-extension