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.

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

LCD communication process (security) #382

Closed
faboweb opened this issue Jan 24, 2018 · 13 comments
Closed

LCD communication process (security) #382

faboweb opened this issue Jan 24, 2018 · 13 comments

Comments

@faboweb
Copy link
Contributor

faboweb commented Jan 24, 2018

The cosmos SDK provides a REST-server, that should provide all the functionality of the SDK. The question is, is the communication with this REST-server secure enough to send local passwords so that only the SDK can handle building of TX / signing of TX / sending of TX. This would be a major improvement for all app-developers building on top of the SDK, as they don't have to implement this round-trip themselves.

Process before:

UI -- (build request, including the data) -- SDK -- (unsigned tx) -- UI
UI -- (sign request, including the local password) -- SDK -- (signed tx) -- UI
UI -- (send request) -- SDK -- (result) -- UI

Process after:

UI -- (send request, including the local password and data) -- SDK -- (result) -- UI

Quoting @jaekwon:

What do you think? Trying to make the UX more secure by enforcing a separate process where one enters the key. We can have another electron app. If we could check for existence of internet (not by pinging our own servers... I wonder if there's a better way, like pinging DNS servers), and quit with a warning message if internet is found.

<electron-wallet Javascript>--(USB)--<HSM>
<electron-wallet ArmorString>--(file,QR,chirp)--<electron-ckeystore>
<electron-wallet ArmorString>--(file)--<ckeystore>

ckeystore will show the transaction JSON visually so one can inspect what they are about to sign.

Quoting @mappum from a meeting: (Correct me if quote you wrong)

The LCD REST-server is only local. The scenarios to compromise this communication assume that the computer already is compromised. In this case there are a variety of other attack vectors more dangerous then this one.

@NodeGuy
Copy link
Contributor

NodeGuy commented Mar 7, 2018

These should be separate programs:
ckeystore The Cosmos key store, another cli. Not specific to Gaia.

I strongly agree that ckeystore should be a separate binary to reduce the attack surface.

REST: curl -x POST /keys --data {name: 'Fabos', password: '1234567890', seed: '...........'}: We shouldn't support this even for debugging. Sending a password over a network stack is introducing way too much surface area. Lets leave the password in RAM in the client, do crypto operations there, or use an HSM, anything but send passwords over a socket.

Agreed.

Every blockchain project seems to go through the pain of realizing that they shouldn't add key/wallet management w/ the daemon binaries. Lets learn from their mistakes and not conflate functions. Lets not add them to begin with, and discourage others from adding key/wallet management to these daemons.

Strong agreement! Monax realized this and put its key management into a separate binary called Monax Keys, written by @ebuchman .

OTOH if Electron can access the filesystem, then we can have it bcrypt and store the private key if we wanted it to. It has the private key in its runtime already so I don't see any significant additional risk in signing transactions right there. Using a socket to transfer the password in plaintext, on the other hand, is introducing a new surface of vulnerability. Either we put work into understanding the full extent of this new surface of vulnerability, or we do the more conservative thing.

Agreed. It's a good idea to have the private key as close to the user as possible.

It's strictly worse to send the private key to go, if it came from electron.

Agreed.

Wouldn't it make more sense for the HSM (ala plugnplay devices as Myetherwallet does it w/ the ledger) to interface directly with the Javascript? I don't see the point of going into the console to connect to the HSM. I mean, I do because I'm extra paranoid, but if we assume that the HSM itself is secure, then there's no need to go to golang to interface with the HSM.

Yes.

Regarding the UX, here is my idea:

The ideal is to use an HSM. For people who aren't using an HSM, the UX and architectural design should be the equivalent, i.e., we should provide a software HSM. We can call the software HSM ckeystore if we want. Ideally it would support exactly the same API as a hardware HSM so that it's easy to switch from one to another.

@jessysaurusrex
Copy link
Contributor

Does the SDK support authentication on the REST web interface? I looked at this awhile back and don't remember it being there, but if that has changed it may change the solution I am thinking of.

@faboweb
Copy link
Contributor Author

faboweb commented Mar 7, 2018

What do you mean by authentication? The SDK allows signing on the REST interface if the application provides the name and password of the private key stored in the key-manager.

@jessysaurusrex
Copy link
Contributor

How does the Electron client authenticate communication to the SDK?

@tarcieri
Copy link

tarcieri commented Mar 7, 2018

Wouldn't it make more sense for the HSM (ala plugnplay devices as Myetherwallet does it w/ the ledger) to interface directly with the Javascript? I don't see the point of going into the console to connect to the HSM. I mean, I do because I'm extra paranoid, but if we assume that the HSM itself is secure, then there's no need to go to golang to interface with the HSM.

For what it's worth, I just implemented the end-to-end encryption protocol used by the YubiHSM2 in Rust:

https://github.com/tendermint/yubihsm-rs

If there's interest in supporting end-to-end encryption between JS and hardware devices like YubiHSM2s, I don't think translating my implementation into JavaScript would be too difficult. The protocol itself is rather simple, at least as far as transport encryption protocols go, and only depends on AES (in CBC mode for encryption, and CMAC for authentication), as well as PBKDF2-SHA-256 for deriving a set of static encryption keys from a password.

The interface to the device itself (yubihsm-connector, an app provided by Yubico) is HTTP(S), so it should be fairly easy to integrate with a JS app. Note that yubihsm-connector is untrusted in that it acts as an HTTP(S)<->USB gateway for the hardware device.

@rigelrozanski
Copy link
Contributor

The LCD REST-server is only local. The scenarios to compromise this communication assume that the computer already is compromised. In this case there are a variety of other attack vectors more dangerous then this one.

hmm does this basically suggest that we shouldn't be concerned about communication hacks because this is effectively comparable to the client machine getting hacked which is effectively the same loss of security as storing the password locally and having it compromised there?


What do you think? Trying to make the UX more secure by enforcing a separate process where one enters the key. We can have another electron app. If we could check for existence of internet (not by pinging our own servers... I wonder if there's a better way, like pinging DNS servers), and quit with a warning message if internet is found.

interesting - sounds like an okay idea. We definitely want to allow/encourage for posh air-gapped setups - although a tiny atom holder who just wants to be involved may not care that much (and shouldn't have too) - maybe we leave this as a setting (default:ON) and allow for small-time users to disable


I'm a bit confused/disconnected from the development - does the local UI client run an instance of the SDK client and call it to sign transactions (proposed)... so we're only concerned about communication within the local machine? I imagine local encrypted communication should be secure enough...... maybe correct me?

@faboweb
Copy link
Contributor Author

faboweb commented Mar 7, 2018

@tarcieri this sounds like a cool idea and thank you for offering this! I would suggest that we think about migrating to JS <-> hsm a little later and use the JS <-> keystore <-> hsm first so we minimize double work and also have a product that uses and therefor tests keystore <-> hsm.

We definitely want to allow/encourage for posh air-gapped setups

Cool feature. The question is which user we want to target first (small holders or wales). I would suggest we target the masses first to get the UX right and then provide features for the few wales later.

the local UI client run an instance of the SDK client and call it to sign transactions (proposed)... so we're only concerned about communication within the local machine?

The UI runs the client formerly known as Gaia locally and exposes the REST API to also build -> sign -> send transactions. You figured correctly, that the communciation between the JS part and the SDK REST server was the issue.

local encrypted communication should be secure enough

@jessysaurusrex @NodeGuy would you agree?

@rigelrozanski
Copy link
Contributor

rigelrozanski commented Mar 7, 2018

* I imagine * not a security expert ;)

@jessysaurusrex
Copy link
Contributor

Provided the communication between the parts in question is all encrypted and has some sort of authentication (implicit based on context or explicit), that seems fine. Even if the interfaces are all local, I'd like to avoid sending bare keys over IPC and have some assurance that the things we're interfacing with are actually what we intend to use/send traffic to.

@jbibla
Copy link
Contributor

jbibla commented Mar 8, 2018

you guys are awesome.

@faboweb
Copy link
Contributor Author

faboweb commented Mar 8, 2018

Created a ticket for the first outcome of this discussion: #595

@NodeGuy
Copy link
Contributor

NodeGuy commented Mar 8, 2018

local encrypted communication should be secure enough

@jessysaurusrex @NodeGuy would you agree?

Yes.

@cwgoes
Copy link
Contributor

cwgoes commented Oct 2, 2018

I think we can close this? The LCD now uses HTTPS.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

9 participants