High CPU usage when using --pw-stdin and a a non-existing database

[joanbm]

KeePassXC can be set, through the command line, to unlock a database when starting up. The password can be provided through the standard input, like so:

echo 1234 | keepassxc Database.kdbx --pw-stdin

(Note that this is broken on release 2.3.0, so using the current HEAD of the git repository is required.)

If the Database.kdbx file exists, this works correctly. However, if Database.kdbx does not exist (e.g. because the user specified the wrong filename), the above command line causes KeePassXC to hang with 100% CPU usage for a long time.

Note that the description above is the common use case for reproducing the bug, but the bug can be reproduced by simply running:

echo randomstring | keepassxc

That is, KeePassXC hangs with 100% CPU when providing garbage input through the command line, which includes the case where the database does not exist (since in that case, --pw-stdin is just ignored).

Expected Behavior

KeePassXC should not hang when providing simple invalid inputs through the command line or standard input.

Current Behavior

KeePassXC hangs with 100% CPU on the examples provided above.

Technical explanation

In the first case, if a database file name and --pw-stdin are provided, but the specified database file does not exist, those parameters are simply ignored, without any byte from the standard input being consumed. So, the first case is equivalent to the second case currently.

In either case, the hang happens in NativeMessagingHost::readStdIn. This is because in the native messaging API, messages are provided through a (u32 length)(byte[length] message) format. KeePassXC will interpret the garbage string in the standard input as a very large 32-bit unsigned length, which will cause a very large character-by-character read which will cause the hang with 100% CPU usage. Note that KeePassXC will try to read length characters even if EOF happens prematurely.

Possible Solution

To fix the main problem (the hang), I propose that in NativeMessagingHost::readStdIn, while doing the read of the actual message, if EOF happens, then the read stops prematurely and the message is ignored. This is similar to the !std::cin.eof() check that is already being done when reading the message length, but it should be done with the content of the message instead of the length.

Additionally, it may be worth changing the behavior of KeePassXC when using an invalid database name along with --pw-stdin. In this case, it may be better if a line of stdin is consumed anyway, to avoid garbage data being provided to NativeMessagingHost in this case. I also don't know if running NativeMessagingHost has any valid use case along with --pw-stdin, in this case, it may be better to not run NativeMessagingHost if --pw-stdin has been passed.

Steps to Reproduce (for bugs)

Run any of the following command line commands:

echo 1234 | keepassxc /does/not/exist.kdbx --pw-stdin
echo randomstring | keepassxc

Context

I was trying to set up an script to unlock my database on login, using the --pw-stdin option and the database file name through the command line. However, sometimes the block device that contains my database is not mounted, so the database file does not exist. I expected KeePassXC to either fail or not run in this case, but instead it hangs on 100% CPU.

Debug Info

KeePassXC - Version 2.3.0-snapshot
Build Type: Snapshot
Revision: a06e85f

Libraries:

• Qt 5.10.1
• libgcrypt 1.8.2

Operating system: Arch Linux
CPU architecture: x86_64
Kernel: linux 4.15.6-1-ARCH

Enabled extensions:

• Auto-Type
• Browser Integration
• Legacy Browser Integration (KeePassHTTP)
• SSH Agent
• YubiKey

[varjolintu]

Thank you for such a detailed explanation!

[TheZ3ro]

I think we should fix this for 2.3.1

[phoerious]

Please test #1628 and see if it fixes the problem for you.

[joanbm]

@phoerious Just did some quick tests with the branch you linked, and I haven't been able to replicate the bug, so it seems it is correctly fixed.