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

localstorage is shared in a shared browser #927

Closed
huumn opened this issue Mar 16, 2024 · 5 comments · Fixed by #1127
Closed

localstorage is shared in a shared browser #927

huumn opened this issue Mar 16, 2024 · 5 comments · Fixed by #1127

Comments

@huumn
Copy link
Member

huumn commented Mar 16, 2024

Got a nostr DM from someone who logged into another account in the same browser and their new account had access to their other account's NWC.

Given we store NWC in localstorage this makes sense. Can we create authenticated localstorage? If not, we should at least clear localstorage when someone logs out.

I don't expect this to be a common problem, but it's worth not letting this happen.

@ekzyis
Copy link
Member

ekzyis commented Mar 17, 2024

Since the problem is similar to common "remember me" or "stay logged in" functionality except it's related to wallet credentials instead of login credentials, I think the solution is to have a setting that asks if this wallet should be "remembered". If this is not checked, we delete the wallet credentials on logout.

We can still prefix the storage keys with account ids so credentials from accounts don't conflict if the same device is used.

@huumn
Copy link
Member Author

huumn commented Mar 17, 2024

I think the solution is to have a setting that asks if this wallet should be "remembered"

I kind of dislike adding things a person has to think about to serve an edge case. Why not just delete them on logout?

@ekzyis
Copy link
Member

ekzyis commented Mar 18, 2024

Why not just delete them on logout?

I thought it would be annoying to configure wallets again if you logged out once. But we can simply delete on logout first and then see if it will be annoying (and prefix storage keys with account id) :)

@benalleng
Copy link
Contributor

benalleng commented Apr 2, 2024

How "dangerous" is this localStorage leak? if its not I guess just changing the name as a prefix is simple enough

const storageKey = `{$userID}__webln:provider:nwc`

const configStr = window.localStorage.getItem(storageKey).split("__")[1]

Otherwise would some encryption of the config json be worth the effort if we end up deciding deleting it is a hassle?

@ekzyis
Copy link
Member

ekzyis commented Apr 3, 2024

How "dangerous" is this localStorage leak?

It gives access to spend external user funds so pretty dangerous. That's why we recommend to set budgets in the wallet setup page:

localhost_3000_~Design(iPhone SE) (1)

Otherwise would some encryption of the config json be worth the effort if we end up deciding deleting it is a hassle?

I think encryption and deleting on logout are separate issues. If you're logging out on a device, I think it's a reasonable expectation that everything about your session is deleted (encrypted or not).

Regarding encryption: We're open for ideas! We definitely want to improve the security around these credentials. All we did so far was to add CSP (nonce-based strict policy) in #805 to make XSS more difficult.

For example, we've discussed storing the credentials on the server but that makes us more of a target. Also, I think it's a reasonable expectation from stackers that we don't store them in plain text. They should be encrypted in some way that we cannot decrypt. But I would consider them always a liability.

So the question comes down to: How should this encryption scheme look like?

Should it use a password or PIN from which we derive a key? Should it be more like 2FA where we store a decryption key on the server and the encrypted content is in their local storage which gets decrypted and stored in memory (never store decrypted credentials in local storage)? How safe is storing the decrypted credentials in memory? Should we not use local storage at all? How much can we trust the Web Crypto API? How safe is it against XSS? How much can we trust ourselves to develop a secure encryption scheme with it? See warning on the Web Crypto API page:

Warning: This API provides a number of low-level cryptographic primitives. It's very easy to misuse them, and the pitfalls involved can be very subtle.

Even assuming you use the basic cryptographic functions correctly, secure key management and overall security system design are extremely hard to get right, and are generally the domain of specialist security experts.

Errors in security system design and implementation can make the security of the system completely ineffective.

Please learn and experiment, but don't guarantee or imply the security of your work before an individual knowledgeable in this subject matter thoroughly reviews it. The Crypto 101 Course can be a great place to start learning about the design and implementation of secure systems.

So imo there is a lot to consider but we should also avoid analysis paralysis.

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

Successfully merging a pull request may close this issue.

3 participants