polkadot-js / extension

Simple browser extension for managing Polkadot and Substrate network accounts in a browser. Allows the signing of extrinsics using these accounts. Also provides a simple interface for compliant extensions for dapps.
Apache License 2.0
965 stars 403 forks source link

Incorrect origin URL shown in the pannel #1293

Closed kvinwang closed 3 months ago

kvinwang commented 7 months ago

While I was browsing a GitHub page, a pop-up window unexpectedly appeared. This occurred possibly after switching from a tab displaying the Phat Contract UI, although I'm not certain of the exact sequence of my actions. The pop-up is captured in the following image: image.

I suspect this could potentially indicate a security vulnerability.

Leechael commented 6 months ago

Today, I encountered a similar issue where the extension prompted a window asking for authorization.

I am choosing to reject and check the authUrls records in local storage. It shows that the origin is GitHub, which should be impossible.

image

image
F-OBrien commented 6 months ago

I've encountered this issue many times and I have struggled to find a solution. From what I've found I believe it is being caused by Chrome preloading pages before they are actually loaded and triggering the extension (or at least before the extension has had a chance to update to the correct origin). This might happen after hovering over a link or when typing in the address bar. If I disable Preload pages in chrome://settings/performance I never get this issue. I find it worse when running a dApp locally vs when using a production application. I'm assuming this is due to slightly longer loading times accessing remote servers.

The issue manifests in several ways. Sometimes you might get random requests to authorize a website as mentioned above. Sometimes when you visit an application that uses a extension dApp based wallet it will prompt you to authorize the site you just left. If I start from a new tab the wallet will throw an error Invalid url chrome://newtab/, expected to start with http: or https: or ipfs: or ipns: (or a version of that for some other substrate wallets)

For me the way I can replicate it is to go to a random website or new tab. Have my application running on localhost. Click the address bar and start typing localhost I generally just type the first 2-3 characters and then hit enter to accept the auto complete url. The page loads but I either get prompted to authorize the page I left or get an error that chrome://newtab/ is not a valid url. For some applications it can result in the website getting stuck in a loading state which gets resolved only by refreshing after which the page loads correctly.

I believe this code in the extension dApp was intended to prevent issues like this but Chrome has possibly become too aggressive in it preloading.

F-OBrien commented 6 months ago

I've attached a video demonstrating the preload issue. I've opened the background page so you can see the messages being passed as soon as I start to type the url. As I mentioned this seems to be at its worst when I'm connecting to a localhost dApp.

https://github.com/polkadot-js/extension/assets/42175565/0c215a8d-f6d8-4217-8e6b-cb7fd02bc7f7

F-OBrien commented 5 months ago

I got to the root of this issue. When chrome prefetches a page it opens a port to the extension. That port remains open even after the page fully loads. The extension takes the from in the first instance as sender.tab.url which in the case of chrome prefetching will not be for the finally loaded site. https://github.com/polkadot-js/extension/blob/128738181afb5796e326632c60f861e278e6481b/packages/extension-base/src/background/handlers/index.ts#L29C1-L31C54

Below is an example of the sender object when a dApp is opened from a new chrome tab

{
    "id": "bbaingoklbnbhejbpaiamcafpcghooca",
    "url": "http://localhost:5173/",
    "origin": "http://localhost:5173/",
    "frameId": 2000,
    "documentId": "A852C2EC89D612016F40D7180536CC85",
    "documentLifecycle": "prerender",
    "tab": {
        "active": true,
        "audible": false,
        "autoDiscardable": true,
        "discarded": false,
        "favIconUrl": "",
        "groupId": -1,
        "height": 1269,
        "highlighted": true,
        "id": 1286970485,
        "incognito": false,
        "index": 23,
        "mutedInfo": {
            "muted": false
        },
        "openerTabId": 1286970479,
        "pinned": false,
        "selected": true,
        "status": "complete",
        "title": "New Tab",
        "url": "chrome://newtab/",
        "width": 1278,
        "windowId": 1286969360
    }
}

Note how the url and origin do not match the tab.url.

A simple fix for this issue would be to use the sender.url as the from instead of the tab.url. I need to look into is there is a downside to not using the tab.url but once I've satisfied it will work I can submit a PR.

Leechael commented 5 months ago

Below is an example of the sender object when a dApp is opened from a new chrome tab

Can we use this property to solve this issue?

"documentLifecycle": "prerender",
F-OBrien commented 5 months ago

Below is an example of the sender object when a dApp is opened from a new chrome tab

Can we use this property to solve this issue?

"documentLifecycle": "prerender",

Yes. I was looking at that. My only concern was the implications of switching from sender.tab.url to sender.url or sender.origin. I was looking at some other wallet repositories and see subwallet recently changed to use sender.url first https://github.com/Koniverse/SubWallet-Extension/commit/6683aab10f4029ee05e8093f1bda61258fd263b0

I'm looking into the best option to fix this but wanted to ensure we consider any potential security risks in doing so. We now know that if sender.documentLifecycle === 'prerender' we cannot use sender.tab.url as this will most likely not match the actual url loading. That leaves us with a choice of using either sender.origin or sender.url. In standard cases both of these will have the same base url. However if the site is wrapped in an iframe I believe they will differ. As it turns out the wallet manifest file doesn't include "all_frames": true so the polkadot wallet doesn't support iframes. However the Talisman and Subwallet extensions do. Subwallet used sender.url as their from while Talisman uses sender.tabs.url (and has the bug discussed in this issue). I tested both using https://iframetester.com/?url=https://portal.polymesh.live/ Talisman prompts to authorize iframetester.com while subwallet prompts to the embedded site. (~I believe sender.origin should be the same as the base url of sender.tabs.url but need to build a version of the wallet that allows iframes and logs the sender to test~ Edit: I tested it and origin and sender.url are the same base url so can differ from tab.url).

So in the case of Talisman the wallet authorizes the site containing the iframe. This means if the iframe was swapped out the authorization would still remain valid and potentially allow a malicious site to be displayed and interact with it. In the case of subwallet the embeded dApp base url gets authorized. Which I think is a bit better. However, It means if I visit https://iframetester.com/?url=https://portal.polymesh.live/ and have previously authorized https://portal.polymesh.live/ I don't get an authorization prompt. My concern with this is if there is a risk of click jacking. However if the attacker tried to initiate a transaction from the page containing the iframe then that site would also need to have been authorized by the wallet. hopefully the user would notice and not authorize a malicious site.

My feeling for now is it's best to not add support for iframes to our wallet (Polymesh) and if sender.documentLifecycle === 'prerender' to use the sender.url as the from. doing what subwallet did may be the simplest approach.

polkadot-js-bot commented 3 months ago

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue if you think you have a related problem or query.