From dde9e31e50c913f5fa56ff1e405dd442dd12e452 Mon Sep 17 00:00:00 2001 From: Devrandom Date: Tue, 1 Sep 2020 13:15:53 +0200 Subject: [PATCH] hdkeychain: correct BIP-32 derivation issue fixes issue #172 --- hdkeychain/bench_test.go | 4 +- hdkeychain/doc.go | 8 ++-- hdkeychain/example_test.go | 10 ++-- hdkeychain/extendedkey.go | 88 ++++++++++++++++++++++++++++++++-- hdkeychain/extendedkey_test.go | 75 ++++++++++++++++++++++++----- 5 files changed, 157 insertions(+), 28 deletions(-) diff --git a/hdkeychain/bench_test.go b/hdkeychain/bench_test.go index dde59b1..583eef0 100644 --- a/hdkeychain/bench_test.go +++ b/hdkeychain/bench_test.go @@ -26,7 +26,7 @@ func BenchmarkDeriveHardened(b *testing.B) { b.StartTimer() for i := 0; i < b.N; i++ { - masterKey.Child(hdkeychain.HardenedKeyStart) + masterKey.Derive(hdkeychain.HardenedKeyStart) } } @@ -41,7 +41,7 @@ func BenchmarkDeriveNormal(b *testing.B) { b.StartTimer() for i := 0; i < b.N; i++ { - masterKey.Child(0) + masterKey.Derive(0) } } diff --git a/hdkeychain/doc.go b/hdkeychain/doc.go index 652eb8c..dcf74f6 100644 --- a/hdkeychain/doc.go +++ b/hdkeychain/doc.go @@ -39,14 +39,14 @@ create a random seed for use with the NewMaster function. Deriving Children Once you have created a tree root (or have deserialized an extended key as -discussed later), the child extended keys can be derived by using the Child -function. The Child function supports deriving both normal (non-hardened) and +discussed later), the child extended keys can be derived by using the Derive +function. The Derive function supports deriving both normal (non-hardened) and hardened child extended keys. In order to derive a hardened extended key, use the HardenedKeyStart constant + the hardened key number as the index to the -Child function. This provides the ability to cascade the keys into a tree and +Derive function. This provides the ability to cascade the keys into a tree and hence generate the hierarchical deterministic key chains. -Normal vs Hardened Child Extended Keys +Normal vs Hardened Derived Extended Keys A private extended key can be used to derive both hardened and non-hardened (normal) child private and public extended keys. A public extended key can only diff --git a/hdkeychain/example_test.go b/hdkeychain/example_test.go index 63d369f..e7f462d 100644 --- a/hdkeychain/example_test.go +++ b/hdkeychain/example_test.go @@ -71,7 +71,7 @@ func Example_defaultWalletLayout() { // Derive the extended key for account 0. This gives the path: // m/0H - acct0, err := masterKey.Child(hdkeychain.HardenedKeyStart + 0) + acct0, err := masterKey.Derive(hdkeychain.HardenedKeyStart + 0) if err != nil { fmt.Println(err) return @@ -80,7 +80,7 @@ func Example_defaultWalletLayout() { // Derive the extended key for the account 0 external chain. This // gives the path: // m/0H/0 - acct0Ext, err := acct0.Child(0) + acct0Ext, err := acct0.Derive(0) if err != nil { fmt.Println(err) return @@ -89,7 +89,7 @@ func Example_defaultWalletLayout() { // Derive the extended key for the account 0 internal chain. This gives // the path: // m/0H/1 - acct0Int, err := acct0.Child(1) + acct0Int, err := acct0.Derive(1) if err != nil { fmt.Println(err) return @@ -101,7 +101,7 @@ func Example_defaultWalletLayout() { // Derive the 10th extended key for the account 0 external chain. This // gives the path: // m/0H/0/10 - acct0Ext10, err := acct0Ext.Child(10) + acct0Ext10, err := acct0Ext.Derive(10) if err != nil { fmt.Println(err) return @@ -110,7 +110,7 @@ func Example_defaultWalletLayout() { // Derive the 1st extended key for the account 0 internal chain. This // gives the path: // m/0H/1/0 - acct0Int0, err := acct0Int.Child(0) + acct0Int0, err := acct0Int.Derive(0) if err != nil { fmt.Println(err) return diff --git a/hdkeychain/extendedkey.go b/hdkeychain/extendedkey.go index 3fe51c1..a0c5e7b 100644 --- a/hdkeychain/extendedkey.go +++ b/hdkeychain/extendedkey.go @@ -120,7 +120,7 @@ type ExtendedKey struct { // fields. No error checking is performed here as it's only intended to be a // convenience method used to create a populated struct. This function should // only be used by applications that need to create custom ExtendedKeys. All -// other applications should just use NewMaster, Child, or Neuter. +// other applications should just use NewMaster, Derive, or Neuter. func NewExtendedKey(version, key, chainCode, parentFP []byte, depth uint8, childNum uint32, isPrivate bool) *ExtendedKey { @@ -198,8 +198,15 @@ func (k *ExtendedKey) ChainCode() []byte { return append([]byte{}, k.chainCode...) } -// Child returns a derived child extended key at the given index. When this -// extended key is a private extended key (as determined by the IsPrivate +// Derive returns a derived child extended key at the given index. +// +// IMPORTANT: if you were previously using the Child method, this method is incompatible. +// The Child method had a BIP-32 standard compatibility issue. You have to check whether +// any hardened derivations in your derivation path are affected by this issue, via the +// IsAffectedByIssue172 method and migrate the wallet if so. This method does conform +// to the standard. If you need the old behavior, use DeriveNonStandard. +// +// When this extended key is a private extended key (as determined by the IsPrivate // function), a private extended key will be derived. Otherwise, the derived // extended key will be also be a public extended key. // @@ -219,7 +226,7 @@ func (k *ExtendedKey) ChainCode() []byte { // index does not derive to a usable child. The ErrInvalidChild error will be // returned if this should occur, and the caller is expected to ignore the // invalid child and simply increment to the next index. -func (k *ExtendedKey) Child(i uint32) (*ExtendedKey, error) { +func (k *ExtendedKey) Derive(i uint32) (*ExtendedKey, error) { // Prevent derivation of children beyond the max allowed depth. if k.depth == maxUint8 { return nil, ErrDeriveBeyondMaxDepth @@ -254,7 +261,9 @@ func (k *ExtendedKey) Child(i uint32) (*ExtendedKey, error) { // When the child is a hardened child, the key is known to be a // private key due to the above early return. Pad it with a // leading zero as required by [BIP32] for deriving the child. - copy(data[1:], k.key) + // Additionally, right align it if it's shorter than 32 bytes. + offset := 33 - len(k.key) + copy(data[offset:], k.key) } else { // Case #2 or #3. // This is either a public or private extended key, but in @@ -342,6 +351,75 @@ func (k *ExtendedKey) Child(i uint32) (*ExtendedKey, error) { k.depth+1, i, isPrivate), nil } +// Returns true if this key was affected by the BIP-32 issue in the Child +// method (since renamed to DeriveNonStandard). +func (k *ExtendedKey) IsAffectedByIssue172() bool { + return len(k.key) < 32 +} + +// Deprecated: This is a non-standard derivation that is affected by issue #172. +// 1-of-256 hardened derivations will be wrong. See note in the Derive method +// and IsAffectedByIssue172. +func (k *ExtendedKey) DeriveNonStandard(i uint32) (*ExtendedKey, error) { + if k.depth == maxUint8 { + return nil, ErrDeriveBeyondMaxDepth + } + + isChildHardened := i >= HardenedKeyStart + if !k.isPrivate && isChildHardened { + return nil, ErrDeriveHardFromPublic + } + + keyLen := 33 + data := make([]byte, keyLen+4) + if isChildHardened { + copy(data[1:], k.key) + } else { + copy(data, k.pubKeyBytes()) + } + binary.BigEndian.PutUint32(data[keyLen:], i) + + hmac512 := hmac.New(sha512.New, k.chainCode) + hmac512.Write(data) + ilr := hmac512.Sum(nil) + + il := ilr[:len(ilr)/2] + childChainCode := ilr[len(ilr)/2:] + + ilNum := new(big.Int).SetBytes(il) + if ilNum.Cmp(btcec.S256().N) >= 0 || ilNum.Sign() == 0 { + return nil, ErrInvalidChild + } + + var isPrivate bool + var childKey []byte + if k.isPrivate { + keyNum := new(big.Int).SetBytes(k.key) + ilNum.Add(ilNum, keyNum) + ilNum.Mod(ilNum, btcec.S256().N) + childKey = ilNum.Bytes() + isPrivate = true + } else { + ilx, ily := btcec.S256().ScalarBaseMult(il) + if ilx.Sign() == 0 || ily.Sign() == 0 { + return nil, ErrInvalidChild + } + + pubKey, err := btcec.ParsePubKey(k.key, btcec.S256()) + if err != nil { + return nil, err + } + + childX, childY := btcec.S256().Add(ilx, ily, pubKey.X, pubKey.Y) + pk := btcec.PublicKey{Curve: btcec.S256(), X: childX, Y: childY} + childKey = pk.SerializeCompressed() + } + + parentFP := btcutil.Hash160(k.pubKeyBytes())[:4] + return NewExtendedKey(k.version, childKey, childChainCode, parentFP, + k.depth+1, i, isPrivate), nil +} + // ChildNum returns the index at which the child extended key was derived. // // Extended keys with ChildNum value between 0 and 2^31-1 are normal child diff --git a/hdkeychain/extendedkey_test.go b/hdkeychain/extendedkey_test.go index 868b840..37a42fa 100644 --- a/hdkeychain/extendedkey_test.go +++ b/hdkeychain/extendedkey_test.go @@ -10,6 +10,7 @@ package hdkeychain import ( "bytes" + "encoding/binary" "encoding/hex" "errors" "math" @@ -224,7 +225,7 @@ tests: for _, childNum := range test.path { var err error - extKey, err = extKey.Child(childNum) + extKey, err = extKey.Derive(childNum) if err != nil { t.Errorf("err: %v", err) continue tests @@ -381,7 +382,7 @@ tests: for _, childNum := range test.path { var err error - extKey, err = extKey.Child(childNum) + extKey, err = extKey.Derive(childNum) if err != nil { t.Errorf("err: %v", err) continue tests @@ -390,7 +391,7 @@ tests: privStr := extKey.String() if privStr != test.wantPriv { - t.Errorf("Child #%d (%s): mismatched serialized "+ + t.Errorf("Derive #%d (%s): mismatched serialized "+ "private extended key -- got: %s, want: %s", i, test.name, privStr, test.wantPriv) continue @@ -500,7 +501,7 @@ tests: for _, childNum := range test.path { var err error - extKey, err = extKey.Child(childNum) + extKey, err = extKey.Derive(childNum) if err != nil { t.Errorf("err: %v", err) continue tests @@ -509,7 +510,7 @@ tests: pubStr := extKey.String() if pubStr != test.wantPub { - t.Errorf("Child #%d (%s): mismatched serialized "+ + t.Errorf("Derive #%d (%s): mismatched serialized "+ "public extended key -- got: %s, want: %s", i, test.name, pubStr, test.wantPub) continue @@ -850,9 +851,9 @@ func TestErrors(t *testing.T) { } // Deriving a hardened child extended key should fail from a public key. - _, err = pubKey.Child(HardenedKeyStart) + _, err = pubKey.Derive(HardenedKeyStart) if err != ErrDeriveHardFromPublic { - t.Fatalf("Child: mismatched error -- got: %v, want: %v", + t.Fatalf("Derive: mismatched error -- got: %v, want: %v", err, ErrDeriveHardFromPublic) } @@ -1072,20 +1073,20 @@ func TestMaximumDepth(t *testing.T) { t.Fatalf("extendedkey depth %d should match expected value %d", extKey.Depth(), i) } - newKey, err := extKey.Child(1) + newKey, err := extKey.Derive(1) if err != nil { - t.Fatalf("Child: unexpected error: %v", err) + t.Fatalf("Derive: unexpected error: %v", err) } extKey = newKey } - noKey, err := extKey.Child(1) + noKey, err := extKey.Derive(1) if err != ErrDeriveBeyondMaxDepth { - t.Fatalf("Child: mismatched error: want %v, got %v", + t.Fatalf("Derive: mismatched error: want %v, got %v", ErrDeriveBeyondMaxDepth, err) } if noKey != nil { - t.Fatal("Child: deriving 256th key should not succeed") + t.Fatal("Derive: deriving 256th key should not succeed") } } @@ -1156,3 +1157,53 @@ func TestCloneWithVersion(t *testing.T) { } } } + +// TestLeadingZero ensures that deriving children from keys with a leading zero byte is done according +// to the BIP-32 standard and that the legacy method generates a backwards-compatible result. +func TestLeadingZero(t *testing.T) { + // The 400th seed results in a m/0' public key with a leading zero, allowing us to test + // the desired behavior. + ii := 399 + seed := make([]byte, 32) + binary.BigEndian.PutUint32(seed[28:], uint32(ii)) + masterKey, err := NewMaster(seed, &chaincfg.MainNetParams) + if err != nil { + t.Fatalf("hdkeychain.NewMaster failed: %v", err) + } + child0, err := masterKey.Derive(0 + HardenedKeyStart) + if err != nil { + t.Fatalf("masterKey.Derive failed: %v", err) + } + if !child0.IsAffectedByIssue172() { + t.Fatal("expected child0 to be affected by issue 172") + } + child1, err := child0.Derive(0 + HardenedKeyStart) + if err != nil { + t.Fatalf("child0.Derive failed: %v", err) + } + if child1.IsAffectedByIssue172() { + t.Fatal("did not expect child1 to be affected by issue 172") + } + + child1nonstandard, err := child0.DeriveNonStandard(0 + HardenedKeyStart) + if err != nil { + t.Fatalf("child0.DeriveNonStandard failed: %v", err) + } + + // This is the correct result based on BIP32 + if hex.EncodeToString(child1.key) != "a9b6b30a5b90b56ed48728c73af1d8a7ef1e9cc372ec21afcc1d9bdf269b0988" { + t.Error("incorrect standard BIP32 derivation") + } + + if hex.EncodeToString(child1nonstandard.key) != "ea46d8f58eb863a2d371a938396af8b0babe85c01920f59a8044412e70e837ee" { + t.Error("incorrect btcutil backwards compatible BIP32-like derivation") + } + + if !child0.IsAffectedByIssue172() { + t.Error("child 0 should be affected by issue 172") + } + + if child1.IsAffectedByIssue172() { + t.Error("child 1 should not be affected by issue 172") + } +}