Encryption integrity

[WardPearce]

It appears NewPass uses AES in CBC mode, this means NewPass has zero encryption integrity what could be considered a massive security flaw. I'd recommend using AES GCM, manually adding integrity checks to CBC using a cipher-based message authentication or using something like libsodium for encryption.

Ideally just use libsodium as it does all the hard work for you.

[starry-shivam]

It appears NewPass uses AES in CBC mode, this means NewPass has zero encryption integrity what could be considered a massive security flaw. I'd recommend using AES GCM, manually adding integrity checks to CBC using a cipher-based message authentication or using something like libsodium for encryption.

Ideally just use libsodium as it does all the hard work for you.

Other than AES-GCM, another way to ensure data integrity is by using a separate HMAC key to generate a hash of the IV and ciphertext combined. This HMAC is then appended to the final encrypted output.

During decryption, the HMAC is recalculated and compared with the appended HMAC. If they match, the integrity is verified, ensuring the ciphertext has not been tampered with.

One benefit of this approach is that it provides flexibility, as it can also be applied to other encryption modes besides CBC. However, it can be a little slow compared to GCM if performance is a concern due to the extra computation required to generate and match hashes, but I doubt it will make any noticeable difference in the real world.

[starry-shivam]

@6eero If you'd like, I can create a PR sometime later to fix the data integrity issue. However, one concern I have is that people who have already been using the app will have their passwords encrypted in AES-CBC mode. If we change the decryption method to something else, the decryption for existing passwords would fail, resulting in users getting locked out of their data.

So, I think we'll need to keep the existing CBC decryption method and either add a tag to our new encrypted passwords, which use GCM or HMAC (let me know what you'd prefer; personally, I think we should go with GCM in this case because reusability doesn't matter here as we don't use any other encryption modes to combine HMAC with, hence GCM seems a straightforward approach), or detect the format of the encrypted data to differentiate between new and old encryption, and decrypt old encrypted data using CBC while all new keys are encrypted and decrypted using GCM.

[WardPearce]

Personally think you should just use http://libsodium.gitbook.io/

[starry-shivam]

Personally think you should just use http://libsodium.gitbook.io/

Libsodium by itself doesn't provide any Android/Java/Kotlin framework, so we'll have to use it probably with JNI (Java Native Interface), which adds unnecessary complexity and probably would also increase the overall size of the APK. Besides, Android itself natively supports GCM-based encryption since Android 5.0 Lollipop without any extra third-party libraries, and the implementation is also straightforward. So, I don't think we should bother with spending time to create a JNI interface to link Libsodium and then delegate calls through it when a native high-level Java API already exists and is supported by the OS.

[6eero]

However, one concern I have is that people who have already been using the app will have their passwords encrypted in AES-CBC mode. If we change the decryption method to something else, the decryption for existing passwords would fail, resulting in users getting locked out of their data.

At the moment, NewPass allows users to export the password database encrypted only by the key entered during the initial setup. This happens because when a database is exported, the decrypt method is called to decrypt the password column. This is done to import the database without issues related to incorrect keys. In fact, when a database is imported, the encrypt method is called to encrypt the entire password column. So, in the exported database (fully encrypted by the user key) the password column is currently stored in plaintext, which is a significant security issue that I need to address in the next update (I'll open an issue soon).

My idea is to entirely switch from AES-CBC to ARS-GCM and include a note in the changelog of the next update advising users to export their database before updating the app and then import it.

For libsodium, I found this repository which might be helpful if we want to use libsodium, but your solution also sounds good.

[starry-shivam]

My idea is to entirely switch from AES-CBC to ARS-GCM and include a note in the changelog of the next update advising users to export their database before updating the app and then import it.

I see. Then, yeah, it sounds like a good idea to just ditch the AES-CBC mode and switch to GCM entirely. Can you assign this issue to me?

For libsodium, I found this repository which might be helpful if we want to use libsodium, but your solution also sounds good.

Hmm, that seems like something we could use, but honestly, I would avoid using a limited [1], [2], and unmaintained wrapper like this due to security issues and unnecessary hurdles to deal with. Also, for the reasons I mentioned above, we already have native support for AES-GCM on Android, and Java's standard cryptography library has all the necessary APIs and functions to implement it ourselves.

[6eero]

Okay, I'm going to assign this to you, thank you.