-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
Implement HTTPS for the LCD REST server #595
Comments
Since the LCD will be serving the data to local clients, would this be a self-signed certificate that gets verified out-of-band? I'm still not convinced protecting this transport is important since the LCD should never be served over the network, it's only for clients on the same machine. The private keys require auth which already means attackers can't do CSRF for sending transactions. The only thing I'm worried about is DNS rebinding, which would let websites read data such as a list of the user's addresses, but this is easily fixable by making the LCD only allow requests with the expected |
Am I correct, that an attacker could read the communication between the electron and Gaia if it happens over HTTP? @jessysaurusrex wanted to have the communication encrypted to prevent readability of the password we need to send to Gaia to sign messages. |
To intercept that local communication, the attacker would need to compromise the machine and have root permissions, and if they have that then the game is already over (they can monitor the keyboard and get the password that way, or get it from memory, etc). |
Even if the interfaces we're using are local, I'd like to avoid sending bare credentials across IPC-- this is considered a best practice in security. While I fully understand the requirements around an attack here, there is significant value in making it harder for an attacker to successfully intercept a password from a machine. When I was on the security team at 1Password, credentials were sent between our application and our browser extension through IPC without any kind of obfuscation (which at least would make the attack harder), or mutual authentication with TLS (which would have taken significant resources to attack). While the argument of physical access might seem like a hard-to-carry-out attack, the acting assumption from the decisionmaker was that our users did not use or share machines with multiple user profiles, and it was a significant oversight on our part. The decision not to obfuscate or encrypt IPC escalated into a highly embarrassing, highly public security incident for us when Tavis Ormandy from Google Project Zero was able to exploit it with a little bit of help from a privilege escalation bug in Windows. We lost trust from our users (even the extremely technical ones) who had a mental model in which they expected that it would not be possible to grab plaintext credentials out of memory because they had assumed that they were secured through encryption or obfuscation. The same happened with many of our industry peers, who still ask me about the issue and how it happened years later. In general, we know that people who own cryptocurrency are at a higher risk of being on the receiving end of attacks-- and that whales are even more of attractive target. Wherever possible, we should be thoughtful of that threat model... especially when we're talking about passwords and keys. In this case, I recommend that we encrypt the IPC so that we are reducing risk and following an established security practice that makes it harder for an attacker to successfully attack a machine and profit from it. |
There are two core threat models at work
The REST server MUST mitigate both these threats. Here is a proposal for how to do this.
The rest server should check that any requests come with a cookie with this value before responding. Otherwise should return a 401 error. The voyager or other wallet app should handle the certificate error and check that sha256 of the certificate presented matches what was on the command line. The error handling in electron for self signed certs is documented here It should be possible to disable both these behaviors via a command line flag especially when running the rest server as a backend for an enterprise service. |
I believe this would prevent the DNS rebinding attack |
See http://latacora.singles/2018/04/03/cryptographic-right-answers.html#client-server-application-security for guidelines on how to implement this securely. |
@zmanian dixit:
Can we fork it into another issue please? I'm happy to tackle both things, though this should be just re: TLS |
In order to guarantee a secure connection between apps and the LCD the communication must be encrypted - even if clients and server run on the same local machine, credentials must never be transmitted in clear text. Upon start up, the server generates a self-signed certificate and a key. Both are stored as temporary files; removal is guaranteed on exit. This new behaviour is now enabled by default, though users are provided with a --insecure flag to switch it off. See #595
PR was merge, hence closing. Work on voyager side is now needed, see luniehq/lunie#1346 |
We want to guarantee a secure connection between apps and the LCD therefor the communication should be encrypted. To do this easily we should enforce a HTTPS connection for the REST server.
The text was updated successfully, but these errors were encountered: