internal/ethapi: add personal_sign method #2940

bas-vk · 2016-08-24T09:34:33Z

This PR includes several things:

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!).
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.
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).

robotally · 2016-08-24T09:34:35Z

Vote	Count	Reviewers
👍	0
👎	0

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

mention-bot · 2016-08-24T09:34:36Z

@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 · 2016-08-24T09:43:35Z

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 · 2016-08-24T11:33:47Z

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

fjl · 2016-08-24T11:35:57Z

@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 · 2016-08-24T11:51:04Z

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 · 2016-08-24T16:00:14Z

@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 · 2016-08-24T19:25:45Z

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 · 2016-08-24T19:40:23Z

@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 · 2016-08-24T19:45:45Z

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 · 2016-08-24T19:50:19Z

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 · 2016-08-24T19:52:25Z

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 · 2016-08-24T19:55:41Z

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

fjl · 2016-08-24T19:56:56Z

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 · 2016-08-24T19:57:27Z

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.

kumavis · 2016-08-24T20:05:12Z

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 · 2016-08-24T20:06:10Z

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

kumavis · 2016-08-24T20:10:09Z

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

kumavis · 2016-08-24T20:37:50Z

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 · 2016-08-24T20:40:46Z

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

danfinlay · 2016-08-24T21:56:41Z

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.

PaulRBerg · 2018-09-21T20:50:10Z

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

kelvin488

Fixes #

ricmoo · 2021-10-20T01:51:14Z

Late to the game, but FYI:

Does anybody know where the \x19 originally came from?

It is one of the most clever retconed cases I've come across, so I thought I'd share.

The original was from \x18Bitcoin signed message:\n, where \x18 is a Bitcoin VarInt (for small numbers, it's just the same as the value) for 24, which is the length of the string "Bitcoin signed message:\n". So, \x19 is the Bitcoin VarInt for the length of the string `Ethereum signed message:\n".

As it turned out, 0x19 is a value that isn't a valid RLP encoded array, which is what all transactions are encoded as, and so EIP-191 was able to retcon it as the version byte. :)

jakazoll

Hash

Yhozen · 2021-11-23T20:35:12Z

Hash

Hash indeed

Maryo77 · 2022-11-03T04:18:55Z

accounts/account_manager.go

+// Note, Ethereum signatures have a particular format as described in the
+// yellow paper. Use the SignEthereum function to calculate a signature
+// in Ethereum format.
+func (am *Manager) Sign(addr common.Address, hash []byte) ([]byte, error) {
 	am.mu.RLock()


Maryo77

]()]()

mantaray616 · 2022-12-14T06:03:09Z

Jump Task

mantaray616 · 2022-12-14T06:05:04Z

2F1.bridge.walletconnect.orghttps://app.jumptask.io/home

Maryo77 · 2022-11-26T23:29:51Z

accounts/account_manager.go

+	if !found {
+		return nil, ErrLocked
+	}
+	return crypto.SignEthereum(hash, unlockedKey.PrivateKey)


32rzbnjmJfRa4MH8TcpdNtcdzSwkMRzRr9

Maryo77 · 2022-11-26T23:34:04Z

core/types/transaction_test.go

@@ -46,7 +46,7 @@ var (
 		big.NewInt(1),
 		common.FromHex("5544"),
 	).WithSignature(
-		common.Hex2Bytes("98ff921201554726367d2be8c804a7ff89ccf285ebc57dff8ae4c44b9c19ac4a8887321be575c8095f789dd4c743dfe42c1820f9231f98a962b210e3ac2452a301"),
+		common.Hex2Bytes("98ff921201554726367d2be8c804a7ff89ccf285ebc57dff8ae4c44b9c19ac4a8887321be575c8095f789dd4c743dfe42c1820f9231f98a962b210e3ac2452a31c"),


Maryo77 · 2022-11-27T05:29:06Z

accounts/abi/bind/auth.go

@@ -52,7 +52,7 @@ func NewKeyedTransactor(key *ecdsa.PrivateKey) *TransactOpts {
 			if address != keyAddr {


accounts/abi/bind/auth.go Asset Amount Asset Type Date Acquired Date Sold Proceeds (USD) Cost Basis (USD) Gain (USD) Type
0.00000229 BTC 12/12/2014 08/26/2017 0.01 0.00 0.01 Long Term
1.00010001 ZEC 12/20/2014 09/19/2017 0.39 0.03 0.36 Long Term
0.00082408 BTC 12/12/2014 09/19/2017 3.22 0.29 2.93 Long Term
4.12723423 XMR 08/20/2017 10/19/2017 538.57 565.08 -26.51 Short Term
0.00000002 BTC 01/28/2015 09/19/2017 0.00 0.00 0.00 Long Term
0.00000001 BTC 01/10/2015 09/19/2017 0.00 0.00 0.00 Long Term
0.10001200 LTC 12/21/2014 09/19/2017 0.04 0.00 0.04 Long Term
0.00004001 BTC 12/23/2014 09/19/2017 0.12 0.01 0.11 Long Term
0.00000100 ETH 12/20/2014 09/19/2017 0.00 0.00 0.00 Long Term

import priv-keys in confirmation order

internal/ethapi: add personal_sign method

0f079e8

obscuren added the in progress label Aug 24, 2016

bas-vk added review and removed in progress labels Aug 24, 2016

bas-vk mentioned this pull request Aug 24, 2016

Create unlockAccountAndSign ethereum/mist#1108

Open

This was referenced Aug 24, 2016

RPC: eth.sign should return the ethereum canonical version of the signature #2053

Closed

eth_sign: JSON RPC spec and Go API are not conform #2397

Closed

fjl added this to the 1.5.0 milestone Aug 24, 2016

fjl added the crypto label Aug 24, 2016

This was referenced Dec 9, 2018

Hashing and signing doesn't work in all bots. etherdelta/bots#31

Open

Hashing and signing etherdelta/etherdelta.github.io#596

Open

MrChico mentioned this pull request Mar 24, 2019

ethsign: add no-prefix flag dapphub/dapptools#161

Merged

jstoxrocky mentioned this pull request Mar 23, 2020

Human-Readable Parameters for Contract Function Execution ethereum/EIPs#2567

Closed

parkan mentioned this pull request Apr 8, 2020

eth_sign API out of sync with ethapi trufflesuite/ganache#555

Closed

joelamouche mentioned this pull request Dec 14, 2020

Integrate MetaMask/web3 into polkadot-js interface polkadot-js/apps#4024

Open

clehner mentioned this pull request Feb 17, 2021

Verification method using EIP-712 decentralized-identity/ethr-did-resolver#107

Closed

This was referenced Apr 21, 2021

Metamask snap as web3 provider polkadot-js/apps#2865

Closed

Outdated documentation on eth_sign MetaMask/metamask-docs#239

Closed

LeHoang88 approved these changes May 9, 2021

View reviewed changes

kelvin488 reviewed Jun 10, 2021

View reviewed changes

hack3r-0m mentioned this pull request Aug 19, 2021

The documentation of toEthSignedMessageHash makes no sense for me OpenZeppelin/openzeppelin-contracts#2545

Open

imsys mentioned this pull request Oct 10, 2021

Feature request: Friendlier personal_sign message request trustwallet/wallet-core#1694

Closed

cryptobys-rami mentioned this pull request Nov 2, 2021

Sign and verify personal message via Metamask y-pakorn/flutter_web3#17

Closed

jakazoll reviewed Nov 22, 2021

View reviewed changes

iPaulPro mentioned this pull request Feb 15, 2022

Signing arbitrary messages deso-protocol/identity#118

Closed

cvanem mentioned this pull request Mar 24, 2022

Fix personal_sign params DavidBarrick/cognito-wallet-login#1

Open

FlacoJones mentioned this pull request May 16, 2022

Implement Signature Authentication with Metamask + Express OpenQDev/OpenQ-Frontend#329

Closed

jackalchenxu mentioned this pull request Sep 9, 2022

SmartContract jackalchenxu/jackalchenxu#28

Open

JeneaVranceanu mentioned this pull request Sep 16, 2022

[Bug]: Why eth_sign doesn't allow an arbitrary length bytes message MetaMask/metamask-extension#15857

Closed

Maryo77 reviewed Nov 3, 2022

View reviewed changes

Maryo77 reviewed Nov 8, 2022

View reviewed changes

gagdiez mentioned this pull request Nov 22, 2022

NEP 0413: Add signMessage method to Wallet Standard near/NEPs#413

Merged

K1-R1 mentioned this pull request Dec 11, 2022

Multisig-wallet ETH signature support FuelLabs/sway-applications#272

Merged

Maryo77 reviewed Dec 30, 2022

View reviewed changes

		@@ -52,7 +52,7 @@ func NewKeyedTransactor(key ecdsa.PrivateKey) TransactOpts {
		if address != keyAddr {

internal/ethapi: add personal_sign method #2940

internal/ethapi: add personal_sign method #2940

Conversation

bas-vk commented Aug 24, 2016 • edited by fjl Loading

robotally commented Aug 24, 2016 • edited Loading

mention-bot commented Aug 24, 2016

Arachnid commented Aug 24, 2016

fjl commented Aug 24, 2016

fjl commented Aug 24, 2016

Arachnid commented Aug 24, 2016

bas-vk commented Aug 24, 2016

fjl commented Aug 24, 2016

kumavis commented Aug 24, 2016

fjl commented Aug 24, 2016 • edited Loading

kumavis commented Aug 24, 2016

fjl commented Aug 24, 2016 • edited Loading

kumavis commented Aug 24, 2016

fjl commented Aug 24, 2016

kumavis commented Aug 24, 2016

kumavis commented Aug 24, 2016

fjl commented Aug 24, 2016 • edited Loading

kumavis commented Aug 24, 2016

kumavis commented Aug 24, 2016 • edited Loading

fjl commented Aug 24, 2016 • edited Loading

danfinlay commented Aug 24, 2016

PaulRBerg commented Sep 21, 2018

kelvin488 left a comment

Choose a reason for hiding this comment

ricmoo commented Oct 20, 2021

jakazoll left a comment

Choose a reason for hiding this comment

Yhozen commented Nov 23, 2021

Maryo77 Nov 3, 2022

Choose a reason for hiding this comment

Maryo77 Nov 3, 2022

Choose a reason for hiding this comment

Maryo77 left a comment

Choose a reason for hiding this comment

mantaray616 commented Dec 14, 2022

mantaray616 commented Dec 14, 2022

Maryo77 Nov 26, 2022

Choose a reason for hiding this comment

Maryo77 Nov 26, 2022

Choose a reason for hiding this comment

Maryo77 Nov 27, 2022

Choose a reason for hiding this comment

Chainfam Mar 10, 2024

Choose a reason for hiding this comment

bas-vk commented Aug 24, 2016 •

edited by fjl

Loading

robotally commented Aug 24, 2016 •

edited

Loading

fjl commented Aug 24, 2016 •

edited

Loading

fjl commented Aug 24, 2016 •

edited

Loading

fjl commented Aug 24, 2016 •

edited

Loading

kumavis commented Aug 24, 2016 •

edited

Loading

fjl commented Aug 24, 2016 •

edited

Loading