Allow websites to require WebAuthn discoverable credentials, a.k.a. resident keys, a.k.a. passkeys

Ortham commented 8 months ago

Summary

Hi, I've been trying out the passkey support that was added late last year, and found that when using the latest snapshot build of KeePassXC (version info below) together with KeePassXC-Browser v1.8.12 in Firefox v122, KeePassXC will refuse to create a passkey when a website requires a resident key, but if the site only prefers a resident key, it will happily create one.

KeePassXC - Version 2.8.0-snapshot
Build Type: Snapshot
Revision: a472ef8

Could someone clarify why this behaviour exists and/or remove what seems to be an unnecessary restriction?

Examples

If I go to https://webauthn.io/, enter a username and choose Discoverable Credential: Required like this:

and then click the Register button, I get an error from KeePassXC-Browser that seems to come from KeepassXC, like this:

However, if I set Discoverable Credential to Preferred then KeePassXC's Passkey credentials dialog is displayed (though I can't seem to take a screenshot of it) and I can successfully create a passkey. I can then view the created passkey in KeePassXC and use it to authenticate without supplying a username, which proves the credential's discoverability. Here's what the site displays for the passkey after authentication:

Context

Passkeys are Discoverable Credentials by definition so I don't understand why KeePassXC can create passkeys but then also claim to not support them.

I did see Resident Keys referenced in #1870 in this comment and this comment and then later in #10050 in this comment: it seems like there's some confusion that Resident Keys are something different?

I did consider that maybe what had been implemented was support for WebAuthn credentials that were not Discoverable Credentials and therefore not actually passkeys, but the credential that KeePassXC creates certainly seems to meet the relevant criteria.

(I do have questions about the implementation more generally, like why do there seem to be separate KPEX_PASSKEY_GENERATED_USER_ID and KPEX_PASSKEY_USER_HANDLE fields when they're the same thing in the WebAuthn spec, but that's out of scope for this issue and I haven't tried reading through the code to see if that explains why.)

droidmonkey commented 8 months ago

Oops, forgot to revert that commit on develop branch. We made a mistake reading the docs. We did not include that commit on the 2.7.7 release branch.

varjolintu commented 8 months ago

I'm making a PR soon that fixes this for the develop branch.

Ortham commented 8 months ago

I've started to read through the implementation, and I'm very concerned that it appears to be noncompliant with the WebAuthn Level 1 spec in ways that I believe could cause security issues. A couple of quick examples that I spotted with a quick scan through the code:

It looks like domain and origin validation are done incorrectly, and that logic is part of how WebAuthn provides its anti-phishing protections, so users could be given a false sense of security if there is a way to take advantage of the issues as part of an attack
It looks like KeePassXC lies to Relying Parties about user verification, as it only performs a user presence test but claims that it has performed user verification so long as the RP did not explicitly discourage it. This downgrades 2FA to 1FA without the RP's knowledge.

So far I've only gotten through what's supposed to be the client's part of createCredential and ~ 16 of the 21 steps are implemented incorrectly - that's roughly, because a couple of steps are like "initialise this variable" so not a big deal themselves, and the last few steps contain a lot of branching logic, and I think some of it can be simplified for a single-authenticator client like the browser extension. I haven't really gone through the code in this repo properly yet, mostly the browser extension code so far, except where I've jumped ahead in the logic to check if a step was moved to later happen in KeePassXC itself.

I'm planning to continue going through the spec and comparing it against the current implementation to build up a list of issues, but I don't want to be checking against the wrong code, and I'd like to try improving the situation rather than just filling a big bug report and leaving it.

What branch should I be checking against, develop, or the 2.7.7 release branch?
How would you like me to proceed? Should I file a single issue about the implementation once I'm done reading through the code, so that we can discuss what I find and next steps, or would you prefer a more granular approach? While I don't want to start opening PRs without any discussion first, I am motivated to make changes myself, and I've got experience with C++, Qt and JS. Saying that, I've never looked at this codebase before or done any browser extension development, so I might need some guidance with those aspects.

I don't know the timeline for 2.7.7's release, but given what I've seen so far I'd be very hesitant to put the current implementation in front of users myself. I know a lot of people are excited for this functionality to be released (including myself!), but is there the option to back it out of that release, or put it behind an experimental / off-by-default config flag?

I know this has been quite a negative comment and I don't want to dump on other people's freely-given hard work, but I believe there are significant issues and that it's better to raise and help address them than to move on silently. I'd just like to finish by saying thanks for the work that's been done so far on passkey support, it looks like it's taken a fair bit of effort and I do appreciate it. 👍

droidmonkey commented 8 months ago

@Ortham your claims are probably valid, but they are also rather exceptional. I would initially push back to say... if it is "that easy to get it so wrong" then the spec is utterly broken, and I fear that no one can get it right. It is unclear how we could be so far off the mark, yet our implementation works fine on every website we have tried it on (except BestBuy.com for some reason).

Please do create a new issue to dump all your findings into, then we can discuss. Do your investigation off the develop branch. Focus on these areas of the code base (PassKeys are relatively isolated from all other code):

https://github.com/keepassxreboot/keepassxc/blob/develop/src/browser/BrowserPasskeys.cpp

https://github.com/keepassxreboot/keepassxc/blob/a472ef8a931dd64990d7f735394e949638e880d5/src/browser/BrowserService.cpp#L614

https://github.com/keepassxreboot/keepassxc/blob/a472ef8a931dd64990d7f735394e949638e880d5/src/browser/BrowserService.cpp#L767

Ortham commented 8 months ago

@Ortham your claims are probably valid, but they are also rather exceptional. I would initially push back to say... if it is "that easy to get it so wrong" then the spec is utterly broken, and I fear that no one can get it right.

FWIW, I'm no security expert, but I've read opinions from them over the years bemoaning the complexity of security-related standards like JWT, OAuth and TLS, saying that their complexity makes it far too easy to make a critical mistake, or that they allow implementors to make insecure choices, so part of me wouldn't be surprised...

I've been implementing an RP to learn that side of things and while it seems simpler, there's still a fair number of checks to perform, and I've had to go back and fix a few I missed even after I thought I'd covered everything.

It is unclear how we could be so far off the mark, yet our implementation works fine on every website we have tried it on (except BestBuy.com for some reason).

From what I've seen so far, the happy path provides the necessary core functionality and there is some validation covering unhappy paths, so it works with sites that correctly act as trustworthy RPs. However, I think the current implementation may have weaknesses that could be exploited if an RP were to be untrustworthy, misleading or send junk.

I'll finish going through the code and file an issue so we can go over what I find.

keepassxreboot / keepassxc