iotaledger / iotaledger/crypto.rs
Improve BIP39 implementation
- Dominant language
- Rust
- Stars
- 63
- Forks
- 24
- PR merge metrics
- No merged PRs in 30d
Description
The implementation of bip39 is questionable. From the first glance the following looks suspicious to me:
1. `mnemonic_to_seed` modifies the input mnemonic (which is a secret) and doesn't clean up the local result (which is also secret). I'd advise against modifying secrets, it feels weird. Maybe there should be some sort of validation, or `Mnemonic` type should encapsulate only valid mnemonics (with unnecessary spaces stripped and in NFKD form).
2. `salt` intermediate value is not cleared in `mnemonic_to_seed`.
3. `Wordlist` accepts some "bad" and "incorrect" words and separators. Maybe, there should be a constructor for normalizing and checking words.
4. `data` in `encode` should be called `secret_entropy` or something to indicate its purpose: it should be handled with care and zeroized after use.
5. `encode` should return `Mnemonic`, not just `String`.
6. `CS` is not zeroized in `encode`.
7. `decode` takes secret mnemonic as input `ms` of type `&str` and is modified (normalized to NFKD form). NFKD form should be validated/converted to in a `Mnemonic` constructor, `decode` should take a (valid) `Mnemonic` as input and can't modify it (leak it into stack/heap memory). `ms` local variable should be zeroized.
8. `separator` in `decode` should already be normalized/valid in `Wordlist`; no need to normalize it all the time.
9. Why `separator` is `&str`? Why not `char`? Is it correct to accept different spaces (tabs, space, invisible space, etc.) in one mnemonic?
10. In `decode` there's no need for `sub_whole_byte_case` function and multiple calls to it. Just compute the last argument once and run the function once.
_Originally posted by @semenov-vladyslav in https://github.com/iotaledger/crypto.rs/issues/197#issuecomment-1562917117_
Contributor guide
Assessment
This issue has not been assessed yet.