infincia / bip39-rs

A Rust implementation of BIP-0039
Apache License 2.0
54 stars 61 forks source link

word phrases typed by people might have extra whitespace, tolerate this #24

Closed rob-solana closed 4 years ago

rob-solana commented 4 years ago

fixes #23

rob-solana commented 4 years ago

CC @maciejhirsz

maciejhirsz commented 4 years ago

@rob-solana this is tricky, because the phrase itself, not the entropy, is what is being hashed, so if you validate a phrase with extra spaces, you will produce an invalid seed for that phrase, which is way worse than rejecting the phrase due to extra spaces. This could be doubly problematic if you handled other whitespace characters (tab, feed, or new line).

IMO a better way to handle this would be to return a specialized error if the phrase contains extra spaces. The Mnemonic::from_phrase method could then handle that error and try to sanitize the string before retrying.

rob-solana commented 4 years ago

@rob-solana this is tricky, because the phrase itself, not the entropy, is what is being hashed, so if you validate a phrase with extra spaces, you will produce an invalid seed for that phrase, which is way worse than rejecting the phrase due to extra spaces. This could be doubly problematic if you handled other whitespace characters (tab, feed, or new line).

IMO a better way to handle this would be to return a specialized error if the phrase contains extra spaces. The Mnemonic::from_phrase method could then handle that error and try to sanitize the string before retrying.

Thanks for taking time to review. Can you help me understand this better? I don't see hashing of the phrase itself, only the bits resulting from the individual words parsed from the phrase.

maciejhirsz commented 4 years ago

@rob-solana that's a bug that's been fixed long ago but hasn't been merged.

rob-solana commented 4 years ago

@maciejhirsz perhaps this PR should be against your fork?

maciejhirsz commented 4 years ago

@rob-solana Sure, if you want, I'm happy to include it as long as it doesn't break the seed generation :).