Move Solana ledger integration to untrusted iframe #14096

nvonpentz · 2022-07-08T18:38:40Z

Summary

This change isolates the js libraries used to interact with Ledger hardware devices (@ledgerhq/hw-app-solana, and @ledgerhq/hw-transport-webhid) such that they are only called in an untrusted iframe, similar to the Trezor integration.

The libraries are now accessed by the chrome://wallet and chrome://wallet-panel contexts via postMessage calls to an iframe whose origin is chrome-untrusted://ledger-bridge.

This is tricky because the navigator.hid.requestDevices API call made by the Ledger lib to prompt the user to give Brave Wallet permission to access their device must be called in the context of a user gesture (e.g. a click). As a result all UI that could trigger a permissions dialog (e.g. the 'connect' button in the import accounts flow, and the 'confirm' button in the send transaction flow) must be inside the iframe. We cannot have the user click a button outside the iframe and prompt the permissions screen in the iframe because the 'click' context is not propagated through the postMessage API.

To handle this, we create a 'Authorize' button UI that's rendered within the untrusted iframe whenever we detect that the user needs to grant permissions. When clicked, the authorize button will prompt the user to grant permissions and the flow will continue.

This is the 'import accounts' flow:

User hits the 'connect' button on the import accounts flow
SolanaLedgerBridgeKeyring creates the untrusted iframe, sends the 'unlock' command via LedgerTrustedMessagingTransport
Within the untrusted iFrame, LedgerUntrustedMessagingTransport processes the unlock command using the ledger libraries to create a connection with the device. If no devices are detected, an 'authorization needed' message is sent back to the LedgerTrustedMessagingTransport and SolanaLedgerBridgeKeyring, and the UI is updated to display the iFrame that has 'Authorize' button
The user clicks the 'Authorize' button and the user is prompted with the permissions screen. Upon success the LedgerUntrustedMessagingTransport (run in chrome-untrusted://ledger-bridge) sends an 'authorization success' message back to LedgerTrustedMessagingTransport (run in chrome://wallet), and a the UI is updated to show the original 'connect' button.
The user clicks 'connect' once again, but this time no authorization is needed, and the LedgerUntrustedMessagingTransport simply returns the list of accounts to the LedgerTrustedMessagingTransport.

The flow is similar in the sendTransaction cases, but differs slightly in the way the UI is updated to (a) show the 'Authorize' button, (b) show the normal UI when authorization is completed. In the import accounts flow, this happens via React hooks, but in the sendTransaction flow, this is done by dispatching actions. Also, in the sendTransaction cases, the parent context (chrome://wallet-panel) is different from the parent context for the import account case (chrome://wallet). As a result permissions need to be granted for both separately, and the implementation abstracts to handle both cases.

Screen shots

Import accounts authorization screen	Send transactions authorization screen (panel)

Remaining

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

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'
4. Select 'Connect' for Solana
5. Verify 'Connect your Ledger wallet directly to your computer' is displayed, hit 'Connect'
6. Verify 'Grant Brave access to your Ledger device.' is displayed, hit 'Authorize'
7. You should be prompted with the permissions screen, select device, hit 'Connect'
8. Verify you are returned to 'Connect your Ledger wallet directly to your computer' screen, hit 'Connect'
9. Verify your accounts are listed for impor for import, hit 'Load more'
10. Verify more accounts loaded
11. Select an account and hit 'Add checked accounts'
12. Verify those accounts are added
13. Repeat the same flow without forgetting the device first and verify no 'Authorize' screen is displayed or permission screen prompted.
Test sign transaction flows
1. Connect ledger device and enter password
2. Create a Solana hardware account with a devnet 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 (kind of need to be quick since this closes not long after the panel closes..)
6. Go to https://pwgoom.csb.app/ and click 'Sign and Send Transaction'
7. Hit sign / confirm
8. You should be shown an 'Authorize' button on the panel, hit it
9. You should be prompted with the permissions screen, select device, hit 'Connect'
10. Verify you are returned to the 'Hardware wallet requires Solana App opened on Ledger' (it may take a second)
11. On your ledger device, you should see the the transaction you entered, confirm.
12. Verify the post transaction stuff happens (if sending): you are shown the details, get a notification when transaction is confirmed, etc
13. Repeat all steps clicking 'Sign All Transactions (multiple)', and 'Sign Transaction' instead of 'Sign and Send Transaction'

...allet_ui/components/desktop/popup-modals/add-account-modal/hardware-wallet-connect/index.tsx

nvonpentz · 2022-07-08T18:59:04Z

...allet_ui/components/desktop/popup-modals/add-account-modal/hardware-wallet-connect/index.tsx

-      setShowAccountsList(false)
+      if (error === 'unauthorized') {
+        setShowAuthorizeDevice(true)
+        setShowAccountsList(false)


Here we check for the 'unauthorized' error which is passed from the SolanaLedgerKeyring class when the SolanaLedgerKeyring class receives a reply from the untrusted iframe saying that permissions need to be granted first.

If that's the case, we show the iframe with the 'authorize' button via setShowAuthorizeDevice.

nvonpentz · 2022-07-08T19:00:30Z

components/brave_wallet_ui/ledger/ledger.ts

+      // TODO handle authorize failure / deny
+    })
+  } 
+})


Here we set up the event listeners for the 'authorize' button in the iframe. When that's clicked we send a message to the parent iframe (either chrome://wallet or chrome://wallet-panel) saying authorization has been completed.

components/brave_wallet_ui/common/api/hardware_keyrings.ts

components/brave_wallet_ui/common/hardware/ledgerjs/ledger-bridge-transport.ts

josheleonard · 2022-07-12T19:53:56Z

components/brave_wallet_ui/common/hardware/ledgerjs/ledger-bridge-transport.ts

+  }
+
+  protected onMessageReceived = (event: MessageEvent) => {
+    // TODO use a better message type than just string 


components/brave_wallet_ui/common/hardware/ledgerjs/ledger-bridge-transport.ts

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

components/brave_wallet_ui/ledger/ledger.html

josheleonard · 2022-07-12T20:25:34Z

components/brave_wallet_ui/ledger/ledger.ts

+
+import { addLedgerCommandHandler } from '../common/hardware/ledgerjs/ledger-command-handler'
+
+const createUnlockResponse = (command: UnlockCommand, result: boolean, error?: any): UnlockResponsePayload => {


avoid any

I think we could probably use https://github.com/LedgerHQ/ledger-live/tree/develop/libs/ledgerjs/packages/errors

My impression is that hardware device error handling probably needs another pass back to front. Some outstanding issues I see -

A lot of errors are not actually being bubbled up to the UI (e.g. all ledger hardware device errors when adding accounts)

Error handling is not consistent between chrome://wallet and chrome://wallet-panel

Conflation of various categories of error

Let me know what you think, but I think I'd rather iterate on those changes in subsequent pull requests

Agreed that this will require more planning/iterating to get right.
Cool with putting aside for another PR

This specific function could maybe use a generic until correct types are determined.
const createUnlockResponse = <T = any | undefined>(command: UnlockCommand, result: boolean, error?: T)

components/brave_wallet_ui/ledger/ledger.ts

brave-builds · 2022-07-18T17:20:09Z

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

components/brave_wallet_ui/common/hardware/ledgerjs/ledger-messaging-transport.ts

components/brave_wallet_ui/ledger/ledger.ts

josheleonard · 2022-07-18T20:40:55Z

components/brave_wallet_ui/panel/async/wallet_panel_async_handler.ts

@@ -532,7 +537,7 @@ handler.on(PanelActions.signMessageHardware.getType(), async (store, messageData
  const payload: SignMessageProcessedPayload =
    signed.success
      ? { approved: signed.success, id: messageData.id, signature: signature }
-      : { approved: signed.success, id: messageData.id, error: signed.error }
+      : { approved: signed.success, id: messageData.id, error: (signed.error) as string }


maybe just call .toString() instead of casting?

Tried, but won't work here just because the signed.error is of type error?, and toString is not defined on undefined. signed is a HardwareOperationalResult:

export type HardwareOperationResult = { success: boolean error?: string | LedgerError code?: string | number }

So this reduces to the same issue I raise here. I am now wondering if I should not try to propagate the full errors (complete with statusCodes) to getErrorMessage in components/brave_wallet_ui/components/desktop/popup-modals/add-account-modal/hardware-wallet-connect/index.tsx, and instead continue just passing strings up. That way we only have one error type (string) and can eliminate the casts. That would be at the expense of being able to case on statusCode in getErrorMessage. Thoughts?

This cast should actually be as string | undefined to rule out LedgerError which is the goal of this cast. as string is actually too narrow since it legitimately may be undefined give the above HardwareOperational result type. I updated in 499d2df.

Let's create a follow-up issue to work-out hardware error types

components/brave_wallet_ui/panel/async/wallet_panel_async_handler.ts

brave-builds · 2022-07-20T05:00:47Z

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

kdenhartog

Just a quick static analysis at this point. Nice job getting the isolation done and figuring out the flow to make this work.

Things look pretty good so far. I'm going to see if I can get a HW wallet to test this in the next few days as well from a dynamic perspective.

browser/ui/webui/brave_wallet/ledger/ledger_ui.cc

components/brave_wallet_ui/common/async/hardware.ts

components/brave_wallet_ui/common/hardware/ledgerjs/ledger-trusted-transport.ts

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

kdenhartog · 2022-07-20T06:28:28Z

components/brave_wallet_ui/components/shared/index.ts

@@ -11,6 +11,7 @@ import CreateNetworkIcon from './create-network-icon'
 import SelectNetworkButton from './select-network-button'
 import LoadingSkeleton from './loading-skeleton'
 import CreateSiteOrigin from './create-site-origin'
+import AuthorizeHardwareDeviceIFrame from './authorize-hardware-device/authorize-hardware-device'


I've not heard the term "barrel file" before. Is that just the generic term for TS index.ts import/export boilerplate code to export code upwards out of a directory?

components/brave_wallet_ui/stories/locale.ts

nvonpentz · 2022-07-20T15:41:02Z

components/test/testPolyfills.ts

@@ -21,6 +21,9 @@ window.requestAnimationFrame = function (cb: FrameRequestCallback) {

 window.loadTimeData = {
  getString (key) {
+    if (key === 'braveWalletLedgerBridgeUrl') {
+      return 'chrome-untrusted://ledger-bridge'
+    }


Needed to so that LEDGER_BRIDGE_URL is an actually URL for the tests, not "braveWalletLedgerBridgeUrl"

brave-builds · 2022-07-20T16:14:03Z

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

brave-builds · 2022-07-20T18:55:31Z

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

components/brave_wallet_ui/ledger/ledger.ts

brave-builds · 2022-07-20T21:05:46Z

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

components/brave_wallet_ui/common/hardware/ledgerjs/ledger-messaging-transport.ts

josheleonard · 2022-07-20T21:30:00Z

components/brave_wallet_ui/ledger/ledger.ts

+
+import { addLedgerCommandHandler } from '../common/hardware/ledgerjs/ledger-command-handler'
+
+const createUnlockResponse = (command: UnlockCommand, result: boolean, error?: any): UnlockResponsePayload => {


Agreed that this will require more planning/iterating to get right.
Cool with putting aside for another PR

This specific function could maybe use a generic until correct types are determined.
const createUnlockResponse = <T = any | undefined>(command: UnlockCommand, result: boolean, error?: T)

josheleonard · 2022-07-20T21:30:41Z

components/brave_wallet_ui/panel/async/wallet_panel_async_handler.ts

@@ -532,7 +537,7 @@ handler.on(PanelActions.signMessageHardware.getType(), async (store, messageData
  const payload: SignMessageProcessedPayload =
    signed.success
      ? { approved: signed.success, id: messageData.id, signature: signature }
-      : { approved: signed.success, id: messageData.id, error: signed.error }
+      : { approved: signed.success, id: messageData.id, error: (signed.error) as string }


Let's create a follow-up issue to work-out hardware error types

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

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

components/brave_wallet_ui/common/hardware/ledgerjs/ledger-messaging-transport.test.ts

components/brave_wallet_ui/common/hardware/ledgerjs/ledger-messaging-transport.ts

components/brave_wallet_ui/common/hardware/ledgerjs/ledger-trusted-transport.ts

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

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

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

josheleonard · 2022-07-21T00:02:07Z

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

+      return result
+    }
+
+    const data = await this.sendCommand<SignTransactionResponsePayload>({


UnlockResponse~~Payload~~

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

brave-builds · 2022-07-21T18:23:48Z

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

brave-builds · 2022-07-21T19:59:10Z

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

brave-builds · 2022-07-21T23:05:13Z

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

kdenhartog

This one looks good to me, approving for the security review since this is a follow on issue for https://github.com/brave/security/issues/919

kdenhartog · 2022-07-22T00:19:01Z

Also, that PR description at the top there is awesome and deserves some recognition!

brave-builds · 2022-07-22T15:04:37Z

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

josheleonard

Frontend LGTM

The Solana Ledger JS libraries are now accessed by the chrome://wallet and chrome://wallet-panel contexts via postMessage calls to an iframe whose origin is chrome-untrusted://ledger-bridge. If we detect that user needs to grant the Wallet permssion to access a Ledger device, an 'Authorize' button is displayed *within* the untrusted frame so that the permission dialog (which requires a user gesture to be displayed) screen is shown. When permission is granted, the chrome-untrusted://ledger-bridge sends a postMessage to the parent (trusted) context instructing it to continue the flow.

mkarolin

chromium_src ++

brave-builds · 2022-07-25T19:49:22Z

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

srirambv · 2022-08-04T07:12:57Z

Verification passed on

Brave	1.44.15 Chromium: 104.0.5112.81 (Official Build) nightly (64-bit)
Revision	`5b7b76419d50f583022568b6764b630f6ddc9208-refs/branch-heads/5112@{#1309}`
OS	Windows 11 Version 21H2 (Build 22000.795)

Verified test plan from PR
Verified able to Authorize Ledger device
Verified able to connect and add accounts from Ledger
Verified able to perform sign transactions after authorizing the ledger device for Solana transactions
Encountered Incorrect message on hardware connect modal when device is already authorized once before brave-browser#24429

Test Case 1	Test Case 2
14096.-.Scenario.1.mp4	14096-Scenario.2.mp4

github-actions bot assigned nvonpentz Jul 8, 2022

github-actions bot added CI/storybook-url Deploy storybook and provide a unique URL for each build potential-layer-violation-fixes This PR touches a BUILD.gn file with check_includes=false labels Jul 8, 2022

nvonpentz force-pushed the solana-ledger-untrusted branch from 866b04f to 83c637e Compare July 8, 2022 18:47

nvonpentz commented Jul 8, 2022

View reviewed changes

...allet_ui/components/desktop/popup-modals/add-account-modal/hardware-wallet-connect/index.tsx Outdated Show resolved Hide resolved

nvonpentz commented Jul 8, 2022

View reviewed changes

josheleonard suggested changes Jul 12, 2022

View reviewed changes

nvonpentz force-pushed the solana-ledger-untrusted branch 4 times, most recently from 4f41771 to 3fcfb77 Compare July 16, 2022 15:39

nvonpentz marked this pull request as ready for review July 16, 2022 15:41

nvonpentz requested review from a team as code owners July 16, 2022 15:41

nvonpentz force-pushed the solana-ledger-untrusted branch 3 times, most recently from d22d6aa to 28657ef Compare July 18, 2022 16:22

josheleonard suggested changes Jul 18, 2022

View reviewed changes

nvonpentz force-pushed the solana-ledger-untrusted branch from 28657ef to e29249d Compare July 20, 2022 04:31

kdenhartog reviewed Jul 20, 2022

View reviewed changes

nvonpentz commented Jul 20, 2022

View reviewed changes