Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix child keys derivation adding bytes padding #1139

Merged
merged 6 commits into from Sep 4, 2018

Conversation

Projects
None yet
6 participants
@gravityblast
Copy link
Member

commented Aug 6, 2018

Problem

While deriving a child key, if i ≥ 2^31 (hardened), the resulting private key is padded with a 0x00 to make it 33 bytes.
The specs says:

Check whether i ≥ 2^31 (whether the child is a hardened key).
    If so (hardened child): let I = HMAC-SHA512(Key = cpar, Data = 0x00 || ser256(kpar) || ser32(i)). (Note: The 0x00 pads the private key to make it 33 bytes long.)

The important part is: The 0x00 pads the private key to make it 33 bytes long.
If we pad it to always be 33 bytes, we should also add more zeros when the key data is less than 32 bytes.

In this specific case the derived private key was 31 bytes.

That private key is anyway right, but the problem raises when deriving its child key.
One of the step to derive the child key is calling our splitHMAC that returns a secretKey and a chainCode.
Inside this function we make a sha512 of a seed that is a 37 bytes with:

  • 1 byte with 0x00
  • 32 bytes for the key data
  • 4 bytes for the child key index

In our case, if the key was less then 32 bytes, it was shifted to the left of that 32 bytes space, resulting in a different seed, and a different data returned from the sha512 call.

Changes

I added a third test vector that has been added to the BIP32 spec after this part of the code has been written (taken from the test vector 3 section of BIP32), though our old codebase without this patch is not failing with those.

I added unit test to test a specific mnemonic key that generates a key of 31 bytes during one of the steps of the derivation of a key with path m/44'/60'/0'/0/0.

The main change is padding the key data with zeros to always have 32 bytes in the field KeyData of ExtendedKey.

Links

child.KeyData = keyBigInt.Bytes()
keyData := keyBigInt.Bytes()
if len(keyData) < 32 {
extra := make([]byte, 32-len(keyData))

This comment has been minimized.

Copy link
@dshulyak

dshulyak Aug 7, 2018

Member

if you want you could do a single allocation instead of 2

extra := make([]byte, 32-len, 32)

This comment has been minimized.

Copy link
@gravityblast

gravityblast Aug 7, 2018

Author Member

@dmitryn yes good point I'll do it!

@dshulyak

This comment has been minimized.

Copy link
Member

commented Aug 7, 2018

@pilu do you have a link to relevant bip where this condition is described?

@gravityblast

This comment has been minimized.

Copy link
Member Author

commented Aug 7, 2018

@pilu do you have a link to relevant bip where this condition is described?

For now I just compared to other implementation. But looking at Private parent key → private child key, the private key should always be 0x00 + 32 bytes, which is not in our case when it's a number smaller then 32 bytes. What do you think @dshulyak ?

@gravityblast

This comment has been minimized.

Copy link
Member Author

commented Aug 7, 2018

@dshulyak so the problem is not in that key (the one smaller than 32 bytes), because it's still a valid one. but in the generation of the child, because the parent private key is smaller than 32 bytes

@gravityblast

This comment has been minimized.

Copy link
Member Author

commented Aug 7, 2018

@dshulyak in fact I think the same logic should be used even when k.IsPrivate is false

@gravityblast

This comment has been minimized.

Copy link
Member Author

commented Aug 7, 2018

in fact I think the same logic should be used even when k.IsPrivate is false

actually if it's public is not actually needed because we call SerializeCompressed that is already calling paddedAppend
https://github.com/status-im/status-go/blob/develop/vendor/github.com/btcsuite/btcd/btcec/pubkey.go#L134

@mandrigin
Copy link
Member

left a comment

since it is very non-trivial to roll out these updates, we need

  1. unit-tests for that particular case to prove that it works correctly

  2. we need the old version of this code still available with a flag or a separate method for migration purposes

  3. we need investigations if all our Instabug reports about this problem are actually fixed by that, so we don’t have anything else that slipped in

@oskarth oskarth requested a review from corpetty Aug 8, 2018

@adambabik

This comment has been minimized.

Copy link
Member

commented Aug 8, 2018

@mandrigin

we need the old version of this code still available with a flag or a separate method for migration purposes

We can only check this when recovering an account. If that happens and we discover that the addresses are different, how the process will look like? If we decide to support both versions in the UI, this will root deep into the codebase responsible for keys and accounts and it will make it harder to reason about.

I would try to make avoid that. For example, we can make an announcement first and then release a version that fixes it without supporting the bugged version. If someone misses the window, they should be able to download an old release that will allow to move founds.

@status-im-jenkins

This comment has been minimized.

Copy link
Collaborator

commented Aug 8, 2018

Can one of the admins verify this patch?

@corpetty

This comment has been minimized.

Copy link

commented Aug 8, 2018

they should be able to download an old release that will allow to move founds.

If this is the route we go with only going with the new proper method, I suggest a how-to guide to go with this announcement.

Who should lead that?

I also agree that we should go this route as it would lead to address/identity issues across the app.

@corpetty
Copy link

left a comment

as key-generation is pivotal to many parts of Status, I would also like to see unit-tests to help give confidence of proper behavior.

@gravityblast

This comment has been minimized.

Copy link
Member Author

commented Aug 8, 2018

@corpetty yes for sure I'll do as soon as I continue with this. I just opened the PR to share that part of the code but it's just a start, so that we can look at it together.

@mandrigin

This comment has been minimized.

Copy link
Member

commented Aug 8, 2018

To avoid having these situations in the future, let's try to find a good test suite for key derivation.
The vast majority of blockchain works is open source, this BIP was around since 2014, maybe there are good test sets for the implementation.

If there aren't, then we should make one and share it with crypto community, I think.

gravityblast added some commits Aug 9, 2018

@gravityblast gravityblast changed the title [WIP] fix child keys derivation adding bytes padding fix child keys derivation adding bytes padding Aug 9, 2018

@gravityblast

This comment has been minimized.

Copy link
Member Author

commented Aug 9, 2018

@mandrigin @adambabik @dshulyak @corpetty I added tests and documentation in the PR description

@@ -111,6 +111,21 @@ func TestBIP32Vectors(t *testing.T) {
"xpub6FnCn6nSzZAw5Tw7cgR9bi15UV96gLZhjDstkXXxvCLsUXBGXPdSnLFbdpq8p9HmGsApME5hQTZ3emM2rnY5agb9rXpVGyy3bdW6EEgAtqt",
"xprvA2nrNbFZABcdryreWet9Ea4LvTJcGsqrMzxHx98MMrotbir7yrKCEXw7nadnHM8Dq38EGfSh6dqA9QWTyefMLEcBYJUuekgW4BYPJcr9E7j",
},
// Test vector 3

This comment has been minimized.

Copy link
@corpetty

corpetty Aug 9, 2018

can you reference where this came from for future folks looking at this?

This comment has been minimized.

Copy link
@gravityblast

gravityblast Aug 9, 2018

Author Member

good point @corpetty, I'm adding it to the PR description, it's from here:

https://github.com/bitcoin/bips/blob/master/bip-0032.mediawiki#test-vector-3

This comment has been minimized.

Copy link
@mandrigin

mandrigin Aug 9, 2018

Member

PR descriptions/commit messages are pretty ephemeral. Let's put all the references in the code so no refactorings can destroy it!

This comment has been minimized.

Copy link
@gravityblast

gravityblast Aug 10, 2018

Author Member

@mandrigin I added more comments in the code taken from the PR description

This comment has been minimized.

Copy link
@gravityblast

gravityblast Aug 14, 2018

Author Member

hey @mandrigin @corpetty is there anything else you want to change in this PR?

@adambabik

This comment has been minimized.

Copy link
Member

commented Aug 11, 2018

Please merge develop. The CI should pass normally for this PR before merging.

@adambabik adambabik merged commit fc3978a into develop Sep 4, 2018

3 checks passed

GPG All commits have a verified GPG signature
codeclimate All good!
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@adambabik adambabik deleted the fix/bip32-keys-derivation branch Sep 4, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.