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 (Quick Unlock) #7384

Merged
merged 2 commits into from
Feb 22, 2022

Conversation

droidmonkey
Copy link
Member

@droidmonkey droidmonkey commented Feb 6, 2022

  • Special thanks to @HexF and @smlu for their contributions towards this feature.

  • Add MVP support for Windows Hello as a Quick Unlock solution using the WinRT API. This works by signing a random challenge vector with the Windows Hello protected key store (typically from TPM). The signed challenge is hashed using SHA-256 and then used as the encryption key to encrypt the database credentials. This ensures the database password can only be decrypted following a successful authentication with Windows Hello in the future.

  • Unify Touch ID and Windows Hello behavior under the Quick Unlock branding. Remove all timeout features of Touch ID as they are unnecessary and complicate the feature for no security gain.

  • Quick Unlock is now reset only when the database key is changed vice whenever database settings are modified.

  • Don't set database unlock dialog as always on top. This allows Touch ID and Windows Hello prompts to appear above the dialog properly.

Screenshots

image

image

image

Example in unlock dialog:
image

Example when cancel / fail operation:
image

Testing strategy

Tested on Windows and macOS (for regression).

Type of change

  • ✅ New feature (change that adds functionality)

@codecov-commenter
Copy link

codecov-commenter commented Feb 6, 2022

Codecov Report

Attention: Patch coverage is 57.33945% with 93 lines in your changes missing coverage. Please review.

Project coverage is 64.30%. Comparing base (acce1bc) to head (579d9c7).
Report is 517 commits behind head on develop.

Files with missing lines Patch % Lines
src/gui/DatabaseOpenWidget.cpp 60.00% 36 Missing ⚠️
src/keys/FileKey.cpp 13.64% 19 Missing ⚠️
src/keys/ChallengeResponseKey.cpp 0.00% 15 Missing ⚠️
src/keys/CompositeKey.cpp 69.39% 15 Missing ⚠️
src/crypto/SymmetricCipher.cpp 57.14% 3 Missing ⚠️
src/gui/Icons.cpp 60.00% 2 Missing ⚠️
src/core/Database.cpp 83.33% 1 Missing ⚠️
src/gui/ApplicationSettingsWidget.cpp 66.67% 1 Missing ⚠️
src/keys/PasswordKey.cpp 95.00% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #7384      +/-   ##
===========================================
- Coverage    64.33%   64.30%   -0.03%     
===========================================
  Files          339      339              
  Lines        43215    43364     +149     
===========================================
+ Hits         27800    27881      +81     
- Misses       15415    15483      +68     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.


🚨 Try these New Features:

Copy link
Contributor

@smlu smlu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Few notes:

  • I suggest unifying procedures which generate the signature of challenge and derivative the encryption key from the signature into single function.

  • Implementation assumes the signature is not randomized (RSA PCKS#1 v1.5/2.0) i.e. deterministic which is the case at the moment. This can change in the future implementations of WindowsHello.

src/winhello/WindowsHello.cpp Outdated Show resolved Hide resolved
@smlu
Copy link
Contributor

smlu commented Feb 7, 2022

This WindowsHello implementation doesn't permanently store the encrypted master key as #6029, is there any future plan to support this?

@droidmonkey droidmonkey force-pushed the feature/windows-hello branch from 39b44f3 to 6859fd4 Compare February 8, 2022 03:56
@droidmonkey
Copy link
Member Author

droidmonkey commented Feb 8, 2022

@HexF I incorporated your WIndows Hello implementation, and it works 100% (I had to make one tweak, don't use QByteArray::fromRawData since that requires the source data to persist). Thank you so much for your work and contributing RIGHT ON TIME. You have been appropriately attributed for your contribution.

Another benefit to your implementation is that it does not require a Windows Hello authentication on initial unlock. I think this is because NCryptEncrypt is using DPAPI in the backend. Canceling the Windows Hello prompt disallows authentication and resets the stored key.

@smlu
Copy link
Contributor

smlu commented Feb 9, 2022

Another benefit to your implementation is that it does not require a Windows Hello authentication on initial unlock. I think this is because NCryptEncrypt is using DPAPI in the backend. Canceling the Windows Hello prompt disallows authentication and resets the stored key.

The reason for not getting WH dialog is probably because the existing WH key is used.
For example the #6029 creates new WH key which requires authentication.
NCryptEncrypt in the backed still uses MS passport api.

@smlu
Copy link
Contributor

smlu commented Feb 9, 2022

Since this PR now uses Win32 API and MS passport API consider handling some general NTE and TPM erros when encrypting/decrypting master key. For example see this and this.

share/translations/keepassxc_en.ts Outdated Show resolved Hide resolved
src/CMakeLists.txt Outdated Show resolved Hide resolved
src/gui/DatabaseOpenWidget.cpp Outdated Show resolved Hide resolved
src/gui/DatabaseOpenWidget.cpp Outdated Show resolved Hide resolved
src/gui/DatabaseOpenWidget.cpp Outdated Show resolved Hide resolved
src/winhello/WindowsHello.cpp Outdated Show resolved Hide resolved
src/winhello/WindowsHello.cpp Outdated Show resolved Hide resolved
src/winhello/WindowsHello.h Outdated Show resolved Hide resolved
src/winhello/WindowsHello.h Outdated Show resolved Hide resolved
src/winhello/WindowsHello.h Outdated Show resolved Hide resolved
src/CMakeLists.txt Outdated Show resolved Hide resolved
src/CMakeLists.txt Show resolved Hide resolved
@droidmonkey droidmonkey force-pushed the feature/windows-hello branch 3 times, most recently from 2490b23 to e4f5969 Compare February 18, 2022 18:27
@droidmonkey droidmonkey force-pushed the feature/windows-hello branch from e4f5969 to f75bceb Compare February 18, 2022 20:21
@HexF
Copy link
Contributor

HexF commented Feb 19, 2022

What was the reasoning for switching back to WinRT APIs?

@droidmonkey
Copy link
Member Author

droidmonkey commented Feb 19, 2022

They are far easier to work with and actually provide a hardware backed (TPM) key. When the proxy is made to obfuscate the quick unlock backend we may re introduce an NCrypt version to support msys builds. But it's really unnecessary.

To be more technical, the chosen implementation with NCrypt worked in that it showed the windows hello authentication prompt. However, in the backend it still relied on an RSA key that was stored in user land via the Microsoft Passport storage mechanism. This means that any application could read that key and use it to decrypt the information held by keepassxc (technically). In other words, we could have done the same thing by just using the vault storage in windows and shown the user consent pop-up prior to accessing it. The consent does not provide the security, it just prevents clicking right through.

With WinRT API you are given a "guarantee" that the private key is stored in the computers TPM chip, if available. Further, that key is only accessible by the application that put it there. The user prompt prevents all access to the mechanism with which to sign the challenge and there is no way to backdoor that.

Additionally, for @HexF implementation, you used the first key available in the store and if one wasn't available just failed silently. This creates two problems: (1) If there are no keys then the feature never works, instead of creating a key for the feature; (2) if a new key is added while an active quick unlock is stored, you will falsely use that key instead of the original one and fail.

@droidmonkey droidmonkey force-pushed the feature/windows-hello branch 2 times, most recently from b54ac19 to 5354634 Compare February 19, 2022 21:00
@droidmonkey droidmonkey force-pushed the feature/windows-hello branch 2 times, most recently from 3630b85 to 605530f Compare February 20, 2022 22:09
@smlu
Copy link
Contributor

smlu commented Feb 21, 2022

With WinRT API you are given a "guarantee" that the private key is stored in the computers TPM chip, if available.

This should hold true also for NCrypt case when MS passport storage is used since both (NCrypt & WinRT) use MS passport for backend.

@droidmonkey
Copy link
Member Author

droidmonkey commented Feb 21, 2022

I'm sorry you are absolutely correct. I need to do some more thinking. I personally don't like that you need to auth with winhello on unlock, this is directly caused by using the key to sign data. The WinRT API misses a crucial ability to use the KeyCredential as a CryptographicKey which would be perfect. As far as I have tried, there is no way to pull a Certificate that would allow you to load from the PersistedKeyProvider.

@droidmonkey droidmonkey force-pushed the feature/windows-hello branch from 605530f to 5ec9efb Compare February 21, 2022 15:30
@droidmonkey droidmonkey force-pushed the feature/windows-hello branch from 9595883 to 7c52cfb Compare February 22, 2022 01:40
* Special thanks to @HexF and @smlu for their contributions towards this feature.

* Add MVP support for Windows Hello as a Quick Unlock solution using the WinRT API. This works by signing a random challenge vector with the Windows Hello protected key store (typically from TPM). The signed challenge is hashed using SHA-256 and then used as the encryption key to encrypt the database credentials. Credentials are encrypted using AES-256/GCM. This ensures the database password can only be decrypted following a successful authentication with Windows Hello in the future.

* Unify Touch ID and Windows Hello behavior under the Quick Unlock branding. Remove all timeout features of Touch ID as they are unnecessary and complicate the feature for no security gain.

* Quick Unlock is automatically reset only when the database key is changed vice whenever database settings are modified.

* Don't set database unlock dialog as always on top. This allows Touch ID and Windows Hello prompts to appear above the dialog properly.

* Prevent quick unlock when using AutoOpen or opening from the command line.
@droidmonkey droidmonkey force-pushed the feature/windows-hello branch from 7c52cfb to 579d9c7 Compare February 22, 2022 01:43
@droidmonkey
Copy link
Member Author

@phoerious made final changes as discussed

return false;
}

// Encrypt the data using AES-256-CBC
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe update the comment.

@droidmonkey droidmonkey merged commit 4f07103 into develop Feb 22, 2022
@droidmonkey droidmonkey deleted the feature/windows-hello branch February 22, 2022 22:53
@phoerious phoerious added pr: new feature Pull request that adds a new feature and removed new feature labels Nov 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature: QuickUnlock pr: new feature Pull request that adds a new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants