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.

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

BryanJacobs · 2024-07-26T22:59:46Z

My "make format" seems to be doing something different from the CI formatting run.

At any rate, this is still ready for review whenever.

droidmonkey · 2024-07-27T13:19:34Z

Yah cmake formatting was recently updated and we have a PR to align to the new version

Arbel-arad · 2024-08-26T23:25:37Z

hi, is there any status update?

BryanJacobs · 2024-08-27T09:11:03Z

hi, is there any status update?

This code is waiting for review by a project maintainer.

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 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

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 pr: refactoring Pull request that refactors code labels Mar 31, 2024

droidmonkey mentioned this pull request Oct 1, 2024

Support Trezor hardware key #9400

Open

droidmonkey force-pushed the develop branch from 99dc514 to 00654c2 Compare October 1, 2024 22:07

droidmonkey mentioned this pull request Oct 22, 2024

WIP: Qt6 transition #10267

Draft

14 tasks

This comment has been minimized.

Sign in to view

droidmonkey force-pushed the develop branch from 00654c2 to 8a39859 Compare December 1, 2024 20:17

droidmonkey mentioned this pull request Dec 21, 2024

Support unlocking the same database in multiple ways #11582

Open

1 task

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 •

edited by droidmonkey

Loading

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

Loading

BryanJacobs commented Feb 21, 2024

kgraefe commented Feb 21, 2024

BryanJacobs commented Feb 21, 2024

BryanJacobs commented Feb 27, 2024 •

edited

Loading

droidmonkey commented Mar 2, 2024

BryanJacobs commented Jul 26, 2024

droidmonkey commented Jul 27, 2024

Arbel-arad commented Aug 26, 2024

BryanJacobs commented Aug 27, 2024

This comment has been minimized.

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 • edited by droidmonkey Loading

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 Loading

BryanJacobs commented Feb 21, 2024

kgraefe commented Feb 21, 2024

BryanJacobs commented Feb 21, 2024

BryanJacobs commented Feb 27, 2024 • edited Loading

droidmonkey commented Mar 2, 2024

BryanJacobs commented Jul 26, 2024

droidmonkey commented Jul 27, 2024

Arbel-arad commented Aug 26, 2024

BryanJacobs commented Aug 27, 2024

This comment has been minimized.

BryanJacobs commented Feb 16, 2024 •

edited by droidmonkey

Loading

BryanJacobs commented Feb 21, 2024 •

edited

Loading

BryanJacobs commented Feb 27, 2024 •

edited

Loading