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.

Sign up for GitHub

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

Validate mnemonic whitespace #59

Merged
merged 3 commits into from
Jul 11, 2023
Merged

Validate mnemonic whitespace #59

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d165953
Validate mnemonic whitespace
lrettig Jul 11, 2023
d77d5e2
Add regression test
lrettig Jul 11, 2023
e1eb04b
Make linter happy smh
lrettig Jul 11, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions wallet/wallet.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,12 +5,15 @@ import (
"encoding/hex"
"encoding/json"
"fmt"
"strings"

"github.com/tyler-smith/go-bip39"

"github.com/spacemeshos/smcli/common"
)

var errWhitespace = fmt.Errorf("whitespace violation in mnemonic phrase")

// Wallet is the basic data structure.
type Wallet struct {
// keystore string
Expand Down Expand Up @@ -91,11 +94,20 @@ func NewMultiWalletFromMnemonic(m string, n int) (*Wallet, error) {
if n < 0 || n > common.MaxAccountsPerWallet {
return nil, fmt.Errorf("invalid number of accounts")
}

// bip39 lib doesn't properly validate whitespace so we have to do that manually.
if expected := strings.Join(strings.Fields(m), " "); m != expected {
return nil, errWhitespace
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not simply normalize the input instead? returning the error would be confusing for user
they then have to visually find where the extra white space is?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMHO this is less error-prone. if there's extra whitespace, then the user did something wrong, likely a copy and paste error. i think proceeding to generate a key in spite of this is a bad idea. the user should be aware of the error and should correct it. the BIP39 spec, such as it is, does not allow extra whitespace.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but then if there are sure of it (extra whitespace in btwn phrases), they will never be able to use this phrase.

should print the exact diff to user and prompt them to confirm then.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in the interest of time, instead of creating more code, maybe just ask user to use the phrase smcli created

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@countvonzero I think this code should be as simple as possible. I don't think it's the responsibility of the wallet app to correct the user's mistake. At most we could link them to a doc that explains the issue and how to correct it, something along those lines.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

then the phrase is wrong and they should use one that's valid. and they should be made aware that it's wrong.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in any case. otherwise code lg

Copy link
Member

@fasmat fasmat Jul 11, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have a related but different question. Whitespace is more than just spaces, does bip39 handle things like \r, \n and \t correctly?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@fasmat all whitespace is treated the same way. this change will complain about \r, \n, and \t as well, see https://pkg.go.dev/strings#Fields. the regression test checks things other than spaces. the only thing that's permitted is a single space between words.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, didn't see the other whitespace characters in the test. Then LGTM

}

// this checks the number of words and the checksum.
if !bip39.IsMnemonicValid(m) {
return nil, fmt.Errorf("invalid mnemonic")
}

// TODO: add option for user to provide passphrase
// https://github.com/spacemeshos/smcli/issues/18

seed := bip39.NewSeed(m, "")
masterKeyPair, err := NewMasterKeyPair(seed)
if err != nil {
Expand Down
16 changes: 16 additions & 0 deletions wallet/wallet_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,3 +101,19 @@ func TestKeysInWalletMaintainExpectedPath(t *testing.T) {
require.Equal(t, expectedPath, HDPathToString(path))
}
}

func TestMnemonicWhitespace(t *testing.T) {
mnemonics := []string{
"film theme cheese broken kingdom destroy inch ready wear inspire shove pudding",
"film theme cheese broken kingdom destroy inch ready wear inspire shove pudding",
"film theme cheese broken kingdom destroy inch ready wear\ninspire shove pudding",
"film theme cheese broken kingdom destroy inch ready wear inspire shove pudding\t",
" film theme cheese broken kingdom destroy inch ready wear inspire shove pudding",
"film theme cheese broken kingdom destroy inch ready wear inspire shove pudding ",
"film theme cheese broken kingdom destroy inch ready wear inspire shove pudding",
}
for _, m := range mnemonics {
_, err := NewMultiWalletFromMnemonic(m, 1)
require.Equal(t, errWhitespace, err, "expected whitespace error in mnemonic")
}
}