Fix: Foundation for the correction of BIP-32 incompatability issue.

btcutil/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)
 	}
 }
 

btcutil/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

btcutil/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

btcutil/hdkeychain/extendedkey.go

@@ -144,7 +144,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 by 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 {
 
@@ -209,8 +209,15 @@ func (k *ExtendedKey) ParentFingerprint() uint32 {
 	return binary.BigEndian.Uint32(k.parentFP)
 }
 
-// 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.
 //
@@ -230,7 +237,7 @@ func (k *ExtendedKey) ParentFingerprint() uint32 {
 // 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, er.R) {
+func (k *ExtendedKey) Derive(i uint32) (*ExtendedKey, er.R) {
 	// Prevent derivation of children beyond the max allowed depth.
 	if k.depth == maxUint8 {
 		return nil, ErrDeriveBeyondMaxDepth.Default()
@@ -250,6 +257,140 @@ func (k *ExtendedKey) Child(i uint32) (*ExtendedKey, er.R) {
 		return nil, ErrDeriveHardFromPublic.Default()
 	}
 
+	// The data used to derive the child key depends on whether or not the
+	// child is hardened per [BIP32].
+	//
+	// For hardened children:
+	//   0x00 || ser256(parentKey) || ser32(i)
+	//
+	// For normal children:
+	//   serP(parentPubKey) || ser32(i)
+	keyLen := 33
+	data := make([]byte, keyLen+4)
+	if isChildHardened {
+		// Case #1.
+		// 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.
+		// 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
+		// either case, the data which is used to derive the child key
+		// starts with the secp256k1 compressed public key bytes.
+		copy(data, k.pubKeyBytes())
+	}
+	binary.BigEndian.PutUint32(data[keyLen:], i)
+
+	// Take the HMAC-SHA512 of the current key's chain code and the derived
+	// data:
+	//   I = HMAC-SHA512(Key = chainCode, Data = data)
+	hmac512 := hmac.New(sha512.New, k.chainCode)
+	hmac512.Write(data)
+	ilr := hmac512.Sum(nil)
+
+	// Split "I" into two 32-byte sequences Il and Ir where:
+	//   Il = intermediate key used to derive the child
+	//   Ir = child chain code
+	il := ilr[:len(ilr)/2]
+	childChainCode := ilr[len(ilr)/2:]
+
+	// Both derived public or private keys rely on treating the left 32-byte
+	// sequence calculated above (Il) as a 256-bit integer that must be
+	// within the valid range for a secp256k1 private key.  There is a small
+	// chance (< 1 in 2^127) this condition will not hold, and in that case,
+	// a child extended key can't be created for this index and the caller
+	// should simply increment to the next index.
+	ilNum := new(big.Int).SetBytes(il)
+	if ilNum.Cmp(btcec.S256().N) >= 0 || ilNum.Sign() == 0 {
+		return nil, ErrInvalidChild.Default()
+	}
+
+	// The algorithm used to derive the child key depends on whether or not
+	// a private or public child is being derived.
+	//
+	// For private children:
+	//   childKey = parse256(Il) + parentKey
+	//
+	// For public children:
+	//   childKey = serP(point(parse256(Il)) + parentKey)
+	var isPrivate bool
+	var childKey []byte
+	if k.isPrivate {
+		// Case #1 or #2.
+		// Add the parent private key to the intermediate private key to
+		// derive the final child key.
+		//
+		// childKey = parse256(Il) + parenKey
+		keyNum := new(big.Int).SetBytes(k.key)
+		ilNum.Add(ilNum, keyNum)
+		ilNum.Mod(ilNum, btcec.S256().N)
+		childKey = ilNum.Bytes()
+		isPrivate = true
+	} else {
+		// Case #3.
+		// Calculate the corresponding intermediate public key for
+		// intermediate private key.
+		ilx, ily := btcec.S256().ScalarBaseMult(il)
+		if ilx.Sign() == 0 || ily.Sign() == 0 {
+			return nil, ErrInvalidChild.Default()
+		}
+
+		// Convert the serialized compressed parent public key into X
+		// and Y coordinates so it can be added to the intermediate
+		// public key.
+		pubKey, err := btcec.ParsePubKey(k.key, btcec.S256())
+		if err != nil {
+			return nil, err
+		}
+
+		// Add the intermediate public key to the parent public key to
+		// derive the final child key.
+		//
+		// childKey = serP(point(parse256(Il)) + parentKey)
+		childX, childY := btcec.S256().Add(ilx, ily, pubKey.X, pubKey.Y)
+		pk := btcec.PublicKey{Curve: btcec.S256(), X: childX, Y: childY}
+		childKey = pk.SerializeCompressed()
+	}
+
+	// The fingerprint of the parent for the derived child is the first 4
+	// bytes of the RIPEMD160(SHA256(parentPubKey)).
+	parentFP := btcutil.Hash160(k.pubKeyBytes())[:4]
+	return NewExtendedKey(k.version, childKey, childChainCode, parentFP,
+		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, er.R) {
+		// Prevent derivation of children beyond the max allowed depth.
+	if k.depth == maxUint8 {
+		return nil, ErrDeriveBeyondMaxDepth.Default()
+	}
+
+	// There are four scenarios that could happen here:
+	// 1) Private extended key -> Hardened child private extended key
+	// 2) Private extended key -> Non-hardened child private extended key
+	// 3) Public extended key -> Non-hardened child public extended key
+	// 4) Public extended key -> Hardened child public extended key (INVALID!)
+
+	// Case #4 is invalid, so error out early.
+	// A hardened child extended key may not be created from a public
+	// extended key.
+	isChildHardened := i >= HardenedKeyStart
+	if !k.isPrivate && isChildHardened {
+		return nil, ErrDeriveHardFromPublic.Default()
+	}
+
 	// The data used to derive the child key depends on whether or not the
 	// child is hardened per [BIP32].
 	//

btcutil/hdkeychain/extendedkey_test.go

@@ -10,6 +10,7 @@ package hdkeychain
 
 import (
 	"bytes"
+	"encoding/binary"
 	"encoding/hex"
 	"math"
 	"testing"
@@ -224,7 +225,7 @@ tests:
 
 		for _, childNum := range test.path {
 			var err er.R
-			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 er.R
-			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 er.R
-			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
@@ -830,9 +831,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 !ErrDeriveHardFromPublic.Is(err) {
-		t.Fatalf("Child: mismatched error -- got: %v, want: %v",
+		t.Fatalf("Derive: mismatched error -- got: %v, want: %v",
 			err, ErrDeriveHardFromPublic)
 	}
 
@@ -1051,19 +1052,70 @@ 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 !ErrDeriveBeyondMaxDepth.Is(err) {
-		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")
+	}
+}
+
+// TestLeadingZero ensures that deriving children from keys with a leading zero byte is 
+// actually 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")
 	}
 }