Move Ethereum Ledger integration in untrusted iframe #14373

nvonpentz · 2022-07-27T22:59:27Z

Security Review: https://github.com/brave/security/issues/969

This changes isolates the js libraries required to interact with the Ethereum app on Ledger devices to an iFrame in a chrome untrusted context. This uses the same framework used in the Solana hardware as #14096, but generalizes some code such that it can be reused for both Solana and Ethereum. For example:

Shared logic in SolanaLedgerBridgeKeyring class was moved to a new parent class that LedgerBridgeKeyring that both SolanaLedgerBridgeKeyring and EthereumLedgerBridgeKeyring inherit from
Solana specific logic was moved from LedgerUntrustedMessagingTransport to a new child class SolanaLedgerUntrustedMessagingTransport. Similarly, Ethereum specific logic is now in a child class EthereumLedgerUntrustedMessagingTransport.
Common message types were kept in ledger-messages.ts, but Solana and Ethereum specific message types were separated into their own sol-ledger-messages.ts and eth-ledger-messages.ts module.

Submitter Checklist:

I confirm that no security/privacy review is needed, or that I have requested one
There is a ticket for my issue
Used Github auto-closing keywords in the PR description above
Wrote a good PR/commit description
Squashed any review feedback or "fixup" commits before merge, so that history is a record of what happened in the repo, not your PR
Added appropriate labels (QA/Yes or QA/No; release-notes/include or release-notes/exclude; OS/...) to the associated issue
Checked the PR locally: npm run test -- brave_browser_tests, npm run test -- brave_unit_tests, npm run lint, npm run gn_check, npm run tslint
Ran git rebase master (if needed)

Reviewer Checklist:

A security review is not needed, or a link to one is included in the PR description
New files have MPL-2.0 license header
Adequate test coverage exists to prevent regressions
Major classes, functions and non-trivial code blocks are well-commented
Changes in component dependencies are properly reflected in gn
Code follows the style guide
Test plan is specified in PR before merging

After-merge Checklist:

Test Plan:

The test plan is very similar to the test plan for the parallel Solana untrusted refactor, except instead of testing Solana Ledger functions, we test the Ethereum ones.

Test import accounts flow (chrome://wallet)
1. Connect ledger device and enter password
2. Run (await navigator.hid.getDevices()).forEach((d) => d.forget()) in the js console to forget devices which have already been granted permissions, then refresh the page
3. Click 'Import from hardware wallet' -> 'Connect' for Ethereum, Select 'Ledger', click 'Connect'
4. Verify 'Grant Brave access to your Ledger device.' is displayed, hit 'Authorize'
5. You should be prompted with the permissions screen, select device, hit 'Connect'
6. Verify you are returned to 'Connect your Ledger wallet directly to your computer' screen, hit 'Connect'
7. Verify your accounts are listed for import for import. Verify you can load 'Load more' accounts
8. Verify you can load different accounts by switching derivation paths
9. Select an account and hit 'Add checked accounts', verify it is added
10. Repeat the same flow without forgetting the device first and verify no 'Authorize' screen is displayed or permission screen prompted.
Test signing flows (chrome://wallet-panel) for (a) sign transaction, (b) sign personal, (c) sign typed data
1. Connect ledger device and enter password
2. Create a Ethereum hardware account with a test net balance if you don't have one already
3. Navigate to chrome://inspect > Other
4. Open wallet panel, and click 'inspect' on the chrome://inspect page under the 'chrome://wallet-panel.top-chrome' item that pops up. This is the js console for the panel
5. Run (await navigator.hid.getDevices()).forEach((d) => d.forget()) to forget any device which already has permissions on the panel (need to be quick since this closes not long after the panel closes)
6. Do one of
  1. Create a standard transaction in the panel sending funds to yourself (sign transaction)
  2. Go to OpenSea and like any NFT (I'm partial to lil nouns) by clicking the heart (sign personal)
  3. Go to https://metamask.github.io/test-dapp/, connect wallet, and sign + verify 'Sign Typed Data V3' and 'Sign Typed Data V4'
7. Hit sign / confirm
8. You should be shown an 'Authorize' button on the panel that says the device is disconnect, click the 'Authorize' button
9. You should be prompted with the permissions screen, select device, hit 'Connect'
10. Verify you are returned to the 'Hardware wallet requires Ethereum App opened on Ledger' (it may take a second)
11. On your ledger device, you should see the the transacition / sign request you entered, confirm.
12. Verify the post transaction / signing stuff happens: you are shown the details, get a notification when transaction is confirmed, the 'like' is registered by OpenSea, etc
13. Repeat all steps until all of sign transaction, sign personal, and sign typed data have been completed successfully

components/brave_wallet_ui/common/api/hardware_keyrings.ts

components/brave_wallet_ui/common/async/hardware.ts

josheleonard · 2022-08-04T21:49:24Z

components/brave_wallet_ui/common/hardware/ledgerjs/eth_ledger_bridge_keyring.test.ts

-    'm/44\'/60\'/0\'/0/0', 'domainSeparatorHex', 'hashStructMessageHex'))
-    .resolves.toStrictEqual({ success: false, error: 'some error', code: 'Error' })
-})
+// class MockApp {


Are we removing this file before merge?

components/brave_wallet_ui/common/hardware/ledgerjs/eth_ledger_bridge_keyring_new.test.ts

components/brave_wallet_ui/common/hardware/ledgerjs/ledger_bridge_keyring.test.ts

brave-builds · 2022-08-05T21:39:55Z

A Storybook has been deployed to preview UI for the latest push

components/brave_wallet_ui/common/hardware/ledgerjs/ledger_bridge_keyring.ts

components/brave_wallet_ui/ledger/ledger.ts

brave-builds · 2022-08-09T21:57:34Z

A Storybook has been deployed to preview UI for the latest push

kdenhartog

few nits spotted, but overall this looks good to me

components/brave_wallet_ui/common/hardware/ledgerjs/eth-ledger-untrusted-transport.ts

components/brave_wallet_ui/common/hardware/ledgerjs/eth_ledger_bridge_keyring.ts

components/brave_wallet_ui/common/hardware/ledgerjs/ledger_bridge_keyring.ts

brave-builds · 2022-08-10T18:49:28Z

A Storybook has been deployed to preview UI for the latest push

brave-builds · 2022-08-10T20:09:19Z

A Storybook has been deployed to preview UI for the latest push

brave-builds · 2022-08-10T21:48:48Z

A Storybook has been deployed to preview UI for the latest push

components/brave_wallet_ui/common/hardware/ledgerjs/ledger-messages.ts

brave-builds · 2022-08-17T22:09:00Z

A Storybook has been deployed to preview UI for the latest push

components/brave_wallet_ui/ledger/ledger.ts

josheleonard

Frontend ++

brave-builds · 2022-08-18T01:06:49Z

A Storybook has been deployed to preview UI for the latest push

...ts/brave_wallet_ui/components/shared/authorize-hardware-device/authorize-hardware-device.tsx

This change isolates the ledgerjs libraries required to interact with the Ethereum app on Ledger devices to an iFrame in a chrome untrusted context. It reuses the framework established in the Ledger Solana Ethereum refactor, but generalizes some code such that it can be reused for both Solana and Ethereum. For example: * Shared logic in SolanaLedgerBridgeKeyring class has been moved to a new parent class that LedgerBridgeKeyring that both SolanaLedgerBridgeKeyring and EthereumLedgerBridgeKeyring that both inherit from * Solana specific logic was moved out of LedgerUntrustedMessagingTransport and into a new child class, SolanaLedgerUntrustedMessagingTransport. Similarly, Ethereum specific logic is now in a new child class, EthereumLedgerUntrustedMessagingTransport that inherits from LedgerUntrustedMessagingTransport. * Common message types were kept in ledger-messages.ts, but Solana and Ethereum specific message types were separated into their own sol-ledger-messages.ts and eth-ledger-messages.ts modules respectively.

brave-builds · 2022-08-18T17:16:00Z

A Storybook has been deployed to preview UI for the latest push

github-actions bot assigned nvonpentz Jul 27, 2022

github-actions bot added the CI/storybook-url Deploy storybook and provide a unique URL for each build label Jul 27, 2022

nvonpentz force-pushed the ethereum-ledger-untrusted branch 2 times, most recently from ce4e043 to a04e699 Compare August 2, 2022 20:44

nvonpentz changed the title ~~WIP: Ethereum ledger untrusted~~ Isolate Ethereum Ledger integration in untrusted iFrame Aug 2, 2022

nvonpentz marked this pull request as ready for review August 2, 2022 20:45

nvonpentz changed the title ~~Isolate Ethereum Ledger integration in untrusted iFrame~~ Move Ethereum Ledger integration in untrusted iframe Aug 4, 2022

nvonpentz requested a review from kdenhartog August 4, 2022 15:55

nvonpentz mentioned this pull request Aug 4, 2022

Move Filecoin Ledger Integration to untrusted iframe brave/brave-browser#24449

Closed

josheleonard reviewed Aug 4, 2022

View reviewed changes

stoletheminerals self-requested a review August 8, 2022 19:50

nvonpentz force-pushed the ethereum-ledger-untrusted branch from aefca33 to 62967c1 Compare August 9, 2022 20:51

diracdeltas reviewed Aug 9, 2022

View reviewed changes

components/brave_wallet_ui/common/hardware/ledgerjs/ledger_bridge_keyring.ts Show resolved Hide resolved

diracdeltas reviewed Aug 9, 2022

View reviewed changes

components/brave_wallet_ui/ledger/ledger.ts Outdated Show resolved Hide resolved

kdenhartog approved these changes Aug 9, 2022

View reviewed changes

nvonpentz force-pushed the ethereum-ledger-untrusted branch from 62967c1 to 985d946 Compare August 10, 2022 18:23

nvonpentz force-pushed the ethereum-ledger-untrusted branch from 90742e6 to be6bc41 Compare August 10, 2022 21:03

stoletheminerals reviewed Aug 11, 2022

View reviewed changes

components/brave_wallet_ui/common/hardware/ledgerjs/ledger-messages.ts Outdated Show resolved Hide resolved

nvonpentz force-pushed the ethereum-ledger-untrusted branch from be6bc41 to 49f9a58 Compare August 17, 2022 20:04

nvonpentz requested a review from stoletheminerals August 17, 2022 23:54

josheleonard reviewed Aug 18, 2022

View reviewed changes

components/brave_wallet_ui/ledger/ledger.ts Show resolved Hide resolved

josheleonard approved these changes Aug 18, 2022

View reviewed changes

kdenhartog mentioned this pull request Aug 18, 2022

properly import the types from window.crypto.randomUUID brave/brave-browser#24764

Open

stoletheminerals approved these changes Aug 18, 2022

View reviewed changes

stoletheminerals reviewed Aug 18, 2022

View reviewed changes

...ts/brave_wallet_ui/components/shared/authorize-hardware-device/authorize-hardware-device.tsx Show resolved Hide resolved

nvonpentz force-pushed the ethereum-ledger-untrusted branch from 5e588d9 to 144d32b Compare August 18, 2022 16:03

nvonpentz merged commit d098592 into master Aug 18, 2022

nvonpentz deleted the ethereum-ledger-untrusted branch August 18, 2022 20:47

github-actions bot added this to the 1.44.x - Nightly milestone Aug 18, 2022

srirambv mentioned this pull request Sep 26, 2022

Move Ethereum Ledger Integration to untrusted iframe brave/brave-browser#24275

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move Ethereum Ledger integration in untrusted iframe #14373

Move Ethereum Ledger integration in untrusted iframe #14373

nvonpentz commented Jul 27, 2022 •

edited

Loading

josheleonard Aug 4, 2022

brave-builds commented Aug 5, 2022

brave-builds commented Aug 9, 2022

kdenhartog left a comment

brave-builds commented Aug 10, 2022

brave-builds commented Aug 10, 2022

brave-builds commented Aug 10, 2022

brave-builds commented Aug 17, 2022

josheleonard left a comment

brave-builds commented Aug 18, 2022

brave-builds commented Aug 18, 2022

Move Ethereum Ledger integration in untrusted iframe #14373

Move Ethereum Ledger integration in untrusted iframe #14373

Conversation

nvonpentz commented Jul 27, 2022 • edited Loading

Submitter Checklist:

Reviewer Checklist:

After-merge Checklist:

Test Plan:

josheleonard Aug 4, 2022

Choose a reason for hiding this comment

brave-builds commented Aug 5, 2022

brave-builds commented Aug 9, 2022

kdenhartog left a comment

Choose a reason for hiding this comment

brave-builds commented Aug 10, 2022

brave-builds commented Aug 10, 2022

brave-builds commented Aug 10, 2022

brave-builds commented Aug 17, 2022

josheleonard left a comment

Choose a reason for hiding this comment

brave-builds commented Aug 18, 2022

brave-builds commented Aug 18, 2022

nvonpentz commented Jul 27, 2022 •

edited

Loading