Fix implementation of hd derivation to be bip32 compliant

docs/hierarchical.md

@@ -15,9 +15,9 @@ var HDPrivateKey = bitcore.HDPrivateKey;
 
 var hdPrivateKey = new HDPrivateKey();
 var retrieved = new HDPrivateKey('xpriv...');
-var derived = hdPrivateKey.derive("m/0'");
-var derivedByNumber = hdPrivateKey.derive(1).derive(2, true);
-var derivedByArgument = hdPrivateKey.derive("m/1/2'");
+var derived = hdPrivateKey.deriveChild("m/0'");
+var derivedByNumber = hdPrivateKey.deriveChild(1).deriveChild(2, true);
+var derivedByArgument = hdPrivateKey.deriveChild("m/1/2'");
 assert(derivedByNumber.xprivkey === derivedByArgument.xprivkey);
 
 var address = derived.privateKey.toAddress();
@@ -39,5 +39,11 @@ try {
 }
 
 var address = new Address(hdPublicKey.publicKey, Networks.livenet);
-var derivedAddress = new Address(hdPublicKey.derive(100).publicKey, Networks.testnet);
+var derivedAddress = new Address(hdPublicKey.deriveChild(100).publicKey, Networks.testnet);
 ```
+
+## Upgrading from v0.13.x and previous to v1.0.0
+
+There was a bug that was discovered with derivation that would incorrectly calculate the child key against the [BIP32 specification](https://github.com/bitcoin/bips/blob/master/bip-0032.mediawiki). The method `derive` has been deprecated and replaced with `deriveChild` and `deriveNonCompliantChild`. As the names indicate `deriveNonCompliantChild` will derive using the non-BIP32 derivation and is the equivalent of `derive` in v0.13 and previous versions. The `deriveNonCompliantChild` method should not be used unless you're upgrading and need to maintain compatibility with the old derivation.
+
+The bug only affected hardened derivations using an extended private key, and did not affect public key derivation. It also did not affect every derivation and would happen 1 in 256 times where where the private key for the extended private key had a leading zero *(e.g. any private key less than or equal to '0fffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff')*. The leading zero was not included in serialization before hashing to derive a child key, as it should have been.

lib/hdkeycache.js

@@ -8,17 +8,17 @@ module.exports = {
   _usedIndex: {},
   _CACHE_SIZE: 5000,
 
-  get: function(xkey, number, hardened) {
+  get: function(xkey, number, hardened, nonCompliant) {
     hardened = !!hardened;
-    var key = xkey + '/' + number + '/' + hardened;
+    var key = (nonCompliant ? '1' : '0') + xkey + '/' + number + '/' + hardened;
     if (this._cache[key]) {
       this._cacheHit(key);
       return this._cache[key];
     }
   },
-  set: function(xkey, number, hardened, derived) {
+  set: function(xkey, number, hardened, derived, nonCompliant) {
     hardened = !!hardened;
-    var key = xkey + '/' + number + '/' + hardened;
+    var key =  (nonCompliant ? '1' : '0') + xkey + '/' + number + '/' + hardened;
     this._cache[key] = derived;
     this._cacheHit(key);
   },

lib/hdprivatekey.js

@@ -127,6 +127,11 @@ HDPrivateKey._getDerivationIndexes = function(path) {
   return _.any(indexes, isNaN) ? null : indexes;
 };
 
+HDPrivateKey.prototype.derive = function() {
+  throw new Error('Method has been deprecated, please use deriveChild or deriveNonCompliantChild' +
+                  'and see documentation for more information');
+};
+
 /**
  * Get a derived child based on a string or number.
  *
@@ -142,25 +147,48 @@ HDPrivateKey._getDerivationIndexes = function(path) {
  * @example
  * ```javascript
  * var parent = new HDPrivateKey('xprv...');
- * var child_0_1_2h = parent.derive(0).derive(1).derive(2, true);
- * var copy_of_child_0_1_2h = parent.derive("m/0/1/2'");
+ * var child_0_1_2h = parent.deriveChild(0).deriveChild(1).deriveChild(2, true);
+ * var copy_of_child_0_1_2h = parent.deriveChild("m/0/1/2'");
  * assert(child_0_1_2h.xprivkey === copy_of_child_0_1_2h);
  * ```
  *
  * @param {string|number} arg
  * @param {boolean?} hardened
  */
-HDPrivateKey.prototype.derive = function(arg, hardened) {
+HDPrivateKey.prototype.deriveChild = function(arg, hardened) {
+  var derived = this._derive(arg, hardened, false);
+  return derived;
+};
+
+/**
+ * WARNING: If this is a new implementation you should NOT use this method, you should be using
+ * `derive` instead.
+ *
+ * This method is explicitly for use and compatibility with an implementation that
+ * was not compliant with BIP32 regarding the derivation algorithm. The private key
+ * must be 32 bytes hashing, and this implementation will use the non-zero padded
+ * serialization of a private key, such that it's still possible to derive the privateKey
+ * to recover those funds.
+ *
+ * @param {string|number} arg
+ * @param {boolean?} hardened
+ */
+HDPrivateKey.prototype.deriveNonCompliantChild = function(arg, hardened) {
+  var derived = this._derive(arg, hardened, true);
+  return derived;
+};
+
+HDPrivateKey.prototype._derive = function(arg, hardened, nonCompliant) {
   if (_.isNumber(arg)) {
-    return this._deriveWithNumber(arg, hardened);
+    return this._deriveWithNumber(arg, hardened, nonCompliant);
   } else if (_.isString(arg)) {
-    return this._deriveFromString(arg);
+    return this._deriveFromString(arg, nonCompliant);
   } else {
     throw new hdErrors.InvalidDerivationArgument(arg);
   }
 };
 
-HDPrivateKey.prototype._deriveWithNumber = function(index, hardened) {
+HDPrivateKey.prototype._deriveWithNumber = function(index, hardened, nonCompliant) {
   /* jshint maxstatements: 20 */
   /* jshint maxcomplexity: 10 */
   if (!HDPrivateKey.isValidPath(index, hardened)) {
@@ -172,15 +200,23 @@ HDPrivateKey.prototype._deriveWithNumber = function(index, hardened) {
     index += HDPrivateKey.Hardened;
   }
 
-  var cached = HDKeyCache.get(this.xprivkey, index, hardened);
+  var cached = HDKeyCache.get(this.xprivkey, index, hardened, nonCompliant);
   if (cached) {
     return cached;
   }
 
   var indexBuffer = BufferUtil.integerAsBuffer(index);
   var data;
-  if (hardened) {
-    data = BufferUtil.concat([new buffer.Buffer([0]), this.privateKey.toBuffer(), indexBuffer]);
+  if (hardened && nonCompliant) {
+    // The private key serialization in this case will not be exactly 32 bytes and can be
+    // any value less, and the value is not zero-padded.
+    var nonZeroPadded = this.privateKey.bn.toBuffer();
+    data = BufferUtil.concat([new buffer.Buffer([0]), nonZeroPadded, indexBuffer]);
+  } else if (hardened) {
+    // This will use a 32 byte zero padded serialization of the private key
+    var privateKeyBuffer = this.privateKey.bn.toBuffer({size: 32});
+    assert(privateKeyBuffer.length === 32, 'length of private key buffer is expected to be 32 bytes');
+    data = BufferUtil.concat([new buffer.Buffer([0]), privateKeyBuffer, indexBuffer]);
   } else {
     data = BufferUtil.concat([this.publicKey.toBuffer(), indexBuffer]);
   }
@@ -194,6 +230,11 @@ HDPrivateKey.prototype._deriveWithNumber = function(index, hardened) {
     size: 32
   });
 
+  if (!PrivateKey.isValid(privateKey)) {
+    // Index at this point is already hardened, we can pass null as the hardened arg
+    return this._deriveWithNumber(index + 1, null, nonCompliant);
+  }
+
   var derived = new HDPrivateKey({
     network: this.network,
     depth: this.depth + 1,
@@ -202,18 +243,18 @@ HDPrivateKey.prototype._deriveWithNumber = function(index, hardened) {
     chainCode: chainCode,
     privateKey: privateKey
   });
-  HDKeyCache.set(this.xprivkey, index, hardened, derived);
+  HDKeyCache.set(this.xprivkey, index, hardened, derived, nonCompliant);
   return derived;
 };
 
-HDPrivateKey.prototype._deriveFromString = function(path) {
+HDPrivateKey.prototype._deriveFromString = function(path, nonCompliant) {
   if (!HDPrivateKey.isValidPath(path)) {
     throw new hdErrors.InvalidPath(path);
   }
 
   var indexes = HDPrivateKey._getDerivationIndexes(path);
   var derived = indexes.reduce(function(prev, index) {
-    return prev._deriveWithNumber(index);
+    return prev._deriveWithNumber(index, null, nonCompliant);
   }, this);
 
   return derived;

lib/hdpublickey.js

@@ -86,6 +86,11 @@ HDPublicKey.isValidPath = function(arg) {
   return false;
 };
 
+HDPublicKey.prototype.derive = function() {
+  throw new Error('Method has been deprecated, please use deriveChild instead' +
+                  'and see documentation for more information');
+};
+
 /**
  * Get a derivated child based on a string or number.
  *
@@ -101,14 +106,14 @@ HDPublicKey.isValidPath = function(arg) {
  * @example
  * ```javascript
  * var parent = new HDPublicKey('xpub...');
- * var child_0_1_2 = parent.derive(0).derive(1).derive(2);
- * var copy_of_child_0_1_2 = parent.derive("m/0/1/2");
+ * var child_0_1_2 = parent.deriveChild(0).deriveChild(1).deriveChild(2);
+ * var copy_of_child_0_1_2 = parent.deriveChild("m/0/1/2");
  * assert(child_0_1_2.xprivkey === copy_of_child_0_1_2);
  * ```
  *
  * @param {string|number} arg
  */
-HDPublicKey.prototype.derive = function(arg, hardened) {
+HDPublicKey.prototype.deriveChild = function(arg, hardened) {
   if (_.isNumber(arg)) {
     return this._deriveWithNumber(arg, hardened);
   } else if (_.isString(arg)) {
@@ -136,7 +141,12 @@ HDPublicKey.prototype._deriveWithNumber = function(index, hardened) {
   var leftPart = BN.fromBuffer(hash.slice(0, 32), {size: 32});
   var chainCode = hash.slice(32, 64);
 
-  var publicKey = PublicKey.fromPoint(Point.getG().mul(leftPart).add(this.publicKey.point));
+  var publicKey;
+  try {
+    publicKey = PublicKey.fromPoint(Point.getG().mul(leftPart).add(this.publicKey.point));
+  } catch (e) {
+    return this._deriveWithNumber(index + 1);
+  }
 
   var derived = new HDPublicKey({
     network: this.network,

lib/privatekey.js

@@ -339,6 +339,15 @@ PrivateKey.prototype.toBuffer = function(){
   return this.bn.toBuffer({ size: 32 });
 };
 
+/**
+ * Will return the private key as a BN buffer without leading zero padding
+ *
+ * @returns {Buffer} A buffer of the private key
+ */
+PrivateKey.prototype.toBufferNoPadding = function() {
+  return this.bn.toBuffer();
+};
+
 /**
  * Will return the corresponding public key
  *

test/hdkeycache.js

@@ -24,29 +24,29 @@ describe('HDKey cache', function() {
   });
 
   it('saves a derived key', function() {
-    var child = master.derive(0);
-    expect(cache._cache[master.xprivkey + '/0/false'].xprivkey).to.equal(child.xprivkey);
+    var child = master.deriveChild(0);
+    expect(cache._cache['0' + master.xprivkey + '/0/false'].xprivkey).to.equal(child.xprivkey);
   });
   it('starts erasing unused keys', function() {
-    var child1 = master.derive(0);
-    var child2 = child1.derive(0);
-    var child3 = child2.derive(0);
-    expect(cache._cache[master.xprivkey + '/0/false'].xprivkey).to.equal(child1.xprivkey);
-    var child4 = child3.derive(0);
-    expect(cache._cache[master.xprivkey + '/0/false']).to.equal(undefined);
+    var child1 = master.deriveChild(0);
+    var child2 = child1.deriveChild(0);
+    var child3 = child2.deriveChild(0);
+    expect(cache._cache['0' + master.xprivkey + '/0/false'].xprivkey).to.equal(child1.xprivkey);
+    var child4 = child3.deriveChild(0);
+    expect(cache._cache['0' + master.xprivkey + '/0/false']).to.equal(undefined);
   });
   it('avoids erasing keys that get cache hits ("hot keys")', function() {
-    var child1 = master.derive(0);
-    var child2 = master.derive(0).derive(0);
-    expect(cache._cache[master.xprivkey + '/0/false'].xprivkey).to.equal(child1.xprivkey);
-    var child1_copy = master.derive(0);
-    expect(cache._cache[master.xprivkey + '/0/false'].xprivkey).to.equal(child1.xprivkey);
+    var child1 = master.deriveChild(0);
+    var child2 = master.deriveChild(0).deriveChild(0);
+    expect(cache._cache['0' + master.xprivkey + '/0/false'].xprivkey).to.equal(child1.xprivkey);
+    var child1_copy = master.deriveChild(0);
+    expect(cache._cache['0' + master.xprivkey + '/0/false'].xprivkey).to.equal(child1.xprivkey);
   });
   it('keeps the size of the cache small', function() {
-    var child1 = master.derive(0);
-    var child2 = child1.derive(0);
-    var child3 = child2.derive(0);
-    var child4 = child3.derive(0);
+    var child1 = master.deriveChild(0);
+    var child2 = child1.deriveChild(0);
+    var child3 = child2.deriveChild(0);
+    var child4 = child3.deriveChild(0);
     expect(_.size(cache._cache)).to.equal(3);
   });
 });