Skip to content
This repository has been archived by the owner on May 13, 2022. It is now read-only.

sanity check length of recovery seed #192

Merged

Conversation

AdamISZ
Copy link
Member

@AdamISZ AdamISZ commented Aug 15, 2015

The issue that this addresses starts in #190 and continues/finishes in the reddit thread.

The fix is trivial, just checking that the seed phrase entered is 12 words. While doing it, I noticed that .split(' ') has the unfortunate effect of treating extra spaces as extra words (so 'a b c'.split(' ') comes out as ['a',b','','c'] which is undesirable if nothing else because it gives a misleading error). So I've edited it to .split() because this will split on any amount of whitespace, which is a more expected behaviour.

A perhaps more 'meta' conclusion from all this is to look again at sanity checking user input generally, and don't assume that used libraries (slowaes, old_mnemonic) are doing sanity checks at the lower level. I personally had never even looked at that code (even slowaes, although I used it before I never tried to use the high level 'encryptData' which does the padding for you).

@AdamISZ
Copy link
Member Author

AdamISZ commented Aug 15, 2015

Oh, and I forgot to mention that the bug is more serious than just "you get a null seed if you press Enter".

If the word list passed to mn_decode is length 0 or 1 or 2, the function will always return '', no matter what those words are. This creates at least one more vector for coin loss: accidentally entering your password instead of the 12 word seed phrase. Even though the password is not in the mnemonic word list, this will not get checked because of the use of range(len(wlist)/3).

chris-belcher added a commit that referenced this pull request Aug 15, 2015
sanity check length of recovery seed
@chris-belcher chris-belcher merged commit ef41e02 into JoinMarket-Org:master Aug 15, 2015
@AdamISZ AdamISZ deleted the recoveryseedsanitycheck branch August 15, 2015 12:26
ghtdak pushed a commit to ghtdak/joinmarket that referenced this pull request Oct 1, 2015
…check [yapf]

sanity check length of recovery seed
ghtdak pushed a commit to ghtdak/joinmarket that referenced this pull request Oct 4, 2015
…check

sanity check length of recovery seed
ghtdak pushed a commit to ghtdak/joinmarket that referenced this pull request Dec 4, 2015
…check

sanity check length of recovery seed
[gitreformat yapf-ify (github/ghtdak) on Fri Dec  4 04:51:01 2015]
[from commit: ef41e02]
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants