eth: add new RPC method (personal.) SignAndSendTransaction #2564

bas-vk · 2016-05-12T17:46:30Z

This PR introduces a new RPC method PrivateAccountAPI.SignAndSendTransaction that is exposed in the personal namespace. The method accepts a transaction and password as arguments. This allows external applications to submit a transaction without the need to first unlock an account before submitting a transaction which can give an adversary a small time window to submit a fraudulent transaction or sign data.

It also fixes several style issues in eth/api.go.

PTAL @obscuren @fjl

robotally · 2016-05-12T17:46:31Z

Vote	Count	Reviewers
👍	1	@karalabe
👎	0

Reaction	Users
:08:	@pat-kim

Updated: Wed May 25 11:39:37 UTC 2016

codecov-io · 2016-05-12T17:53:44Z

Current coverage is 55.76%

Merging #2564 into develop will decrease coverage by 0.44%

@@            develop      #2564   diff @@
==========================================
  Files           215        215          
  Lines         24471      24243   -228   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
- Hits          13757      13519   -238   
- Misses        10712      10723    +11   
+ Partials          2          1     -1

2 files (not in diff) in p2p were modified. more
- Misses +1
- Hits -1
4 files (not in diff) in eth/downloader were modified. more
- Misses -14
- Hits -233
4 files (not in diff) in eth were modified. more
- Misses -2
- Hits -3
2 files (not in diff) in accounts were modified. more
- Misses -14
- Hits -1
File core/blockchain.go (not in diff) was modified. more
- Misses +6
- Partials -2
- Hits -4
File eth/api.go was modified. more
- Misses +35
- Partials 0
- Hits 0
File ...saction_test_util.go (not in diff) was modified. more
- Misses 0
- Partials +1
- Hits -1
File node/node.go (not in diff) was modified. more
- Misses 0
- Partials 0
- Hits +1
File event/event.go (not in diff) was modified. more
- Misses 0
- Partials 0
- Hits -1
File cmd/geth/js.go (not in diff) was modified. more
- Misses +2
- Partials 0
- Hits 0

Powered by Codecov. Last updated by 847aaff...1866558

karalabe · 2016-05-13T08:32:01Z

accounts/account_manager.go

+	_, key, err := am.getDecryptedKey(Account{Address: addr}, keyAuth)
+	if err != nil {
+		return nil, err
+	}


Please add a defer zeroKey(key.PrivateKey) here to make sure the key doesn't remain in main memory.

kumavis · 2016-05-19T18:58:17Z

introduces new security concerns -- seems like brute forcing the password becomes an attack vector -- maybe could add some mechanism for tar-pitting subsequent failures from the same ip address?

bas-vk · 2016-05-20T09:04:17Z

This is already possible with personal_unlockAccount. It's one of the reasons why this method is added to the personal namespace which is by default not exposed over the http and websocket interface.

I don't feel much for keeping track of failed unlock attempts from ip addressen tbh. It complicates the rpc implementation and to make such an attack possible it requires the user to:

enable the personal namespace over http/ws
start http/ws interface on an address that is accessible from outside

If a users enables both options than the user probably has good reasons and is aware of the security implications. And yes, this might not always be the case. Most modern firewalls have protection for brute force attacks and have support for content scanning. I think that is a better place to guard against these kind of brute force attacks.

karalabe · 2016-05-20T09:36:25Z

Also don't forget that we're using a very expensive scrypt encryption on the password. Unlocking an account can actually take 1-2 seconds, so brute forcing isn't really feasible since the machine being attacked won't be able to process the unlock requests/attempts anyway.

karalabe · 2016-05-20T10:57:27Z

accounts/account_manager.go

@@ -147,9 +147,21 @@ func (am *Manager) Sign(addr common.Address, hash []byte) (signature []byte, err
 	return crypto.Sign(hash, unlockedKey.PrivateKey)
 }

+// SignHash signs hash if the private key matching the given address can be


SignHash -> SignWithPassphrase

karalabe · 2016-05-20T11:19:23Z

Please rebase, code LGTM apart from a few documentation issues and minor code style things. Plus one question not related to the PR, but it'd be nice to have it answered.

karalabe · 2016-05-20T12:11:19Z

eth/api.go

+
+	var tx *types.Transaction
+	contractCreation := (args.To == nil)
+	if contractCreation {


Ah sorry, I didn't see this one before, could you also please get rid of this variable? :D

I should have seen this myself, was rushing to push it before standup.
Fixed.

karalabe · 2016-05-20T12:12:15Z

LGTM 👍 (if you feel like fixing the code style I just opened, great, if not, it's fine like this too)

pat-kim · 2016-05-20T14:49:23Z

Dev I wonder why still you guys impose the issue only RPC? which attack from outside.
The most important thing is IPC socket is being affected as well as RPC.

I believe you guys know what is the fundamental issue since mist coded with the unlock timeout mechanism and not easy to convert a logic. You guys took immediately action to get patch with this new method in order to renewal code of mist.

Even if you guys are in a decentralized organization you guys get investment from users at least for users right should state what's the real fact not only describing it as not a big deal..

See the below video. How it can be simply implemented with 2 lines of batch script.

https://www.youtube.com/watch?v=PNSwFy__m-8

karalabe · 2016-05-20T15:19:39Z

@pat-kim Your attack assumes that the machine is already compromised, otherwise an attacker would not have access to the IPC interface. If the machine is already compromised, and furthermore the attacker has access to the user's private files (in order to be able to send commands to the IPC socket), then he might as well replace Geth/Mist with a custom binary that does whatever he wants.

karalabe · 2016-05-20T15:21:22Z

PS: RPC is the protocol, not the transport. Our RPC APIs run on all the IPC, HTTP and WebSocket endpoints.

pat-kim · 2016-05-20T15:35:39Z

@karalabe apparently this method is making after I reported the issue below. Like you said if that is nothing matter unless user PC is compromised. Again what's importance of this patching ? Because you guys usually said RPC open is always users problem. Then why care about the RPC?

ethereum/mist#611 (comment)

karalabe · 2016-05-20T15:43:17Z

People are conflating RPC with HTTP.

What you're talking about is having the HTTP and/or WebSocket endpoints open publicly for the internet. That is something we never recommend (nor have ever recommended), quite on the contrary.

RPC is a protocol on top of either HTTP, or WebSockets, or IPC. This PR makes fixes to the RPC, i.e. to the Geth APIs, that is available on all above mentioned transports. This isn't meant to patch any HTTP vulnerability. We still state that a node should never open an HTTP endpoint for the internet to abuse.

pat-kim · 2016-05-20T16:04:57Z

@karalabe from user perspective nobody aware mist has 2 second unlock window.
Actually the following code is for the transaction for now. Mist itself does not decrypt a password
But just forwarding a password in clear text (also significant issue) with "personal.unlockAccount" for 2 seconds.

mist code
web3.personal.unlockAccount(Session.get('data').from, pw || '', 2, function(e, res){

Geth debug
I0513 21:08:07.428322 28478 mergedapi.go:61] personal_unlockAccount ["0x2134e2a2b718dca96580ecac31e653ea1fdd58da","xxxxxxxx%",2]

This is security is flaw. So the method "PrivateAccountAPI.SignAndSendTransaction" is created to
eliminated 2 seconds unlock issue.

What I am asking is nowhere it stated that 2 second is vulnerable for IPC attack however it's been exposed when I claimed and reported it's attack vector what I have affected.

But you guys still imposing it as users's fault nor program's issue but in fact the patch is ongoing.

karalabe · 2016-05-20T16:14:28Z

And we still maintain that the cause of theft was an improper use of the program, as it was stated everywhere that opening up access to your node to the internet is an extremely bad idea waiting for abuse.

We are not patching the software to plug up any security holes (it works as intended), rather we are extending it feature wise to ensure that someone not careful enough doesn't get bitten again so easily. It's just an extra security precaution to protect the users.

pat-kim · 2016-05-20T17:08:55Z

@karalabe security is first thing you guys have to concern in order to build a program .....Let me know is there any kind of program commenting with.....

for someone
" it feature wise to ensure that someone not careful enough doesn't get bitten again so easily"

And I am sad I was the someone not protected users.

All I wanted to let you know is that hope you guys consider everybody as protected users from now on please consider that someone as well in your protection.

Like you said "we are not patching the software to plug up any security holes"

From Mist's gitter channel:

subtly @subtly 21:02
@GriffGreen looks sane. the 'account unlock' mechanism used by Mist needs to be replaced with something more secure.

Ramesh Nair @hiddentao 21:44
I can confirm that a more secure replacement - personal.signAndSendTransaction is on its way. Geth will soon have it, we just need to add support in Mist. See #2564

obscuren · 2016-05-23T11:14:12Z

👍

trustfarm-dev · 2016-05-25T11:39:37Z

I can't trust anymore. In case of SW engineering there's always side-effect, what DEV didn't expected.
I'll wait for long time, until field verification.
Bulky, work-around.

…reum#2564)

obscuren added the in progress label May 12, 2016

bas-vk added review and removed in progress labels May 12, 2016

obscuren added this to the 1.4.5 milestone May 12, 2016

bas-vk force-pushed the submit-tx branch from 1866558 to 8631a66 Compare May 13, 2016 05:48

karalabe reviewed May 13, 2016
View reviewed changes

bas-vk force-pushed the submit-tx branch 2 times, most recently from e97a848 to 42d642b Compare May 16, 2016 08:14

tomusdrw mentioned this pull request May 18, 2016

Draft dapp html transaction authorization without CORS ethereum/EIPs#107

Merged

bas-vk mentioned this pull request May 19, 2016

Unlock mechanism flaw mist opens 2 second unlock timout window. #2584

Closed

tomusdrw mentioned this pull request May 19, 2016

signAndSendTransaction RPC API openethereum/parity-ethereum#1108

Closed

karalabe reviewed May 20, 2016
View reviewed changes

bas-vk force-pushed the submit-tx branch from 42d642b to 6b7ecf3 Compare May 20, 2016 11:55

karalabe reviewed May 20, 2016
View reviewed changes

eth: add new RPC method (personal.) SignAndSendTransaction

64a6c2c

bas-vk force-pushed the submit-tx branch from 6b7ecf3 to 64a6c2c Compare May 20, 2016 13:55

obscuren added the merge imminent label May 23, 2016

obscuren merged commit 7f515b0 into ethereum:develop May 23, 2016

obscuren removed the review label May 23, 2016

bas-vk deleted the submit-tx branch May 24, 2016 07:48

adamschmideg added area:rpc and removed area:rpc labels Dec 14, 2018

maoueh pushed a commit to streamingfast/go-ethereum that referenced this pull request Aug 1, 2024

fix: the bug of blobsidecars and downloader with multi-database (ethe…

87e622e

…reum#2564)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

eth: add new RPC method (personal.) SignAndSendTransaction #2564

eth: add new RPC method (personal.) SignAndSendTransaction #2564

bas-vk commented May 12, 2016

robotally commented May 12, 2016 •

edited

Loading

codecov-io commented May 12, 2016 •

edited

Loading

karalabe May 13, 2016

kumavis commented May 19, 2016

bas-vk commented May 20, 2016

karalabe commented May 20, 2016 •

edited

Loading

karalabe May 20, 2016

karalabe commented May 20, 2016

karalabe May 20, 2016

bas-vk May 20, 2016

karalabe commented May 20, 2016

pat-kim commented May 20, 2016 •

edited

Loading

karalabe commented May 20, 2016 •

edited

Loading

karalabe commented May 20, 2016

pat-kim commented May 20, 2016

karalabe commented May 20, 2016

pat-kim commented May 20, 2016

karalabe commented May 20, 2016

pat-kim commented May 20, 2016 •

edited

Loading

obscuren commented May 23, 2016

trustfarm-dev commented May 25, 2016

eth: add new RPC method (personal.) SignAndSendTransaction #2564

eth: add new RPC method (personal.) SignAndSendTransaction #2564

Conversation

bas-vk commented May 12, 2016

robotally commented May 12, 2016 • edited Loading

codecov-io commented May 12, 2016 • edited Loading

Current coverage is 55.76%

karalabe May 13, 2016

Choose a reason for hiding this comment

kumavis commented May 19, 2016

bas-vk commented May 20, 2016

karalabe commented May 20, 2016 • edited Loading

karalabe May 20, 2016

Choose a reason for hiding this comment

karalabe commented May 20, 2016

karalabe May 20, 2016

Choose a reason for hiding this comment

bas-vk May 20, 2016

Choose a reason for hiding this comment

karalabe commented May 20, 2016

pat-kim commented May 20, 2016 • edited Loading

karalabe commented May 20, 2016 • edited Loading

karalabe commented May 20, 2016

pat-kim commented May 20, 2016

karalabe commented May 20, 2016

pat-kim commented May 20, 2016

karalabe commented May 20, 2016

pat-kim commented May 20, 2016 • edited Loading

obscuren commented May 23, 2016

trustfarm-dev commented May 25, 2016

robotally commented May 12, 2016 •

edited

Loading

codecov-io commented May 12, 2016 •

edited

Loading

karalabe commented May 20, 2016 •

edited

Loading

pat-kim commented May 20, 2016 •

edited

Loading

karalabe commented May 20, 2016 •

edited

Loading

pat-kim commented May 20, 2016 •

edited

Loading