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

internal/ethapi: add personal_sign method #2940

Merged
merged 5 commits into from Oct 28, 2016

Conversation

Projects
None yet
@bas-vk
Copy link
Member

bas-vk commented Aug 24, 2016

This PR includes several things:

  1. the behavior of eth_sign is changed. It now accepts an arbitrary message, prepends a known message, hashes the result using keccak256 it calculates the signature of the hash (breaks backwards compatability!).
  2. add personal_sign, same semantics as eth_sign but also accepts a password. The private key used to sign the hash is temporary unlocked in the scope of the request.
  3. add personal_recover, returns the address for the account that created the signature.

personal_recover(message, signature)
personal_sign(hash, address [, password])

The known message added to the input before hashing is

"\x19Ethereum Signed Message:\n" + len(message).

@alexvandesande

@robotally

This comment has been minimized.

Copy link

robotally commented Aug 24, 2016

Vote Count Reviewers
👍 0
👎 0

Updated: Tue Sep 6 09:00:42 UTC 2016

@mention-bot

This comment has been minimized.

Copy link

mention-bot commented Aug 24, 2016

@bas-vk, thanks for your PR! By analyzing the annotation information on this pull request, we identified @karalabe, @zsfelfoldi and @fjl to be potential reviewers

@Arachnid

This comment has been minimized.

Copy link
Contributor

Arachnid commented Aug 24, 2016

With the impending advent of hardware wallets, I'm concerned this is overly internal-account-specific; is there any way to avoid hardcoding in passwords as an authentication mechanism? (I don't have a suggestion just yet, just thinking out loud)

@fjl

This comment has been minimized.

Copy link
Contributor

fjl commented Aug 24, 2016

If you use a hardware wallet, it's better to talk to it on the client side.

@fjl

This comment has been minimized.

Copy link
Contributor

fjl commented Aug 24, 2016

@bas-vk Can we seize this opportunity to fix some of the other issues with eth_sign?

  • It should hash the input on the server side.
  • There was some issue with the Ethereum-specific +27 v value
@Arachnid

This comment has been minimized.

Copy link
Contributor

Arachnid commented Aug 24, 2016

If you use a hardware wallet, it's better to talk to it on the client side.

For signing, perhaps, but I'm thinking more about what sort of interfaces we offer that are backed by accounts, and how those will work if we support accounts other than those whose private keys are held by Geth.

@bas-vk

This comment has been minimized.

Copy link
Member

bas-vk commented Aug 24, 2016

@bas-vk Can we seize this opportunity to fix some of the other issues with eth_sign?

We can, this PR is rather small so I have no problem adding additional changes.

  • It should hash the input on the server side.

Changing this would break backwards compatibility for eth_sign. Exploiting this requires the key to be unlocked. And in that case obtaining the private key is nice but if it's possible to sign arbitrary data a thief basically has full control over the account during the time he can sign data. I will make this change for the new method.

  • There was some issue with the Ethereum-specific +27 v value

I believe the issue was that according to the yellow paper Ethereum hashes should have a signature with v+27 just like bitcoin. eth_sign currently doesn't add 27. Will add 27 to the signature of the new method to make it compliant with the yellow paper and leave eth_sign untouched.

Maybe at some point we can mark eth_sign deprecated.

@fjl

This comment has been minimized.

Copy link
Contributor

fjl commented Aug 24, 2016

Thanks. I agree that eth_sign should be deprecated, ideally quite soon. We can keep it around for the 1.5 release cycle and remove it in 1.6.

@kumavis

This comment has been minimized.

Copy link
Member

kumavis commented Aug 24, 2016

@fjl does the api take the hash or the message?
personal_sign(messageHash, signerAddress [, password])
personal_sign(messageBody, signerAddress [, password])

Its important that we pass the message so that the id mgmt layer (mist/metamask) can present the message to the user for approval

@fjl

This comment has been minimized.

Copy link
Contributor

fjl commented Aug 24, 2016

My proposal is as follows:

personal_sign(message, address [, password]) should sign keccak256(message) with the key that belongs to address and return the signature format that is commonly used in Ethereum (with V + 27). This means that the message would be available for display in MetaMask.

In internal discussions we have considered to prepend "Signed Ethereum Message:\n" to the message
on the server side. This would make it much harder to misuse the API (i.e. you can't sign a transaction this way). The idea of adding the prefix is not set in stone though.

@kumavis

This comment has been minimized.

Copy link
Member

kumavis commented Aug 24, 2016

I'm not sure how the prefix makes it much harder to misuse the API. can you expand?
If the attacker is trying to find a hash collision with a tx sig, they would just build a rainbow table with that prefix as the 'salt'.

I understand how a salt might be better than none, but maybe a dynamic salt like a timestamp would provide more security.

@fjl

This comment has been minimized.

Copy link
Contributor

fjl commented Aug 24, 2016

What we are concerned with specifically is the case of a malicious dapp creating (transaction) signatures on behalf of inexperienced users who just click yes.

@kumavis

This comment has been minimized.

Copy link
Member

kumavis commented Aug 24, 2016

Yes, that is the primary attack vector im worried about.

@fjl

This comment has been minimized.

Copy link
Contributor

fjl commented Aug 24, 2016

If you have more ideas for making this safer please dump them here. Adding a timestamp sounds nice at first, but keep in mind that something (probably a contract) will need to verify the signature eventually and it needs to add the same prefix. The user would need to submit the timestamp to said contract later and that sounds complicated. But maybe it's the right thing to do.

@kumavis

This comment has been minimized.

Copy link
Member

kumavis commented Aug 24, 2016

FWIW we are currently using the old version of the eth_sign API and displaying the messages.
We're currently reworking this UI and it will be more clear what exactly you are signing, the size of the message, and have a bunch of warnings not to sign any weird messages.

image

@kumavis

This comment has been minimized.

Copy link
Member

kumavis commented Aug 24, 2016

or just use a random salt and include it in the serialized message signature

personal_sign( message ){
  salt = random()
  sig = sign( hash( salt + message ) )
  return [sig, salt]
}
@fjl

This comment has been minimized.

Copy link
Contributor

fjl commented Aug 24, 2016

Yes that looks better (and pragmatic). I'll think about it.

@fjl fjl added this to the 1.5.0 milestone Aug 24, 2016

@fjl fjl added the area:crypto label Aug 24, 2016

@kumavis

This comment has been minimized.

Copy link
Member

kumavis commented Aug 24, 2016

requesting comment from @Georgi87 and @mhhf who make use of user message signatures in the https://www.gnosis.pm/ predication market dapp

@kumavis

This comment has been minimized.

Copy link
Member

kumavis commented Aug 24, 2016

Theres still a problem with signing messages with the same key we use for tx sigs.

given a message signature, theres still a large search space for valid tx hashes that could match the message signature. Tx parameters such as value and toAddress provide a lot of search space that can be explored in parallel to find a hash collision.

evilTx: { value: allYourMoney, to: attacker01 }
evilTx: { value: allYourMoney, to: attacker02 }
evilTx: { value: allYourMoney, to: attacker03 }
...

While significant increasing scope and complexity of this issue, and perhaps breaking compat with existing geth single-key behavior, this could be alleviated by using a different key available on an hd-tree
ethereum/EIPs#84

@fjl

This comment has been minimized.

Copy link
Contributor

fjl commented Aug 24, 2016

@nagydani would be good to have your input here as well

@danfinlay

This comment has been minimized.

Copy link

danfinlay commented Aug 24, 2016

Since we're talking about eth_sign, I'd like to chime in here that I think it would be way more useful if a method allowed signing/decrypting arbitrary lengthed data, not just hashes, and I've expressed this opinion in an EIP that hasn't gotten much of a response.

@danfinlay danfinlay referenced this pull request Feb 25, 2017

Closed

Fix personal recover #118

danfinlay added a commit to danfinlay/ethjs-schema that referenced this pull request Feb 25, 2017

Add personal_sign and personal_ecRecover support
These methods were added to geth after [this discussion](ethereum/go-ethereum#2940).

The methods and their types are these:

- personal_sign takes an address and an arbitrary hex message to sign, returns a variable-length signed message.
- personal_ecRecover takes the original arbitrary hex message, and the signed output of `personal_sign`, and returns the address that signed it.

Let me know if there's anything I missed.  Looks like maybe there's a build step?
@kumavis

This comment has been minimized.

Copy link
Member

kumavis commented Mar 7, 2017

where are the actual rpc parameters + encoding documented?

@kumavis

This comment has been minimized.

Copy link
Member

kumavis commented Mar 8, 2017

seems here https://github.com/ethereum/go-ethereum/wiki/Management-APIs#personal_sign
please provide public test data for new rpc calls so other clients can match

@kumavis

This comment has been minimized.

Copy link
Member

kumavis commented Mar 8, 2017

for js devs, we captured the behavior here https://github.com/MetaMask/eth-sig-util

@digitaldonkey

This comment has been minimized.

Copy link

digitaldonkey commented Jun 16, 2017

Why the personal_recover is located in personal API?
Why is it not a global function like web3_sha3, because doesn't require an unlocked geth.

@3sGgpQ8H

This comment has been minimized.

Copy link

3sGgpQ8H commented Aug 28, 2017

@bas-vk

the behavior of eth_sign is changed. It now accepts an arbitrary message, prepends a known message, hashes the result using keccak256 it calculates the signature of the hash (breaks backwards compatability!).

This is kind of things you should never ever do. If you want to change behavior of the method in non backward-compatible way, you should just remove old method and add new method with different name.

@buhrmi

This comment has been minimized.

Copy link

buhrmi commented Jul 28, 2018

Does anybody know where the \x19 originally came from?

@digitaldonkey

This comment has been minimized.

Copy link

digitaldonkey commented Jul 30, 2018

^ Interesting question. Maybe @danfinlay knows?

@alexanderbez

This comment has been minimized.

Copy link

alexanderbez commented Jul 30, 2018

I think it's because RLP encoding will never start with \x19?

@PaulRBerg

This comment has been minimized.

Copy link
Contributor

PaulRBerg commented Sep 21, 2018

@alexanderbez No, \x19 could still mean a small integer in RLP-encoding, it's just that is definitely not an RLP-encoded transaction.

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