Merge pull request #116 from gabegattis/bip32-compliance

Bip32 compliance-round3

docs/hierarchical.md

@@ -15,7 +15,7 @@ var HDPrivateKey = bitcore.HDPrivateKey;
 
 var hdPrivateKey = new HDPrivateKey();
 var retrieved = new HDPrivateKey('xpriv...');
-var derived = hdPrivateKey.derive("m/0'");
+var derived = hdPrivateKey.derive("m/0'"); // see deprecation warning for derive
 var derivedByNumber = hdPrivateKey.derive(1).derive(2, true);
 var derivedByArgument = hdPrivateKey.derive("m/1/2'");
 assert(derivedByNumber.xprivkey === derivedByArgument.xprivkey);
@@ -39,5 +39,14 @@ try {
 }
 
 var address = new Address(hdPublicKey.publicKey, Networks.livenet);
-var derivedAddress = new Address(hdPublicKey.derive(100).publicKey, Networks.testnet);
+var derivedAddress = new Address(hdPublicKey.derive(100).publicKey, Networks.testnet); // see deprecation warning for derive
 ```
+
+## Deprecation Warning for `HDPublicKey.derive()` and `HDPrivateKey.derive()`
+
+
+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 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.
+
+As a result, `HDPublicKey.derive()` and `HDPrivateKey.derive()` are now deprecated. These methods will throw an error in the next major release.
+`HDPublicKey.deriveChild()`, `HDPrivateKey.deriveChild()`, and `HDPrivateKey.deriveNonCompliantChild()` have been implemented as alternatives. Note that these new methods will not be officially supported until v1.0.0.  `deriveNonCompliantChild` will derive using the non-BIP32 derivation and is equivalent to the buggy version, `derive`. The `deriveNonCompliantChild` method should not be used unless you're upgrading and need to maintain compatibility with the old derivation.

lib/hdprivatekey.js

@@ -127,6 +127,9 @@ HDPrivateKey._getDerivationIndexes = function(path) {
 };
 
 /**
+ * WARNING: This method is deprecated. Use deriveChild or deriveNonCompliantChild instead. This is not BIP32 compliant
+ *
+ *
  * Get a derived child based on a string or number.
  *
  * If the first argument is a string, it's parsed as the full path of
@@ -150,6 +153,39 @@ HDPrivateKey._getDerivationIndexes = function(path) {
  * @param {boolean?} hardened
  */
 HDPrivateKey.prototype.derive = function(arg, hardened) {
+  return this.deriveNonCompliantChild(arg, hardened);
+};
+
+/**
+ * WARNING: This method will not be officially supported until v1.0.0.
+ *
+ *
+ * Get a derived child based on a string or number.
+ *
+ * If the first argument is a string, it's parsed as the full path of
+ * derivation. Valid values for this argument include "m" (which returns the
+ * same private key), "m/0/1/40/2'/1000", where the ' quote means a hardened
+ * derivation.
+ *
+ * If the first argument is a number, the child with that index will be
+ * derived. If the second argument is truthy, the hardened version will be
+ * derived. See the example usage for clarification.
+ *
+ * WARNING: The `nonCompliant` option should NOT be used, except for older implementation
+ * that used a derivation strategy that used a non-zero padded private key.
+ *
+ * @example
+ * ```javascript
+ * var parent = new HDPrivateKey('xprv...');
+ * 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.deriveChild = function(arg, hardened) {
   if (_.isNumber(arg)) {
     return this._deriveWithNumber(arg, hardened);
   } else if (_.isString(arg)) {
@@ -159,7 +195,33 @@ HDPrivateKey.prototype.derive = function(arg, hardened) {
   }
 };
 
-HDPrivateKey.prototype._deriveWithNumber = function(index, hardened) {
+/**
+ * WARNING: This method will not be officially supported until v1.0.0
+ *
+ *
+ * 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) {
+  if (_.isNumber(arg)) {
+    return this._deriveWithNumber(arg, hardened, true);
+  } else if (_.isString(arg)) {
+    return this._deriveFromString(arg, true);
+  } else {
+    throw new hdErrors.InvalidDerivationArgument(arg);
+  }
+};
+
+HDPrivateKey.prototype._deriveWithNumber = function(index, hardened, nonCompliant) {
   /* jshint maxstatements: 20 */
   /* jshint maxcomplexity: 10 */
   if (!HDPrivateKey.isValidPath(index, hardened)) {
@@ -173,8 +235,16 @@ HDPrivateKey.prototype._deriveWithNumber = function(index, hardened) {
 
   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]);
   }
@@ -188,6 +258,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,
@@ -200,14 +275,14 @@ HDPrivateKey.prototype._deriveWithNumber = function(index, hardened) {
   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,9 @@ HDPublicKey.isValidPath = function(arg) {
 };
 
 /**
+ * WARNING: This method is deprecated. Use deriveChild instead.
+ *
+ *
  * Get a derivated child based on a string or number.
  *
  * If the first argument is a string, it's parsed as the full path of
@@ -108,6 +111,35 @@ HDPublicKey.isValidPath = function(arg) {
  * @param {string|number} arg
  */
 HDPublicKey.prototype.derive = function(arg, hardened) {
+  return this.deriveChild(arg, hardened);
+};
+
+/**
+ * WARNING: This method will not be officially supported until v1.0.0.
+ *
+ *
+ * Get a derivated child based on a string or number.
+ *
+ * If the first argument is a string, it's parsed as the full path of
+ * derivation. Valid values for this argument include "m" (which returns the
+ * same public key), "m/0/1/40/2/1000".
+ *
+ * Note that hardened keys can't be derived from a public extended key.
+ *
+ * If the first argument is a number, the child with that index will be
+ * derived. See the example usage for clarification.
+ *
+ * @example
+ * ```javascript
+ * var parent = new HDPublicKey('xpub...');
+ * 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.deriveChild = function(arg, hardened) {
   if (_.isNumber(arg)) {
     return this._deriveWithNumber(arg, hardened);
   } else if (_.isString(arg)) {
@@ -131,7 +163,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

@@ -336,6 +336,19 @@ PrivateKey.prototype.toBigNumber = function(){
  * @returns {Buffer} A buffer of the private key
  */
 PrivateKey.prototype.toBuffer = function(){
+  // TODO: use `return this.bn.toBuffer({ size: 32 })` in v1.0.0
+  return this.bn.toBuffer();
+};
+
+/**
+ * WARNING: This method will not be officially supported until v1.0.0.
+ *
+ *
+ * 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();
 };
 

test/hdkeys.js

@@ -13,6 +13,7 @@
 var _ = require('lodash');
 var should = require('chai').should();
 var expect = require('chai').expect;
+var sinon = require('sinon');
 var bitcore = require('..');
 var Networks = bitcore.Networks;
 var HDPrivateKey = bitcore.HDPrivateKey;
@@ -221,6 +222,111 @@ describe('BIP32 compliance', function() {
       .xpubkey.should.equal(vector2_m02147483647h12147483646h2_public);
   });
 
+  it('should use full 32 bytes for private key data that is hashed (as per bip32)', function() {
+    // https://github.com/bitcoin/bips/blob/master/bip-0032.mediawiki
+    var privateKeyBuffer = new Buffer('00000055378cf5fafb56c711c674143f9b0ee82ab0ba2924f19b64f5ae7cdbfd', 'hex');
+    var chainCodeBuffer = new Buffer('9c8a5c863e5941f3d99453e6ba66b328bb17cf0b8dec89ed4fc5ace397a1c089', 'hex');
+    var key = HDPrivateKey.fromObject({
+      network: 'testnet',
+      depth: 0,
+      parentFingerPrint: 0,
+      childIndex: 0,
+      privateKey: privateKeyBuffer,
+      chainCode: chainCodeBuffer
+    });
+    var derived = key.deriveChild("m/44'/0'/0'/0/0'");
+    derived.privateKey.toString().should.equal('3348069561d2a0fb925e74bf198762acc47dce7db27372257d2d959a9e6f8aeb');
+  });
+
+  it('should NOT use full 32 bytes for private key data that is hashed with nonCompliant flag', function() {
+    // This is to test that the previously implemented non-compliant to BIP32
+    var privateKeyBuffer = new Buffer('00000055378cf5fafb56c711c674143f9b0ee82ab0ba2924f19b64f5ae7cdbfd', 'hex');
+    var chainCodeBuffer = new Buffer('9c8a5c863e5941f3d99453e6ba66b328bb17cf0b8dec89ed4fc5ace397a1c089', 'hex');
+    var key = HDPrivateKey.fromObject({
+      network: 'testnet',
+      depth: 0,
+      parentFingerPrint: 0,
+      childIndex: 0,
+      privateKey: privateKeyBuffer,
+      chainCode: chainCodeBuffer
+    });
+    var derived = key.deriveNonCompliantChild("m/44'/0'/0'/0/0'");
+    derived.privateKey.toString().should.equal('4811a079bab267bfdca855b3bddff20231ff7044e648514fa099158472df2836');
+  });
+
+  it('should NOT use full 32 bytes for private key data that is hashed with the nonCompliant derive method', function() {
+    // This is to test that the previously implemented non-compliant to BIP32
+    var privateKeyBuffer = new Buffer('00000055378cf5fafb56c711c674143f9b0ee82ab0ba2924f19b64f5ae7cdbfd', 'hex');
+    var chainCodeBuffer = new Buffer('9c8a5c863e5941f3d99453e6ba66b328bb17cf0b8dec89ed4fc5ace397a1c089', 'hex');
+    var key = HDPrivateKey.fromObject({
+      network: 'testnet',
+      depth: 0,
+      parentFingerPrint: 0,
+      childIndex: 0,
+      privateKey: privateKeyBuffer,
+      chainCode: chainCodeBuffer
+    });
+    var derived = key.derive("m/44'/0'/0'/0/0'");
+    derived.privateKey.toString().should.equal('4811a079bab267bfdca855b3bddff20231ff7044e648514fa099158472df2836');
+  });
+
+  describe('edge cases', function() {
+    var sandbox = sinon.sandbox.create();
+    afterEach(function() {
+      sandbox.restore();
+    });
+    it('will handle edge case that derived private key is invalid', function() {
+      var invalid = new Buffer('0000000000000000000000000000000000000000000000000000000000000000', 'hex');
+      var privateKeyBuffer = new Buffer('5f72914c48581fc7ddeb944a9616389200a9560177d24f458258e5b04527bcd1', 'hex');
+      var chainCodeBuffer = new Buffer('39816057bba9d952fe87fe998b7fd4d690a1bb58c2ff69141469e4d1dffb4b91', 'hex');
+      var unstubbed = bitcore.crypto.BN.prototype.toBuffer;
+      var count = 0;
+      var stub = sandbox.stub(bitcore.crypto.BN.prototype, 'toBuffer', function(args) {
+        // On the fourth call to the function give back an invalid private key
+        // otherwise use the normal behavior.
+        count++;
+        if (count === 4) {
+          return invalid;
+        }
+        var ret = unstubbed.apply(this, arguments);
+        return ret;
+      });
+      sandbox.spy(bitcore.PrivateKey, 'isValid');
+      var key = HDPrivateKey.fromObject({
+        network: 'testnet',
+        depth: 0,
+        parentFingerPrint: 0,
+        childIndex: 0,
+        privateKey: privateKeyBuffer,
+        chainCode: chainCodeBuffer
+      });
+      var derived = key.derive("m/44'");
+      derived.privateKey.toString().should.equal('b15bce3608d607ee3a49069197732c656bca942ee59f3e29b4d56914c1de6825');
+      bitcore.PrivateKey.isValid.callCount.should.equal(2);
+    });
+    it('will handle edge case that a derive public key is invalid', function() {
+      var publicKeyBuffer = new Buffer('029e58b241790284ef56502667b15157b3fc58c567f044ddc35653860f9455d099', 'hex');
+      var chainCodeBuffer = new Buffer('39816057bba9d952fe87fe998b7fd4d690a1bb58c2ff69141469e4d1dffb4b91', 'hex');
+      var key = new HDPublicKey({
+        network: 'testnet',
+        depth: 0,
+        parentFingerPrint: 0,
+        childIndex: 0,
+        chainCode: chainCodeBuffer,
+        publicKey: publicKeyBuffer
+      });
+      var unstubbed = bitcore.PublicKey.fromPoint;
+      bitcore.PublicKey.fromPoint = function() {
+        bitcore.PublicKey.fromPoint = unstubbed;
+        throw new Error('Point cannot be equal to Infinity');
+      };
+      sandbox.spy(key, '_deriveWithNumber');
+      var derived = key.derive("m/44");
+      key._deriveWithNumber.callCount.should.equal(2);
+      key.publicKey.toString().should.equal('029e58b241790284ef56502667b15157b3fc58c567f044ddc35653860f9455d099');
+    });
+  });
+
   describe('seed', function() {
 
     it('should initialize a new BIP32 correctly from test vector 1 seed', function() {

test/privatekey.js

@@ -330,6 +330,29 @@ describe('PrivateKey', function() {
       var fromBuffer = PrivateKey.fromBuffer(toBuffer.toBuffer());
       fromBuffer.toString().should.equal(privkey.toString());
     });
+
+    it('will output a 31 byte buffer', function() {
+      var bn = BN.fromBuffer(new Buffer('9b5a0e8fee1835e21170ce1431f9b6f19b487e67748ed70d8a4462bc031915', 'hex'));
+      var privkey = new PrivateKey(bn);
+      var buffer = privkey.toBufferNoPadding();
+      buffer.length.should.equal(31);
+    });
+
+    // TODO: enable for v1.0.0 when toBuffer is changed to always be 32 bytes long
+    // it('will output a 32 byte buffer', function() {
+    //   var bn = BN.fromBuffer(new Buffer('9b5a0e8fee1835e21170ce1431f9b6f19b487e67748ed70d8a4462bc031915', 'hex'));
+    //   var privkey = new PrivateKey(bn);
+    //   var buffer = privkey.toBuffer();
+    //   buffer.length.should.equal(32);
+    // });
+
+    // TODO: enable for v1.0.0 when toBuffer is changed to always be 32 bytes long
+    // it('should return buffer with length equal 32', function() {
+    //   var bn = BN.fromBuffer(buf.slice(0, 31));
+    //   var privkey = new PrivateKey(bn, 'livenet');
+    //   var expected = Buffer.concat([ new Buffer([0]), buf.slice(0, 31) ]);
+    //   privkey.toBuffer().toString('hex').should.equal(expected.toString('hex'));
+    // });
   });
 
   describe('#toBigNumber', function() {