From 5a5328afa6abcda4040ae28a456aa27dd8bbae5b Mon Sep 17 00:00:00 2001 From: Andrea Franz Date: Tue, 7 Aug 2018 01:39:09 +0200 Subject: [PATCH 1/5] add bytes padding in derived keys --- extkeys/hdkey.go | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/extkeys/hdkey.go b/extkeys/hdkey.go index 8412e8a132..cff6603fc2 100644 --- a/extkeys/hdkey.go +++ b/extkeys/hdkey.go @@ -194,7 +194,13 @@ func (k *ExtendedKey) Child(i uint32) (*ExtendedKey, error) { keyBigInt.Add(keyBigInt, parentKeyBigInt) keyBigInt.Mod(keyBigInt, btcec.S256().N) - child.KeyData = keyBigInt.Bytes() + keyData := keyBigInt.Bytes() + if len(keyData) < 32 { + extra := make([]byte, 32-len(keyData)) + keyData = append(extra, keyData...) + } + + child.KeyData = keyData child.Version = PrivateKeyVersion } else { // Case #3: childKey = serP(point(parse256(IL)) + parentKey) From 19d1bede9879566f339926c2cf5ec4a8b6e86ff4 Mon Sep 17 00:00:00 2001 From: Andrea Franz Date: Thu, 9 Aug 2018 17:39:32 +0200 Subject: [PATCH 2/5] add 3rd test vector taken from BIP32 specs --- extkeys/hdkey_test.go | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/extkeys/hdkey_test.go b/extkeys/hdkey_test.go index f5ccda6edd..2991ef9c3a 100644 --- a/extkeys/hdkey_test.go +++ b/extkeys/hdkey_test.go @@ -111,6 +111,21 @@ func TestBIP32Vectors(t *testing.T) { "xpub6FnCn6nSzZAw5Tw7cgR9bi15UV96gLZhjDstkXXxvCLsUXBGXPdSnLFbdpq8p9HmGsApME5hQTZ3emM2rnY5agb9rXpVGyy3bdW6EEgAtqt", "xprvA2nrNbFZABcdryreWet9Ea4LvTJcGsqrMzxHx98MMrotbir7yrKCEXw7nadnHM8Dq38EGfSh6dqA9QWTyefMLEcBYJUuekgW4BYPJcr9E7j", }, + // Test vector 3 + { + "test vector 3 chain m", + "4b381541583be4423346c643850da4b320e46a87ae3d2a4e6da11eba819cd4acba45d239319ac14f863b8d5ab5a0d0c64d2e8a1e7d1457df2e5a3c51c73235be", + []uint32{}, + "xpub661MyMwAqRbcEZVB4dScxMAdx6d4nFc9nvyvH3v4gJL378CSRZiYmhRoP7mBy6gSPSCYk6SzXPTf3ND1cZAceL7SfJ1Z3GC8vBgp2epUt13", + "xprv9s21ZrQH143K25QhxbucbDDuQ4naNntJRi4KUfWT7xo4EKsHt2QJDu7KXp1A3u7Bi1j8ph3EGsZ9Xvz9dGuVrtHHs7pXeTzjuxBrCmmhgC6", + }, + { + "test vector 3 chain m/0H", + "4b381541583be4423346c643850da4b320e46a87ae3d2a4e6da11eba819cd4acba45d239319ac14f863b8d5ab5a0d0c64d2e8a1e7d1457df2e5a3c51c73235be", + []uint32{HardenedKeyStart}, + "xpub68NZiKmJWnxxS6aaHmn81bvJeTESw724CRDs6HbuccFQN9Ku14VQrADWgqbhhTHBaohPX4CjNLf9fq9MYo6oDaPPLPxSb7gwQN3ih19Zm4Y", + "xprv9uPDJpEQgRQfDcW7BkF7eTya6RPxXeJCqCJGHuCJ4GiRVLzkTXBAJMu2qaMWPrS7AANYqdq6vcBcBUdJCVVFceUvJFjaPdGZ2y9WACViL4L", + }, } tests: From ac3e8b0a77ef3ee2fa4ad47d9dd4b8d588e6a165 Mon Sep 17 00:00:00 2001 From: Andrea Franz Date: Thu, 9 Aug 2018 17:41:09 +0200 Subject: [PATCH 3/5] test specific case where keyData is less then 32 bytes --- extkeys/hdkey_test.go | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/extkeys/hdkey_test.go b/extkeys/hdkey_test.go index 2991ef9c3a..06e39f6024 100644 --- a/extkeys/hdkey_test.go +++ b/extkeys/hdkey_test.go @@ -637,6 +637,34 @@ func TestHDWalletCompatibility(t *testing.T) { } } +func TestPrivateKeyDataWithLeadingZeros(t *testing.T) { + mn := NewMnemonic() + words := "radar blur cabbage chef fix engine embark joy scheme fiction master release" + key, _ := NewMaster(mn.MnemonicSeed(words, "")) + + path := []uint32{ + HardenedKeyStart + 44, // purpose + HardenedKeyStart + 60, // cointype + HardenedKeyStart + 0, // account + 0, // change + 0, // index + } + + for _, part := range path { + key, _ = key.Child(part) + if length := len(key.KeyData); length != 32 { + t.Errorf("expected key length to be 32, got: %d", length) + } + } + + expectedAddress := "0xaC39b311DCEb2A4b2f5d8461c1cdaF756F4F7Ae9" + address := crypto.PubkeyToAddress(key.ToECDSA().PublicKey).Hex() + + if address != expectedAddress { + t.Errorf("expected address %s, got: %s", expectedAddress, address) + } +} + //func TestNewKey(t *testing.T) { // mnemonic := NewMnemonic() // From a55d38735ec800841c126776ffb2301e74ba7c92 Mon Sep 17 00:00:00 2001 From: Andrea Franz Date: Thu, 9 Aug 2018 17:43:18 +0200 Subject: [PATCH 4/5] add comment on key data padding snippet --- extkeys/hdkey.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/extkeys/hdkey.go b/extkeys/hdkey.go index cff6603fc2..3e40831f8d 100644 --- a/extkeys/hdkey.go +++ b/extkeys/hdkey.go @@ -195,6 +195,8 @@ func (k *ExtendedKey) Child(i uint32) (*ExtendedKey, error) { keyBigInt.Mod(keyBigInt, btcec.S256().N) keyData := keyBigInt.Bytes() + // make sure that KeyData is 32 bytes of data even if + // the value is represented with less bytes if len(keyData) < 32 { extra := make([]byte, 32-len(keyData)) keyData = append(extra, keyData...) From edd3416837e4e8b1c083ba88773c91691310f83f Mon Sep 17 00:00:00 2001 From: Andrea Franz Date: Fri, 10 Aug 2018 17:32:10 +0200 Subject: [PATCH 5/5] add more docs comments --- extkeys/hdkey.go | 11 +++++++++-- extkeys/hdkey_test.go | 18 ++++++++++++++++++ 2 files changed, 27 insertions(+), 2 deletions(-) diff --git a/extkeys/hdkey.go b/extkeys/hdkey.go index 3e40831f8d..bf66442aa7 100644 --- a/extkeys/hdkey.go +++ b/extkeys/hdkey.go @@ -194,9 +194,16 @@ func (k *ExtendedKey) Child(i uint32) (*ExtendedKey, error) { keyBigInt.Add(keyBigInt, parentKeyBigInt) keyBigInt.Mod(keyBigInt, btcec.S256().N) + // Make sure that child.KeyData is 32 bytes of data even if the value is represented with less bytes. + // When we derive a child of this key, we call splitHMAC that does a sha512 of a seed that is: + // - 1 byte with 0x00 + // - 32 bytes for the key data + // - 4 bytes for the child key index + // If we don't padd the KeyData, it will be shifted to left in that 32 bytes space + // generating a different seed and different child key. + // This part fixes a bug we had previously and described at: + // https://medium.com/@alexberegszaszi/why-do-my-bip32-wallets-disagree-6f3254cc5846#.86inuifuq keyData := keyBigInt.Bytes() - // make sure that KeyData is 32 bytes of data even if - // the value is represented with less bytes if len(keyData) < 32 { extra := make([]byte, 32-len(keyData)) keyData = append(extra, keyData...) diff --git a/extkeys/hdkey_test.go b/extkeys/hdkey_test.go index 06e39f6024..11aed96a6f 100644 --- a/extkeys/hdkey_test.go +++ b/extkeys/hdkey_test.go @@ -18,6 +18,8 @@ const ( ) func TestBIP32Vectors(t *testing.T) { + // Test vectors 1, 2, and 3 are taken from the BIP32 specs: + // https://github.com/bitcoin/bips/blob/master/bip-0032.mediawiki#test-vectors tests := []struct { name string seed string @@ -637,6 +639,22 @@ func TestHDWalletCompatibility(t *testing.T) { } } +// TestPrivateKeyDataWithLeadingZeros is a regression test that checks +// we don't re-introduce a bug we had in the past. +// For a specific mnemonic phrase, we were deriving a wrong key/address +// at path m/44'/60'/0'/0/0 compared to other wallets. +// In this specific case, the second child key is represented in 31 bytes. +// The problem raises when deriving its child key. +// One of the step to derive the child key is calling our splitHMAC +// that returns a secretKey and a chainCode. +// Inside this function we make a sha512 of a seed that is a 37 bytes with: +// 1 byte with 0x00 +// 32 bytes for the key data +// 4 bytes for the child key index +// In our case, if the key was less then 32 bytes, it was shifted to the left of that 32 bytes space, +// resulting in a different seed, and a different data returned from the sha512 call. +// https://medium.com/@alexberegszaszi/why-do-my-bip32-wallets-disagree-6f3254cc5846#.86inuifuq +// https://github.com/iancoleman/bip39/issues/58 func TestPrivateKeyDataWithLeadingZeros(t *testing.T) { mn := NewMnemonic() words := "radar blur cabbage chef fix engine embark joy scheme fiction master release"