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

keepkey, trezor: Handle if ButtonRequest is sent during sendpin #542

Merged
merged 2 commits into from
Nov 10, 2021

Conversation

achow101
Copy link
Member

@achow101 achow101 commented Nov 9, 2021

Keepkey and Trezor may ask the user for confirmation of their passphrase after the PIN is sent. This could cause some issues as ButtonRequests were not being handled by sendpin. Additionally, TrezorClient.call would not work because it needs access to TrezorClient.features, which is not set for the sendpin command. In order to resolve this, TrezorClient.call is modified to optionally skip the firmware check that requires TrezorClient.features, and sendpin uses TrezorClient.call instead of TrezorClient.call_raw.

Fixes #526
Fixes $539

@prusnak
Copy link
Collaborator

prusnak commented Nov 9, 2021

Seems like this is the same issue that PR #540 is trying to fix

In another word, this also fixes #539

@cjackie can you please confirm this works for you?

Keepkey is now displaying the passphrase to users after it receives it
and responding with a ButtonRequest message. However this was not being
properly handled by sendpin which would cause the device to hang as it
waited for a ButtonAck. To resolve this, use self.call but avoid the
firmware check which will fail due to the lack of self.features.
@achow101 achow101 force-pushed the keepkey-pass-confirm branch from 0d31d78 to 2fa0c94 Compare November 9, 2021 18:55
@achow101
Copy link
Member Author

achow101 commented Nov 9, 2021

Oh, interesting, I didn't realize that #539 was basically the same problem. I've updated this to actually handle all the callbacks correctly.

Keepkey and Trezor had firmware changes that made sendpin not work when
passphrase protection was enabled and a passphrase was provided. Modify
the test to check for this case.
@cjackie
Copy link

cjackie commented Nov 10, 2021

Hi @prusnak, I can verify that this patch works.

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

Successfully merging this pull request may close these issues.

Keep Key HWI issue?
3 participants