Conversation
paberr
left a comment
There was a problem hiding this comment.
Also, I would explicitly state that it is ok for us to leak equality (because it's deterministic).
- If an attacker ever sees a backup code pair (or even one of them), they can test whether another pair belongs to the same key by recomputing/relating (depending on what they have). It is strictly more information than in a random case.
- If the user stores codes in multiple places, compromise of one place can enable correlation across other leaks.
danimoh
left a comment
There was a problem hiding this comment.
Thanks for the review. Do you think the issues with code equality and code correlation are worth switching to truly random codes? I do like the elegance of deterministic codes and the fact that no separate checksum needs to be included.
Adding a checksum has a similar effect: you can check if two codes match with high likelihood. So, as long as we want to have some sort of check, it's not worth it changing this. |
Ah right, of course 😄 |
Make utilities for starting a view transition available as ViewTransitionHandler.
…esInput Split common code into a base class BackupCodesIllustrationBase.
…Codes) Add a general Import handler that orchestrates the more specific handlers ImportFile, ImportWords and ImportBackupCodes and avoids code duplication between them via shared methods and UIs (e.g. storing of keys, password setter, download of new Login File).
Avoid legacy NimiqPoW staying assigned to window.Nimiq when an exception occurs during import of legacy Login Files, for example due to wrong password entered.
RecoveryWords now sets the words correctly regardless of whether it's interactive with input elements or not, and re-evaluates the set recovery words.
When pasting multiple words at once, avoid re-validating the recovery words for each word entered. This especially also deduplicates the RecoveryWords.Events.COMPLETE event and expensive computations following it.
This brings the implementation in line with the create and export flows. The same fix is also applied to the change-password flow.
…-encrypting Encrypting the secret is expensive, therefore avoid re-encrypting it for the Login File download, and instead take it from the KeyStore, where we already encrypted the secret.
…f adresses/pub keys Run encryption and storage of key in the background, which is quite expensive but luckily runs on a secondary thread, in parallel to the derivations on the main thread, which are also expensive due to the involved mnemonicToSeed which runs a pbkdf2 on the main thread. There is also more potential for plenty more performance improvements, which we'll not tackle right now though: - (re)encrypting the secret is expensive. If the secret is already encrypted with the latest version in the imported LoginFile, store this already encypted data, instead of re-encrypting. - mnemonicToSeed is duplicated between Key.deriveAddress, BitcoinKey.deriveExtendedPublicKey and (via ethers) in PolygonKey.deriveAddress, and is a very expensive call due to the called pbkdf2, which is on top running synchronously and blocking the main thread. Consider caching the seed in the KeyStore, or at least as variable in the Key. This should also speed up other situations, where many addresses are derived, for example the address detection via Keyguard iframe in the Hub. - change or replace mnemonicToSeed with an implementation of pbkdf2 that doesn't run on the main thread, for example via the web crypto api, and then move the creation of the key results out of here into a separate method, that can already be launched when the _importedKeys are received, and runs in the background while the user can continue setting a password.
|
It looks good to me 👍 |
- Firefox + Safari: change detection of oldPageId on browser navigations, by caching the value, because the previous implementation of reading it from the DOM was not compatible with Firefox and Safari. - Firefox: enforce breaking of placeholder codes into multiple lines via overflow-wrap: anywhere; - Firefox: adapt parsing of transition keyframe positions for Firefox, which uses matrix3d() instead of matrix(). - Firefox: clear all applied animations manually after transition end, because Firefox doesn't do it correctly itself. - Firefox: throws "Skipped ViewTransition due to document being hidden" when attempting to start a transition while in a different app. I tried catching exceptions in various places, but wasn't able to catch this one, so the issue is not resolved. I did keep one catch which generally makes sense though. - Firefox: fix transition of Backup Codes import warning by not transitioning it individually, which Firefox was struggling with, transitioning from/to the empty warning. Instead, it now just fades with the rest of the page, apart from the message bubbles which have their custom transition. - Safari: fix rendering of faded message bubbles during transitions via alternative css. Safari was struggling with the previous implementation of making the message bubble white via a filter: brightness(255) opacity(.1), and rendered them entirely white without opacity during transitions. The same, when setting opacity: .1 as separate propterty. So now the implementation of .faded has been changed entirely to not be based on a filter anymore, but instead setting the white background and hiding icons and shadows manually.
…odes Code font sizes are optimized for the zoomed state to not break into three lines in most cases. For some codes with many wide characters the non zoomed code might break into three lines though, because of different rounding of paddings etc. To avoid this, we slightly reduce the non-zoomed font-size. This might lead to the code breaking at different spots though, compared to the zoomed state.
…df optional - Avoid attacker controllable salt by applying the useCase only in a final HKDF expansion, instead of mixing it into the salt. - Use SHA-512 instead of SHA-256 as internal hash function for better quantum resistance and cheaper derivation of secret lengths longer than 256 bit. - Increase the length of the salt to 512 bit as it should match the length of the used hashes for HKDF. - As the seed bytes we're using as key material has very high entropy, using HKDF really should be sufficient, so applying an additional, more expensive kdf on top is now optional. On the other hand, a HKDF is now always involved for the final key expansion for a specific useCase.
We previously derived the salt from the key material, but taking extra care to avoid attacker controllable salts. However, as independent salts are generally preferable anyway and our derived salts did not really provide any benefit while complicating the implementation, we now simply go with a fixed salt, following HKDF's specification.
As the seed has a very high entropy, a more expensive kdf than HKDF should not be necessary.
de50330 to
f4ae816
Compare
Primarily German and partially French
| // Derive different secrets for legacy PrivateKey based accounts and modern Entropy based accounts, even | ||
| // if their underlying secret bytes are the same. | ||
| this.secret instanceof Nimiq.PrivateKey ? 'PrivateKey' : 'Entropy', | ||
| ].join()), |
There was a problem hiding this comment.
It's important to make sure a user can't create collisions of the info string. Since all use-case strings are hardcoded (not user-controlled), I don't see a practical risk for collision. But as a defensive measure, using an unambiguous separator (e.g. a length-prefixed encoding) would make this robust by construction rather than by convention.
bb5465f to
1b7f063
Compare
This PR contains the Backup Codes feature, which serves as an alternative backup option to the Login File and Recovery Words.
So far, I pushed the export flow, which is ready for review. The import flow will follow soon after in the coming days.
Before final release, the link to the instructions will have to be updated with the link to the actual blog post.
@paberr, from you a review of the technical parts should suffice. Please have a look at the changes in
src/lib/BackupCodes.jsandsrc/lib/Key.js.