vwxyzjn / portwarden

Create Encrypted Backups of Your Bitwarden Vault with Attachments
MIT License
577 stars 33 forks source link

Portwarden leaks the encryption passphrase to all users on the system #37

Open samcv opened 2 years ago

samcv commented 2 years ago

On Linux, all processes can each other's command line arguments. Portwarden accepts the encryption passphrase with a command line argument. Because of this, any program running when backup is occuring can read the passphrase.

How to replicate

Using Linux (MacOS should also work) and a bash shell we will run a loop continuing until it reads the portwarden passphase.

while ! ps aux | grep 'portwarden.*passphras[e]'; do true; done

In another window, we will back up our data using portwarden

portwarden --passphrase foobar encrypt backup.portwarden

OS's affected

Linux is affected. MacOS is almost certainly affected but I haven't tested it. I don't know if Windows is affected or not.

Recommended way to read passphrases

Portwarden should accept the passphrase supplied in the following ways:

  1. An interactive prompt which requests to enter the passphrase. The user's typing could be hidden if desired.
  2. Using an environment variable. These are private and not visible to all processes. This will allow a way for scripts and automation of portwarden. A possible environment variable name could be PORTWARDEN_PASSPHRASE.

Full removal of --passphrase or making it less convenient

--passphrase can be fully removed, or it could be made more difficult to use, or more noisy.

Option 1: Full removal

--passphrase would be removed completely. This would force users to update their scripts to use an environmental variable. When the program is run with the option, an error should display. The error can clearly state that the option was removed due to it leaking the passphrase to other processes, and they should change their passphrase they use for encryption. A Github document or file could be linked to, giving more space to explain exactly what risks existed, and how to use the new methods of entering the passphrase.

Option 2: Making --passphrase more difficult and noisier

The Readme should be updated be updated to strongly recommend how and why this argument should not be used. If the option is used, there should be a warning stating the risk of passphrase leakage to other processes.

Option 3: Hybrid of 1 and 2

We could combine the properties of Options 1 and 2 by adding a new --i-know-passphrase-is-unsafe-and-may-be-leaked option. If this new argument was used, the --passphrase option would work again. If not, it would fail, similarly to what would happen in "Choice 1". This would ensure that user scripts break on the update, forcing users to decide whether to rotate their password, and if they want to accept the risk or switch to safer methods of supplying the passphrase. Using --passphrase without the special new argument would explain the risks, and link to a Github page for more information.

Conclusion

I was very happy to find this software, as I wanted to make an encrypted backup of Bitwarden. It was a bit disapointing that I noticed the security vulnerability from just reading the Readme.

**Please let me know if you want any help with the security side of things, as I handle things like this in my dayjob. I could help with a clear warning/error or other Github document. Or, if you agree with my suggestions, I may be able to try and fix it. Though it may take me longer as I am not very experienced with Go. Thanks in advance for your reply.

vwxyzjn commented 2 years ago

Thank you for raising this issue! So sorry it took more than a month to get back to you. Recently it has just been really busy.

This all makes sense to me. If by chance you have the bandwidth, would you mind try submitting a fix PR?

Another security issue I wasn’t sure is salt. The current implementation uses a Salt environment variable as the password salt for all distributed binaries.

https://github.com/vwxyzjn/portwarden/blob/3f0729876867713bc8f45e06bafd33298037f0ed/docker-compose.yaml#L7-L12

Is this a bad practice? I wasn't sure what was the best way to do it...

samcv commented 2 years ago

I am not sure how you are using a salt. Though usually a new salt is generated randomly at time of password hashing. The password hash is then stored with the salted hash.

If the password is stored hashed (rather than encrypted) you must always use a random unique salt.

If you are just using a passphrase to derive encryption/decryption used this usually is not required because the encryption mode (such as AES-CBC or AES-CTR) should handle it by itself. Assuming you use one of the secure modes if AES, a salt should be needed (assuming you don't store the hashed password anywhere other than memory).

I don't have time to get into the code as I am even busier than when I first opened the ticket 😄. I do remain here to answer anyquestions and give advice as required. Thanks for your reply.

avt00 commented 2 years ago

I have to add that the passphrase also stays in the Bash / ZSH history. So if someone gets access to the terminal or history file (e.g., zsh_history), they can easily decrypt the password archive.