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

Print the sha256 hash of data presented to trezor for Ethereum signing #3170

Closed
catwith1hat opened this issue Jul 21, 2023 · 8 comments
Closed
Labels
code Code improvements

Comments

@catwith1hat
Copy link

In Ethereum when signing a smart-contract related transaction with calldata, the require_confirm_data transaction defined in layout.py displays all the bytes inside the data object with pagination. That's great, except impractical for larger amounts of data. E.g. initiating a staking deposit transaction will take up at least 5 screens full of random numbers on the Trezor T. I would strongly prefer that Trezor would show the sha256 hash right at the beginning, so that I have reasonable confidence that individual bytes were not mutated by the host. (I am aware that I need to get the data unmodified to another trusted device with a display, but that is not too hard.)

I would suggest that we either change the function require_confirm_data function that's specific to Ethereum or the generic confirm_blob function defined here. E.g. inserting:

    description += "SHA256: " + sha256(bytes).digest()

into confirm_blob should do the trick. I assumed that the sha256 class here could be imported somehow.

@Hannsek I saw that you are working on a new Eth signing flow. Please consider adding a hash.

@catwith1hat catwith1hat added the code Code improvements label Jul 21, 2023
@matejcik
Copy link
Contributor

(I am aware that I need to get the data unmodified to another trusted device with a display, but that is not too hard.)

Isn't it? I was under the impression that this is the hard problem in this whole thing. Without it there is no point in doing this at all -- a subverted Metamask can just lie to you about the hash.

Anyway, if we did want to do it, it wouldn't be a big problem to use the hash as the content of the screen with the "show all" button.

But right now we're actually reviving #69 so this is might become obsolete.

@catwith1hat
Copy link
Author

To make this a bit more concrete: I am trying to safely sign a Deposit transaction that calls the Ethereum staking contract with my Trezor. Call it "directly staking from your hardware wallet" if you want. In the Deposit transaction I can already set the withdrawal address, and I could pick a Trezor address for that. This would allow me to enter and exit staking from a hardware wallet. (It would nice publicity for Trezor to have a blog post about that btw). Anyways...

For staking, I need a trusted offline device that computes the BLS keys for my validators from my mnemonic. The staking-deposit-cli also drops a deposit-data.json, that I must use as calldata in the Deposit transaction mentioned above. I can easily sha256 deposit-data.json on my trusted device before exposing it to a potentially malicious Metamask.

To stay safe currently, I need to visually verify the whole deposit calldata blob and make sure Metamask didn't modify the calldata and for example substituted the withdrawal key with something else. However, the calldata is 800 hex characters long. This is really tedious and error prone. My withdrawal address is hidden somewhere in the middle. Here is an example contract invocation. Click on "+ Click to show more" to see the input data.

(I am aware that I need to get the data unmodified to another trusted device with a display, but that is not too hard.)

Isn't it? I was under the impression that this is the hard problem in this whole thing. Without it there is no point in doing this at all -- a subverted Metamask can just lie to you about the hash.

Assume my Metamask is compromised. Not in a money-stealing way, but my Metamask just hates people. It introduce bit flips in the calldata. With a "show hash of calldata" feature, I could:

  • I prepare my transaction on my untrusted host.
  • Metamask shows me the transaction calldata.
  • I copy that data to a trusted device or I use the data that's already on a trusted device.
  • On that device, I verify that this is indeed the transaction data, that I want to have signed, e.g. by running an ABI decoder on it. I produce the sha256.
  • Returning to my untrusted device with Metamask, I submit the transaction to Trezor.
  • Metamask is malicious and silently introduces a bit flip in the calldata before sending the data to my Trezor.
  • Trezor shows a different hash.
  • I abort the transaction.

If Trezor wouldn't show a hash, I am very likely to miss a bit flip in 800 characters.

But right now we're actually reviving #69 so this is might become obsolete.

#69 does somewhat help here. It strips length delimiters and makes the data more readable. It assume that you just want to commit some large blob to a contract. Then ABI decoding doesn't really help. Also #69 is technically complex (I believe that you need authoritative smart contract data inside Trezor, also IIRC the ABI is only a convention, so the whole process might fail). In contrast, displaying the sha256 inline with the data is relatively trivial. It's a one-liner fix plus an import statement. We can also opt to stick the hash output to the last page.

Thanks and sorry for the long response.

@Hannsek
Copy link
Contributor

Hannsek commented Aug 4, 2023

How come it doesn't really help here? The contracts would be in a human readable form and user would be able to see and approve all important data such a withdrawal address.

@matejcik
Copy link
Contributor

matejcik commented Aug 4, 2023

@catwith1hat thanks for the detailed explanation, but uhh...
when you have (a) a trusted offline device (b) and you can get data to and from this device
then why not connect Trezor to the trusted device, sign the transaction that way, and sidestep Metamask completely?

As you're describing it, the purpose of showing the SHA is basically error correction. If your environment is trusted, you can just sign the thing and see whether the signature matches the data that you provided. If the environment is not trusted, the right thing to do is not sign in that environment.

I mean, adding a SHA at the end of the output is so simple as to be worth considering, but then there's the thing about costs of mere presence of a feature. And it doesn't seem that anyone but you would (should, even) be using it.


that you just want to commit some large blob to a contract. Then ABI decoding doesn't really help.

True, but neither does the SHA -- unless, again, you have a trusted environment to produce it, in which case just sign in that environment in the first place.

also IIRC the ABI is only a convention, so the whole process might fail

Oh. Oh dear.
I mean, do blockchain devs hate hardware wallets? Do they even want actual real-world people to be able to interact with their tech securely?

@catwith1hat
Copy link
Author

@Hannsek ABI decoding would help my personal case, because in the case of the Deposit transaction, the amount of data to check actually goes down a bit (The deposit_root can be ignored). So I would appreciate it strongly if an ABI decoder would ship. However as said above, it doesn't help with the general case where a user wants to commit an arbitrarily large blob.

From UI point of view, Trezor is already happy to drop 10 pages of Deposit calldata all in raw hex on me, so by my personal taste adding a sha256 hash to the beginning or end of it, is not too much of UI clutter.

Alternatively, we could also opt to expose the hash that gets created under the surface by the signing process. We could alternatively hoist the marked function block in sign_tx.py and notify the user of the content of the digest variable in some of the require_ calls that are inside the function block.

@matejcik
Copy link
Contributor

matejcik commented Aug 4, 2023

Alternatively, we could also opt to expose the hash that gets created under the surface by the signing process.

There's even less point of doing that, because a malicious metamask will just lie about the hash it got back, and an user-hating metamask will corrupt it independent of incoming data.

If you want error correction, you can calculate the expected transaction data on your side (rlp is very simple to implement even by hand, and there's libraries) and verify that the Trezor signature signs it.

@catwith1hat
Copy link
Author

@catwith1hat thanks for the detailed explanation, but uhh... when you have (a) a trusted offline device (b) and you can get data to and from this device then why not connect Trezor to the trusted device, sign the transaction that way, and sidestep Metamask completely?

I don't want to do that because it's not the common staking flow. The officially supported flow is still through Metamask and the staking website of the Ethereum foundation. This is unsafe for many reasons, but that's the way it is. ethereal is the only CLI tool that I found that has offline support. It might have Trezor support (it uses the go-ethereum account abstraction), but why would I risk 32 ETH on an untested integration? That doesn't seem like the right choice. I'd rather build something to insta-OCR the Trezor screen and verify 10 pages of raw hex.

There's even less point of doing that, because a malicious metamask will just lie about the hash it got back, and an user-hating metamask will corrupt it independent of incoming data.

I don't understand why you keep saying that Metamask is involved. The hash would be shown on the Trezor in the confirmation flow. Metamask can't fiddle with it.

As you're describing it, the purpose of showing the SHA is basically error correction.

Yes. SHA is excellent for error correction. A single bit flip changes the whole thing in an unpredictable way. It makes spotting errors really easy. If I would be able to check 10 pages of raw hex reliably, I wouldn't have filed this bug.

If you want error correction, you can calculate the expected transaction data on your side (rlp is very simple to implement even by hand, and there's libraries) and verify that the Trezor signature signs it.

Nit: I would rather like to check things before Trezor produces a signature.

Anyway, looks like there is no consensus for this. Thanks for the discussion.

@matejcik
Copy link
Contributor

matejcik commented Aug 4, 2023

I don't understand why you keep saying that Metamask is involved. The hash would be shown on the Trezor in the confirmation flow. Metamask can't fiddle with it.

right, sorry, I misunderstood what you're suggesting (i.e., "send the data hash outside the UI", as opposed to "show in UI a hash of something else")

I don't want to do that because it's not the common staking flow.
(...)
untested integration

Well, you need the capability to build/check the ABI representation on the trusted machine anyway. If you have that, you should have no trouble identifying the rest of the parameters, namely:

  • chain_id (1 for ETH)
  • derivation path (m/44h/60h/0h/0/0 is metamask default)
  • gas limit
  • max gas fee + max priority fee (for eip1559) or the older single gas-price
  • your next nonce
  • destination contract address
  • ETH value to send

then you can use trezorctl to sign fully offline.
From my POV, the difficult part is getting the calldata bytes (and their hash) -- but if you solve that (or if it is easy for you), the "sign on Trezor" part is a single trezorctl ethereum sign-tx invocation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code Code improvements
Projects
Archived in project
Development

No branches or pull requests

3 participants