Fix implementation of hd derivation to be bip32 compliant #97

braydonf · 2016-09-12T22:17:23Z

https://github.com/bitcoin/bips/blob/master/bip-0032.mediawiki#child-key-derivation-ckd-functions

Closes: #94
Closes: #93

axic · 2016-09-13T03:06:39Z

Great work! Can you also include #93?

braydonf · 2016-09-13T18:53:16Z

There is validation of the private key at: https://github.com/bitpay/bitcore-lib/blob/master/lib/hdprivatekey.js#L416 when the private key is constructed, however I don't see logic for proceeding to the next value for the index.

braydonf · 2016-09-13T23:03:24Z

Okay, handling of invalid private key derivation is handled. The test that was just added would be broken without the fix, and would give an error message about an invalid private key rather than incrementing the index.

braydonf · 2016-09-13T23:06:04Z

There may also be this case: https://github.com/bitcoin/bips/blob/master/bip-0032.mediawiki#public-parent-key--public-child-key

braydonf · 2016-09-14T17:13:22Z

Just tested the bitcore-lib tests against libbtc here: https://github.com/braydonf/libbtc/tree/cross-ref There seems to be an issue with the fingerprint in the test that still needs to be worked out.
Edit: Fingerprint issue fixed it was using the parent fingerprint in the test

https://github.com/bitcoin/bips/blob/master/bip-0032.mediawiki#child-key-derivation-ckd-functions

danfinlay · 2016-10-07T22:10:45Z

When can we look forward to having this merged & pushed to npm? There are a lot of broken HD wallets out there right now, would be nice to get people using the same algorithms.

gabegattis · 2016-10-10T21:17:23Z

@FlySwatter Hopefully, we can get our 1.0.0 release (which should include this fix) sometime in the next couple of weeks.

I cannot give a certain date though.

braydonf · 2016-10-20T01:47:33Z

More cross testing:

libbtc at https://github.com/braydonf/libbtc/tree/cross-ref
hdkey at test: include derivation test when private key has leading zeros cryptocoinjs/hdkey#9
bcoin at test: include derivation test when private key has leading zeros bcoin-org/bcoin#84
bitcoinjs-lib at test: include derivation test when private key has leading zeros bitcoinjs/bitcoinjs-lib#673
yours-bitcoin at test: include derivation test when private key has leading zeros moneybutton/yours-bitcoin#48

This is to avoid any accidental upgrades to a bugfixed version without awareness of the change.

braydonf · 2016-12-06T07:24:52Z

This PR has been updated from feedback in #115

matiu · 2016-12-06T11:54:08Z

ACK

gabegattis · 2016-12-06T20:25:22Z

The plan was to leave .derive() the way it is for the time being.

The 2 new methods would be be for getting a compliant or non-compliant child. We don't want to make .derive() throw util we make breaking changes in v1.0.0.

Also something I mentioned internally, but not on GitHub, I think we should remove the hdkey cache now. We know we want to remove it in v1.0.0. This PR requires changes to the cache code. We can avoid that by just removing the cache. Removing the cache would not be a breaking change. It is not documented and, where it is exposed in index.js, we comment that it is only exposed for internal usage and testing.

gabegattis · 2016-12-07T00:10:44Z

I opened a new PR that reflects my comment above.
#116

braydonf · 2016-12-07T01:19:43Z

Removing hdkeycache can be it's own atomic change and is not related to fixing BIP32 derivation.

levino · 2017-02-27T15:32:44Z

lib/hdkeycache.js

    hardened = !!hardened;
-    var key = xkey + '/' + number + '/' + hardened;
+    var key = (nonCompliant ? '1' : '0') + xkey + '/' + number + '/' + hardened;


There should a comment in the code here. Nobody will get what and why this is done.

levino · 2017-02-27T15:33:35Z

lib/hdkeycache.js

    hardened = !!hardened;
-    var key = xkey + '/' + number + '/' + hardened;
+    var key =  (nonCompliant ? '1' : '0') + xkey + '/' + number + '/' + hardened;


Same as above. This should be commented. See also http://butunclebob.com/ArticleS.TimOttinger.ApologizeIncode

levino · 2017-02-27T15:34:41Z

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');


Is this really what one would expect? Why not the remove the function altogether? Also makes the code better readible in the future.

This is to make sure that if someone upgrades to 1.0.0, that they will not mistakenly be unaware of the bug that affected the deprecated function.

I unterstand that. So just completely remove the method. This will also throw an error of someone wants to use it, but then it does not show in IDE autocomplete and so on. Just think how annoying this method will be in the future.

levino · 2017-02-27T15:39:19Z

lib/hdprivatekey.js

 */
-HDPrivateKey.prototype.derive = function(arg, hardened) {
+HDPrivateKey.prototype.deriveChild = function(arg, hardened, nonCompliant) {


I think "deriveNonComplientChild" and "deriveChild" should both use a function "_derive". In the future everyone will use the function "deriveChild" and they will forever wonder why there is a parameter "nonCompliant". This should not be there. So this should be

HDPrivateKey.prototype.deriveChild = function(arg, hardened) { return this._derive(arg, hardened) }

and in deriveNonCompliantChild

HDPrivateKey.prototype.deriveNonCompliantChild = function(arg, hardened) { return this._derive(arg, hardened, true); };

This is the most important remark of my review.

Does the implementation in #116 satisfy some of your comments? It was based off of this PR, but with some slight changes.

I'll take a look at updating, though I agree, nonCompliant likely shouldn't be hanging around on public API method.

levino · 2017-02-27T16:12:28Z

lib/hdprivatekey.js

+ * 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.


As much as I like this comment, it is easy to overlook it. One should log a warning when someone is using this function too.

I feel like the method name deriveNonCompliantChild is warning enough.

levino · 2017-02-27T16:13:10Z

lib/hdprivatekey.js

@@ -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);


Same here. Do we really want to drag this "nonCompliant" parameter through the ages from now on?

levino · 2017-02-27T16:14:35Z

lib/hdprivatekey.js

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


What if it is hardened = false && nonCompliant = true?

The nonCompliant derivation (bug) only affects hardened derivations.

levino · 2017-02-27T16:16:41Z

test/hdkeys.js

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


This is not a proper sentence...

levino · 2017-02-27T16:17:53Z

test/hdkeys.js

+    afterEach(function() {
+      sandbox.restore();
+    });
+    it('will handle edge case that derived private key is invalid', function() {


I do not understand what this test is about. Is it feeding in data that triggered the bug and now we have test that the bug is fixed? Maybe say so in the comments?

It's not related to the zero padding bug, it has to do with behavior if the derived private key is invalid, which is nearly impossible.

levino · 2017-02-27T16:18:45Z

test/privatekey.js

+      var bn = BN.fromBuffer(new Buffer('9b5a0e8fee1835e21170ce1431f9b6f19b487e67748ed70d8a4462bc031915', 'hex'));
+      var privkey = new PrivateKey(bn);
+      var buffer = privkey.toBufferNoPadding();
+      buffer.length.should.equal(31);


Is this something that one wants? What is the usecase in the future?

For the same reason that deriveNonCompliant exists. If there is hashing of privateKey.toBuffer(), the padding will make a difference, and thus there is still a method for it.

matiu · 2017-02-28T15:42:05Z

ACK. Lets merge this and move forward.

isocolsky · 2017-02-28T16:06:32Z

ACK

From vectors added in bitcoin/bips#497

braydonf · 2017-03-01T08:39:16Z

@matiu @isocolsky Sounds good!

levino

Yes, much better. Thanks!

mpolci · 2017-08-12T07:54:29Z

Why this PR is not yet merged?

matiu · 2017-08-12T11:50:23Z

This one was replaced by #116

braydonf mentioned this pull request Sep 12, 2016

BIP32 private derivation produces invalid results in certain cases #94

Open

braydonf added this to the 1.0.0 milestone Sep 12, 2016

braydonf added the api change label Sep 12, 2016

This was referenced Sep 12, 2016

Accounts sometimes generated differently from TestRPC MetaMask/metamask-extension#640

Closed

Fix PrivateKey.toBuffer #48

Closed

Braydon Fuller added 4 commits September 16, 2016 10:08

Fix implementation of hd derivation to be bip32 compliant

b79a9b2

https://github.com/bitcoin/bips/blob/master/bip-0032.mediawiki#child-key-derivation-ckd-functions

Handle edge case that invalid private key is derived

0906d98

Handle invalid public key derivation rather than throw error

d0e3f84

Add toBufferNoPadding method to private key

e9d1237

braydonf force-pushed the bip32-compliance branch from 1291b99 to e9d1237 Compare September 16, 2016 14:14

braydonf mentioned this pull request Oct 6, 2016

Fix implementation of hd derivation to be bip32 compliant litecore-archive/litecore-lib#4

Merged

braydonf mentioned this pull request Oct 20, 2016

test: include derivation test when private key has leading zeros cryptocoinjs/hdkey#9

Merged

This was referenced Nov 21, 2016

problem with metamask, i forget two accounts with i have deposited ETH MetaMask/metamask-extension#835

Closed

Publish blog post regarding BIP44 derivation fix MetaMask/metamask-extension#854

Closed

matiu mentioned this pull request Dec 1, 2016

Bip32fix #115

Closed

Change name of derive to deriveChild

d32ae41

This is to avoid any accidental upgrades to a bugfixed version without awareness of the change.

gabegattis mentioned this pull request Dec 7, 2016

Bip32 compliance-round3 #116

Merged

danfinlay mentioned this pull request Jan 23, 2017

BIP44 Derivation is Non-Standard (bitcore issue) Consensys/eth-lightwallet#131

Open

iancoleman mentioned this pull request Feb 23, 2017

(CRITICAL) Incorrect derivation for certain BIP39 keys, fund loss >:( iancoleman/bip39#58

Closed

prusnak mentioned this pull request Feb 23, 2017

(CRITICAL) Wrong key derivation for specific mnemonics, BIP39/BIP44 issue? bitpay/wallet#5667

Closed

levino reviewed Feb 27, 2017

View reviewed changes

braydonf added 3 commits March 1, 2017 03:12

Remove nonCompliant argument from public method

4760c90

Fix comment

fb606ea

Add test vectors 3

57b3d95

From vectors added in bitcoin/bips#497

braydonf mentioned this pull request Mar 1, 2017

BIP 32: Test vectors for leading zeros bitcoin/bips#497

Merged

levino approved these changes Mar 1, 2017

View reviewed changes

matiu closed this Aug 12, 2017

This was referenced Jan 19, 2018

Can't access to my wallet anymore MyEtherWallet/etherwallet#1528

Open

Several people have lost Neo due to a fluke in the Neon wallet and Ledger Nano S. CityOfZion/neon-wallet#524

Closed

mattpass mentioned this pull request Apr 25, 2018

Dynamic merchant wallets BTCPrivate/btcp-widget#6

Closed

Fix implementation of hd derivation to be bip32 compliant #97

Fix implementation of hd derivation to be bip32 compliant #97

Conversation

braydonf commented Sep 12, 2016 • edited Loading

axic commented Sep 13, 2016

braydonf commented Sep 13, 2016 • edited Loading

braydonf commented Sep 13, 2016 • edited Loading

braydonf commented Sep 13, 2016

braydonf commented Sep 14, 2016 • edited Loading

danfinlay commented Oct 7, 2016

gabegattis commented Oct 10, 2016 • edited Loading

braydonf commented Oct 20, 2016 • edited Loading

braydonf commented Dec 6, 2016

matiu commented Dec 6, 2016

gabegattis commented Dec 6, 2016

gabegattis commented Dec 7, 2016

braydonf commented Dec 7, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matiu commented Feb 28, 2017

isocolsky commented Feb 28, 2017

braydonf commented Mar 1, 2017

levino left a comment

Choose a reason for hiding this comment

mpolci commented Aug 12, 2017

matiu commented Aug 12, 2017

braydonf commented Sep 12, 2016 •

edited

Loading

braydonf commented Sep 13, 2016 •

edited

Loading

braydonf commented Sep 13, 2016 •

edited

Loading

braydonf commented Sep 14, 2016 •

edited

Loading

gabegattis commented Oct 10, 2016 •

edited

Loading

braydonf commented Oct 20, 2016 •

edited

Loading