yogeshpaliyal/KeyPass

KeyPass does not securely handle credential storage

Closed this issue · 4 comments

BenEdridge commented

Describe the bug

Hi there, always great to see more Apps making use of F-Droid! I thought I'd take a quick look due to the popularity.

I believe KeyPass in it's current status does not meet the security standards of other password managers or the recommendations by organisations such as OWASP and MITRE. This project should probably contain a warning on usage and details on encryption mechanisms so users can make an informed choice on on how their sensitive data is protected.

Issues

  1. The current database (storing passwords) and shared preferences (storing the login password) are are not encrypted and thus accessible in plaintext to potential attackers without logging in.
  2. Database backups use a static IV of: private val iV = byteArrayOf(0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0) and when encrypted this results in a more predictable and potentially vulnerable exported backup.

To Reproduce

Steps to reproduce the behavior:

  1. Download source code.
  2. Build and then run on an emulator or real device with debugging enabled.
  3. Open KeyPass then create a password to access.
  4. Create a few example accounts.
  5. Run adb shell "run-as com.yogeshpaliyal.keypass cat /data/data/com.yogeshpaliyal.keypass/databases/KeyPass-wal" > data.db to save the Write Ahead Log file containing the most recent changes to the DB.
  6. cat data.db

Leak of login password
image

Leak of accounts
image

Recommendations

yogeshpaliyal commented

Thanks @BenEdridge for raising this, will address these ASAP

yogeshpaliyal commented

@BenEdridge Security has been improved in the latest version, can you verify once, thanks once again for raising this. ❤️

BenEdridge commented

Hi @yogeshpaliyal I've taken a quick look. Much better than before but one issue I raised still remains.

Static IV use (This should be generated with secure random number generator) and stored alongside encrypted data. Then extracted and used during decryption.

common/src/main/java/com/yogeshpaliyal/common/dbhelper/EncryptionHelper.kt
32:    private val iV = byteArrayOf(0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0)

Additionally, user chosen passwords (Weak passwords quite often) should be used to derive a more secure encryption key using PBKDF2 or Argon2 (Similar to what 1Password and Bitwarden does).

User password -> PBKDF2/Argon2 -> Database Encryption/Database Backup Encryption

yogeshpaliyal commented

Sure will look into this, thanks