iotaledger / iotaledger/crypto.rs

Improve BIP39 implementation

Open
#198 1 comment 1 reaction 0 assignees View on GitHub
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

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.