Attempting to unlock a KeePass v3.1 database results in infinite loop

[sts10]

In attempting to my keepass-rs dependency in Medic from keepass-rs 0.4.1 to 0.4.4, I discovered a quirky bug.

A test in which I attempt to unlock a KeePass database that's version 3.1 (rather than the newer 4.0) and does not require a keyfile seemingly results in an infinite loop. Here's the test, and here's the test database. I believe this test would pass if I were using keepass-rs 0.4.1, but thanks to some strange build situation, I've found it difficult to revert back to 0.4.1.

Strangely, Medic (using keepass-rs 0.4.4) can successfully unlock a 3.1 database if it requires a keyfile to be decrypted.

This could totally be an issue with my code -- I just did a large refactoring to handle errors better.

[sseemayer]

Thanks for the bug report, I can reproduce it independent of your codebase, so it's definitely a problem on the side of keepass-rs.

[sseemayer]

I have also been able to reproduce the error when checking out the v0.4.1 tag and trying to open the database with it.

Something that I'm noticing is that the linked test database has many more key transformation rounds (28571428) than the usual test databases (ranging from 100 to 100000) and that the code gets stuck on the key transformation bit doing very heavy work but not actually having an infinite loop.

That means that there is either an issue in how I'm parsing the header for this particular database or I have to significantly speed up my AES key derivation implementation since KeePassXC can open the same database in less than a second 😆

Edit: Yep, if you wait for the 28M key transformation iterations to succeed, the database is still opened and parsed correctly. This means that this is only a performance issue in the key derivation. Looking into it further....

[sseemayer]

OK, upon re-reading the docs for the aes crate, it turns out that AES is only compiled with the fast CPU-native AES instructions iff the target-feature=+aes is enabled and falls back to a hardware-independent implementation otherwise.

You can get much faster key derivation if you set the environment variable RUSTFLAGS='-C target-cpu=native' when running cargo build, cargo test, etc.

On my system, I did not see a significant difference (p=0.05, t-test, 10 samples each using hyperfine with compilation done in a --prepare step) in runtime when additionally adding target-feature=+aes on top of target-cpu=native:

(I didn't do 10 samples of RUSTFLAGS='' but you'll believe me that there is a significant runtime difference 😄 )

[sseemayer]

My suggestion for medic: Decrease the KDF iterations in your test database and potentially give the RUSTFLAGS compilation instructions in your install guide.

I will add something to the keepass-rs docs to also point out this performance gotcha.

[sts10]

Wow, great job getting to the bottom of this. As you said, after lowering the transformations rounds of the test DB, the test passes in a much more reasonable amount of time.

So if I'm understanding RUSTFLAGS correctly, ideally users of Medic who have databases that use AES-KDF should/can install Medic using RUSTFLAGS='-C target-cpu=native' cargo install --git https://github.com/sts10/medic? Should I just have that as the installation step for all users?

I updated Medic's README to explain this as best I could. Let me know if there are any glaring errors?

[sseemayer]

Thanks for the kind words, it was actually an enjoyable puzzle to figure out.

Without having tested your install instructions whether cargo install honors the RUSTFLAGS environment variable (I would guess that it does), your instructions are exactly how I would write them 👍