MetaMask / metamask-extension

:globe_with_meridians: :electric_plug: The MetaMask browser extension enables browsing Ethereum blockchain enabled websites
https://metamask.io
Other
12.05k stars 4.92k forks source link

[Bug]: domain mismatch when prompted to sign from iframe #19189

Closed julien51 closed 1 year ago

julien51 commented 1 year ago

Describe the bug

Our UI is often embedded on other websites as an iframe. We use SIWE to authenticate users. When constructing the message to sign, we identify if we are indeed in an iframe, and if so, then we use the parent window's for the domain and uri using the following code:

       const parent = new URL(
        window.location != window.parent.location
          ? document.referrer
          : document.location.href
      )
      let resources = undefined
      if (parent.host !== window.location.host) {
        resources = [window.location.origin]
      }

      const siwe = new SiweMessage({
        domain: parent.host,
        uri: parent.origin,
        address,
        chainId: network,
        version: '1',
        statement,
        nonce,
        resources,
      })

Unfortunately this results in a warning like the one I am showing there:

Screenshot 2023-05-17 at 2 24 28 PM

We have built a reproducible app: https://wagmi-example-git-simple-embed-julien-unlock-proto.vercel.app/

As you can see there is a warning here. We are trying to make sense of what is the way to actually get this to work reliably, without warning.

The URI itself is coming from the parent, but the "prompt" to sign is coming from inside the iframe, at app.unlock-protocol.com

Steps to reproduce

Go to https://wagmi-example-git-simple-embed-julien-unlock-proto.vercel.app/

Error messages or log output

No response

Version

10.30.2

Build type

None

Browser

Chrome

Operating system

MacOS

Hardware wallet

No response

Additional context

No response

julien51 commented 1 year ago

We found a way to reproduce consistently! And I suspect the issue is because MM expects the domain to match the domain from which the user "connected" the wallet the first time, rather than the domain on which the user is currently!

Here is how to reproduce:

  1. Make sure you disconnect the wallet from both https://app.unlock-protocol.com/dashboard and https://wagmi-example-git-simple-embed-julien-unlock-proto.vercel.app/
  2. Go to https://app.unlock-protocol.com/dashboard and click on on the connect button. Do NOT click on Confirm Ownership (that would trigger a SIWE prompt).
  3. Go to https://wagmi-example-git-simple-embed-julien-unlock-proto.vercel.app/
  4. Click on Paywall
  5. At that point, the iframe is loaded and prompts you to Sign a message to continue
  6. Click there. That triggers a SIWE prompts and you should see the warning.
  7. If you were to connect the wallet from https://wagmi-example-git-simple-embed-julien-unlock-proto.vercel.app/ directly (skipping step 2), then no warning would show.

If you were to connect AND sign from https://app.unlock-protocol.com/dashboard and then continue, no warning would show.

If the user connected and signed at step 2, BUT enough time has passed before they do 3, the SIWE message has expired, the user is prompted to sign again at step 4... triggering the warning.

bschorchit commented 1 year ago

Thank you for the repro steps and info, @julien51 ! I'm not able to get to the Sign a message button within the iframe without connecting my wallet there. Anything I should do in order to be able to do such?

julien51 commented 1 year ago

I'm not able to get to the Sign a message button within the iframe without connecting my wallet there.

Make sure you connect the wallet first on from https://app.unlock-protocol.com/dashboard (step 2) but do NOT sign the SIWE prompt there

julien51 commented 1 year ago

Per Oliver's (from Spruce) we changed the domain to use window.location.host (the origin of the iframe): as you can see there: https://github.com/unlock-protocol/unlock/blob/1dcdbc4023e706b8e4c7b57d234b410d0956a3c6/unlock-app/src/hooks/useSIWE.tsx#L112

But we now get consistent warnings. It would be really helpful to have a clear guidance on what to use for the domain when the message signing is triggered from inside an iframe from a different origin.

awoie commented 1 year ago

IMO, wallets should check that the SIWE domain message field (see EIP-4361) matches the host of origin the request was made. If the request came from a cross-origin iframe, the SIWE domain has to match the iframe's origin.

IMO, an iframe should never assert its ancestor's (e.g. parent) origin/domain for security reasons.

For me, it is hard to tell where the problem now really occurs if both, the embedded SDK and the MM extension implement domain binding the way I described above. It is either in the SDK that is embedded in the iframe, or it is in the MM extension. If window.location.host reliably gives the iframe's origin and the SIWE domain field is always set to the iframe's origin, then I believe it is more likely that the issue is in the MM extension.

awoie commented 1 year ago

It seems that sometimes MM is connected to both sites:

When it is only connected to https://app.unlock-protocol.com/, then there is no issue.

If it is connected to both, or to https://wagmi-example-git-simple-embed-julien-unlock-proto.vercel.app/ only, then MM displays a warning. It almost seems that MM is reading the domain from the connected account origin and not necessarily from the iframe (or current SIWE / personal_sign request) and then displays a warning when matching against the domain field in the SIWE message.

With the following steps I can reproduce both cases, the happy path and the warning:

  1. Clear all application data for https://wagmi-example-git-simple-embed-julien-unlock-proto.vercel.app/ and disconnect account from all sites
  2. Restart and open Google Chrome with MM extension installed (rarely, it doesn’t reproduce if I don’t restart for whatever reason)
  3. Go to https://app.unlock-protocol.com/
  4. Connect account -> with site https://app.unlock-protocol.com/
  5. Confirm account is connected to the following sites: -> https://app.unlock-protocol.com/
  6. Go to https://wagmi-example-git-simple-embed-julien-unlock-proto.vercel.app/ (DON’T do hard reload! If hard reload then I cannot see Sign message to Continue button and I cannot reproduce the behaviour)
  7. Click on paywall
  8. Confirm account is connected to the following sites: -> https://app.unlock-protocol.com/
  9. Click on Sign message to Continue
  10. Confirm that there is no warning in MM and confirm request
  11. Click Disconnect in iframe (bottom right corner)
  12. Confirm account is connected to the following sites: -> https://app.unlock-protocol.com/ (this seems weird since we just disconnected the account)
  13. Connect account using the iframe MM icon
  14. Confirm nothing happens, button doesn’t work and doesn’t open MM
  15. Soft reload page (no hard reload needed)
  16. Connect account using the iframe MM icon (again) -> with site https://wagmi-example-git-simple-embed-julien-unlock-proto.vercel.app (This seems a bit weird to me too, not sure if this is what supposed to happen. Since the origin of the iframe is https://app.unlock-protocol.com/, it should probably connect https://app.unlock-protocol.com/ here as well)
  17. Confirm account is connected to the following sites: -> https://app.unlock-protocol.com/ -> https://wagmi-example-git-simple-embed-julien-unlock-proto.vercel.app/
  18. Sign message to Continue
  19. Confirm that there is a warning in MM: -> https://wagmi-example-git-simple-embed-julien-unlock-proto.vercel.app does not match

Updated:

digiwand commented 1 year ago

Hey @julien51, thanks for reporting this issue. We are actively investigating this issue.

Hey @awoie, thanks for the head start and the thorough repro steps.

IMO, wallets should check that the SIWE domain message field (see EIP-4361) matches the host of origin the request was made. If the request came from a cross-origin iframe, the SIWE domain has to match the iframe's origin.

IMO, an iframe should never assert its ancestor's (e.g. parent) origin/domain for security reasons.

agreed. The current request origin that is checked against the SIWE domain seems off.

naugtur commented 1 year ago

@julien51 Could you explain how, if at all, would the app running in the iframe be able to send a request via window.ethereum of the parent? They should be cross-origin. Is there postMessage involved?

julien51 commented 1 year ago

Thanks @naugtur We actually have both scenario.

Sometimes, our "checkout" UI is embedded like that:

// instantiate the Paywall object
const paywall = new Paywall(paywallConfig, networkConfigs)
// Loads the checkout UI
const response = await paywall.loadCheckoutModal()

In that case, inside the iframe, our app will access window.ethereum and connect to it. It will send the signature requests from there....

Other times, it can be embedded like this:

const provider = window.ethereum /* It can be any "provider" object, like the injected one, or a Magic Wallet one... etc*/

const paywall = new Paywall(paywallConfig, networkConfigs, provider)

// Loads the checkout UI
const response = await paywall.loadCheckoutModal()

In that case, we "communicate" from the iframe to the parent's window.ethereum with the request method so messages to be signed will originate from the parent.

Does it help?

julien51 commented 1 year ago

agreed. The current request origin that is checked against the SIWE domain seems off.

The rationale for this IMO is that when it is an embedded iframe the user has 0 knowledge of the origin of that iframe, which may be quite confusing to them...

awoie commented 1 year ago

Thanks @naugtur We actually have both scenario.

In both scenarios, you are setting the domain in the SIWE message field to window.location.host which is fine if the signing request is sent in the same context.

@julien51 However, if you embed the SDK in a cross-origin iframe, are you 1) constructing the SIWE message in the iframe, then 2) sending the constructed SIWE message to the parent (via window.postMessage) and 3) performing the actual signing request from the parent? if this is the case, the SIWE domain field would have the host of the iframe origin, the request itself would have the origin of the parent window, and MM would report a domain binding warning correctly.

julien51 commented 1 year ago

@awoie Sorry for the delay here. We actually have multiple ways for implementers to trigger things (per above).

Some users will instantiate the Paywall without passing it a provider. In that case we ask the user to connect one (injected, like MM, or walletconnect, or Coinbase's wallet... etc) and the signature requests are coming from inside the iframe.

Some other users will instantiate the Paywall object while passing a provider that the paywall should use. In that case, we DO not ask the user to connect as we just use the one provided by the application. In that case, the signature request is coming from the parent.

In all cases we build the message inside the iframe. We could try to pass the parent's domain in the 2nd case if that is helpful.

skgbafa commented 1 year ago

Hey @julien51! If you pass the domain of the parent the iframe when you are calling the signer from the parent and build the SIWE message with that domain, the signature request from the parent should work as expected.

Likewise, use the origin from the iframe if the signature request is coming from the iframe.

MetaMask enforces that the domain doing the signature request of a SIWE message is the domain in the message (after much investigation to confirm this)

julien51 commented 1 year ago

Just to make sure I understand what you are asking is that we always use the domain of the origin from which we are sending the signature request?

julien51 commented 1 year ago

@skgbafa I am sorry but I think this is what we are currently doing in the example listed above but we're still seeing the warning.

This URL https://wagmi-example-git-simple-embed-julien-unlock-proto.vercel.app/ is deployed from this branch: https://github.com/julien51/wagmi-example/tree/simple-embed and the code to "embed" the paywall is like this:

  const checkout = async () => {
    const paywall = new Paywall(paywallConfig, networks);
    paywall.loadCheckoutModal(
      paywallConfig,
      "https://app.unlock-protocol.com/"
    );
    return false;
  };

So clearly, we are not passing a provider object, which means that the provider used is the one inside the iframe, so if I understand what you say we should use the iframe's origin.

Now look at how we connect there:


      const siwe = new SiweMessage({
        domain: window.location.host, 
        uri: parent.origin,
        address,
        chainId: network,
        version: '1',
        statement,
        nonce,
        resources,
      })

And you can see we are indeed using the iframe's origin... and yet seeing the warning.

awoie commented 1 year ago

@awoie Sorry for the delay here. We actually have multiple ways for implementers to trigger things (per above).

Some users will instantiate the Paywall without passing it a provider. In that case we ask the user to connect one (injected, like MM, or walletconnect, or Coinbase's wallet... etc) and the signature requests are coming from inside the iframe.

Some other users will instantiate the Paywall object while passing a provider that the paywall should use. In that case, we DO not ask the user to connect as we just use the one provided by the application. In that case, the signature request is coming from the parent.

In all cases we build the message inside the iframe. We could try to pass the parent's domain in the 2nd case if that is helpful.

Since one cannot pass MM providers between windows (parents, childs/iframes etc.), the window that sends the signing request has to include the host of the window's origin in the SIWE domain message field. Otherwise, MM or in general, EIP-4361 would complain about domain verification warnings.

When I was playing with the demo dapp you shared, it felt like that the signing request was sometimes sent from the parent and sometimes from the child window. You can reproduce that by following the steps I described above. IMO, that looked like a bug or side effect in the embedded iframe or the embedding parent since we debugged quite extensively that MM is reading the origin correctly. For that reason, it would make sense to take another look at the iframe and dapp code.

awoie commented 1 year ago

@skgbafa I am sorry but I think this is what we are currently doing in the example listed above but we're still seeing the warning.

This URL https://wagmi-example-git-simple-embed-julien-unlock-proto.vercel.app/ is deployed from this branch: https://github.com/julien51/wagmi-example/tree/simple-embed and the code to "embed" the paywall is like this:

  const checkout = async () => {
    const paywall = new Paywall(paywallConfig, networks);
    paywall.loadCheckoutModal(
      paywallConfig,
      "https://app.unlock-protocol.com/"
    );
    return false;
  };

So clearly, we are not passing a provider object, which means that the provider used is the one inside the iframe, so if I understand what you say we should use the iframe's origin.

Now look at how we connect there:


      const siwe = new SiweMessage({
        domain: window.location.host, // parent.host,
        uri: parent.origin,
        address,
        chainId: network,
        version: '1',
        statement,
        nonce,
        resources,
      })

And you can see we are indeed using the iframe's origin... and yet seeing the warning.

What we have to check here is two things: 1) WHEN gets the SIWE message created, i.e., that impacts what is read from window.location.host and what is eventually included in the SIWE domain message field. 2) WHEN and WHERE the MM provider sends signing request containing the SIWE message to MM.

If 1) and 2) happen in two different windows, then this is the problem.

julien51 commented 1 year ago

WHEN gets the SIWE message created, i.e., that impacts what is read from window.location.host and what is eventually included in the SIWE domain message field.

It is created inside the iframe. I am not sure how to clarify this better. I posted the links to the code above.

WHEN and WHERE the MM provider sends signing request containing the SIWE message to MM.

In the example I shared above it also happens inside the iframe, in the SAME WINDOW.

I understand this is hard ti debug and I am more than happy to help. Can you please make sure you look at the code I sent above?

awoie commented 1 year ago

WHEN gets the SIWE message created, i.e., that impacts what is read from window.location.host and what is eventually included in the SIWE domain message field.

It is created inside the iframe. I am not sure how to clarify this better. I posted the links to the code above.

WHEN and WHERE the MM provider sends signing request containing the SIWE message to MM.

In the example I shared above it also happens inside the iframe, in the SAME WINDOW.

I understand this is hard ti debug and I am more than happy to help. Can you please make sure you look at the code I sent above?

Got it.

In the meantime, can you try to take a screenshot that shows in debug mode at a breakpoint of the line of code that sends the signing request to MM and that shows the values of window.location.host and the SIWE message (with SIWE the domain message field) in the debugger? That might already eliminate a few sources of potential errors.

awoie commented 1 year ago

WHEN gets the SIWE message created, i.e., that impacts what is read from window.location.host and what is eventually included in the SIWE domain message field.

It is created inside the iframe. I am not sure how to clarify this better. I posted the links to the code above.

WHEN and WHERE the MM provider sends signing request containing the SIWE message to MM.

In the example I shared above it also happens inside the iframe, in the SAME WINDOW. I understand this is hard ti debug and I am more than happy to help. Can you please make sure you look at the code I sent above?

Got it.

In the meantime, can you try to take a screenshot that shows in debug mode at a breakpoint of the line of code that sends the signing request to MM and that shows the values of window.location.host and the SIWE message (with SIWE the domain message field) in the debugger? That might already eliminate a few sources of potential errors.

Asking this because it is really hard to debug / reproduce.

julien51 commented 1 year ago

There!

image

julien51 commented 1 year ago

and I verified in the console that window.location.host is indeed:

Screenshot 2023-06-08 at 11 10 53 AM
julien51 commented 1 year ago

and that matches this: Screenshot 2023-06-08 at 11 11 14 AM

julien51 commented 1 year ago

Posting a Loom video in an effort to bring even more clarity: https://www.loom.com/share/ab4730f2036c480ca6e732a577660643

awoie commented 1 year ago

@julien51 I debugged the dapp frontend myself on chrome / macos. I can reproduce the error and also when debugging, I can see that both SIWE domain and window.location.host have the same value at the time of sending the signing request but MM displays a warning that the domain and the origin don't match. MM thinks the origin is the dapp domain which cannot be the case since my browser console tells me that window.location.host is the iframe origin. This looks like there is an issue in the MM extension, I agree.

julien51 commented 1 year ago

So... after even more deeper debugging, it looks like the root cause is in fact on our end, due to a race condition, where we indeed sometimes (depending on how quickly we get an injected provider) we will use the parent's provider, instead of the iframe provider.

I am pushing a fix on our end that will let us know for sure if we indeed fixed it.

awoie commented 1 year ago

Great you found the issue. That explains why the issue sometimes surfaced and sometimes it didn't.

julien51 commented 1 year ago

Confirming that indeed we found a fix and that it is now live. Thanks everyone for your patience and help debugging this.

digiwand commented 1 year ago

Came across this and realized I forgot to comment here following our debugging session @julien51. Glad we could help!

For clarity and documentation purposes:

Internal in-depth security analysis was done by @weizman to inspect the details of the iframe use-case and confirm MetaMask is working as expected.