search

sparrowwallet / sparrow

Desktop Bitcoin Wallet focused on security and privacy. Free and open source.
https://sparrowwallet.com/
Apache License 2.0
1.32k stars 188 forks source link

Multisig signing on Trezor T missing transaction details #621

Closed Francisco-DAnconia closed 2 years ago

Francisco-DAnconia commented 2 years ago

Somewhere between Sparrow 1.6.3 and 1.6.5, the Trezor lost the ability to confirm a change amount & address. Additionally, the change amount is excluded from the total. This happens on a Trezor Model T with either 2.4.3 or 2.5.1 firmware.

A Testnet transaction with: input 0 - 40,509 sats output 0 - external - 10,000 sats to tb1q54m7… output 1 - change - 30,320 sats to tb1qgvun… fee 189 sats

With Sparrow 1.6.5, when signing the multisig transaction on the Trezor, one is asked to confirm: 0.0001 TEST to tb1q54m7… Locktime of 2285229 Total 0.00010189 TEST including a fee of .00000189 TEST

The change output and the address are missing! The change output is NOT included in the total.

In contrast, Sparrow 1.6.3 for the identical transaction, one is asked to confirm: 0.0003032 TEST to tb1qgvun… 0.0001 TEST to tb1q54m7… Locktime of 2285213 Total 0.00040509 TEST including fee of .00000189 TEST

I would consider this somewhat serious. If the machine with Sparrow were to be compromised and an attacker routed the change to an address they controlled, you would not be able to discern this on the Trezor.

craigraw commented 2 years ago

Yes, Sparrow 1.6.3 uses HWI 2.0.2, and Sparrow 1.6.5 uses HWI 2.1.1 which incorporates change detection in this scenario. Change detection arguably makes the UX when approving a transaction more understandable and thus the verification process more effective by separating out amounts that are leaving the wallet from amounts that are transferred within it.

If the machine with Sparrow were to be compromised and an attacker routed the change to an address they controlled, you would not be able to discern this on the Trezor.

The Trezor calculates which output is change based on its own information (the stored seed), not on the data provided by Sparrow.

Francisco-DAnconia commented 2 years ago

The Trezor calculates which output is change based on its own information (the stored seed), not on the data provided by Sparrow.

While it may make the UX cleaner for single signature application, I can't see how it isn't a huge step backwards wrt security in a multi-signature environment. Here's my understanding - please disabuse me if I've misunderstood something.

The Trezor is a stateless machine.

When a multi-signature wallet is setup, there is absolutely no means to register the other co-signers' XPUBs from the multi-sig coordinator (Sparrow) on the Trezor. Thus all the Trezor "knows" is it's own XPUB (derived from the seed). Since it lacks access to the other XPUBs in the multi-signature wallet, it cannot know if an address (internal or external) is legitimate or not. By choosing not to display the address on the trusted display of the hardware keystore, the user looses a mechanism to double check an address and know exactly the nature of the transaction they are signing.

Here's Michael Flaxman on this specific attack surface: https://btcguide.github.io/known-issues/verify-receive-address

Another Multisignature pitfalls article: https://shiftcrypto.ch/blog/the-pitfalls-of-multisig-when-using-hardware-wallets/

Once you managed to receive some coins securely, how can you spend them with confidence? The hardware wallet has to verify, or let you verify, the following information provided by the untrusted computer: • The recipient’s address, displayed and confirmed by the user like with singlesig • The change address, having the same cosigners and threshold in an ordinary multisig script with no other spending conditions • The change goes to an address at a keypath recoverable by the user

If I were interested in pursuing this, how might you recommend I proceed? (Andrew Chow? Trezor?)

Thanks (and Sparrow is awesome - I appreciate the effort)

craigraw commented 2 years ago

You make a fair point on multisig - it is indeed a weakness of the Trezor that it cannot store a multisig wallet descriptor, and thus cannot independently verify change addresses.

If I were interested in pursuing this, how might you recommend I proceed? (Andrew Chow? Trezor?)

Please add an issue on the HWI project: https://github.com/bitcoin-core/HWI/issues

I will reopen this in the meantime.

Francisco-DAnconia commented 2 years ago

I don't understand the technical details, which concerns me. But the clear consensus over at HWI is that this is not a problem and Andrew closed it.

https://github.com/bitcoin-core/HWI/issues/620

craigraw commented 2 years ago

I think it's now been well explained in that issue - always good to check your understanding!