eth_sign: JSON RPC spec and Go API are not conform

[Georgi87]

The JSON-RPC interface describes that the data returned from the eth_sign call is the signed data while the go implementation signs a hash of the data:

https://github.com/ethereum/wiki/wiki/JSON-RPC#eth_sign

Go:
https://github.com/ethereum/go-ethereum/blob/develop/eth/api.go#L1108

func (s *PublicTransactionPoolAPI) Sign(address common.Address, data string) (string, error) {
    signature, error := s.am.Sign(accounts.Account{Address: address}, common.HexToHash(data).Bytes())
    return common.ToHex(signature), error
}

[obscuren]

Thank you for reporting this. There are at the moment two issues:

• The specification is wrong (you should always sign hashes, not data)
• The implementation's function definition is wrong; data string should be common.Hash

[obscuren]

Implementation fixed. See linked PR. Will address the spec soon.

[Georgi87]

Thank you!

[kumavis]

its important that the eth_sign request passes in the message and not the hash, so that the id mgmt (browser/wallet) can show the message to the user for approval before signing. I agree that ( internally ) the id mgmt should hash the message then sign it, but that should not affect the parameters in eth_sign method

the danger of passing in just a hash is that it could be the hash of a tx that the user is not aware the are signing for

I think the specification was designed with this in mind, and @obscuren's assumption that it implied that the data should directly be signed ( instead of the hash of the data ) was incorrect

[fjl]

Feeding the message into the signing function without hashing allows extraction of the private key.
This issue is not fixed. I don't know why we changed it like this, but the go-ethereum eth_sign endpoint does not hash the message, but forces the message to be 32 bytes long, assuming that it is a hash of some kind. From a security POV this doesn't achieve anything because users of the RPC interface are still 100% in control of the input to the signing function.

[fjl]

A better fix would be to deprecate eth_sign and create a new method, perhaps personal_sign, that hashes the passed message prior to signing it.

[Georgi87]

I would argue that not sending hashes but clear text to a (remote) rpc-node would also raise privacy concerns. Maybe eth_sign could sign a sent hash together with a random salt and return signature and salt?

What if a user wants to sign a gigabyte large file? I would not send this to the RPC.

[fjl]

personal_sign is being added in #2940.

[kumavis]

I would argue that not sending hashes but clear text to a (remote) rpc-node would also raise privacy concerns.

@Georgi87 im not sure in what case you would be doing this, unless that remote node held your private key, in which case you've got nothing to hide from it.

[Georgi87]

Agreed @kumavis