lightning-terminal icon indicating copy to clipboard operation
lightning-terminal copied to clipboard

ui: add home screen and update sessions list

Open jamaljsr opened this issue 1 year ago • 8 comments

This PR adds a new home screen to showcase new features recently added to Terminal on the web. It also adds the ability to quickly pass session info to your node via hyperlinks or QR codes for mobile.

Looking for feedback on security: The session data is currently encoded with base64("<phrase>||<proxy>") then passed to Terminal via the URL in the format https//terminal.lightning.engineering/#/connect/pair/<encoded-data>. The data is just decoded and pre-filled into the Connect page's form fields. The user will need to click the Connect button to perform the handshake.

From a security perspective, I believe passing the encoded data in the URL is fine since the data after the # never leaves the user's browser. It's probably a bit more secure than copying the phrase to the clipboard since every app on the device has clipboard access.

Is this encoding secure enough of should we do more?

jamaljsr avatar Aug 03 '22 22:08 jamaljsr

It's been a while since I've read typescript or thought about web security, but I think that this is fine since the scheme isn't really changing besides adding base64. The risk would be that the link is somehow compromised, but from what I remember (and quickly looked up), no other browser tabs should be able to access that unless the browser is hacked. I didn't know every app on mobile had access to the clipboard, so it's good this isn't on the clipboard

Crypt-iQ avatar Aug 04 '22 15:08 Crypt-iQ

One potential security concern with putting a pairing phrase in the URL is the persistence of browser history. As far as I know, it's not possible for a website to to clear a URL out of browser history. This means the pairing phrase would end up being stored in the user's autocomplete history indefinitely, unless the user manually cleared out their browser cache.

This is a situation where it would be very helpful to have a minimal back-end for terminal-web. We could generate a temporary symmetrical encryption key which is stored on our servers and used to decrypt the URL in terminal, preventing anyone with access to the URL (besides us) from extracting any useful information.

itsrachelfish avatar Aug 08 '22 16:08 itsrachelfish

Very good point @itsrachelfish. Data after the # doesn't get shared with the server but still gets saved to browser history.

kaloudis avatar Aug 08 '22 16:08 kaloudis

Been thinking on this for the past hour and I think the biggest attack surface would be on the terminal-web side - where we're decoding information and putting it onto the page. Any situation where you're accepting arbitrary user-input and then rendering it on the page has the potential for an XSS exploit or maliciously changing data in the redux state.

  • What happens if the encoded URL is malformed? (Including binary / null bytes?)
  • Or if the URL includes an unexpected amount of data?
  • What's the possibility of having a malicious proxy server?
  • What if a user was sent a malicious URL in a phishing email? It could be very hard for a user to know that something is wrong because the URL will look legitimate, but the populated information won't be

itsrachelfish avatar Aug 08 '22 17:08 itsrachelfish

Great feedback @itsrachelfish, thanks so much for taking a look. Below are my responses to your questions.

  • What happens if the encoded URL is malformed? (Including binary / null bytes?)

The encoded url segment is always treated as a string by the code that decodes it. It is not possible to pass in binary or null bytes via the querystring AFAIK. There once was a vulnerability in the Buffer class which could be exploited if the attacker could inject a Number value into the payload. This isn't possible via the url, so I believe we are safe on this one. We do catch any decoding errors in the in case the base64 data is malformed. In this case, we abort pre-filling the values in the form.

  • Or if the URL includes an unexpected amount of data?

We use the Buffer class to decode the base64 string. Buffer has a size limit of 1GB or 2GB depending on the platform. We'd never reach this limit though because browsers have a limit on the length of ~2000 chars.

  • What's the possibility of having a malicious proxy server?

This is a good question. I am not sure if any sensitive data is leaked during the handshake with the proxy server. @ellemouton may be better suited to answer this. One thing we can do on the frontend is display the proxy server and a warning message if the server address is different from our standard one.

  • What if a user was sent a malicious URL in a phishing email? It could be very hard for a user to know that something is wrong because the URL will look legitimate, but the populated information won't be

What do you imagine could be exploited by providing the wrong info. I suppose the worst case is the same as your previous question where the proxy server is malicious. Other than that, the wrong data would just be populated in the form fields of the connect page.

jamaljsr avatar Aug 10 '22 12:08 jamaljsr

Response from Elle via DM.

Jamal: hey, I have a security ques for you regarding LNC. Suppose an attacker sets up a malicious aperture mailbox server. If a user uses a legit pairing phrase from litd to connect to the malicious server, would the attacker now have the user's pairing phrase or any info that would allow them to use the mainnet mailbox server to connect to the user's node?

Elle: No the attacker wont be able to know the users pairing phrase. The attacker will only know the mailbox ID that the user is using which is the hash of the pairing phrase. But the attacker wont be able to use this to derive the paring phrase and thus wont be able to decrypt any of the data flowing through the stream

jamaljsr avatar Aug 10 '22 12:08 jamaljsr

@itsrachelfish: review reminder @jamaljsr, remember to re-request review from reviewers when ready

lightninglabs-deploy avatar Aug 17 '22 13:08 lightninglabs-deploy

One potential security concern with putting a pairing phrase in the URL is the persistence of browser history. As far as I know, it's not possible for a website to to clear a URL out of browser history. This means the pairing phrase would end up being stored in the user's autocomplete history indefinitely, unless the user manually cleared out their browser cache.

After looking into this, it appears that this is a pretty big deal. We are essentially storing the pairing phrase on the machine permanently in an insecure format. Anyone with access to the machine could view the browser history, obtain the pairing phrase, then have full access to the node. No bueno!

I've been investigating how to prevent the browser from persisting the history entry on the client side. There are two browser APIs that can manipulate the history, History.replaceState and Location.replace. These will update the session history so that the back button will not navigate to the url with the encoded phrase, but they do not alter the persisted global history in the browser.

The only way I could find to prevent the browser from persisting urls in the global history is to use an HTTP 301 redirect, but this requires a backend. In our current use case, having a backend wouldn't work either because that would leak the node's pairing phrase to our server.

With all of that said, it seems to me that passing unsecured sensitive data via the url is just not going to work. We have to figure out another way to pass the phrase from litd to TW. I have a few ideas, but it will alter the flow. The user won't be able to just click a link and have the plain text phrase pre-filled.

Alternative approaches:

  1. In litd, prompt the user for a password/pin to encrypt the phrase. include that encrypted phrase in the TW url. When they click the link, prompt the user in TW to enter the password/pin to decrypt the phrase.
  2. Similar to the above, but instead of prompting the user for a password, we can use some info from their node (ex: alias or pubkey) to encrypt the phrase. Then we'd need to prompt them to enter it on the TW side. It may not be great to use public info as the encryption password though since an attacker may know it.
  3. Also similar to the above but we automatically generate a random pin number in litd that would then need to be entered into TW after clicking the link with the encrypted phrase.
  4. This one is completely different. There is a way to have sites on different domains communicate with each other using a hidden iframe and Window.postMessage. We could add a hidden iframe in litd that sends the phrase to TW via postMessage. TW temporarily stores the phrase in localStorage when it receives it. When the user clicks the link, TW will load and remove the phrase from sessionStorage to pre-fill the connect form. The downside to this approach is that it wouldn't work for third-party apps or different devices. The link would have to be opened by the same browser on the same device. This also means the QR code wouldn't work.

I am trying to think of more solutions but I just wanted to post on update on where I am with this PR. If anyone else has any ideas, please do share.

jamaljsr avatar Aug 17 '22 17:08 jamaljsr

Anyone with access to the machine could view the browser history, obtain the pairing phrase, then have full access to the node. No bueno!

worth noting that the thing we are encrypting, the pairing phrase, is a one-time use thing iff the litd node is on a version using second handshake. So the only situation we really need to worry about is if an attacker gets their hands on the link before the user is able to use it or if the initial connection to the node didnt work and the user waits for a while before retrying.....

this also makes me think that with second handshake, Litd should timeout/expire a session after a few mins if no initial connection (via tha passphrase) is made in a timely manner.

ie - give the user a very short window in which they can switch the connection over to using second handshake. which then also makes an attacker's life harder and further decreases the risk of the passphrase leaking or sitting around in history

ellemouton avatar Aug 18 '22 05:08 ellemouton

Thank you for providing a much better solution @ellemouton 🙌

worth noting that the thing we are encrypting, the pairing phrase, is a one-time use thing iff the litd node is on a version using second handshake. So the only situation we really need to worry about is if an attacker gets their hands on the link before the user is able to use it or if the initial connection to the node didnt work and the user waits for a while before retrying.....

This new link functionality will only be available on future versions of litd so the user will definitely be running a version with second handshake.

Given that the pairing phrases will now timeout after a short period of time, I will need to make some small changes to this PR. Currently the frontend will detect if there is an available session that isn't revoked, expired, or in use. If there isn't one available, it automatically creates a new one when the home screen loads. With the sessions expiring quickly, this could cause many new sessions being repeatedly created then revoked.

I am going to update the home screen so that a new session isn't created until the user expresses intent to connect to TW by either clicking the "Connect to Terminal" or "Connect with QR" buttons.

jamaljsr avatar Aug 18 '22 15:08 jamaljsr

Based on the conversation we had today, I will make the following changes to this PR:

  • [x] Do not automatically create a session until the user clicks on one of the Connect buttons on the home screen
  • [x] When a session is created, display a toast notification stating that the pairing phrase is valid only for the next 10 mins
  • [x] Remove the screen that is currently displayed when there is only one available session. This is very similar to the new home screen. The LNC page will always show the session list going forward.

jamaljsr avatar Aug 18 '22 22:08 jamaljsr

Doing a proper review this evening/tomorrow morning, so should have some nits and comments in parallel with the latest set of TODOs as well.

dstrukt avatar Aug 19 '22 05:08 dstrukt

I've made all of the changes listed by myself and @dstrukt in the latest commit https://github.com/lightninglabs/lightning-terminal/pull/401/commits/8eec94d7293e69dae5371133645e43e4d420ccfe.

jamaljsr avatar Aug 19 '22 15:08 jamaljsr

Awesome, got sign off on the copy change as well.

Will re-review here momentarily.

dstrukt avatar Aug 19 '22 19:08 dstrukt

Thanks for the review @dstrukt. I'm going to hold off on merging this until #408 is finalized just in case additional changes are needed in the UI.

jamaljsr avatar Aug 22 '22 02:08 jamaljsr

Rebased on master which no includes #408

jamaljsr avatar Aug 23 '22 19:08 jamaljsr