Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for Windows Hello #6029

Closed
wants to merge 1 commit into from

Conversation

smlu
Copy link
Contributor

@smlu smlu commented Jan 30, 2021

This is draft PR which adds support to store and retrieve KeePassXC database password key using Windows Credential API and Microsoft Passport Api. This feature can be used if user has set Windows Hello PIN or biometrics to unlock their account.
In essence, CNG creates RSA 2048 bit encryption key via Microsoft Passport storage provider and "locks" it on Windows Hello authenticator when key is used for decryption. Note, windows store this key either in TPM if available or is SW protected via system. This key is then used to encrypt database key, and encrypted database key is stored on windows using Credential API. The back-end was inspired by KeePassWinHello, thought temporary storage is not implemented.

On the code side, there are 3 parts:

  • back-end: WinHelloKeyManager class, which is responsible for storing, authenticating and retrieving the database key.
    Class tries to mimics a bit C# KeyCredentialManager class
  • middle-end: WindowsHello class. This class acts as a "glue" code between KeePassXC code base and WinHelloKeyManager. It interacts with user and handles errors, such as error when user canceled operation or when stored key can't be decrypted for various reasons i.e. disconnected TPM chip. Class is currently not implemented in a singleton patter fashion because it needs parent widget pointer to pin GUI/Windows Hello prompt window to the app.
  • front-end: currently being implemented only in KeePassXC GUI code base. The forntend is a draft at the moment and should be considered only for demonstration purpose. Note: the GUI was influenced by implementation of Windows Hello in Bitwarden .

To compile this PR it's suggested to build it on top of #5874, due to C++17 requirements (std::wstring_view).

What is still considered to be done:

  • implement storage of a password key for 1 password vault and KeePass 1 database.
    At the moment only storing key for KeePass 2 is implemented.
  • implement cli/app logic to reset windows hello storage through command line.
    When KeePassXC is uninstalled it should also delete all stored encrypted database keys and encryption key.
    For now you can only reset Windows Hello storage in app via Settings->Security.
  • implement DB migration logic, so the stored key will be still valid when DB moves to the new location
  • common interface for system key storage for different platforms.
  • Windows Hello authentication for KeePassXC-cli
  • consider if WindowsHello class should be implemented as singleton

Disclaimer: If there is any bounty to be earned by this PR, it should go to the maintainers of this project!

Screenshots

img
img
img

Example of WindowsHello class handling storage error:
img

Type of change

  • ✅ New feature (change that adds functionality)

#2462 #3337

@smlu
Copy link
Contributor Author

smlu commented Jan 30, 2021

Example of how common system key storage interface could look like:

class OSKeychain
{
   static bool isAvailable();
   static bool isPersistentStorageAvailable();
   static const QString& name();

   bool storeKey(QStringView dbPath, const QByteArray& key);
   QSharedPointer<QByteArray> getKey(QStringView dbPath) const;
   void removeKey(QStringView dbPath);
   bool reset() const;
};

And used in the code:

DatabaseOpenWidget::DatabaseOpenWidget(QWidget* parent)
{
    ...
    if (OSKeychain::isPersistentStorageAvailable()) {
        m_ui->buttonOsUnlock->setText(tr("Unlock with %1").arg(OSKeychain::Name()));
    }
    else if(OSKeychain::isAvailable()) {
        m_ui->checkTmpOsUnlock->setText(tr("%1 for quick unlock").arg(OSKeychain::Name()))
        m_ui->checkTmpOsUnlock->setVisible(true);
    }
    ...
}

DatabaseOpenWidget::buildDatabaseKey(bool useOsStoredKey)
{
    ...
    if (useOsStoredKey
        && OSKeychain::isAvailable()
        && OSKeychain::containsKey(m_filename)) {
        databaseKey->clear();
        QSharedPointer<QByteArray> passwordKey = OSKeychain(this).getKey(m_filename);
        if (passwordKey.isNull()) { // If null, either user cancelled or an error has occurred
            m_ui->buttonOsUnlock->setVisible(OSKeychain::containsKey(m_filename));
            return nullptr;
        }
        databaseKey->addKey(PasswordKey::fromRawKey(*passwordKey));
    }
    else {
        // use password provided by user
    }
    ...
}

The backend of the interface could be implemented via pimpl patter or singleton pattern for each platform, depending on what's needed at runtime.

@phoerious phoerious marked this pull request as draft January 31, 2021 19:48
@musm
Copy link

musm commented Feb 20, 2021

I just wanted to chime and thank you for your work on supporting Windows Hello. Can't wait to use this.

@fritut08
Copy link

What is the status on this?

@smlu
Copy link
Contributor Author

smlu commented Aug 9, 2021

@fritut08 Unfortunately due to lack of time on my end the progress of this PR has stalled for some time now. I'm still planning to finish it though.
For anyone interesting in this PR I've been testing it in production for the last couple of months and the underlying implementation works as intended. Until now, I had 0 issues, had never lost stored master key, and I was always able to decrypt the stored passwords in KPXC through Windows Hello.

Note: I'm also waiting for the #5874 to be pulled in, to rebase this PR on it. And, some further development on the platform based common key storage interface as discussed in this thread: #3337 (comment). Especially, getting something like this in: #3337 (comment), wold save this PR some troubles on how to retrieve stored keys.

@sylph520
Copy link

Thank you for the work. Looking forward to this features 👍

@droidmonkey
Copy link
Member

This works but is clunky to setup. I am debating doing the full "Quick Unlock" overhaul first and them incorporate the essence of this PR into that one.

@vampyren
Copy link

Just wondering if this will also include faceID within Windows Hello? Right now i unlock my pc + another password manger using faceid through windows hello.

Any lastly this is the only missing piece for me to migrate to KeePassXC. Keep the great work guys.

@droidmonkey
Copy link
Member

Windows Hello will accept Face or Fingerprint if you have the capability. That is on Windows side, not us.

@vampyren
Copy link

Windows Hello will accept Face or Fingerprint if you have the capability. That is on Windows side, not us.

Thank you for the answer.

Exactly! That is what i was assuming, as long as its set under windows Hello i assume it should work. But the windows look different compared to touchid....maybe there are other messages the app need to catch and act on.
My other password manager worked fine with touch but had issue with camera at first but they fix it after some time....

By the way if you want tester i can help ;)

@droidmonkey
Copy link
Member

droidmonkey commented Jan 10, 2022

I want to merge this prior to the 2.7.0 release... I don't understand the layers in the credential storage. From what I gather from the code, you use the windows credential store to store an ncrypt encrypted RSA key that encrypts the password data. Is the ncrypt part necessary for windows hello or is the credential manager enough? This solution seems like overkill to store a simple pass word and retrieve it using windows hello.

@JohnLGalt
Copy link

I want to merge this prior to the 2.7.0 release... I don't understand the layers in the credential storage. From what I gather from the code, you use the windows credential store to store an ncrypt encrypted RSA key that encrypts the password data. Is the ncrypt part necessary for windows hello or is the credential manager enough? This solution seems like overkill to store a simple pass word and retrieve it using windows hello.

I'm always running the latest 2.7.0 pre-release when I see a new one out, and I have a Logitech Brio that I've set up with Windows Hello, so I can help test as needed with facial recog via Windows Hello.

@vampyren
Copy link

I'm always running the latest 2.7.0 pre-release when I see a new one out, and I have a Logitech Brio that I've set up with Windows Hello, so I can help test as needed with facial recog via Windows Hello.

Where do you get this build? cant find under tags!
I can also in that case download and use + test it + report if i find bugs.

@JohnLGalt
Copy link

JohnLGalt commented Jan 10, 2022

I'm always running the latest 2.7.0 pre-release when I see a new one out, and I have a Logitech Brio that I've set up with Windows Hello, so I can help test as needed with facial recog via Windows Hello.

Where do you get this build? cant find under tags! I can also in that case download and use + test it + report if i find bugs.

https://keepassxc.org/download/

Scroll down to the Pre-Releases and Snapshots section.

@smlu
Copy link
Contributor Author

smlu commented Jan 11, 2022

@droidmonkey from my testing (a year ago), I couldn't get Win32 Credential API to prompt for additional authentication (WindowsHello authn) before stored credential is returned to the program which invoked CredRead. (i.e.: CRED_TYPE_DOMAIN_PASSWORD & CRED_TYPE_GENERIC plain credential blob with no authentication, and cred stored as CRED_TYPE_DOMAIN_VISIBLE_PASSWORD can be read only by authentication packages).

This is the reason why the NCrypt (hack) is used here. I.e.: NCrypt generates MS Passport persistent encryption key (stored by the system e.g. TPM) which is used to encrypt master password, and on decryption user WindowsHello authn is required by the system to use this key. Encrypted master password is stored in windows credential store.

Note, that the windows credential store is used just for convenience to mimic the KeyCredentialManager (apple keychain) but it's not necessary since it doesn't provide any extra layer of security. In practice, KPXC could also use user's local storage to store the encrypted password.

@droidmonkey
Copy link
Member

Ok so I did read the code correctly cause it looked like a workaround. There is a direct implementation of windows hello using the api that requires use of the shared headers between .net/c++. I certainly prefer that route, but this hack does work now to provide the same experience to the end user.

@smlu
Copy link
Contributor Author

smlu commented Jan 12, 2022

Yes, since building with msvc is now supported KeyCredentialManager could be used instead. Though I don't know what would that mean for Qt framework because WinRT will be required for this. And it won't work with MinGW compiler.

Btw I forgot to mention that I haven't tested if persistent MS Passport key is only available to the current user session or to any session. The KeePassWinHello plugin exploits undocumented MS Passport function NgcGetDefaultDecryptionKeyName to get local persistent key of the current session. I guess, using local persistent key instead of custom key would ease the logic to make sure the key is deleted after KPXC is removed from the system.

@droidmonkey
Copy link
Member

I did some research on the key credential manager solution and it looks very easy and promising once you get the dependencies squared away. We are permanently moving away from mingw.

@smlu
Copy link
Contributor Author

smlu commented Jan 12, 2022

I guess in that case, it'd make sense to explore first what are the options with KeyCredentialManager before proceeding with this PR.

@droidmonkey
Copy link
Member

Good news, I got the KeyCredentialManager functions to work and am now working to replace everything with those.

@droidmonkey
Copy link
Member

droidmonkey commented Jan 16, 2022

I have a full working quick unlock using the UWP API. I am closing this PR and will open a new one with that code once I polish it up. Took about 50 lines of code, and A LOT of research 😄

@smlu
Copy link
Contributor Author

smlu commented Jan 17, 2022

That's awesome news! Getting native WinHello support will be a lot easier to maintain than having custom code. I'll dive into the code once you make the new PR.

@droidmonkey
Copy link
Member

I unified the experience between Windows Hello and TouchID and rebranded both under "Quick Unlock". The code is still in flux but here is a visual preview of initial login and then subsequent unlocking.

image

image

One major improvement from the TouchID experience is to disable the password field and give a hint to the user that they are using quick unlock. I also rename the Unlock button to "Quick Unlock" to reinforce that behavior.

@musm
Copy link

musm commented Jan 18, 2022

Might I suggest sticking to the native windows hello interface instead:

image

Here's a screenshot from KeePass. I think sticking to the native UI would be better than rolling custom UI. Whenever you want to unlock KeePass, the native Windows Hello dialog pops up, which seamlessly integrates with the rest of the Windows 10/11 UI.

@droidmonkey
Copy link
Member

droidmonkey commented Jan 18, 2022

Yes that dialog pops up when you need to authenticate. I didn't screenshot it... before you get that dialog you need to tell us you actually want to use windows hello, and be told that windows hello is configured.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants