Groundwork to support flexible multifactor database authentication and FIDO2 #10311

BryanJacobs · 2024-02-16T07:18:38Z

Screenshots

No user-visible changes.

Testing strategy

Unit tests for parsing of the header included, and a Kdbx test opening a database secured using header-described password authentication.

Type of change

✅ New feature (change that adds functionality)

Relates to #9506 .

Description

This PR allows KeePassXC to read files containing an XML blob in their header describing multifactor authentication for the database. This blob can describe a replacement for the existing composite key, or append to it. One or more "groups" can be used, and each group is one-of-N.

Either way, the multifactor methods' output goes through the EXISTING key derivation function - it's just input to the KDF.

The target use case here is to support using one-of-N FIDO authenticators: for that use case, the header would describe a single group with N-many Factors in it.

I have implemented end-to-end support for FIDO2 in this format in KeePassPy at libkeepass/pykeepass#373 , including human-readable documentation of the file format. But this repository is generally cleaner than that one so I figured I'd raise an intermediate PR rather than a big-bang here.

What is Done

Parsing an authentication_factors blob from the KDBX4 outer header if present
Error handling for the parser
On read, using a "sha256 password" type authentication factor if present in the header

What is Not Yet Done

Saving files in this format: writing the database clears the header
User interface, either CLI or GUI. Note that the existing UI will work if a password is entered and it is valid for one password factor in each group due to how the code is implemented...
FIDO2 factor support, which is a significant amount of code on top of this groundwork

Please let me know if this is going in an acceptable direction. If so, I'll keep going, but I'd rather not have a PR with 3000-plus lines in it.

Note that this PR contains two cleanly separated commits and it may be easier to look at the first one first.

This adds the ability to parse and validate (but not use) authentication factor information contained within the KDBX outer header.

Saving the database will still discard the header information, but read-only access works.

BryanJacobs · 2024-02-16T07:19:23Z

If this general feature works okay I'll implement support for FIDO2 in KeePassDX next, so we'll have compatible implementations everywhere relevant.

BryanJacobs · 2024-02-16T07:22:17Z

src/crypto/SymmetricCipher.cpp

@@ -197,6 +197,8 @@ QString SymmetricCipher::modeToString(const Mode mode)
        return QStringLiteral("AES-128/CBC");
    case Aes256_CBC:
        return QStringLiteral("AES-256/CBC");
+    case Aes256_CBC_UNPADDED:


The AES256-CBC implementation here is encrypting exactly-32-byte-long key parts which are randomly generated. So padding doesn't really make sense.

You shouldn't need to introduce a new algorithm. If padding is not required (in this case a 32-byte aligned block) then padding won't be added.

I added this because decryption did actually fail with aligned blocks. Botan expects there to be a minimum of one padding block - how else would it know how much padding had been added?

In other words, if you have a 31 byte block, that would have one byte of padding and end up 32 bytes long. But if you have a 32 byte long block... How do you know on decryption whether it's 32 or 31 bytes before padding? There must be at least one byte of padding to store the padding length itself...

I think we need this "new" algorithm (well, variant).

Hmmmm good point

BryanJacobs · 2024-02-16T07:23:52Z

src/format/KdbxReader.cpp

@@ -95,6 +96,30 @@ bool KdbxReader::readDatabase(QIODevice* device, QSharedPointer<const CompositeK
        return false;
    }

+    auto authenticationFactorInfo = m_db->authenticationFactorInfo();


This is one of the two changes to the existing database reading code: if we read information about authentication factors from the header, either add a MultiAuthenticationHeaderKey to the CompositeKey or replace the CompositeKey with one entirely.

BryanJacobs · 2024-02-16T07:24:46Z

src/format/Kdbx4Reader.cpp

@@ -224,6 +224,23 @@ bool Kdbx4Reader::readHeaderField(StoreDataStream& device, Database* db)
        variantBuffer.open(QBuffer::ReadOnly);
        QVariantMap data = readVariantMap(&variantBuffer);
        db->setPublicCustomData(data);
+
+        auto it = data.constFind(AUTHENTICATION_FACTORS_HEADER_KEY);


This is the second change to the existing database reading code: read, validate, and store authentication factor info from one member of the outer header.

BryanJacobs · 2024-02-16T07:26:12Z

tests/TestKdbx4.cpp

+
+    auto key = QSharedPointer<CompositeKey>::create();
+    auto passwordKey = QSharedPointer<PasswordKey>::create();
+    passwordKey->setPassword(QByteArray::fromStdString("somepassword"));


This KDBX file created with keepasspy doesn't actually use SHA256("somepassword") as its key. It uses a random key stored encrypted in the header, and SHA256("somepassword") can decrypt that key part.

In other words, this test shows the whole flow working end-to-end.

src/format/multifactor/FactorKeyDerivation.cpp

src/format/KdbxXmlAuthenticationFactorReader.cpp

droidmonkey · 2024-02-16T09:39:39Z

Oh this is fun! Will comb through this soon.

Cyanogenbot · 2024-02-16T21:18:09Z

I am willing to provide my design skills for some of the UI parts, although I'm definitely not a king at programming!

droidmonkey · 2024-02-20T22:35:10Z

src/format/multifactor/FactorKeyDerivation.h

+    explicit FactorKeyDerivation() = default;
+    ~FactorKeyDerivation() override = default;
+
+    virtual bool derive(QByteArray& data, const QByteArray& key, const QByteArray& salt);


This should be a purely virtual function (= 0)

droidmonkey · 2024-02-20T22:36:38Z

src/crypto/SymmetricCipher.cpp

@@ -197,6 +197,8 @@ QString SymmetricCipher::modeToString(const Mode mode)
        return QStringLiteral("AES-128/CBC");
    case Aes256_CBC:
        return QStringLiteral("AES-256/CBC");
+    case Aes256_CBC_UNPADDED:


You shouldn't need to introduce a new algorithm. If padding is not required (in this case a 32-byte aligned block) then padding won't be added.

BryanJacobs · 2024-02-21T04:27:06Z

I've added a commit to attempt to address build failures. I think your test coverage agent is running a different compiler (likely gcc) than I am (clang). It appears there are some minor differences in how the QT libraries work between the two.

If it fails again please forgive me - on my local machine, make coverage passes fine.

BryanJacobs · 2024-02-21T05:06:01Z

I believe the MacOS build failure is test flake, rather than a problem with this PR.

04:50:16 
  process_add_identity: parse: bignum is negative

04:50:16 
  FAIL!  : TestSSHAgent::testKeyGenRSA() 'agent.addIdentity(key, settings, m_uuid)' returned FALSE. ()

04:50:16 
     Loc: [/Users/KPXC/buildAgent/work/c401303cba1b4098/tests/TestSSHAgent.cpp(242)]

I don't have permission to rerun it, but other than that the automated checks now pass.

kgraefe · 2024-02-21T07:04:59Z

I believe the MacOS build failure is test flake, rather than a problem with this PR.
04:50:16 
  process_add_identity: parse: bignum is negative

04:50:16 
  FAIL!  : TestSSHAgent::testKeyGenRSA() 'agent.addIdentity(key, settings, m_uuid)' returned FALSE. ()

04:50:16 
     Loc: [/Users/KPXC/buildAgent/work/c401303cba1b4098/tests/TestSSHAgent.cpp(242)]
I don't have permission to rerun it, but other than that the automated checks now pass.

I just stumbled over that myself: #10320

BryanJacobs · 2024-02-21T07:07:25Z

I just stumbled over that myself: #10320

There is another piece of flake in TestPassphraseGenerator: sometimes the title-case word generator doesn't produce a string matching the test regex. Overall the tests seem reliable to me though.

BryanJacobs · 2024-02-27T04:37:31Z

@droidmonkey I think I've addressed all the feedback. I also checked that botan uses PKCS7 padding. PKCS7, when you need N bytes of padding, puts the number N N times.

So a 15-byte message gets 0x01 added to it, a sixteen-byte message gets 0x10 (binary 16) 16 times, and a seventeen-byte message gets 0x0F (binary 15) 15 times. There is always at least one byte of padding, so we either need an unpadded mode here or to use some well-defined padding in the header (for, to my mind, no particular benefit and a file size and complexity increase).

Are you waiting for something else from me here? If this direction is okay I can proceed to implement the saving and FIDO2 parts of the change.

droidmonkey · 2024-03-02T04:41:44Z

I'm good, will review as I can

# Conflicts: # share/translations/keepassxc_en.ts # src/CMakeLists.txt # src/core/Database.h

BryanJacobs added 2 commits February 16, 2024 18:08

Add parsing of authentication factor headers

a9f4c0f

This adds the ability to parse and validate (but not use) authentication factor information contained within the KDBX outer header.

Use authentication factor headers if present for opening database

b9b7054

Saving the database will still discard the header information, but read-only access works.

BryanJacobs commented Feb 16, 2024

View reviewed changes

github-advanced-security bot found potential problems Feb 16, 2024

View reviewed changes

src/format/multifactor/FactorKeyDerivation.cpp Fixed Show fixed Hide fixed

src/format/KdbxXmlAuthenticationFactorReader.cpp Fixed Show fixed Hide fixed

droidmonkey reviewed Feb 20, 2024

View reviewed changes

Code review and build feedback

bb185e3

BryanJacobs added 3 commits February 21, 2024 15:33

Ubuntu has an older QtXml library

b8068b5

More minor compiler differences

c01b8ed

Figuring out something that will work for both clang and gcc

6232810

Evidlo mentioned this pull request Feb 29, 2024

Implement framework for flexible 2FA libkeepass/pykeepass#373

Closed

droidmonkey added this to the v2.8.0 milestone Mar 31, 2024

droidmonkey added security refactor labels Mar 31, 2024

BryanJacobs and others added 2 commits April 16, 2024 12:44

Merge remote-tracking branch 'origin/develop' into develop

dd5f489

# Conflicts: # share/translations/keepassxc_en.ts # src/CMakeLists.txt # src/core/Database.h

Merge branch 'develop' into develop

ba62a3e

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Groundwork to support flexible multifactor database authentication and FIDO2 #10311

Groundwork to support flexible multifactor database authentication and FIDO2 #10311

BryanJacobs commented Feb 16, 2024

BryanJacobs commented Feb 16, 2024

BryanJacobs Feb 16, 2024

droidmonkey Feb 20, 2024

BryanJacobs Feb 21, 2024

droidmonkey Feb 21, 2024

BryanJacobs Feb 16, 2024

BryanJacobs Feb 16, 2024

BryanJacobs Feb 16, 2024

droidmonkey commented Feb 16, 2024

Cyanogenbot commented Feb 16, 2024

droidmonkey Feb 20, 2024

droidmonkey Feb 20, 2024

BryanJacobs commented Feb 21, 2024 •

edited

BryanJacobs commented Feb 21, 2024

kgraefe commented Feb 21, 2024

BryanJacobs commented Feb 21, 2024

BryanJacobs commented Feb 27, 2024 •

edited

droidmonkey commented Mar 2, 2024

Groundwork to support flexible multifactor database authentication and FIDO2 #10311

Are you sure you want to change the base?

Groundwork to support flexible multifactor database authentication and FIDO2 #10311

Conversation

BryanJacobs commented Feb 16, 2024

Screenshots

Testing strategy

Type of change

Description

What is Done

What is Not Yet Done

BryanJacobs commented Feb 16, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

droidmonkey commented Feb 16, 2024

Cyanogenbot commented Feb 16, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BryanJacobs commented Feb 21, 2024 • edited

BryanJacobs commented Feb 21, 2024

kgraefe commented Feb 21, 2024

BryanJacobs commented Feb 21, 2024

BryanJacobs commented Feb 27, 2024 • edited

droidmonkey commented Mar 2, 2024

BryanJacobs commented Feb 21, 2024 •

edited

BryanJacobs commented Feb 27, 2024 •

edited