trezor / trezor-core

:lock: Don't use this repo, use the new monorepo instead:
https://github.com/trezor/trezor-firmware
GNU General Public License v3.0
354 stars 204 forks source link

Validate BIP32 paths according to BIP44, etc. #326

Closed alepop closed 5 years ago

alepop commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

Recognized paths for all coins:

Recognized paths for all coins which use change addresses:

Recognized paths for segwit-enabled coins:

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

tsusanka commented 6 years ago

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.

yura-pakhuchiy commented 6 years ago

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 commented 6 years ago

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

yura-pakhuchiy commented 6 years ago

BTW, firmware 2.0.7 partially fixed this issue in https://github.com/trezor/trezor-core/commit/51df2499495cbf9337fcade365fc10c252dd1e79 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 commented 6 years ago

@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 commented 6 years ago

@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 commented 6 years ago

@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 commented 6 years ago

Hi all,

I have proposed something like this to be implemented in Lisk wallets, Lisk Hub - https://github.com/LiskHQ/lisk-hub/issues/1192 Lisk Nano - https://github.com/LiskHQ/lisk-nano/issues/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 commented 6 years ago

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 commented 6 years ago

@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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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

tsusanka commented 6 years ago

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 commented 6 years ago

@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 commented 6 years ago

@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 commented 6 years ago

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

tsusanka commented 6 years ago

Let's move that discussion to another issue please.

ghost commented 6 years ago

@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 commented 6 years ago

@karek314 yes

yura-pakhuchiy commented 6 years ago

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 commented 6 years ago

This is a soft limit.

tsusanka commented 6 years ago

Anyone has a better wording?

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

tsusanka commented 5 years ago

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