From b79a9b274ae8b48701daf5a1284e4f9035c3df0d Mon Sep 17 00:00:00 2001 From: Braydon Fuller Date: Mon, 12 Sep 2016 18:14:30 -0400 Subject: [PATCH] Fix implementation of hd derivation to be bip32 compliant https://github.com/bitcoin/bips/blob/master/bip-0032.mediawiki#child-key-derivation-ckd-functions --- lib/hdkeycache.js | 8 +++---- lib/hdprivatekey.js | 51 ++++++++++++++++++++++++++++++++++++--------- test/hdkeycache.js | 10 ++++----- test/hdkeys.js | 32 ++++++++++++++++++++++++++++ 4 files changed, 82 insertions(+), 19 deletions(-) diff --git a/lib/hdkeycache.js b/lib/hdkeycache.js index 721d9da..606e00c 100644 --- a/lib/hdkeycache.js +++ b/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); }, diff --git a/lib/hdprivatekey.js b/lib/hdprivatekey.js index 3826e0b..8d5ed1b 100644 --- a/lib/hdprivatekey.js +++ b/lib/hdprivatekey.js @@ -139,6 +139,9 @@ HDPrivateKey._getDerivationIndexes = function(path) { * 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...'); @@ -149,18 +152,38 @@ HDPrivateKey._getDerivationIndexes = function(path) { * * @param {string|number} arg * @param {boolean?} hardened + * @param {boolean?} nonCompliant - This should not be used and only for backwards compatibility + * */ -HDPrivateKey.prototype.derive = function(arg, hardened) { +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) { +/** + * 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.deriveNonCompliant = function(arg, hardened) { + var derived = this.derive(arg, hardened, true); + return derived; +}; + +HDPrivateKey.prototype._deriveWithNumber = function(index, hardened, nonCompliant) { /* jshint maxstatements: 20 */ /* jshint maxcomplexity: 10 */ if (!HDPrivateKey.isValidPath(index, hardened)) { @@ -172,15 +195,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]); } @@ -202,18 +233,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; diff --git a/test/hdkeycache.js b/test/hdkeycache.js index db1f4fc..692c01d 100644 --- a/test/hdkeycache.js +++ b/test/hdkeycache.js @@ -25,22 +25,22 @@ 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); + 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); + expect(cache._cache['0' + master.xprivkey + '/0/false'].xprivkey).to.equal(child1.xprivkey); var child4 = child3.derive(0); - expect(cache._cache[master.xprivkey + '/0/false']).to.equal(undefined); + 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); + expect(cache._cache['0' + 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); + 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); diff --git a/test/hdkeys.js b/test/hdkeys.js index d4bd6bd..377e18b 100644 --- a/test/hdkeys.js +++ b/test/hdkeys.js @@ -221,6 +221,38 @@ 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.derive("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.deriveNonCompliant("m/44'/0'/0'/0/0'"); + derived.privateKey.toString().should.equal('4811a079bab267bfdca855b3bddff20231ff7044e648514fa099158472df2836'); + }); + describe('seed', function() { it('should initialize a new BIP32 correctly from test vector 1 seed', function() {