Skip to content
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.

Sign up for GitHub

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

Jump to bottom

cmd/clef, signer: security fixes #17554

Merged
merged 13 commits into from
Sep 25, 2018
Merged

cmd/clef, signer: security fixes #17554

merged 13 commits into from
Sep 25, 2018

Conversation

holiman
Copy link
Contributor

@holiman holiman commented Aug 30, 2018 •
edited
Loading

Replaces #17427 .

  • Remove local path info when communicating externally (still available in internal channel)
  • Adds Origin and User-Agent to the context, and makes it visible for Clef
  • Audit fix: document that external fields are set by remote caller
  • Audit fix: harden storage kv-store, so values cannot be swapped around
  • Audit fix: By default reject on tx validation errors, have --advanced mode to enable warnings instead of rejections
  • Audit fix: Implement password complexity check on new accounts
  • Audit fix: remove import functionality from external interface

@holiman holiman requested a review from karalabe as a code owner August 30, 2018 12:14
@holiman holiman changed the title Clefchanges 2 Clef: Remove local path info from external API Aug 30, 2018
karalabe
karalabe reviewed Aug 30, 2018
View reviewed changes
signer/core/cliui.go Outdated Show resolved Hide resolved
@holiman
Copy link
Contributor Author

holiman commented Aug 31, 2018

I've now modified it a bit. The cli ui, when performing a listing, shows this (full info):

------- Signer info -------
* extapi_http : n/a
* extapi_ipc : /home/user/.clef/clef.ipc
* extapi_version : 3.0.0
* intapi_version : 2.0.0
-------- List Account request--------------
A request has been made to list all accounts. 
You can select which accounts the caller can see
  [x] 0x8A8eAFb1cf62BfBeb1741769DAE1a9dd47996192
    URL: keystore:///home/user/.ethereum/keystore/UTC--2018-05-18T18-24-52.475027848Z--8a8eafb1cf62bfbeb1741769dae1a9dd47996192
    Type: Account
  [x] 0x64f951c2C1B289638b5c7FB6eb6657ee359Dd984
    URL: keystore:///home/user/.ethereum/keystore/UTC--2018-08-17T08-48-34.811873860Z--64f951c2c1b289638b5c7fb6eb6657ee359dd984
    Type: Account

I also added User-Agent and Origin to the ctx provided by http.Server.
Making a request:

[user@work clef]$ curl -H "Content-Type: application/json" -X POST --data-binary @signtx.json http://localhost:8550 -i

This is how it looks:

--------- Transaction request-------------
to:    0x07a565b7ed7d7a678680a4c162885bedbb695fe0

WARNING: Invalid checksum on to-address!

from:     0xcf6cd422f9b45778ad8a564edfbf8abab96e363a [chksum INVALID]
value:    16 wei
gas:      0x333 (819)
gasprice: 291 wei
nonce:    0x0 (0)
data:     0x4401a6e40000000000000000000000000000000000000000000000000000000000000012

Transaction validation:
  * WARNING : Invalid checksum on to-address
  * Info : safeSend(address: 0x0000000000000000000000000000000000000012)


Request context:
	127.0.0.1:44728 -> HTTP/1.1 -> localhost:8550

	User-Agent: curl/7.53.1
	Origin: 
-------------------------------------------
Approve? [y/N]:

@holiman holiman changed the title Clef: Remove local path info from external API Clef: Remove local path info, implement UA + Origin Aug 31, 2018
@holiman holiman changed the title Clef: Remove local path info, implement UA + Origin Clef security fixes Sep 19, 2018
@chikeichan chikeichan mentioned this pull request Sep 21, 2018
Closed
@fjl
Copy link
Contributor

fjl commented Sep 25, 2018

This looks good, just a small request: Please improve the error message for invalid characters in password. It shouldn't list all possible characters.

ligi
ligi reviewed Sep 25, 2018
View reviewed changes
signer/storage/aes_gcm_storage.go Outdated
@@ -129,7 +129,10 @@ func (s *AESEncryptedStorage) writeEncryptedStorage(creds map[string]storedCrede
return nil
}

func encrypt(key []byte, plaintext []byte) ([]byte, []byte, error) {
// encrypt encrypts plaingtext with the given key, with additionaldata
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is one g too much in plaintext and one space missing between additional and data

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

@holiman
Copy link
Contributor Author

holiman commented Sep 25, 2018

Fixed!

@fjl
Copy link
Contributor

fjl commented Sep 25, 2018

Sorry, I merged a typo fix PR that made this one unmergable.

@holiman
Copy link
Contributor Author

holiman commented Sep 25, 2018

rebased

@fjl fjl changed the title Clef security fixes cmd/clef, signer: security fixes Sep 25, 2018
@holiman holiman merged commit d3441eb into ethereum:master Sep 25, 2018
cryptomental pushed a commit to cryptomental/go-ethereum that referenced this pull request Jan 9, 2019
@holiman
cmd/clef, signer: security fixes (ethereum#17554)
90f304a 
* signer: remove local path disclosure from extapi

* signer: show more data in cli ui

* rpc: make http server forward UA and Origin via Context

* signer, clef/core: ui changes + display UA and Origin

* signer: cliui - indicate less trust in remote headers, see ethereum#17637

* signer: prevent possibility swap KV-entries in aes_gcm storage, fixes ethereum#17635

* signer: remove ecrecover from external API

* signer,clef: default reject instead of warn + valideate new passwords. fixes ethereum#17632 and ethereum#17631

* signer: check calldata length even if no ABI signature is present

* signer: fix failing testcase

* clef: remove account import from external api

* signer: allow space in passwords, improve error messsage

* signer/storage: fix typos
cryptomental pushed a commit to cryptomental/go-ethereum that referenced this pull request Jan 9, 2019
@holiman @cryptomental
cmd/clef, signer: security fixes (ethereum#17554)
9fba306 
* signer: remove local path disclosure from extapi

* signer: show more data in cli ui

* rpc: make http server forward UA and Origin via Context

* signer, clef/core: ui changes + display UA and Origin

* signer: cliui - indicate less trust in remote headers, see ethereum#17637

* signer: prevent possibility swap KV-entries in aes_gcm storage, fixes ethereum#17635

* signer: remove ecrecover from external API

* signer,clef: default reject instead of warn + valideate new passwords. fixes ethereum#17632 and ethereum#17631

* signer: check calldata length even if no ABI signature is present

* signer: fix failing testcase

* clef: remove account import from external api

* signer: allow space in passwords, improve error messsage

* signer/storage: fix typos
cryptomental pushed a commit to cryptomental/go-ethereum that referenced this pull request Jan 9, 2019
@holiman @cryptomental
cmd/clef, signer: security fixes (ethereum#17554)
ca43f4a 
* signer: remove local path disclosure from extapi

* signer: show more data in cli ui

* rpc: make http server forward UA and Origin via Context

* signer, clef/core: ui changes + display UA and Origin

* signer: cliui - indicate less trust in remote headers, see ethereum#17637

* signer: prevent possibility swap KV-entries in aes_gcm storage, fixes ethereum#17635

* signer: remove ecrecover from external API

* signer,clef: default reject instead of warn + valideate new passwords. fixes ethereum#17632 and ethereum#17631

* signer: check calldata length even if no ABI signature is present

* signer: fix failing testcase

* clef: remove account import from external api

* signer: allow space in passwords, improve error messsage

* signer/storage: fix typos
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ligi ligi ligi left review comments

@karalabe karalabe karalabe left review comments

@fjl fjl fjl approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@holiman @fjl @ligi @karalabe