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.

index.js

@@ -6,7 +6,7 @@ var bitcore = module.exports;
 bitcore.version = 'v' + require('./package.json').version;
 bitcore.versionGuard = function(version) {
   if (version !== undefined) {
-    var message = 'More than one instance of bitcore-lib found. ' + 
+    var message = 'More than one instance of bitcore-lib found. ' +
       'Please make sure to require bitcore-lib and check that submodules do' +
       ' not also include their own bitcore-lib dependency.';
     throw new Error(message);
@@ -66,5 +66,4 @@ bitcore.deps.elliptic = require('elliptic');
 bitcore.deps._ = require('lodash');
 
 // Internal usage, exposed for testing/advanced tweaking
-bitcore._HDKeyCache = require('./lib/hdkeycache');
 bitcore.Transaction.sighash = require('./lib/transaction/sighash');

lib/hdprivatekey.js

@@ -11,7 +11,6 @@ var Base58 = require('./encoding/base58');
 var Base58Check = require('./encoding/base58check');
 var Hash = require('./crypto/hash');
 var Network = require('./networks');
-var HDKeyCache = require('./hdkeycache');
 var Point = require('./crypto/point');
 var PrivateKey = require('./privatekey');
 var Random = require('./crypto/random');
@@ -128,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
@@ -151,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)) {
@@ -160,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)) {
@@ -172,15 +233,18 @@ HDPrivateKey.prototype._deriveWithNumber = function(index, hardened) {
     index += HDPrivateKey.Hardened;
   }
 
-  var cached = HDKeyCache.get(this.xprivkey, index, hardened);
-  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 +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,
@@ -202,18 +271,18 @@ HDPrivateKey.prototype._deriveWithNumber = function(index, hardened) {
     chainCode: chainCode,
     privateKey: privateKey
   });
-  HDKeyCache.set(this.xprivkey, index, hardened, derived);
+
   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

@@ -8,7 +8,6 @@ var Base58 = require('./encoding/base58');
 var Base58Check = require('./encoding/base58check');
 var Hash = require('./crypto/hash');
 var HDPrivateKey = require('./hdprivatekey');
-var HDKeyCache = require('./hdkeycache');
 var Network = require('./networks');
 var Point = require('./crypto/point');
 var PublicKey = require('./publickey');
@@ -87,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
@@ -109,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)) {
@@ -125,18 +156,19 @@ HDPublicKey.prototype._deriveWithNumber = function(index, hardened) {
   if (index < 0) {
     throw new hdErrors.InvalidPath(index);
   }
-  var cached = HDKeyCache.get(this.xpubkey, index, false);
-  if (cached) {
-    return cached;
-  }
 
   var indexBuffer = BufferUtil.integerAsBuffer(index);
   var data = BufferUtil.concat([this.publicKey.toBuffer(), indexBuffer]);
   var hash = Hash.sha512hmac(data, this._buffers.chainCode);
   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,
@@ -146,7 +178,7 @@ HDPublicKey.prototype._deriveWithNumber = function(index, hardened) {
     chainCode: chainCode,
     publicKey: publicKey
   });
-  HDKeyCache.set(this.xpubkey, index, false, derived);
+
   return derived;
 };
 

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