New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIP] Add eth_signTypedData as a standard for machine-verifiable and human-readable typed data signing with Ethereum keys #712

Merged
merged 58 commits into from Jun 9, 2018

Conversation

@LogvinovLeon
Contributor

LogvinovLeon commented Sep 12, 2017

@danfinlay

This comment has been minimized.

Contributor

danfinlay commented Sep 12, 2017

Previously, the signing story in the Ethereum space has been plagued by incompatibilities, insecurities, and indecipherability. This proposal moves the signing API a big step towards usability, usefulness, and security, so as far as I can tell, it's just a fantastic feature that I'd be excited to see what developers can do with.

Unless there are major issues caught in this discussion, I'm very open to implementing an experimental version of this for MetaMask as soon as we have tests to help implementers ensure compatibility. We actually already have community members lining up to implement this feature, it's very needed for things like state channels.

@alex-miller-0

This comment has been minimized.

alex-miller-0 commented Sep 12, 2017

I would use this. I've never liked the idea of users signing non-ASCII gibberish.

@SilentCicero

This comment has been minimized.

SilentCicero commented Sep 12, 2017

I like this. I am all for this one.

@andytudhope

This comment has been minimized.

andytudhope commented Sep 12, 2017

Also a big fan of this EIP, would be very useful to me 👍

@jamesyoung

This comment has been minimized.

jamesyoung commented Sep 13, 2017

Yes, very useful. I would use this too.

@iam-peekay

This comment has been minimized.

iam-peekay commented Sep 13, 2017

Incredibly useful.

@mickys

This comment has been minimized.

mickys commented Sep 13, 2017

Rather useful when you want to show a proper Signing Request for an off-chain transaction.
That could be a Login request for example, or signing a message that everyone can validate the sender.

@MicahZoltu

This comment has been minimized.

Contributor

MicahZoltu commented Sep 13, 2017

Why personal_* namespace instead of eth_* namespace? I have never really understood the difference, but I kind of always assumed that things in eth_* namespace were more central to the operation of Ethereum while personal_* were more ancillary. To me this feels like something that will eventually be core to Ethereum (state channels) so it feels like it should go in eth_*.


Why is the hashing done in a nested fashion? It seems like just a lot of wasted effort vs hashing everything in one shot like:

string message = 'Hi, Alice!';
unit value = 42;
const hash = keccak256('string message', 'uint value', message, value);
address recoveredSignerAddress = ecrecover(hash, v, r, s);

Since hashes are one-way, there isn't really any value in decomposing them into sub-hashes since you can't unpack the data. A contract wanting to validate the signature would look something like:

contract Foo {
  function apple(string message, uint256 value, message, value, v, r, s) {
    bytes32 hash = keccak256('string message', 'uint256 value', message, value);
    address recoveredAddress = ecrecover(hash, v, r, s);
  }
}

Perhaps the idea is to be able to pre-hash the type signature so contracts don't have to re-hash that part each time? If so, then I recommend having the signature hash as the first parameter, then the parameter values as the 2-n parameters:

contract Foo {
  function apple(string message, uint256 value, message, value, v, r, s) {
    bytes32 hash = keccak256(signatureHash, message, value);
    address recoveredAddress = ecrecover(hash, v, r, s);
  }
}

This avoids hash nesting in most cases in the contract, yet still provides the value-add of pre-hashing the signature.


The EIP should include very clear wording on how the types should be encoded. For example, I think we should only use explicit types like uint256 and not uint. Linking to the Solidity docs is too vague and subject to change. Perhaps there is an EIP that defines the set of valid types? The same argument against linking to Solidity GitHub, and the link you provided doesn't actually give a list of valid type names.


This EIP should explicitly state how strings are encoded. My vote is strongly for UTF-8 as the defacto standard of the internet.


Discussion

I wonder if there is value in supporting nested objects? e.g., arrays, structs, etc. While a flat item list is OK, it does put some upper-bound limits on how useful this signing method can be. If we supported arrays and nested objects I think it opens up most of the doors we would want in the foreseeable future.

@danfinlay

This comment has been minimized.

Contributor

danfinlay commented Sep 13, 2017

Why personal_* namespace instead of eth_* namespace?

I believe the personal_ prefix was an attempt to separate key-signing/account management operations from state-reading operations. Ideally, an RPC node only provides eth_* endpoints, and the wallet/signer handles the personal_* space. Since it involves signing with keys, the current pattern suggests this belongs in the personal space.

This EIP should explicitly state how strings are encoded.

I'm also in favor of UTF-8 for string encoding.

value in supporting nested objects? e.g., arrays, structs, etc.

I'm in favor of initially launching with whatever is the minimal set of supported types that we're interested in (likely current solidity primitives to start), with more complex types (like nested arrays & structs) being good subjects for later, expansion proposals.

@CJentzsch

This comment has been minimized.

CJentzsch commented Sep 13, 2017

Very useful!

@tomusdrw

This comment has been minimized.

tomusdrw commented Sep 13, 2017

Great proposal!

In Parity we don't expose personal_ APIs for web-dapps by default. IMHO personal_ should expect a password for private key to be provided and have their equivalents with no password in eth_ namespace: eth_sendTransaction(transactionRequest) -> personal_sendTransaction(transactionRequest, password)

IMHO the dapps should use eth_signTypedData(typedData) and personal_signTypedData(typedData, password) should be exposed only on more secure transports than HTTP (since it allows for handling scenarios where user is not required to confirm each transaction, like some automated services).

@LogvinovLeon

This comment has been minimized.

Contributor

LogvinovLeon commented Sep 13, 2017

@MicahZoltu

Personal vs Eth

Dan Finlay already helped me with this one ;)

Hashing

Yes, we want the smart-contract to burn as little gas as possible while remaining secure. I like your second approach with keccak256(signatureHash, message, value). I'll update the EIP.

Types

Kinda agree but would like to get more arguments for this one. As far as I know every type alias maps to exactly one real type and it's well-defined. uint -> uint256. Then why would we disallow uint? Smart contract might also have arguments of those type aliases. Can you point any attack vectors if we allow those type aliases?

I was trying to link to more persistent places, but failed to find any. Solidity doesn't even have the BNF grammar defined. Here is the list of solidity types defined by IntelliJ Solidity Plugin, but it's not official. If someone proves me wrong and finds a better source - happy to include it.

UTF-8

Agree. Will update

Nesting

I'll need to read more into how solidity alligns structs in memory.
There are two approaches. Allowing nested lists (like RLP), or allowing Structs also.
I think - this can be implemented as a non-breaking extension to the existing EIP, so even if we don't agree on this extension now (we should still try), we can postpone it and still have this one.

@tomusdrw I'm talking about Parity Signer. The way Parity Signer communicates with the backing node is more of a Parity implementation detail. As an untrusted DApp I don't/shouldn't have access to user's password, so I'll ask Parity Signer to show user the request and request the password. There is a little bit of confusion here, cause the same set of RPC calls is used to communicate with node and with the signer.

@chriseth

This comment has been minimized.

Contributor

chriseth commented Sep 13, 2017

I think having the ability to sign structured data is a very good step forward!

I'm wondering why you don't just use ABI encoding for the data and the json-ABI-specification for the schema? Note that the latest version also supports arbitrarily nested structures.

This would have the benefit that it is clear which types are allowed, we already have encoding and decoding functions and some of them are even accessible from within smart contracts.

@fabioberger

This comment has been minimized.

fabioberger commented Sep 13, 2017

Although enabling arbitrarily nested data structures rather then simply an array of typed elements would give developers greater expressive power, it does come at a cost of making it much harder for the Signer UI's to anticipate and properly display the data to the user. Instead of a simple list of name => value pairs, they would need to handle the edge case of displaying a triple-nested object.

In addition, any nested data structure can be flattened to a simple array by using more verbose names for the elements, so it's not that we are depriving a developer from including any value they want hashed.

@chriseth

This comment has been minimized.

Contributor

chriseth commented Sep 13, 2017

@fabioberger there are lots of use-cases where you have an array of pairs. A simple example is creating a multisig wallet and initializing it with owners and their personal daily allowance. Flattening this structure will make it much harder for users to check which allowance corresponds to which owner.

@danfinlay

This comment has been minimized.

Contributor

danfinlay commented Sep 13, 2017

IMHO personal_ should expect a password for private key to be provided and have their equivalents with no password in eth_ namespace

I'm not a fan of methods that require a password. Since the web3 API is exposed to untrusted websites, for one to have a password means for a user to be entering their password in an untrusted UI. I think it's much better for the signer to take responsibility for requiring password for personal_* methods, even when the password is not an argument to the function.

should be exposed only on more secure transports than HTTP

That sounds fine to me, but is an implementation detail for the signers. For example, personal_sign on MetaMask is never broadcast over the network, it's just a method name. Same would be true for this.

why you don't just use ABI encoding for the data and the json-ABI-specification for the schema?

The strongest reason I'm aware of to not just use ABI encoding is that since there is no smart contract to enforce rules, enforcing parameter names adds a layer of signed intention to the interaction.

@Arachnid

This comment has been minimized.

Collaborator

Arachnid commented Sep 13, 2017

I'm not a fan of methods that require a password. Since the web3 API is exposed to untrusted websites, for one to have a password means for a user to be entering their password in an untrusted UI. I think it's much better for the signer to take responsibility for requiring password for personal_* methods, even when the password is not an argument to the function.

This - but also, the account may not even have a password; for instance if it's a hardware wallet.

@MicahZoltu

This comment has been minimized.

Contributor

MicahZoltu commented Sep 13, 2017

The strongest reason I'm aware of to not just use ABI encoding is that since there is no smart contract to enforce rules, enforcing parameter names adds a layer of signed intention to the interaction.

@FlySwatter I don't believe @chriseth was arguing for using ABI encoding only, but rather ABI encoding for the parameter data and then JSON-ABI-Specification for the schemea and supplying them both (as this EIP recommends).

My argument against ABI encoding is that ABI encoding is way harder than the encoding proposed here and generally human unreadable. I personally 😡 at ABI encoding and RLP encoding because they are so opaque to humans, hard to code against, and there generally aren't a lot of libraries available that help with this for many languages. Their advantages, in theory, is data size though I would be curious as to how much is actually being saved and whether that is meaningful in this context.

@ameensol

This comment has been minimized.

ameensol commented Sep 14, 2017

Hi all. If no one has any objections my team is going to try and have this spec tested and shipped as part of MetaMask by next Friday. This is a critical dependency for the state channel auction system we've developed for our upcoming ICO. Specifically, it will prevent attacks where malicious client JS attempts to replace the hash the user is about to sign, which in the worst case could be a real Ethereum TX that sends the attacker all the user's ether/tokens.

Based on the above discussion, our implementation targets (for v1) are:

  1. personal_signTypedData over eth_*
  2. No nesting
  3. No JSON-ABI

@ukstv author of Machinomy will lead development and testing.

He has also offered this schema for a Machinomy payment update as an example for this EIP.

@ameensol

This comment has been minimized.

ameensol commented Sep 15, 2017

Going to take silence as meaning there are no objections.

@sekisanchi

This comment has been minimized.

sekisanchi commented Sep 16, 2017

Apart from main discussion point, I'm concerned about text coding of long bytes characters won't produce any drawbacks on relevant future implementations. I'd like to see the UTF8 implementation will not, by quick experiment, and hope getting attentions from those who concerned similar in double bytes countries to validate the implementation options.

@Arachnid

This comment has been minimized.

Collaborator

Arachnid commented Sep 16, 2017

While I appreciate the goal here, I think some assumptions need re-examining; namely that a function name and set of named parameters constitute a user interface that an average user will be able to understand.

I think that evidence shows that this isn't a good user experience, and that the vast majority of users will not understand what they're being shown any more than they understand a long hex string. Just look at how often users ignore things like Android permission dialogues - and those are explained in plain english.

My argument against ABI encoding is that ABI encoding is way harder than the encoding proposed here and generally human unreadable.

I believe the suggestion is to send the ABI encoded data alongside the ABI. This seems like a simple solution to me, and compliant software will already need an ABI en/decoder to be able to function anyway.

@danfinlay

This comment has been minimized.

Contributor

danfinlay commented Sep 16, 2017

I agree with @sekisanchi that some double-byte text samples should be included in the test suite for any implementation.

@Arachnid I'm a little unclear, are you saying providing the (method name, param names, and type data) is or is not a good enough UX? Your first paragraph suggests it's not (and I'm all for constant improvement) but then you end By saying ABI data would be sufficient.

Of the two binary encoding options suggested here, I think they both give a similar end-user experience (method name, params names, type data), so we should consider other reasons for choosing an encoding scheme.

I personally could go either way. I agree with Micah that RLP is a bit cumbersome, but also I agree with Nick that every client will have this decoder anyways, so who cares?

I wouldn't mind hearing more points for and against the encoding types, although ultimately I don't think it's a deal breaker either way. For example, price of on-chain decoding I think would be an important metric.

@Arachnid

This comment has been minimized.

Collaborator

Arachnid commented Sep 16, 2017

I'm a little unclear, are you saying providing the (method name, param names, and type data) is or is not a good enough UX? Your first paragraph suggests it's not (and I'm all for constant improvement) but then you end By saying ABI data would be sufficient.

I'm saying that providing that information is not a good enough UX. There's no way an average user will be able to decipher this data; for many API calls even a developer can't understand the implications without also reading the source code.

I'm also saying that if someone wanted to implement this, JSON ABI + ABI-encoded payload seems like a sensible way to do it, and the dependencies for parsing that are already required by apps.

@jannikluhn

This comment has been minimized.

jannikluhn commented Sep 17, 2017

This EIP is a big step into the right direction. However, one flaw I see is that it adds unnecessary workload on the smart contract: It has to process and potentially store parameter names and types in human readable form. This increases gas costs and makes the code ugly. Making sure that the user signs only messages she wants to is not job of the blockchain, I don't think contracts should have to do anything with this.

Here's how I think this should work in an ideal world: Allowed message types are defined in the contract's metadata, maybe as part of the ABI or in a separate section. Nodes have the ability to import the metadata of contracts they trust. Calls to the sign method reference one message type and specify the values to sign. The UI can then show argument values, types, names and even additional documentation (which is also part of the metadata). As the metadata is not provided by the potentially malicious dapp, argument names and types don't have to be part of the signed data, only the actual values have to be signed.

Unfortunately, nodes don't know about contract metadata as of today to my knowledge (despite them containing useful information to display for normal transactions as well), so this is approach would take longer to implement. But in my opinion it would be cleaner and also more secure, due to the additional information that can be presented to the user.

Note that I'm not necessarily advocating for this, just wanted to put the idea out there. As I said, this EIP may be good enough for the time being.

@MicahZoltu

This comment has been minimized.

Contributor

MicahZoltu commented Sep 17, 2017

It has to process and potentially store parameter names and types in human readable form

The way it is designed, only the hash of the human readable method/parameter names needs to be stored on-chain. This is enough information to validate the data presented to the user was correct.

@MicahZoltu

This comment has been minimized.

Contributor

MicahZoltu commented Sep 17, 2017

Back on the topic of eth_* vs personal_*, I'm more compelled by #712 (comment) than by #712 (comment). I would like to get more feedback from other clients as to their long-term trajectory for each namespace though. It sounds like we have Parity represented with intent to have personal_* be password protected, un-exposed and bypasses the signer while eth_* is exposed and handled by the signer.

Can anyone represent Geth or is anyone interested in defending a trajectory different from the one presented by Parity here?

@danfinlay

This comment has been minimized.

Contributor

danfinlay commented Sep 18, 2017

I'm saying that providing that information is not a good enough UX. There's no way an average user will be able to decipher this data; for many API calls even a developer can't understand the implications without also reading the source code.

I don't think this is worse than the legibility of other on-chain transaction signatures, though. It'll be a continual effort for signer/clients to make these proposals more intelligible, including adding references to reviews of given state channels, links to their source codes, etc.

is anyone interested in defending a trajectory different from the one presented by Parity here?

I'd like some clarity on exactly what Parity does for personal_* prefixed methods.

In Parity we don't expose personal_ APIs for web-dapps by default.

When do you expose those APIs? When would you ever trust a web UI with the user's password? If it was trusted once with it, what would stop it from submitting many methods with that password?

If the personal_* namespace is all intended to use passwords, and geth devs agree, maybe MetaMask has been mis-implementing it, and we could stick to eth_*, but I don't understand why a site would ever be trusted with a password, especially since (as Nick pointed out), some types of signers don't even have passwords to request (like hardware wallets, or offline/cold signers).

@wighawag

This comment has been minimized.

Contributor

wighawag commented Oct 10, 2018

@rmeissner

If the evm doesn't know about the chain id, it is necessary to hardcode it in the smart contract. By doing so you will run into problems in the case of a hardfork where a new chainID is introduced.

good point. We should then push for adding chainId access in the evm. Any EIP for that yet ?

Note though that this lead to an interesting problem of what become the next valid chainId anyway. I mean if you signed to agree on something on one chain but then you disagree on where the chainId should continue, you should have right to cancel the validity of your signature.

Maybe chainId should not be part of the spec at all ?

Also if you only enforce the origin and chainId in the browser and not the smart contract, what stops a malicious dapp from just saying this is for chainId 4 and then send it to mainnet (since the contract won't check)?

The point is that it add security for the smart contract that can and do check. If they don't check (or cannot check as you rightly mention) the application would not be able to specify a chainId since if it does and the smart contract doesn't the signature won't match. (the chainId not being part of the expected message)

But the point anyway is that enforcing chainid on the browser does not remove flexibility on the smart contract. The only things this does is to provide extra security for when it can be checked.

In other words, by making web3 browser enforce chainId today we can potentially benefit from it in a future hardfork with new contract that can check for chainId.

It is also worth pointing that signatures do not necessarily need to be used by smart contract and can be used for other purpose where chainId could have an importance.

@0xbitcoin

This comment has been minimized.

0xbitcoin commented Oct 10, 2018

@rmeissner

This comment has been minimized.

rmeissner commented Oct 10, 2018

I opened an EIP exactly because of this issue some time ago 😉
#1344

I do agree that in case where the chainId is specified it should be made clear to the user if there is a mis-match (same for a potential origin parameter).

@0xbitcoin we are already testing and integrating it in our dapp ... actually is pretty easy 🤔
Here we use the ganache implementation of EIP-712 but it should be the same as soon as MetaMask merges their PR (https://github.com/gnosis/safe-contracts/blob/v0.0.2-alpha/test/gnosisSafePersonalEditionEthSignTypeData.js#L57)

@wighawag

This comment has been minimized.

Contributor

wighawag commented Oct 10, 2018

@0xbitcoin
The proposal to add origin and chainId checks is not to make these fields mandatory, just to make it the duty of web3 browser to enforce their validity when added to a message. This would remove the information load off the users

if your smart contract do not care of these, your application frontend does not need to request them.

But your users will be vulnerable to some attack that push them to accept confirmation blindly. I ll hopefully finish my article to explain it.

@wighawag

This comment has been minimized.

Contributor

wighawag commented Oct 10, 2018

I opened an EIP exactly because of this issue some time ago 😉

great!

@0xbitcoin

This comment has been minimized.

0xbitcoin commented Oct 10, 2018

@rmeissner

This comment has been minimized.

rmeissner commented Oct 10, 2018

@wighawag I would have to look into the requirements again to get the opcode in, and it will probably not make it into constantinople 😞

The proposal to add origin and chainId checks is not to make these fields mandatory, just to make it the duty of web3 browser to enforce their validity when added to a message

I think this was not clear from previous comments 😉

@wighawag

This comment has been minimized.

Contributor

wighawag commented Oct 10, 2018

@0xbitcoin
yes optional (as described below, that was not how I introduced the idea)

On the other hand if origin is included and the signer can verify the current document's origin, it must verify and refuse to sign if the two do not match

@rmeissner
yes I was targeting at making it mandatory but from the discussion it became clear that it is possible to make it optional while preserving security for those whore care and allow other use case that do not care or where there is no actual "document's origin" to skip it

@dekz

This comment has been minimized.

dekz commented Oct 10, 2018

Have you read this older EIP155 which adds simple replay protection as part of the v field in an ec signatures v, r, s parameters?

You are correct in that hard coding a chainId in a contract will result in that contract having this hardcoded value on both chains, so it's worthless.

It is not immediately obvious how enforcing origin increases security without sacrificing usability and resulting in additional callData parameters and contract complexity.

@NoahHydro we've spoken with the solidity team to get built-ins to make integration easier.

@AusIV

This comment has been minimized.

AusIV commented Oct 11, 2018

@dekz - The replay protection on the v field of ec signatures blocks things from accidentally getting picked up by both chains, but if someone wanted to maliciously apply a signature from one chain onto another they need only manipulate the v field in a very predictable way.

I don't see a scenario where an EIP712 signature accidentally gets picked up on the wrong chain; the concern here is pretty exclusive to deliberate replays.

That said, without having the chainId available on-chain, I don't know that there's much for EIP712 to do about it.

@wighawag

This comment has been minimized.

Contributor

wighawag commented Oct 12, 2018

Finally published my post explaining 3 different proposal 2 of which would improve EIP712:
https://medium.com/@wighawag/3-proposals-for-making-web3-a-better-experience-974f97765700

The last proposal requires automated "origin checks" to work and should bring a far greater experiences to users. Check it out.

@0xbitcoin

This comment has been minimized.

0xbitcoin commented Oct 12, 2018

@0xbitcoin

This comment has been minimized.

0xbitcoin commented Oct 12, 2018

@MicahZoltu

This comment has been minimized.

Contributor

MicahZoltu commented Oct 12, 2018

If you want informed signing, you should push for #719 rather than this. End-users do not have the background/context to do informed signing when given only function names and parameter names. The signer needs to be able to present the user with a trusted signing interface (could be text-only) that is put into human terms, and potentially localized.

@0xbitcoin

This comment has been minimized.

0xbitcoin commented Oct 12, 2018

@wighawag

This comment has been minimized.

Contributor

wighawag commented Oct 15, 2018

Hi Infernaltoast (@0xbitcoin )

Here are some answers :

Is there a Gitter channel or something like that for this EIP 712 ?

I don't know any but would like to chat too :)

this EIP is instrumental to
Metamask and the way that Ethereum Dapps will interact with their smart
contracts (through Metamask) and so it is of extreme importance that this
be done correctly.

I agree, hence the reason I push for these improvements

and what exactly is being
checked (IPFS hash of document origin? What is an example?)

If your application's frontend is on IPFS, its origin will contain the hash of the content.
As such the web3 browser can actually ensure the content could not be modified. As such if the frontend code has been audited or proven to not contains any code that would steal your tokens, and the smart contract checks for origin, the user (who approved that origin) can feel completely safe.

If your application's frontend is on a classical DNS name pointing to an IP, the web3 can still check the origin but the user's overall security relies on both the security of the DNS system, the security of the company in control of the DNS and whether your application is honest or not (as it can change where the DNS points to or simply change the content). This is an unfortunate consequence of how the old web was designed.

There could be some scope to specify a url scheme allowing application on traditional DNS to specify the hash of their content. But that is another story.

and why is
it necessary to encrypt data when we use HTTPS for everything now anyways ?

In the post linked, there are 3 proposals but only the last 2 pertain to EIP712. The non-interactive decryption scheme does not need EIP712 nor EIP712 needs it. But note that the use of HTTPS is orthogonal to the proposed scheme.

I am building a series of interconnected dapps
that depend on Typed Signed Data and I want to help out EIP712 because it
is a major bottleneck for my Dapp and I cannot progress forward until i
know that this is finalized and will not change.

Down the roads, you will thanks me for pushing these proposals :) They increase both security and usability but if you do not want or can't use them, you can continue to do what the current spec does (by omitting the origin field). In other words, unless the EIP evolve in a different way as a result of the inclusion of such proposals, I can see the new spec to be backward compatible with the current spec.

Just by hardcoding the hash of the domain string and
the url address in the contract code and assuming the javascript will pass
it in properly ?? Couldnt hacked javascript lie about the URL?

The url does not need to be hardcoded, This would be highly undesirable for the smart contract to only have one origin allowed. Instead the smart contract can allow users to decide which origin are trusted, by allowing the users to approve more origins. This actually provides a nice mechanism for requiring users to approve applications before being able to use them.

Also is
that optional to do or enforced ?

As I replied to one of your previous question, your application can omit the origin field. In that case the web3 browser won't check for the current document's origin and your smart contract does not need to add any origin for checking signature.
As such it is optional, but I would recommend smart contract to use the origin checks to ensure users knows they interacting with the set of trusted origin, unless your application is in a context that have no concept of document's origin.

In my opinion, the whole point of EIP712
is that the front end >does not matter at all< and can no longer trick
you.

Please re-read the article and the part on Alice's app vs Bob's app. Bob's app could trick the user in thinking they interact with Bob's smart contract while they actually interact with Alice's app by letting the user approve signatures request very often, removing the user's natural guard and awareness (authorization fatigue)

because obviously we cant trust the frontend javascript to tell
us the document origin and URL, correct? since it could be hacked? )

That's the point of the origin field. It is the web3 browser that check the origin. If the javascript try to request a signature with a different origin, it will be refused.

Hope that it clarify the proposals for you and answers your questions.

@dekz

This comment has been minimized.

dekz commented Oct 16, 2018

My opinion is that Origin has multiple downsides that outweigh any potential security benefits.

Open ecosystem

Introducing an Origin transitions away from an open ecosystem A dApp which uses 0x would have a different origin to what the 0x contracts expect. So to enable a thriving ecosystem where contracts and protocols are open, the dApp would need to specify augur origin as its origin (likely raising flags in a Signer window).

If we assume the user can specify or whitelist the origins (and whitelist dApp.com) the EIP712 implementor would need a way to query the users whitelist and compare with what was digested and signed.

If we assume the contract/protocol creator must whitelist individual origins, then it is no longer an open protocol.

Gas costs and callData

Either origin would need to be supplied in the callData or the contract would need to iterate through each supported origin and check for a hash match. This increases gas cost by N for N origins. If we assume a low level protocol which could be used by many dApps this is unacceptable. In addition there are gas costs to approving origins on the various contracts.

Proposed security

Matching an origin to DNS or any offchain data doesn't add any additional security. IPFS hash may do so but this is unintelligible for a user and there are less ways to verify this than contract addresses.

The Domain already allows for name fields, contract addresses, versions. Some are optional but the combination of Name and Contract address provides the user with enough information to:

  • See this is for the 0x contract name: 0x Protocol
  • Match the address via many sources of information. E.g whitelist in signer, etherscan, 0xproject.com

Origin can be spoofed (oxproject.com, unicode fields in domains) and IPFS hashes do not provide a user a sane way to validate (users likely only check the first 4 bytes).

@wighawag

This comment has been minimized.

Contributor

wighawag commented Oct 16, 2018

@dekz
Thanks for your input. Let me reply to your points:

Open ecosystem

It is as open as the smart contract make it to be

Introducing an Origin transitions away from an open ecosystem A dApp which uses 0x would have a different origin to what the 0x contracts expect.

only if 0x contracts expect a particular origin. But to be an open protocol it should allow users to approve their own trusted origin, being 0x 's own frontend or another company's. This adds security and openness at the same time

So to enable a thriving ecosystem where contracts and protocols are open, the dApp would need to specify augur origin as its origin (likely raising flags in a Signer window).

As I mentioned the contract can (and should) make the user controls which origins is allowed. Of course, this is the smart contract's author that decide in the end, but that would be silly to force your smart contract to be used by only one origin. If you use ipfs or swarm hashes (combined with ENS) you would need a way to upgrade anyway. You would either have to secure the keys that are allowed to approve new origin (never safe in the long run) or let the users decide, which is much more secure.

If we assume the user can specify or whitelist the origins (and whitelist dApp.com) the EIP712 implementor would need a way to query the users whitelist and compare with what was digested and signed.

Which implementor ?

  • The smart contract would have such origin in its storage as soon as the user approve one and can simply look it up based on the origin passed in. Please have a look at the example code : https://github.com/wighawag/eip712-origin
  • The signer would not need to store anything, nor have any list. it simply ensure the document's origin matches the origin inserted as part of the message to be signed

If we assume the contract/protocol creator must whitelist individual origins, then it is no longer an open protocol.

As I said, this is only the case if the smart contract's author decides to do this way. As you say this would make whatever the smart contract implements, not an open protocol. I have hard time to see that as the way most protocols are going to be implemented.

It is in some way similar to how some decides to make a smart contract with overarching control or one without. In the ends, users (including developers) would always prefer open smart contract and the one that do not rely on the security of few keys.

In the end I guess, your point is that because now smart contract would be able to close themselves in a easier manner than before thanks to how web3 browser would enforce origin. you expect this to be the new norm. I hope this won't be the case but I also believe that it won't be. An open protocol would be more attractive to users and developers alike. Also since an open version of a closed protocol could always exists, I would bet on the latest.

Gas costs and callData

Either origin would need to be supplied in the callData

that's only 32 bytes of data and is optional for smart contract that do not want the extra security

or the contract would need to iterate through each supported origin and check for a hash match. This increases gas cost by N for N origins.

that's not an efficient implementation. forget about it

If we assume a low level protocol which could be used by many dApps this is unacceptable. In addition there are gas costs to approving origins on the various contracts.

Again this is optional

Proposed security

Matching an origin to DNS or any offchain data doesn't add any additional security.

That's not true. Please re-read the article, especially Alice's app example which show how an external attacker can trick users to sign messages belonging to another.

DNS in itself has security issues but the attack mentioned in the article would also be possible if the origin was an ipfs hash. As such the security provided by origin checks is an extra security beyond the DNS vs content-addressable network divide.

IPFS hash may do so but this is unintelligible for a user and there are less ways to verify this than contract addresses.

Did you read the note on ENS and how a web3 browser could use ENS to redirect the user to the correct ipfs hash but still use that underlying hash as origin to ensure the user is not tricked by the ENS owner changing the underlying registered hash.

This is only possible if we add origin checks

The Domain already allows for name fields, contract addresses, versions. Some are optional but the combination of Name and Contract address provides the user with enough information to:
See this is for the 0x contract name: 0x Protocol
Match the address via many sources of information. E.g whitelist in signer, etherscan, 0xproject.com

Please re-read the attack on Alice's app in the article. This attack is not prevented with such check.
As mentioned in the article, all of this extra information relies on the user checking them all the time

Origin can be spoofed (oxproject.com, unicode fields in domains) and IPFS hashes do not provide a user a sane way to validate (users likely only check the first 4 bytes).

That's why these origin checks are automated! The user does not need to read the origin by themselves. They can now trust their web3 browser to do their check for them, this include unicode differences that the user's eye would not be able to match.

non-interactive signatures

And then you forgot to mention the added usability only possible if we have automated origin checks. While this sounds scary, you should have a look at existing dapps. They either have too much popups that users approve them without reading much or they provide a mechanism to delay the signing (peepeth) to offer a seamless experience.

With non-interactive signature, an application can now provide its own UI for not-so-important signatures, increasing usability and thus security (by reducing authorization fatigue).
With content-addressable networks, the hash can also guarantee that such UI will never change and if a formal proof (or an audit) can guarantee that such frontend can never submit without prior user confirmation, then the user can trust it fully, the same way you can trust an audited smart contract.

I would also like to add that signed messages could have uses beyond smart contracts.

@dekz

This comment has been minimized.

dekz commented Oct 16, 2018

that's only 32 bytes of data and is optional for smart contract

Which proportional to the signature is high.

Again this is optional

Optional is difficult in standards. You will have implementors choose to ignore this if it is optional.

Did you read the note on ENS and how a web3 browser

How will Ledger or offline hardware Signers verify these origins?

Please re-read the attack on Alice's app in the article.

This example is difficult to debate since it is purely anecdotal. Could this user just as likely be tricked into approving the origin, since they have become accustomed to approving origins? Possibly.

Anecdotally alarm bells would be ringing for me if CryptoKitties suddenly asked me to sign a Augur message.

A Signer like a web3 browser could also use heuristics and flag an unusual sign request for this domain. Just like it can with sending ETH to a new address or interacting with a new contract. Identicons can be displayed in signers for the addresses in the Domain field.

If there was a proposal where this was bulletproof (i.e not optional) then of course it would make sense to add it. When it breaks down and the solution falls back to "oh its optional" you have to ask yourself if it is worth including in the first place. Does it work as a general standard or just your particular use case? EIP712 is already an order of magnitude improvement on the current signing of hashes.

@wighawag

This comment has been minimized.

Contributor

wighawag commented Oct 16, 2018

Which proportional to the signature is high.

The signature is accompanied with data, that's not fair to compare the size with the signature alone and again this is a decision made by the smart contract author to include such data. I would personally add it for the security (and usability) benefit that it gives to my users.

Optional is difficult in standards. You will have implementors choose to ignore this if it is optional.

This is not optional for the web3 browser to implement the origin check. It is only optional for the message signing request to add the origin field. As such this is optional only for the smart contract that do not want to check it. The same way as it is optional for them to use EIP712 or not.

How will Ledger or offline hardware Signers verify these origins?

Such offline hardware do not act on their own. They are connected to bridges like metamask that can perform the origin check.

If for some reason such bridge are not available, the security of origin check is not available but that's not worse that current EIP712 without origin checks.

This example is difficult to debate since it is purely anecdotal.
That's not a fair argument in my opinion. The possibility for the attacker is real and the origin check objectively protect users from it.

Could this user just as likely be tricked into approving the origin, since they have become accustomed to approving origins? Possibly.

That's a valid point but that 's a lot more unlikely as this kind of request would happen less often, especially if non-interactive signature become more common as the result of this proposal being adopted.

Also we could make the "origin approval mechanism" part of the standard and make it mandatory for web3 browser to show a different kind of popup with a warning. As I mentioned in the article, this could provide a nice general mechanism for application pre-aproval.

Anecdotally alarm bells would be ringing for me if CryptoKitties suddenly asked me to sign a Augur message.

The point was that the users would not realise it!

A Signer like a web3 browser could also use heuristics and flag an unusual sign request for this domain. Just like it can with sending ETH to a new address or interacting with a new contract. Identicons can be displayed in signers for the addresses in the Domain field.

That's true but a lot less robust that the proposal I put forward. The proof is that it does not allow the safe "non-interactive" signature.

If there was a proposal where this was bulletproof (i.e not optional) then of course it would make sense to add it. When it breaks down and the solution falls back to "oh its optional" you have to ask yourself if it is worth including in the first place. Does it work as a general standard or just your particular use case? EIP712 is already an order of magnitude improvement on the current signing of hashes.

As I said, it is not optional. Only the addition of the origin field in the message is optional. I am not sure I understand your point then when you say that if it was not optional it would make sense. Does it means you agree on web3 browser to check the origin and forbid the use of an origin that differs from the one serving the document ?

As a mandatory check, it makes lots of sense and provide not only an increased security (think again about audited frontends) and usability (non-interactive signatures)

@dekz

This comment has been minimized.

dekz commented Oct 16, 2018

Such offline hardware do not act on their own. They are connected to bridges like metamask that can perform the origin check.

This is not true, dApps have supported Ledger and hardware wallets long before Metamask added support.

Which means Ledger would have an implementation which sees origin and completely ignores it, trusting something else to do the verification. This is where origin and optional breaks down

If for some reason such bridge are not available, the security of origin check is not available but that's not worse that current EIP712 without origin checks.

So the Bob dApp just needs to wait until a user is using a "non-bridge" signer and then is free to spoof the origin.

My issue with Optional additions to a standard is that they can lead to edge cases where the developers and users assume they are safe but in some circumstances they are not.

@wighawag

This comment has been minimized.

Contributor

wighawag commented Oct 16, 2018

This is not true, dApps have supported Ledger and hardware wallets long before Metamask added support.

Fair enough, I still think this would be a shame to remove the benefit for users that can rely on a trusted bridge just because some cannot. We should look forward to the future where origin checks are a basic requirement for web3 browser. That's the whole point of making it part of the standard.

Also it is worth pointing that it is the dapps themselves who choose to act as bridge. As such if they want to continue they can simply opt for using message with no origin specified.

Since they provide the bridge it also means that the origin is implicitly check by the way

Which means Ledger would have an implementation which sees origin and completely ignores it, trusting something else to do the verification. This is where origin and optional breaks down

That something else would be the user and while this is basically falling back to the current EIP712, I would not say that it break down the concept entirely. Users would simply start to move to use more trustworthy setup where they can rely on their web3 browser to do the checks for them. The same way as users started to move to use hardware wallet for the extra security it provides.

So the Bob dApp just needs to wait until a user is using a "non-bridge" signer and then is free to spoof the origin.

You would have to agree though that it makes Bob's job a lot harder :) especially since the users would start to understand the benefit of using a web3 browser that can check the origin.

My issue with Optional additions to a standard is that they can lead to edge cases where the developers and users assume they are safe but in some circumstances they are not.

That's a fair point but in my opinion that should not prevent us from adding mandatory origin checks (for those that can obviously) to the web3 browser so we can start to live a safer and more usable future.

Note also my use of the terms "web3 browser" vs "web3 signer". Web3 browser are basically the one that should be capable of checking origin.

@ryanio

This comment has been minimized.

Contributor

ryanio commented Oct 27, 2018

Hey all, we are looking to implement this EIP into Mist but without an origin or url in the domainSeparator, we feel like the most basic security premise of ensuring you are signing the message for the right dApp is compromised.

Another option is to have a ethereum-lists-like mapping for dApp Name to Contract Address to verify against. Or, a value in the contract itself that specifies the origin or url to verify.

@rmeissner

This comment has been minimized.

rmeissner commented Oct 27, 2018

@ryanio I don't see why a browser would require the origin. There are many cases where an origin is unnecessary or even makes the verification way harder. One of these cases would be our contract (Gnosis Safe) where we don't really care from where the signature is. Also there are multiple signers in our case, so each signer could have a different origin. This introduces quite some unnecessary complexity.

Also another note on the origin. Currently it feels like that the assumption is made that this is always a website (or a specific url). I think this is also an assumption that will not hold up, as native mobile dapps will start to emerge and at some point probably overtake html5 dapps.

I do agree with @wighawag that supporting an origin provides the possibility to add more security. I also think that if a origin is provided in the signature request then it MUST be validated and if it is not as expected the message MUST NOT be signed (same for chainID actually, where it is SHOULD NOT right now)

For a more readable representation of the sign request (as I think reading the smart contract is just not feasible) I think it would be possible to integrate #719 into this EIP as part of the domain. Again here you could say the description MUST be displayed if provided.

@rmeissner

This comment has been minimized.

rmeissner commented Oct 27, 2018

@ryanio I also think that making the user aware which dapp requested the signature/transaction is independent from this EIP. E.g. MetaMask is currently not displaying which url triggered the pop-up and just displays it.

@ryanio

This comment has been minimized.

Contributor

ryanio commented Oct 27, 2018

I also think that making the user aware which dapp requested the signature/transaction is independent from this EIP

Then what is the purpose of domainSeparator?

From the EIP:

The domain separator is designed to include bits of DApp unique information such as the name of the DApp, the intended validator contract address, the expected DApp domain name, etc. The user and user-agent can use this information to mitigate phishing attacks, where a malicious DApp tries to trick the user into signing a message for another DApp.

We wouldn't stop the user from continuing to sign the data, but we would want to warn them.

@rmeissner

This comment has been minimized.

rmeissner commented Oct 27, 2018

This is the purpose of the domain, but not all information are required (to be exact no information are required, you can use an empty domain). And I also see that this is useful, but I wouldn't make it required.

And my second comment was more, that this EIP enforces this on a contract level. But a lot of phishing attacks can already be prevented by just improving the existing UI/UX. If MetaMask displays me for every transaction which tab/website requested this transaction, I would feel a lot more confident that I don't accidentally approve a transaction from a different site.

@wighawag

This comment has been minimized.

Contributor

wighawag commented Nov 2, 2018

Hi,

The newest version of the code at https://github.com/wighawag/eip712-origin is now using "_originHash" in the application specific data.

This works but I avoided mentioning this solution before as it mix application specific data with protocol data. It will also require to define a set of reserved word for adding more specific scheme in the future (see below).

The only benefit is that it does not require existing wallet to change the existing signing code.( Is that really an issue since it is in both case backward compatible ?)

Then web3 browser would simply have to check the origin's validity if it is included in the application data. In that case though, we need a naming convention to avoid collision with application data.

Actually we need to provide a convention for new fields too. As @naure mentioned we can simply set a reserved prefix like "_" and no application should use such prefix else they will run the risk of inadvertently enabling a future scheme.

Of course if we separate application data from protocol data as I originally envisioned, we do not need such convention since the protocol data is not mixed with the application data.

@wighawag

This comment has been minimized.

Contributor

wighawag commented Nov 2, 2018

@rmeissner :

Also another note on the origin. Currently it feels like that the assumption is made that this is always a website (or a specific url). I think this is also an assumption that will not hold up, as native mobile dapps will start to emerge and at some point probably overtake html5 dapps.

The origin concept should still be relevant to native apps. Origin is a general concept that is not necessarily tied to the web or url.

I am not sure what you mean by native dapps. We can separate them in 2 :

You either have a native wallet app that get signature request from other native apps, in which case the operating system could provide the origin (whatever the os can provide to identify the app that requested a signature) to the wallet.

Or you have a built-in wallet in your application (not a good idea in my opinion) in which case the origin is the app itself. Since this require users trusting the app itself, the origin check is implicit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment