Fix crash when using Windows Hello in a Remote Desktop session #9006
Conversation
|
Dang I didn't put enough in the try/catch block. BTW this behavior is undocumented in the winrt documentation. |
Gibstick
force-pushed
the
fix/windows-hello-rdp
branch
from
b2e83c8
to
8414b14
Compare
|
Yeah no worries, I couldn't find anything about it either. I neglected to consider all platforms in |
Gibstick
force-pushed
the
fix/windows-hello-rdp
branch
from
8414b14
to
8d8fd04
Compare
Codecov ReportBase: 64.62% // Head: 64.41% // Decreases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## develop #9006 +/- ##
===========================================
- Coverage 64.62% 64.41% -0.21%
===========================================
Files 342 341 -1
Lines 44334 44258 -76
===========================================
- Hits 28649 28505 -144
- Misses 15685 15753 +68
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
|
You seem to have taken a very complex route to solve the problem cancel vs error problem. It would have been easier to either emit a "canceled()" signal or an error signal. In fact our current way of handling errors is incorrect and not thread safe. If another thread were to start a Windows Hello interaction then the error may get swallowed up. |
|
Good to know that signals exist, I'll redo it 😅 |
|
May I ask, what will happen after unlocking by fallback method in remote session? Do I still need to reopen keepassxc to re-enable fingerprint unlock in local session? |
droidmonkey
force-pushed
the
fix/windows-hello-rdp
branch
from
95d260a
to
f05a863
Compare
|
I figured out an elegant solution and also used the nice error messages that were going to waste! |
The KeyCredentialManager::RequestCreateAsync call can fail because we can end up in a situation where Windows Hello is initially available but then becomes unavailable, such as during a remote desktop session. This commit prevents a crash by moving the call into the try-catch. Fixes keepassxreboot#7890 Also resets quick unlock if there is an unrecoverable error. This will not occur if the user merely canceled the Windows Hello dialog.
droidmonkey
force-pushed
the
fix/windows-hello-rdp
branch
from
f05a863
to
d422f38
Compare
Release 2.7.5 - Add menu option to allow screenshots [keepassxreboot#8841] - Add support for Botan 3 [keepassxreboot#9388] - Increase max TOTP step to 24 hours [keepassxreboot#9149] - Improve HTML export layout [keepassxreboot#8987] - Turn search reset off by default [keepassxreboot#9153] - Use QClipboard::clear() instead of setting blank text [keepassxreboot#9148] - Hide group column header choice when not in search [keepassxreboot#9171] - Improve look of KeePassXC logo and icons [keepassxreboot#9355] - Add keyboard shortcuts for app and database settings [keepassxreboot#9007] - Hide rename button from attachments preview panel [keepassxreboot#8842] - Linux: Set SingleMainWindow in .desktop file [keepassxreboot#7430] - Fix crash when search clears while creating new entry [keepassxreboot#9230] - Fix crash when using Windows Hello in a Remote Desktop session [keepassxreboot#9006] - Fix crash in Group Edit after enabling Browser Integration [keepassxreboot#8778] - Fix canceling quick unlock when it is unavailable [keepassxreboot#9034] - Set password input field font correctly [keepassxreboot#8732] - Greatly improve performance when rendering entry view [keepassxreboot#9398] - Fix various accessibility issues [keepassxreboot#9138] - Fix arrows size when expand/collapse a group [keepassxreboot#9096] - Select the clone instead of the original after cloning an entry [keepassxreboot#9070] - Fix bugs with preview widget [keepassxreboot#9170] - Fix status bar update when switching to other DB [keepassxreboot#9073] - Fix database settings spin box bug [keepassxreboot#9101] - Fix Ctrl+Tab shortcut to cycle databases in unlock dialog [keepassxreboot#8839] - Fix TOTP QR code maintaining square ratio [keepassxreboot#9027] - Fix Auto-Type configuration page on custom sequence selection [keepassxreboot#8752] - Fix unexpected behavior of `--lock` when KeePassXC is not running [keepassxreboot#8889] - Make open folder icon exempt from "Apply group icon to entry" [keepassxreboot#9205] - Allow setting default file open directory with env var [keepassxreboot#9192] - SSH Agent: Fix support for AES-256/GCM openssh keys [keepassxreboot#8968] - Browser: Fix Native Messaging script path with BSD OS's [keepassxreboot#8835] - MacOS: Fix text selection for Auto-Type clear field [keepassxreboot#9066] - MacOS: Don't rely on AppleInterfaceStyle for theme switching [keepassxreboot#8615] - Windows: Remove registry detection of desktop shortcut [keepassxreboot#9380]
Summary: This change fixes #7890 by catching a previously uncaught exception, and adds fallback behaviour when quick-unlock fails.
Windows Hello can suddenly become unavailable with a permission denied exception. According to #7890 (comment):
Note that this might not be the only way to trigger this. For example, one could disable Windows Hello and probably end up with the same crash.
So far I've been calling this "Windows Hello", but I'm not too familiar with the Windows APIs, so I'm not sure if
KeyCredentialManager.RequestCreateAsync Methodis technically Windows Hello. In any case, it comes up as a crash in the Windows Hello flow. It crashes because of an uncaught exception being returned, and it seems the error is by design because of extra restrictions Microsoft wanted to place on remote desktop sessions.In the second commit, I made a quick attempt to improve the overall experience by falling back to the other unlock methods if quick unlock fails for any reason. This is a bigger change than just preventing the crash, so I'm open to feedback, including simply removing the second commit from this PR if it's contentious. This fallback behaviour I implemented was suggested in the original issue report and I think it's sensible.
Screenshots
Clicking "Unlock Database" in a remote desktop session used to crash, now it doesn't.
Testing strategy
I tested this by using the reproduction steps in the linked issue #7890, using a real RDP session. I first reliably reproduced the crash on an unmodified build, and then verified that my fix stopped the crashes.
Type of change