Merge pull request #182 from lightning-signer/bip32-zeros

Correct BIP-32 derivation issue

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

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
+discussed later), the child extended keys can be derived by using the Derive
-function.  The Child function supports deriving both normal (non-hardened) and
+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

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

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
+// Derive returns a derived child extended key at the given index.
-// extended key is a private extended key (as determined by the IsPrivate
+//
+// 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

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")
+	}
+}