iotaledger / crypto.rs

The canonical source of cryptographic ground-truth for IOTA projects that use Rust.
https://iota.org
65 stars 19 forks source link

Improve BIP39 implementation #198

Open thibault-martinez opened 1 year ago

thibault-martinez commented 1 year ago
          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

WesleyBatista commented 5 months ago

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...