Feat/connect: Entropy check workflow in ResetDevice #15887
Conversation
|
🚀 Expo preview is ready!
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
szymonlesisz
force-pushed
the
feat/connect-display_random
branch
5 times, most recently
from
cfdd832 to
89b3054
Compare
| this.params.entropy_check = false; | ||
| } | ||
| // Entropy check workflow: | ||
| // https://github.com/trezor/trezor-firmware/blob/andrewkozlik/display_random/docs/common/message-workflows.md#entropy-check-workflow |
There was a problem hiding this comment.
TODO: update link
szymonlesisz
force-pushed
the
feat/connect-display_random
branch
from
89b3054 to
c4ee1cc
Compare
szymonlesisz
requested review from
mroz22,
martykan,
marekrjpolak and
Nodonisko
as code owners
|
i guess it was premature excitement regarding it takes ~8sec. to perform i'm fallbacking to WebCrypto available in node.js and web (takes ~300ms to calculate) |
| xpubs, | ||
| }); | ||
| if (res.error) { | ||
| throw new Error(res.error); |
There was a problem hiding this comment.
TODO: return specific error to suite
| return response.message; | ||
| if (this.params.entropy_check && this.device.unavailableCapabilities['entropyCheck']) { | ||
| // entropy check requested but not supported by the firmware | ||
| this.params.entropy_check = false; |
There was a problem hiding this comment.
From a security perspective, if the firmware does not support the entropy check, we should display the same error message as we would if the entropy check failed.
The reason is that a counterfeit trezor could bypass the entropy check by pretending not to support it, that is the trezor can present itself as having a lower firmware version than it actually does. However, we can instruct the user to update the firmware before generating their seed.
There was a problem hiding this comment.
this feature needs to be backward compatible with firmwares which does not support it for example afaik T1 will not be supported.
i need to rely on something to make the decision whether ResetDeviceContinue will be sent or not.
following your example counterfeit trezor could bypass this check by pretending that its T1 model.
I think suite in the first onboarding step does FW version check (and asks you to update) then in the next step you create a seed.
We were discussing this version check with @andrewkozlik
| BackupType.Slip39_Basic, | ||
| BackupType.Slip39_Advanced, |
There was a problem hiding this comment.
The implementation of slip39 in this file assumes that the backup is extendable, meaning that using a non-extendable backup will likely result in the "verifyEntropy xpub mismatch" error.
There was a problem hiding this comment.
i did it 1:1 with python-trezor implementation, which assumes that as well:
https://github.com/trezor/trezor-firmware/blob/ce803a5452c5d12eb7b026fa2c20b521957fb8c8/python/src/trezorlib/device.py#L268
seed = shamir_mnemonic.cipher.decrypt(
secret, b"", iteration_exponent=1, identifier=0, extendable=True
)
There was a problem hiding this comment.
Fair enough.
However, I think both implementations should raise an exception in this case. Nonetheless, this is not a security issue.
|
|
||
| return currentData; | ||
| } | ||
|
|
||
| async run() { |
There was a problem hiding this comment.
If the entropy check fails, a small error message is displayed to the user and the user can continue with the onboarding process. We should warn the user similarly to how they are warned when the authenticity check fails.
I think this has already been discussed in person so I'm adding this comment merely as a reminder.
There was a problem hiding this comment.
yes, i've left a TODO above: return specific error to suite
btw. when this happens user cannot continue with onboarding as the seed was not created (ResetDeviceFinish was not called)
| private async entropyCheck(prevData: EntropyRequestData): Promise<EntropyRequestData> { | ||
| const cmd = this.device.getCommands(); | ||
| const paths = ["m/84'/0'/0'", "m/44'/60'/0'"]; | ||
| const xpubs: Record<string, string> = {}; // <path, xpub> |
There was a problem hiding this comment.
We should verify that these XPUBs match the XPUBs later used by the suite to derive change and receive addresses.
I think this has already been discussed in person so I'm adding this comment merely as a reminder.
There was a problem hiding this comment.
yes, marked as "followup" above
szymonlesisz
force-pushed
the
feat/connect-display_random
branch
from
75c85bd to
407ff07
Compare
|
rebased, resolved conflicts (dependencies, yarn.lock) and implemented recent changes - renamed protobuf fields |
szymonlesisz
force-pushed
the
feat/connect-display_random
branch
from
407ff07 to
dc41db0
Compare
Description
implementation of entropy check workflow
im using
@noble/hasheswhich looks legit and is widely used by our dependencies already.we should consider to use it in packages which are woriking with
cryptoand maybe dropcrypto-browserify?i had some doubts should we use slip39-ts lib (see disclaimer) to get decryped slip-39 secret so i ended up porting 3 functions from our python-shamir-mnemonic repo
TODO:
Follow up: