Skip to content
This repository has been archived by the owner on May 28, 2019. It is now read-only.

Validate BIP32 paths according to BIP44, etc. #326

Closed
alepop opened this issue Aug 20, 2018 · 26 comments
Closed

Validate BIP32 paths according to BIP44, etc. #326

alepop opened this issue Aug 20, 2018 · 26 comments
Assignees
Milestone

Comments

@alepop
Copy link
Contributor

alepop commented Aug 20, 2018

Altcoins use constants for coin_type' path in bip32 that are defined in slip-0044. But Trezor does not validate this path for altcoins and you can pass for example ETH address to Lisk functions and vice versa. Should validation be implemented in a device or it is not worth for it?

@prusnak
Copy link
Member

prusnak commented Aug 21, 2018

Yes, as a part of bigger refactoring, we are going to validate all used BIP32 paths. If an unknown path is encountered, a warning/error is shown, depending on the coin. Coin strictly adhering to SLIP44 will use error, bitcoin-like coins will show warning to support non-BIP44 wallets and various claim tools.

@prusnak prusnak changed the title Validate coin_type path level Validate BIP32 paths according to BIP44, etc. Aug 21, 2018
@prusnak
Copy link
Member

prusnak commented Aug 21, 2018

Recognized paths for all coins:

  • m/44'/c'/a'/0/x where c is the matching SLIP44 coin type, a < 10, x < 1000000 (bip44 external)

Recognized paths for all coins which use change addresses:

  • m/44'/c'/a'/1/x where c is the matching SLIP44 coin type, a < 10, x < 1000000 (bip44 change)

Recognized paths for segwit-enabled coins:

  • m/49'/c'/a'/0/x where c is the matching SLIP44 coin type, a < 10, x < 1000000 (bip49 external)
  • m/49'/c'/a'/1/x where c is the matching SLIP44 coin type, a < 10, x < 1000000 (bip49 change)
  • m/84'/c'/a'/0/x where c is the matching SLIP44 coin type, a < 10, x < 1000000 (bip84 external)
  • m/84'/c'/a'/1/x where c is the matching SLIP44 coin type, a < 10, x < 1000000 (bip84 change)

For GetPublicKey just check the first 3 elements, that is m/49'/c'/a' for example.

@tsusanka
Copy link
Contributor

Ok, great! Just a recap: we introduce a function, which takes address_n and coin as an argument and returns a node. Along the way it checks the bip-32 path against the list provided by @prusnak.

In case of bitcoin-like coins failure leads to a warning, in other coins it is a hard error.

I'll take this.

@tsusanka tsusanka self-assigned this Aug 21, 2018
@yura-pakhuchiy
Copy link
Contributor

What about multisig wallets? Are they expected to use the same paths or different paths?
Electrum uses m/45' for legacy addresses and m/48' for segwit. Copay also uses m/48'. I think it is worth to support these 2 cases.

@prusnak
Copy link
Member

prusnak commented Aug 21, 2018

@yura-pakhuchiy yes, for multisig we should rather check BIP45 and BIP48 schemes.

@yura-pakhuchiy
Copy link
Contributor

BTW, firmware 2.0.7 partially fixed this issue in 51df249
However there is a glitch, if user clicks red button on warning screen, it does not actually cancel transaction signing and just continues as if user clicked green button.
Also warning might be confusing for users when they see it for the first time, I suggest to add shortened URL which will explain the issue and possible risks.

@tsusanka
Copy link
Contributor

@alepop the Lisk bip32 path should start m/44'/134'/a' that's for sure. But does Lisk have a concept of "change" similar to bitcoin? Or is it more like Ethereum where one account equals one long-term address? If so, we could use only the three part address m/44'/134'/a', where a' is the account index and validate it is not longer. What do you think?

@alepop
Copy link
Contributor Author

alepop commented Aug 23, 2018

@tsusanka no, Lisk doesn't have "change" concept. But I prefer to have account and address_index path in Lisk bip32. So I would like to allow to use this m/44'/134'/a' as minimum allowed path format and this m/44'/134'/a'/0'/b' as a maximum. Of course, better is to have m/44'/134'/a'/b' format but this is not a standard way.

@tsusanka
Copy link
Contributor

@alepop what is the rational behind having an address_index? Since it does not have the change concept it seems rather redundant. Since account = address you can create another one by simple incremeting a

@ghost
Copy link

ghost commented Aug 27, 2018

Hi all,

I have proposed something like this to be implemented in Lisk wallets,
Lisk Hub - LiskHQ/lisk-desktop#1192
Lisk Nano - LiskArchive/lisk-nano#1084

Lisk accounts are more similar to ethereum than to Bitcoin, however Lisk allows to enable second signature to each account.
Meaning that primary accounts shall be derived from

m/44'/134'/0'/0'/0'
m/44'/134'/0'/0'/1'
m/44'/134'/0'/0'/2'
m/44'/134'/0'/0'/3'

and respectively second signature (if requested)

m/44'/134'/1'/0'/0'
m/44'/134'/2'/0'/0'
m/44'/134'/3'/0'/0'
m/44'/134'/4'/0'/0'

I'm neither strong, nor very familiar with cryptography but I believe that ed25519 provides 2^126 bits of security, so enabling second signature in Lisk should actually increase security up to 2^127 (?) either way, since this is common in Lisk to have second signature in account - enabling it with another path in Trezor, sounds good to me.

Here is simple script utilising trezorctl with mentioned paths implemented.
https://github.com/karek314/LiskTrezor-CLI-wallet-manager

@prusnak
Copy link
Member

prusnak commented Aug 27, 2018

I don't like the idea you presented. I think the usage should be the following:

m/44'/134'/0'
m/44'/134'/1'
m/44'/134'/2'
etc.

Can you describe the "second signature" concept? It seems dubious at best.

@alepop
Copy link
Contributor Author

alepop commented Aug 27, 2018

@prusnak in the future Lisk will drop SDK for developing side chains and this will allow creating dApp's on Lisk blockchain. I think it is better to provide the next usage for Lisk bip32 - m/44'/134'/chain/index

About the second signature.

Lisk also offers the option of an additional layer of security. Using a specific type of transaction, the user can register a second passphrase that is associated with the account. This relationship requires all subsequent transactions to be additionally signed using the second passphrase in order to be considered valid. The process of generating the second key pair is the same as the one for the initial key pair.

@tsusanka
Copy link
Contributor

I think let's allow m/44'/134'/a' at this moment only and all the other usage will generate a warning. If some common usage of m/44'/134'/a'/0'/i' comes to Lisk (dapps or whatever), we will allow those as well (with no warning).

@ghost
Copy link

ghost commented Aug 28, 2018

How about second signature? It is very commonly used. While adding it from the same device provides rather minimal increase of security there could be an option in Lisk wallets to add it with another Trezor device.

@alepop
Copy link
Contributor Author

alepop commented Aug 28, 2018

@tsusanka @prusnak ok, let's go with m/44'/134'/a' for now and with the warning for other usages.

@tsusanka
Copy link
Contributor

How about second signature? It is very commonly used. While adding it from the same device provides rather minimal increase of security there could be an option in Lisk wallets to add it with another Trezor device.

@karek314 as said, let's deal with the basic scenario first. If we add multiple signature support to trezor, we'll allow those paths.

@tsusanka @prusnak ok, let's go with m/44'/134'/a' now and with the warning for other usages.

Great.

@prusnak
Copy link
Member

prusnak commented Aug 28, 2018

@karek314 It's a warning, so user will still be able to do this.

But anyway, I would advise stricly against using these two schemes:

m/44'/134'/a'/0'/0'
m/44'/134'/0'/0'/b'

They are non-systematic and not very obvious why it was selected like this. If I were you, I'd retract the two pull requests you linked above.

@ghost
Copy link

ghost commented Aug 28, 2018

@prusnak So what else do you propose, because I think that using
m/44'/134'/n' for basic account
m/44'/134'/5000-n' for second signature

5000 is just example, it can be max value of allowed variable, but still.

Isn't better design than what I proposed and I don't see any other possibilities

@prusnak
Copy link
Member

prusnak commented Aug 28, 2018

m/44'/134'/n' for basic account
m/44'/134'/n'/0' for second signature for example

@tsusanka
Copy link
Contributor

Let's move that discussion to another issue please.

@ghost
Copy link

ghost commented Aug 28, 2018

@prusnak Can we agree that it will be official path of generating second signature for Lisk in the same device? So I can adjust my cli wallet and amend accordingly issues raised in Lisk wallets repos ?

@prusnak
Copy link
Member

prusnak commented Aug 28, 2018

@karek314 yes

@yura-pakhuchiy
Copy link
Contributor

Why a in m/44'/c'/a'/0/x must be lower than 10? I think users who like to use many accounts will easily hit this limit. Why not 100 or 1000?

@prusnak
Copy link
Member

prusnak commented Aug 29, 2018 via email

@tsusanka
Copy link
Contributor

Anyone has a better wording?

screenshot from 2018-08-29 09-00-54

@tsusanka
Copy link
Contributor

tsusanka commented Dec 3, 2018

This is done and was merged. It does not yet work for Bitcoin's GetPublicKey, which needs some modifications in trezor wallet (maybe already done). Will be done in https://github.com/trezor/trezor-core/issues/406

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

4 participants