Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve BIP39 implementation #198

Open
thibault-martinez opened this issue May 25, 2023 · 1 comment
Open

Improve BIP39 implementation #198

thibault-martinez opened this issue May 25, 2023 · 1 comment

Comments

@thibault-martinez
Copy link
Member

          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 #197 (comment)

@WesleyBatista
Copy link

I was wondering if it would be worth it to borrow ideas, implementation and/or tests from https://github.com/rust-bitcoin/rust-bip39/

Excuse my inexperience with Rust. This is probably is not trivial...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants