MetaMask / metamask-extension

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

Ensure signTypedData is restricted to a domain #10576

Open danfinlay opened 3 years ago

danfinlay commented 3 years ago

EIP712 has a domain field that is meant to be used as a security measure, to restrict the range of possible signatures.

We do not currently enforce domain in any way that could prove to a contract that a signature was suggested to the wallet from a given web app.

This has been requested by an L2 team using signTypedData.

danfinlay commented 3 years ago

But since we haven’t added this behavior yet, this could break existing Ðapps.

Proposal: We add a metrics event for that method, which just records when a domain requests a signature from a domain that does not match itself, and record those two values. If after a sprint this is an acceptable number of reports, we can roll out this security improvement.

danfinlay commented 2 years ago

Another way to avoid breaking as badly: make this an optional param. Contracts can then require it if they like. This allows contracts to only work with some sites, which damages interoperability and permissionless innovation but can provide a type of security in an opt in way.

Gudahtt commented 2 years ago

The EIP-712 spec includes a domainSeparator component with various optional fields, but there is no origin field currently defined (or any similar alternative). In effect, the domain is not present for us to verify.

There has been much discussion on including an origin field, but none has been accepted yet in the current draft of EIP-712.

There is a verifyingContract field, but I don't understand how we're supposed to use this field. The description states:

the address of the contract that will verify the signature. The user-agent may do contract specific phishing prevention.

bentobox19 commented 2 years ago

Some good literature on Self Authenticated Domains:

(referenced by @holantonela, btw)

adonesky1 commented 2 years ago

Proposal: We add a metrics event for that method, which just records when a domain requests a signature from a domain that does not match itself, and record those two values. If after a sprint this is an acceptable number of reports, we can roll out this security improvement.

Should we implement this metric event and get a sense of how frequently this mismatch occurs and what domain fields dapps are most often including in signTypedData calls?

Since as @Gudahtt points out there is currently no full "origin" field on the domain object, do we feel ok matching the name field against the URL hostname for some base verification (showing a non-blocking warning to the user when there is a mismatch)? This was the approach I was taking here

cc @Gudahtt @danfinlay

holantonela commented 2 years ago

We may be able to reuse domain binding check coming with SIWE implementation. Ref: https://github.com/spruceid/metamask-extension-siwe/tree/feat/siwe-update

holantonela commented 1 year ago

https://github.com/MetaMask/metamask-extension/pull/16616

danfinlay commented 1 year ago

my impression is that it is very rare to find a 712 signature bound to a website domain. Usually the domain separator is just used to specify the verifying contract and chain ID, a version, and the name of the contract. Might be interesting to even have a metric for if people ever use domain binding.

holantonela commented 1 year ago

my impression is that it is very rare to find a 712 signature bound to a website domain.

I agree, especially when login front-ends are dissociated from the main domain. We will quickly learn about dapp implementations when this gets released. We can even be more flexible by doing something like wildcards for subdomains.

While discussing this enforcement with stakeholders, most people in the room agreed that this is a great way to mitigate phishing attacks. Also, if we warn users about this mismatch, there is not available recovery flow. More details here https://github.com/MetaMask/metamask-extension/issues/16295

Might be interesting to even have a metric for if people ever use domain binding.

Yes. We have metrics in place to learn about SIWE usage.