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

Remove display_random feature #4119

Merged
merged 1 commit into from
Aug 27, 2024
Merged

Conversation

andrewkozlik
Copy link
Contributor

@andrewkozlik andrewkozlik commented Aug 22, 2024

Summary

The display_random flag in the ResetDevice message is used during seed generation to display the entropy that was generated internally by Trezor before asking for external entropy from the host. This feature, which is also known as show-entropy in trezorctl, allows the user to cross-check that Trezor generated a valid mnemonic sentence for the given internal and external entropy, i.e. that Trezor really used the externally provided entropy to generate the seed. I have come to the conclusion that this feature lacks purpose and have decided to remove it. (See detailed explanation below.)

To be clear, the device tests still perform the cross-check by looking at the internal entropy, which is always provided in DebugLinkState.reset_entropy in debug firmware builds, regardless of the display_random flag.

Rationale

The intent of the user is to verify that Trezor is really using the externally provided entropy to generate the seed in order to protect themself from a Trezor that might have malicious firmware which could be generating seeds that come from a relatively small set which is known to the author of the malicious firmware.

Using this feature properly requires that the user has a good understanding of how the two entropies are combined together and how they are converted into a BIP-39 or SLIP-39 backup. The user also needs to at least have the ability to compute SHA-256 on the two entropy values. This implies that the validation cannot be done by hand, but needs to be done on a computer. In actual fact, the user should also compute the XPUB from the seed ideally for all accounts that they will be using, and ensure that the device shows the same XPUBs in the future. Otherwise a malicious firmware could be using a completely different seed to generate XPUBs and addresses than what it presented in the backup.

The user may choose to use this feature in one of several ways:

  1. Generate a seed using display_random, cross-check on their computer that the backup shown on Trezor is correct, and that both the computer and Trezor arrive at the same XPUBs. Repeat several times to be sure. Finally generate a seed without using display_random, record the XPUBs shown on Trezor and use that as their seed, i.e. trust the firmware in the final generation without cross-checking.
  2. Same as in method 1, but generate the final seed using display_random, record the XPUBs shown on Trezor and use that as their seed, i.e. again trust the firmware in the final generation without cross-checking.
  3. Generate a seed using display_random, cross-check on their computer that the backup and XPUB shown on Trezor is correct and use it as their seed.

Method 1 is actually flawed and creates a false sense of security. If the Trezor firmware is malicious, then it can choose to behave correctly when display_random is used, but maliciously when it is not used. Thus the final generated seed and XPUBs can be rigged.

Method 2 is correct, but as it turns out, not entirely intuitive.

Method 3 means exposing the final seed to the computer. The process needs to be performed on a highly secured computer that is guaranteed to dispose of all secret data when finished. Ideally a live linux distribution without an internet connection.

With all the tooling and knowledge that the user needs, it is easier to just generate a seed outside of Trezor and recover it in Trezor. For this the user will need the same setup as in method 3, but does not need to know the details of how Trezor combines the randomness and derives the seed and thus no need for Trezor-specific tooling. To reap the full benefits of Trezor's internal TRNG, they can use trezorctl crypto get-entropy and combine the result with randomness from any other sources in any way they choose.

@andrewkozlik andrewkozlik added core Trezor Core firmware. Runs on Trezor Model T and T2B1. T1B1 legacy Trezor One protobuf Structure of messages exchanged between Trezor and the host python Pull requests that update Python code labels Aug 22, 2024
@andrewkozlik andrewkozlik self-assigned this Aug 22, 2024
Copy link

github-actions bot commented Aug 22, 2024

core UI changes device test click test persistence test
T2T1 Model T test(screens) main(screens) test(screens) main(screens) test(screens) main(screens)
T2B1 Safe 3 test(screens) main(screens) test(screens) main(screens) 2724
T3T1 test(screens) main(screens) test(screens) main(screens) test(screens) main(screens)
All main(screens)

Copy link

github-actions bot commented Aug 22, 2024

legacy UI changes device test(screens) main(screens)

@andrewkozlik andrewkozlik force-pushed the andrewkozlik/display_random branch 2 times, most recently from 5864ca8 to d09fe09 Compare August 22, 2024 20:40
@@ -253,7 +253,6 @@ class TR:
device_name__change_template: str = "Change device name to {0}?"
device_name__title: str = "Device name"
entropy__send: str = "Do you really want to send entropy?"
entropy__title: str = "Internal entropy"
entropy__title_confirm: str = "Confirm entropy"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the entropy__title_confirm still used somewhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it's a different thing. When you use trezorctl crypto get-entropy.

@andrewkozlik andrewkozlik added the translations Put this label on a PR to run tests in all languages label Aug 23, 2024
@andrewkozlik andrewkozlik force-pushed the andrewkozlik/display_random branch from d09fe09 to fec3881 Compare August 23, 2024 06:28
@matejcik
Copy link
Contributor

i'd prefer doing this only in combination with the "iterated ResetDevice" thing you proposed on Slack. as it stands, we'd be removing a good if obscure feature (specifically method 2) without replacement

@andrewkozlik andrewkozlik force-pushed the andrewkozlik/display_random branch from fec3881 to f32474a Compare August 26, 2024 17:33
@andrewkozlik andrewkozlik marked this pull request as ready for review August 27, 2024 11:23
@andrewkozlik andrewkozlik requested a review from matejcik as a code owner August 27, 2024 11:23
@andrewkozlik
Copy link
Contributor Author

I intend to prioritize the development of the proof of seed randomness (iterated ResetDevice) feature, which should be a more user friendly and more effective replacement for display_random.

@andrewkozlik andrewkozlik force-pushed the andrewkozlik/display_random branch from 2df710a to 1655c53 Compare August 27, 2024 15:18
@andrewkozlik andrewkozlik merged commit 2a567f3 into main Aug 27, 2024
122 checks passed
@andrewkozlik andrewkozlik deleted the andrewkozlik/display_random branch August 27, 2024 17:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Trezor Core firmware. Runs on Trezor Model T and T2B1. protobuf Structure of messages exchanged between Trezor and the host python Pull requests that update Python code T1B1 legacy Trezor One translations Put this label on a PR to run tests in all languages
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants