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

Model T no longer allows arbitrary paths for signMessage or signTransaction #1044

Closed
jlopp opened this issue Jun 5, 2020 · 43 comments
Closed
Assignees
Labels
core feature
Milestone

Comments

@jlopp
Copy link

@jlopp jlopp commented Jun 5, 2020

Describe the bug
Prior to firmware 2.3.1 (2.3.0 works) it is possible to sign a message with a nonstandard derivation path; the Model T would simply prompt that this "unknown path" was acceptable to sign for. As of 2.3.1 the prompt no longer occurs and message signing immediately fails.

Firmware version or revision
Firmware 2.3.1

Desktop/smartphone setup:
Trezor Connect 6, 7, & 8
Debian 9
Chrome 83.0

To Reproduce
Use Trezor Connect to request signing a message using a nonstandard path.

Expected behavior
A prompt to sign the message, which then succeeds upon being accepted by the user.

Screenshots
Screenshot from 2020-06-04 18-55-22

@jlopp jlopp added the bug label Jun 5, 2020
@tsusanka
Copy link
Contributor

@tsusanka tsusanka commented Jun 5, 2020

Thanks for the report. Could you share with us which nonstandard derivation paths you are using and what is the motivation behind it?

@prusnak prusnak removed the bug label Jun 5, 2020
@jlopp
Copy link
Author

@jlopp jlopp commented Jun 5, 2020

Casa's hardware device-based multisig system uses paths in the form of m/49/0/X/0/0 (where X increments with each device rotation)

Message signing along these paths is used as a health check verification for each device to prove that it is still functional. This is a critical feature of our system so that users can regularly verify the integrity of their distributed setup without having to transact.

@matejcik
Copy link
Contributor

@matejcik matejcik commented Jun 5, 2020

Casa's hardware device-based multisig system uses paths in the form of m/49/0/X/0/0 (where X increments with each device rotation)

so it's a non-hardened path all the way?

@jlopp
Copy link
Author

@jlopp jlopp commented Jun 5, 2020

Correct. I'd also note that the Trezor One's latest firmware still supports message signing on arbitrary paths; this appears to be a Model T specific change.

@prusnak
Copy link
Member

@prusnak prusnak commented Jun 5, 2020

You can use any of the valid paths as a health check. I recommend using Testnet path such as m/44'/1'/0'/0/0 or m/44'/1'/0'/0/X for your case.

@prusnak prusnak closed this as completed Jun 5, 2020
@jlopp
Copy link
Author

@jlopp jlopp commented Jun 5, 2020

Theoretically (with additional work) yes, any path could suffice.

To be clear, Casa's back end only has root xpubs that it uses to coordinate generation of multisig addresses, verification of signed messages, and so on. It does not have access to private keys and, as such, is incapable of hardened path derivation.

I suppose we can add exports of hardened child xpubs, though that will take some time for us to implement. What I don't understand is why this message signing functionality has been removed and why it's inconsistent with the Trezor One.

@jlopp
Copy link
Author

@jlopp jlopp commented Jun 5, 2020

After further testing I see that this "Forbidden Key Path" error is also presenting itself during transaction signing. This effectively breaks Model T usage within Casa's multisig system. Please revert this logic to how it worked in the 2.3.0 firmware.

@jlopp jlopp changed the title Model T signMessage no longer allows arbitrary paths Model T no longer allows arbitrary paths for signMessage or signTransaction Jun 5, 2020
@tsusanka tsusanka reopened this Jun 5, 2020
@tsusanka tsusanka self-assigned this Jun 5, 2020
@tsusanka tsusanka added the feature label Jun 5, 2020
@tsusanka
Copy link
Contributor

@tsusanka tsusanka commented Jun 5, 2020

To provide a bit of context: this change in Trezor T is a first step to provide better sandboxing of our apps. In 2.3.1 we are giving the apps (such as bitcoin etc.) only a subset of the keys. That means the bitcoin app now receives only 44'/0' subtree (or similar for segwit etc.) and is not allowed to manipulate with the other keys.

What I don't understand is why this message signing functionality has been removed and why it's inconsistent with the Trezor One.

The fact that this is not present in Trezor One is only due to the fact that we are actively developing the model T firmware and we want to port the whole code base to Trezor One eventually (#24). Tl;dr it will get there eventually.


To summarize: I understand changing xpubs could work for you but the fact that we have forbidden also signing is very problematic for you because users could not access their coins. Correct? Any chance to make them resend their coins to the appropriate paths?

If this is a no-go, could you elaborate whether you are using other paths different than m/49/0/X/0/0?

@jlopp
Copy link
Author

@jlopp jlopp commented Jun 5, 2020

the fact that we have forbidden also signing is very problematic for you because users could not access their coins. Correct?

Yes, that device would become unusable for spending. Depending upon what other devices are in the user's setup, it may or may not completely prevent them from being able to spend funds.

Any chance to make them resend their coins to the appropriate paths?

This will require us to develop a migration plan; I would expect a minimum of several weeks for us to change our xpub export logic to export multiple hardened child xpubs rather than single root xpubs. Then we need to develop a means to prompt all users to perform a "rotation" transaction to sweep their funds.

could you elaborate whether you are using other paths different than m/49/0/X/0/0?

We use m/49/0/X/0/0 for mainnet (incrementing the final two paths per the standard for receive and change addresses. We also use m/49/1/X/0/0 for testnet in the same way. The X portion of the path gets incremented as the user rotates new devices into their key set.

In general I think we are happy to work toward using m/49'/0' and m/49'/1' subtrees, it's just going to take us time to implement and migrate users.

@destrys
Copy link

@destrys destrys commented Jun 5, 2020

I agree with Lopp on this, and restricting signing to particular BIP32 paths will also break Unchained Capital's products.

BIP32 path restrictions aren't part of consensus, I don't think it should be implemented at the hardware wallet level.

And this is a backward-incompatible change. For Trezor users with BTC locked in cold storage addresses using permitted BIP32 previously, you're forcing them to use a different signing method when they choose to move their coins.

@destrys
Copy link

@destrys destrys commented Jun 5, 2020

Another thing to consider: If these restrictions aren't implemented equally across the majority of hardware wallets, you will have users with addresses that aren't able to switch to a different vendor.

There isn't an accepted standard around this, we went with BIP45 when we designed our multisig product so our keys are derived from m/45'/0|1'/

@destrys
Copy link

@destrys destrys commented Jun 5, 2020

We don't have a problem with users being shown the BIP32 path at each interaction (we actually prefer it) and a warning that an action is on a non-standard BIP32 path is fine too, but prohibiting an action because of the BIP32 path seems too far.

@willccole
Copy link

@willccole willccole commented Jun 5, 2020

For the Unchained Capital use case: Upon further investigation, 45' is not currently impacted (it's not mentioned above, but is working fine). We'd like to know if ultimately this path would be impacted so we can make preparations.

@prusnak
Copy link
Member

@prusnak prusnak commented Jun 5, 2020

@willccole No, m/45' is fine, because it is a well-known path.

@browep
Copy link

@browep browep commented Jun 5, 2020

I'm a little surprised at this decision to not allow access outside a subtree after previously allowing users to export xpubs higher up. Unless I am not understanding it, such a change is guaranteed to brick funds. I understand how this increases security in a green field application ( allowing an app to sign something for another coin may be re-used where it was not intended. ) but that would be at the loss of funds for existing users ( which is what the security increase is supposed to prevent! ).

I'll offer some unsolicited advice: in order for backwards compatibility to be achieved, allow api callers to supply the xpub that derived those pub keys in order to access something outside their designated subtree and continue with a warning. This would allow those who have sent coins to those unapproved subtrees to have additional information that an attacker would not likely have.

@greenaddress
Copy link

@greenaddress greenaddress commented Jun 5, 2020

Haven't tested it yet but from what I read in this thread this will likely break all Blockstream Green/GreenAddress users that are using it with Trezor T. Will do some testing and get back.

@prusnak prusnak added this to the backlog milestone Jun 7, 2020
@tsusanka
Copy link
Contributor

@tsusanka tsusanka commented Jun 8, 2020

We have decided we will add exceptions for those of you affected in the next firmware release (most likely 2.3.1 to be released on July 1st or August 5th) if there are no security implications of such exceptions (we will discuss this shortly).


If I am reading the discussion correctly, we will add:

  • m/49/0/X/ and m/49/1/X/ for Casa (cc @jlopp).
  • No exceptions for Unchained Capital because they are unaffected (cc @willccole).
  • Something for @greenaddress - could you please let us know what paths you are using?

We are suggesting we will guarantee these exceptions till the end of 2020, however we still encourage you to start using the proper paths. Although I do not think we will remove these exceptions first day in 2021, the time might come when these exceptions can no longer be present due to some internal changes. I hope this is alright for you?

@tsusanka tsusanka modified the milestones: backlog, 2020-07 Jun 8, 2020
@destrys
Copy link

@destrys destrys commented Jun 8, 2020

Could you point us to where the 'proper paths' are documented?

@prusnak
Copy link
Member

@prusnak prusnak commented Jun 8, 2020

Could you point us to where the 'proper paths' are documented?
https://github.com/trezor/trezor-firmware/blob/master/docs/misc/coins-bip44-paths.md

@jlopp
Copy link
Author

@jlopp jlopp commented Jun 12, 2020

It's not optimal, but I have determined that Model T users can downgrade to 2.3.0 and fix their issue. One of the bigger annoyances is that Trezor Connect displays warnings prompting people to upgrade their firmware; it would be great if we could disable those for our users in order to prevent unnecessary support cases.

@shea256
Copy link

@shea256 shea256 commented Jun 15, 2020

I would like to echo @jlopp and @destrys on this.

This change would make it extremely difficult for many users to continue to use Trezor.

For example, I use some non-standard paths for my keys and this change would mean I would have one of several options:

  1. Not upgrade the firmware, thus forgoing future security updates (not acceptable)
  2. Migrate all of my coins out of these addresses and into new ones (extremely inconvenient)
  3. Switch away from Trezor to hardware wallets that preserve path flexibility (most convenient, lowest risk)

@shea256
Copy link

@shea256 shea256 commented Jun 15, 2020

Additionally, it seems this change imposes undue burden on developers who are creating new apps and experimenting with new currencies and new paths.

There is nothing that makes m/44', m/45', m/48', and m/49' special as paths.

The beauty of the system is that it preserves a decentralized standard creation process.

Each standard is confined to a root number, and since standards are much less common than cryptocurrencies, there is a low chance of a collision happening with future standards proposed.

Anyone can create a new path protocol and pick a new root # and doesn't need permission from a hardware wallet provider to use their standard.

When that standard becomes more widely adopted, it can be officially recognized.

This update breaks that.

@tsusanka
Copy link
Contributor

@tsusanka tsusanka commented Jun 17, 2020

@jlopp we have temporarily disabled the prompt for 2.3.1 upgrade in Trezor Connect (trezor/connect#608). It should be live in production already. Hope that helps.


As mentioned above we will implement #1064 which means allow greenaddress/Casa paths and at the same time we will add a trezorctl setting which will allow all paths for advanced users. So that should work also for you @shea256 if you simply set this setting on.


Since there is no other action now other than implementing #1064 I am proposing to close this issue. I will ping here after we are releasing firmware with #1064 implemented.

@matejcik
Copy link
Contributor

@matejcik matejcik commented Jul 14, 2020

sign tx

  • m/branch/address_pointer
  • m/harden(3)/harden(subaccount_pointer)/branch/address_pointer

where branch is either 1 or 4 and subaccount_pointer is non 0

sign msg

  • m/0x4741b11e
  • m/0x4741b11e/6/pointer

@greenaddress what are the possible values of address_pointer, subaccount_pointer and pointer ?
are these analogous to Trezor's accounts (we currently allow 20, but might bump that to 100) or address indices (we allow 1 000 000) ?

@tsusanka
Copy link
Contributor

@tsusanka tsusanka commented Jul 24, 2020

Ping @greenaddress could you please answer @matejcik's question?

@LeoComandini
Copy link

@LeoComandini LeoComandini commented Jul 30, 2020

sign tx

  • m/branch/address_pointer
  • m/harden(3)/harden(subaccount_pointer)/branch/address_pointer

where branch is either 1 or 4 and subaccount_pointer is non 0
sign msg

  • m/0x4741b11e
  • m/0x4741b11e/6/pointer

@greenaddress what are the possible values of address_pointer, subaccount_pointer and pointer ?
are these analogous to Trezor's accounts (we currently allow 20, but might bump that to 100) or address indices (we allow 1 000 000) ?

subaccount_pointer must be less than 16384
https://github.com/Blockstream/gdk/blob/master/src/ga_session.cpp#L1959

pointer must be less than 0x80000000
https://github.com/Blockstream/gdk/blob/master/src/ga_session.cpp#L1700-L1703

For address_pointer, there is no explicit limit, but the server enforces that are generated incrementally (no gaps) and does some rate limiting. Thus the same limit as yours (1 000 000) should be ok for all our users.

@tsusanka
Copy link
Contributor

@tsusanka tsusanka commented Jul 30, 2020

@LeoComandini Thanks!


Also @greenaddress @jlopp do you think you could find a few minutes and test current master against your products? It should be fixed in master.

@tsusanka
Copy link
Contributor

@tsusanka tsusanka commented Jul 31, 2020

@greenaddress @jlopp We will leave these exceptions for now, but we will most likely reevaluate it next year. Don't worry it won't happen before July 2021, but the time might come when we decide to forbid these paths. We will definitely allow it for people that will explicitly ask for it via the trezorctl settings (see #1064) but it might be an inconvenience for your users. So in a nutshell: we promise to keep these exceptions till July 2021 but we still encourage you to migrate your users to the aforementioned paths since we can not guarantee we will keep the exceptions forever. So if you can put this into your roadmap it would be great.

@jlopp
Copy link
Author

@jlopp jlopp commented Aug 13, 2020

Following up on this because I'm trying to sign a testnet transaction with a Model T on the recent 2.3.2 firmware release. I do receive a warning on device: "Path m/49/1/9/0/5 is unknown. Are you sure?" and then I confirm the output addresses on the transaction and don't see any errors on device. However, it looks like trezorconnect is still returning a "Forbidden key path" error.

@matejcik
Copy link
Contributor

@matejcik matejcik commented Aug 14, 2020

@jlopp turns out there is a bug in the fix for this: the allowed paths are m/49/0'/* and m/49/1'/*.
The reason this is succeeding for you is most likely because you have the safety-checks feature enabled.

reopening to make sure this gets fixed properly as part of #1137

@matejcik matejcik reopened this Aug 14, 2020
@matejcik matejcik modified the milestones: 2020-08, 2020-09 Aug 14, 2020
@tsusanka
Copy link
Contributor

@tsusanka tsusanka commented Aug 16, 2020

reopening to make sure this gets fixed properly as part of #1137

@matejcik haven't you meant to link to #1184?

@tsusanka
Copy link
Contributor

@tsusanka tsusanka commented Aug 16, 2020

@jlopp turns out there is a bug in the fix for this: the allowed paths are m/49/0'/* and m/49/1'/*.

PR in #1190.

@tsusanka
Copy link
Contributor

@tsusanka tsusanka commented Aug 19, 2020

#1190 will fix it for Casa. @matejcik let me know if you would like to keep this opened.

@trezor trezor deleted a comment Aug 27, 2020
@tsusanka tsusanka added the core label Aug 28, 2020
@wojt21
Copy link

@wojt21 wojt21 commented Mar 12, 2022

hey

I,m also unable to sign and verify message with trezor model T and electrum
Getting message on my trezor: "wrong address path for selected coin"
Is this a bug?
What I should do?

@bosomt
Copy link

@bosomt bosomt commented Mar 14, 2022

QA OK

i was able to sign and verify Bitcoin message via model T in ELectrum 4.1.5
I got warning that path m/84'/0'/0'/0/0 is unknown path.

Info:

  • OS: MacIntel
  • Screen: 1680x1050
  • Device: model T 2.4.3 regular

obrazek

Screenshot 2022-03-14 at 12 35 31

Screenshot 2022-03-14 at 12 35 22

@matejcik
Copy link
Contributor

@matejcik matejcik commented Mar 14, 2022

that looks like an instance of spesmilo/electrum#7670 @bosomt

@wojt21
Copy link

@wojt21 wojt21 commented Mar 28, 2022

After Umbrell update it's all good.
Nice!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core feature
Projects
None yet
Development

No branches or pull requests