eth_sign API out of sync with ethapi

[parkan]

As of ethereum/go-ethereum#2940, eth_sign api has changed. It is now eth_sign(address, msg), accepting arbitrary input and putting it through keccak256.

Expected Behavior

Upstream eth_api introduced a breaking change where eth_sign now accepts the actual message, rather than a 32 byte hash digest. Signature is now eth_sign(address, msg)

Current Behavior

testrpc currently emulates existing eth_sign(address, sha3(msg)) behavior

Possible Solution

Change ethereumjs-util.ecsign to call keccak256 before calling secp256k1.sign

Context

We need to sign statements with web3, which is now incompatible between testrpc and real ethapi.

Your Environment

• Version used: v3.0.3
• Environment name and version (e.g. PHP 5.4 on nginx 1.9.1): n/a
• Server type and version:
• Operating System and version: os x
• Link to your project: https://github.com/mediachain/aleph-ethereum

[yusefnapora]

I believe this may also need to prefix msg with Buffer.from('\u0019Ethereum Signed Message:\n' + msg.length.toString()) before running through keccak256, to be compatible with the eth_api implementation, assuming msg is a Buffer.

[parkan]

@yusefnapora good point, I mostly copied over the impl notes from the Parity issue, which omitted that

[fumecow]

I made a diagram of the current differences between testrpc, geth and parity with respect to eth_sign behavior and on-chain ecrecover.

[tcoulter]

Wow, that's a great diagram. If you have it handy, could you give me some messages, a test private key / mnemonic and the signed results of geth/parity for testing?

[fumecow]

Unfortunately accounts differ when testing on the different nodes so an apples to apples comparison isn't really possible. I have some test script output that gives some data but missing is the private key for the account:

Using node=EthereumJS TestRPC
Using account: 0x264821d83625ed0cb177bb01a486e304cdece092
Using contract: 0x59e4afee0cc669cfbcefd894429e27d2a839c45d
Generating signature
address=0x264821d83625ed0cb177bb01a486e304cdece092
encoded message=0x067f5bca343172b01203e79581e024f62046b4a935afc81bd1006704a22fbb33
signature=0xfe34b8ea0483c74e5099c275e381f6ded244436af9fef41a6ba36a8c93901ae91faabe7269e459433b3794ca4ea24b268e915d566d78bbf41f95da024d17f24101
Verifying signature
address=0x264821d83625ed0cb177bb01a486e304cdece092
encoded message=0x067f5bca343172b01203e79581e024f62046b4a935afc81bd1006704a22fbb33
r: 0xfe34b8ea0483c74e5099c275e381f6ded244436af9fef41a6ba36a8c93901ae9
s: 0x1faabe7269e459433b3794ca4ea24b268e915d566d78bbf41f95da024d17f241
v: 0x01
Recovered address: 0x264821d83625ed0cb177bb01a486e304cdece092
Address matched: true

[fumecow]

Here's the script that generated the above output (it will run correctly on geth, parity or testrpc because it checks the client and makes adjustments to the input data accordingly)

Hope that helps!

var Web3 = require('web3');
var web3 = new Web3();

web3.setProvider(new web3.providers.HttpProvider('http://localhost:8545'));

var node = web3.version.node.split('/')[0];
console.log("Using node=" + node);
var testrpc = false;
var geth = false;
var parity = false;
if (node == "Geth") geth = true;
if (node == "EthereumJS TestRPC") testrpc = true;
if (node == "Parity") parity = true;

var useAccount = web3.eth.accounts[0];
var useContract = '0x59e4afee0cc669cfbcefd894429e27d2a839c45d';

/* sample contract source
pragma solidity ^0.4.4;

contract SignAndVerifyExample {
   	function RecoverAddress(bytes32 msgHash, uint8 v, bytes32 r, bytes32 s) constant returns (address) {
       	if (v < 27) v += 27;
       	if (v != 27 && v != 28) throw;
       	return ecrecover(msgHash, v, r, s);
   	}
}
*/
var contractABI = [{"constant":true,"inputs":[{"name":"msgHash","type":"bytes32"},{"name":"v","type":"uint8"},{"name":"r","type":"bytes32"},{"name":"s","type":"bytes32"}],"name":"RecoverAddress","outputs":[{"name":"","type":"address"}],"payable":false,"type":"function"}];
var verifyContract = web3.eth.contract(contractABI).at(useContract);

var message = 'Lorem ipsum mark mark dolor sit amet, consectetur adipiscing elit. Tubulum fuisse, qua illum, cuius is condemnatus est rogatione, P. Eaedem res maneant alio modo.';

console.log("Using account: " + useAccount);
console.log("Using contract: " + useContract);

function generateSignature(address, message, cb) {
	console.log("Generating signature");
	console.log("  address=" + address);
	if (testrpc) {
		var encoded = web3.sha3(message);
	}
	if (geth || parity) {
		encoded = '0x' + Buffer.from(message).toString('hex');
	}
	console.log("  encoded message=" + encoded);
	web3.eth.sign(address, encoded, cb);
}

function verifySignature(address, message, sig, cb) {
	console.log("Verifying signature");
	console.log("  address=" + address);
	if (testrpc) {
		var encoded = web3.sha3(message);
	}
	if (geth || parity) {
		encoded = web3.sha3('\x19Ethereum Signed Message:\n' + message.length + message);
	}
	console.log("  encoded message=" + encoded);
	if (sig.slice(0,2) == '0x') sig = sig.substr(2);
	if (testrpc || geth) {
		var r = '0x' + sig.substr(0, 64);
		var s = '0x' + sig.substr(64, 64);
		var v = '0x' + sig.substr(128, 2);
	}
	if (parity) {
		v = '0x' + sig.substr(0, 2);
		r = '0x' + sig.substr(2, 64);
		s = '0x' + sig.substr(66, 64);
	}
	console.log("  r: " + r);
	console.log("  s: " + s);
	console.log("  v: " + v);
	verifyContract.RecoverAddress(encoded, v, r, s, cb);
}

generateSignature(useAccount, message, function(err, sig) {
	if (err) {
		console.error("Could not sign message: " + err);
	} else {
		console.log("  signature=" + sig);
		verifySignature(useAccount, message, sig, function(err, res) {
			if (!err) {
				console.log('Recovered address:', res);
				console.log('  Address matched:', useAccount === res);
			} else {
				console.error('Could not recover address:', err);
			}
		});
	}
});

[fumecow]

Note: you'll need to change the 'useContract' variable to point to the deployed contract once it's deployed... the contract source code is in the comment.

[fumecow]

Here's a proposed fix I posted to the Parity team:

I'm not particularly vested in one versus the other. The documentation on the ethereum wiki for Web3 gives a format for the signature byte order so I would personally lean toward that format. However, v should probably be returned as 27 or 28 and not 00 or 01 as it states in the documentation.

https://github.com/ethereum/wiki/wiki/JavaScript-API#web3ethsign

I'd be inclined to change the byte ordering for Parity to match TestRPC and Geth, fix TestRPC to accept the hex encoded messages and prepend "Ethereum Signed Message" as the other clients do. Finally, TestRPC should return the correct value for v (27 or 28).

openethereum/parity-ethereum#5490 (comment)

[Arachnid]

This just stung me today. provider-framework accepts raw data and hashes it itself (without a prefix), matching the documentation at https://github.com/ethereum/wiki/wiki/JavaScript-API#web3ethsign, but testrpc expects you to hash the input yourself.

I believe that provider-framework's behaviour is correct; a prefix string should not be used except on the personal.sign API.

[fumecow]

Confirmed, the behavior has changed between 3.0 and 4.x. I've been out of the loop recently so not sure what the differences are between TestRPC versions or what the correct behavior has been decided to be.

…

On Wed, Jul 12, 2017 at 12:39 AM, Chris Hitchcott ***@***.***> wrote: Has the behaviour changed between ***@***.*** and 4.x ?? ECVerify tests failing on 4.x but passing on 3. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <https://github.com/ethereumjs/testrpc/issues/243#issuecomment-314669025>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AaCIsS-42RODZklxre_-WeGTg9RZOWomks5sNGoSgaJpZM4LobfT> .

[tcoulter]

I'm looking into this. Either I merged a PR and didn't document it, or a dependency updated that changed behavior.

The 0x project noticed this as well. It looks like this comment could be correct:

// Parity and TestRpc nodes add the personalMessage prefix itself

[Arachnid]

Since I posted, testrpc appears to have been modified to apply the personal prefix and hash its input. The following test passes:

  it("should sign", function() {
    return TestContract.deployed().then(function(tc) {
      var sigdata = web3.eth.sign(accounts[0], '');
      return tc.recover(web3.sha3('\u0019Ethereum Signed Message:\n0'), parseInt(sigdata.slice(130, 132), 16) + 27, sigdata.slice(0, 66), "0x" + sigdata.slice(66, 130)).then(function(addr) {
        assert.equal(accounts[0], addr);
      });
    });
  });

Where tc.recover simply calls the ecrecover builtin.

This is problematic first because it breaks compatibility, but more importantly because only personal_sign should apply the prefix; eth_sign should just take the raw input argument, if it's supported at all.

String concatenation in Solidity is difficult at the best of times (and with the added issue of converting an integer to a string for the length, even harder if you want to verify variable-length signatures), so this especially unusable if you want to verify signatures onchain.

[benjamincburns]

@Arachnid I assume you'd be happy if TestRPC behaved identically to either geth or parity per the diagram submitted above?

[pkieltyka]

the change to the behaviour of eth_sign in geth to mimic personal_sign without a password makes sense, except it should have never happened because its caused a terrible mess between different implementations across geth, parity, testrpc, metamask and web3. what a mess.

the only solution is for all clients/nodes to update to what is the current spec, the new eth_sign semantics as per geth.

what about also supporting personal_sign in ganache-core?

[rhlsthrm]

I have spent hours debugging issues related to signing and recovering signatures due to the fact that Metamask's signature comes out differently than truffle's client. Is there any way for this to be reconciled? How can you set up your contract functions to use ecrecover to take care of both cases? Does anyone have an example?

[fumecow]

@rhlsthrm the script above used to work on geth, parity, testrpc (truffle's old client). it's a starting point but it's almost a year old and it was never tested against MetaMask / Infura.

[rhlsthrm]

@fumecow the problem is I want the contract to work regardless of which client signs the message. When I am in my test environment I am using ganache which includes the prefix, but Metamask does not. This is really bad for me, since I have to use two versions of the contract in each test environment.

[fumecow]

without knowing the details, i think you'll have to make adjustments in the calling code based on the node you're running on, like the script above does. note that the contract then works with all the variations because it adjusts the 'v' parameter internally. other than that, i don't think there's a better solution until the signing is standardized between clients.

[dryruner]

So would metamask's web3.eth.sign(msg) prepend the string "\x19Ethereum Signed Message:\n${msg.length}" before signing?

Now we have metamask, testrpc, geth, parity, mew, and what else?

[rhlsthrm]

Not to mention now we are supposed to use personal_sign or even eth_signTypedData instead of eth_sign (see this).

Does ganache plan to support these new features anytime soon?

[benjamincburns]

I recently accepted a PR for eth_signTypedData which leveraged MetaMask's signing module for its implementation. I think this also includes an implementation for personal_sign.

When we work this issue we should just leverage that library and call it a day.

[nachomazzara]

Any news?

[mikeseese]

Unfortunately @nachomazzara, there isn't any news on this one. We currently only have one developer working these high-priority issues. We're doing much better now (in the last 23 days) with tracking issues, triaging them, and working them. The other dev (me) is working on some other
Ganache UI changes. Good news is we're getting another dev today so we should hopefully be addressing these high priority issues faster.

If you want to know if we're working it, just check out the Assigned field, if it's not assigned yet, then it's still in the backlog.

Sorry I couldn't give you better news! Sorry it's taken us so long to get to this too; we're trying to catch up!!

[eggplantzzz]

Looking at this issue, it seems like it has been updated. Can anyone here confirm that this is indeed updated and behaving correctly?

[tcoulter]

Closing this because after review, we think it's no longer an issue. Please open a new ticket if you believe this to still be a problem.