Don't put passwords in localStorage

[ehuggett]

Plaintext passwords are being stored in LocalStorage.

screenshot

send/app/fileManager.js

Lines 180 to 181 in f08dd59

	file.password = password;
	state.storage.writeFiles();

The only use of this value appears to be as a parameter to passwordComplete which displays the users password on their screen.

send/app/templates/share.js

Line 47 in f08dd59

? passwordComplete(state, file.password)

[shikhar-scs]

@ehuggett Yeah that's true. I'll work on this and file a PR soon.
Also should I completely remove the password or put an encrypted text in its place ?

[dannycoates]

@shikhar-scs do not work on this yet

[shikhar-scs]

Umm Okay

[akuckartz]

That should never have been done.

[dannycoates]

We store the password locally so that the sender can come back to the page later (after closing the tab) and see the password if they need to and also to allow the password to be changed without manually re-entering the old one.

We take precautions to prevent XSS attacks that could allow a remote attacker to read the local storage. If it can be demonstrated that this is not the case we will change our approach.

Someone concerned with the physical security of their local device (or localStorage in general) should use Send in private browsing mode so the data will be deleted when the tab is closed.

[ehuggett]

Store the authKey instead of the password; it can be used to reset the password in combination with the ownerToken (a change password option).

And increase the number of PBKDF2 rounds when calculating the authKey as its would then be stored with its salt #607

(after closing the tab) and see the password

Being able to unmask it before you set it can be useful, but if it can be reset then I don't think it's necessary or even desirable to be able to unmask it after it has been set and the tab closed!

[dannycoates]

@ehuggett what threat are you concerned about wrt having the password in local storage?

[ehuggett]

I don't think a specific threat is relevant, even if you do feel there is no risk of injected JavaScript, plaintext passwords are being written to disk.

# this is the file that firefox uses for localStorage, turns out you don't always need a sqlite binary to read a sqlite database!
$ strings ~/.mozilla/firefox/[deleted-profileName]/webappsstore.sqlite | grep passwordpasswordpassword 
ten.swazom.ved.dnes.:https:443ten.swazom.ved.dnes.:https:443[deleted-fileID]{"url":"https://send.dev.mozaws.net/download/[deleted-fileID]/#XISUPenIvG1BFDijP7yu1g","id":"[deleted-fileID]","secretKey":"XISUPenIvG1BFDijP7yu1g","ownerToken":"1dc11f38af9fbd864373","nonce":"wuGsb5FArQeo2BAnG59KYg==","name":"send_logo.svg","size":1144,"type":"click","time":233,"speed":4909.871244635193,"createdAt":1515893935036,"expiresAt":1515980335036,"password":"passwordpasswordpassword"}
# in this example, the password is found at the very end of the previous line 

Its worth noting that the plaintext passwords may remain in localStorage for significantly longer than the 24 hour file expiry if the user never returns to Send for them to be "actively" deleted (IIRC localStorage has no configurable expiry but the browser probably won't store it indefinitely).

I think its fair to assume many users will reuse important passwords (especially if they are sending a file to themselves). Of course anything the user does on their computer after its compromised may not be secure, but its still a worthwhile improvement if passwords are not also immediately compromised.

It seems rather unfortunate that only users who opt for "more security" by adding a password will be exposed to this additional risk.

To summarise/reiterate,

• Don't put passwords in localStorage, this is likely to limit the unmasking of the password to the current session
• store the authKey instead of the password so that forgotten passwords can still be changed after the session has ended (conveniently the change password PR has now been merged)

[shikhar-scs]

Also, the way I had set it, I realised just now that it also is visible in the inspector (toggle Ctrl+Shift+I).

Here is a preview

[dannycoates]

Thanks for your thoughts @ehuggett, as always. The most compelling parts of your argument (to me) are that 1) people reuse passwords 2) it can reside on disk for an indeterminate amount of time.

Since the password is optional I wonder if we should take that as a signal that the user is willing to trade some convenience features for safety? That'd open the way for not storing the password (and maybe more)

I'll bring this up at the next team meeting.

[dannycoates]

We've decided to remove the password from localStorage. The change has been merged and will be in the next release.