search

keepassxreboot / keepassxc

KeePassXC is a cross-platform community-driven port of the Windows application “Keepass Password Safe”.
https://keepassxc.org/
Other
20.78k stars 1.44k forks source link

Unable to open KDBX 4.0 database created by Strongbox #2581

Closed vinzf closed 5 years ago

vinzf commented 5 years ago

KeePassXC fails to open KDBX 4.0 database created by Strongbox. It fails with error mesage "Unsupported key derivation function (KDF) or invalid parameters":

image

Here is a sample file using the password "1234"

strongbox.kdbx.gz

Also see Strongbox issue https://github.com/mmcguill/Strongbox/issues/23

Debug Info

KeePassXC - Version 2.3.4 Revision: 6fe821c

Libraries:

Operating system: OS X El Capitan (10.11) CPU architecture: x86_64 Kernel: darwin 15.6.0

Enabled extensions:

droidmonkey commented 5 years ago

This is not a KeePassXC issue. We implement Argon2 according to the KeePass2 specification and are fully compatible with databases created in KeePass2. See my comment on the StrongBox issue you linked.

KeePass' Argon2 implementation supports all parameters that are defined in the official specification, but only the number of iterations, the memory size and the degree of parallelism can be configured by the user in the database settings dialog. For the other parameters, KeePass chooses reasonable defaults: a 256-bit salt is generated by a CSPRNG each time a database is saved, the tag length is 256 bits, no secret key or associated data. All versions of Argon2d (1.0 to 1.3) are supported; KeePass uses the latest version 1.3 by default.

https://keepass.info/help/kb/kdbx_4.html

vinzf commented 5 years ago

Perfect, thanks for checking/verifying 👍

droidmonkey commented 5 years ago

Can you tell me what you used for the three parameters in the sample kdbx you posted. Specifically threads, memory, and iterations.

vinzf commented 5 years ago

Unfortunately not, since Strongbox does not let the user modify these parameters.

image

I guess it uses the defaults defined in https://github.com/mmcguill/Strongbox/blob/master/model/keepass/Argon2KdfCipher.m

static const uint64_t kDefaultIterations = 2;
static const uint64_t kDefaultMemory = 1024 * 1024;
static const uint32_t kDefaultParallelism = 2;

@mmcguill should know ...

mmcguill commented 5 years ago

@vincf is correct, for new databases those are the defaults that will be used. I believe the issue is with Strongbox using 16 rather than 32 byte salt by default. I’ll get this change in in the next release.

droidmonkey commented 5 years ago

@mmcguill cheers thanks for looking into this. It's great to have all kdbx apps able to interoperate with each other. Also, I'll be skiing out at Park City next week, enjoy the fresh POW!

mmcguill commented 5 years ago

@droidmonkey Thanks a mill! Agreed, good to have this KDBX interoperability. I'm based here in Europe, but we're getting some nice fresh stuff in the Alps too!! Have a great one out there, enjoy the POWPOW!

vinzf commented 5 years ago

Just FYI: Keepass2Android (Version 1.06f) does not have any issues with this database created by Strongbox. Also the official Windows KeePass Password Safe App (Version 2.40) is able to open that file.

vinzf commented 5 years ago

I've compiled the current develop branch and verified that the seed size 16 is what keeps KeePassXC from opening that file.

https://github.com/keepassxreboot/keepassxc/blob/develop/src/crypto/kdf/Kdf.cpp#L57

bool Kdf::setSeed(const QByteArray& seed)
{
    if (seed.size() != m_seed.size()) {
        return false;
    }
    m_seed = seed;
    return true;
}

m_seed is 32 bytes (it was inititialized with KDF_DEFAULT_SEED_SIZE which is 32) whereas the Strongbox seed has 16 bytes.

However, after removing that size check the file opens perfectly fine in KeePassXC.

droidmonkey commented 5 years ago

This is where we fail due to the 16 byte seed: https://github.com/keepassxreboot/keepassxc/blob/develop/src/crypto/kdf/Kdf.cpp#L59

It checks to see if the supplied seed size is the same as the expected seed size (32 bytes). We could loosen this check, but technically it is against KDBX standards AND less secure.

vinzf commented 5 years ago

Maybe it is best to loosen the check according to what is being done in the official source from KeePass-2.40-Source.zip in KeePassLib/Cryptography/KeyDerivation/Argon2Kdf.cs line 105:

if((pbSalt.Length < MinSalt) || (pbSalt.Length > MaxSalt))
    throw new ArgumentOutOfRangeException("p.Salt");

These are defined in the same file at line 44

private const int MinSalt = 8;
private const int MaxSalt = int.MaxValue; // .NET limit; 2^32 - 1 in spec
droidmonkey commented 5 years ago

Huh, I am pretty sure that code is in error since it would accept a salt length of 4,294,967,295. MaxSalt would make sense from a value check, not a length check.

vinzf commented 5 years ago

Saw that too, better safe than sorry ;)

droidmonkey commented 5 years ago

No that is a big problem. Can definitely lead to memory over allocation. I'll report it to him.