Kunzisoft / KeePassDX

Lightweight vault and password manager for Android, KeePassDX allows editing encrypted data in a single file in KeePass format and fill in the forms in a secure way.
https://www.keepassdx.com/
GNU General Public License v3.0
4.7k stars 276 forks source link

Improve opening performance #933

Open J-Jamet opened 3 years ago

J-Jamet commented 3 years ago

Optimize unlocking code and decipher implementation to be faster.

Requires refactoring of the code. It is planned to make a module dedicated to encryption with implementation of more advanced unit tests.

J-Jamet commented 3 years ago

I studied this feature and there are two things:

J-Jamet commented 3 years ago

I created a Go library to compare and the results are amazing :

I also improved the code for better encapsulation.

Here is a package compiled with the method of the historical C libraries and the Go library that I designed especially for the derivation methods.

KeePassDX_2.9.15_beta01.zip

Feel free to do a comparison on your devices and tell me if you have any changes. The big problem with the Go library, it is experimental and the compilation cannot be done for F-Droid. But if you don't have any bug, I can implement it for the Play Store versions.

ghost commented 3 years ago

I had the opposite results on my device (Google Pixel 3a). C version is faster than Go version in many cases.

J-Jamet commented 3 years ago

Mhhh OK, good to know, thank you for the feedback, I'm going to keep the C version for now. These performance differences are weird, it must depend on the type of processor (ARM / 64 ...) and optimization of the compiler I only tested with a parallelism of 2. I will modify this code later, it is low level so it is not obvious.

shadow00 commented 3 years ago

Please make sure that whatever crypto implementation you end up using runs in constant time. Last thing you want is a side-channel vulnerability in a password manager, and rolling your own crypto is the quickest way to get there :cold_sweat:

J-Jamet commented 3 years ago

For the moment I just refactored the code in a module for better visibility and C code is still the method used. I wanted to test the Go to see what would be the gains or losses but there are too many side effects. So don't worry, the proven methods are still the ones used.

Compiling the Go code into machine language is done with a proven open source compiler and uses the JNI to be launched from the JVM. If you have a vulnerability in mind, explain it precisely, it will be useful for the go mobile community.

J-Jamet commented 3 years ago

If you are interested in the Go code, it is very simple and uses only the methods of the libs : crypto/aes crypto/sha256 github.com/aead/argon2

shadow00 commented 3 years ago

Oh ok good, I thought you wanted to rewrite the actual crypto implementation.

There are many side-channel vulnerabilities that can affect some cryptographic algorithms. Earlier I was referring to a timing attack: an external process could try to guess the contents of the encrypted data by looking at how long an AES encryption/decryption operation is taking (I mean each single calculation step).

To mitigate this, you have to make sure that the crypto code runs in constant time: each calculation will always take the same amount of time regardless of the content of the data you're encrypting. This can be tricky because compiler optimizations can sometimes introduce this vulnerability even if your code is supposed to be safe.

chpio commented 3 years ago

Somewhat related: can we improve the opening speed for advanced unlocking by storing the result hash/actual encryption key (instead of the password)? Im assuming that it's calculating the password hash via argon2 (or something like that) while "Retrieving database key..." is displayed on the screen, is my assumption correct?

Is there a reason why it's better to store the unhashed password over the hashed password/key? Security wise it should be the same. We already know that the stored password is correct, so it doesn't have a benefit against brute forcing, it just adds a constant delay to each open.

J-Jamet commented 3 years ago

@chpio The advantage of linking only the password to the fingerprint is that it is possible to put a second factor completely untethered. For example, if I have a key file on a USB stick, I have to put the second factor and the fingerprint is not enough.