Security, privacy and other issues with WebAuthn/passkeys support

[Ortham]

Overview

Following on from #10269, I've checked the develop branches of the KeePassXC implementation (the app itself and the browser extension) against parts of the WebAuthn Level 1 spec and it looks like there are a few serious security issues:

• KeePassXC does not perform domain and RP ID validation correctly, which I think cause privacy and security issues:
  • Privacy example: I'm pretty sure that it would be possible for one .github.io site to gather information about what credentials a user has for other .github.io sites.
  • Security example: If bank.com uses passkeys, an attacker could create a phishing site at fakebank.com and use an RP ID of bank.com and I think KeePassXC would provide the user's bank.com assertion, which the attacker could then use to sign into the user's bank account.
  • Fixing this would involve changes to the browser extension to implement the correct logic.
• KeePassXC does not correctly determine when it is required to perform user presence tests
  • This is a security issue, as while it's not obvious, WebAuthn always requires either user presence or user verification to be performed (and user verification implies user presence), so this violates the assumptions that RPs can make about authenticated users.
  • I think this might be chained together with the RP ID validation issue and the user presence requirement being ignored so that an attacker could get KeePassXC to provide a user's bank account assertion without any interaction with or visibility to the user.
  • Fixing this would involve changes in the browser extension.
• KeePassXC does not correctly determine when it is required to perform user verification
  • This is a security issue, as user verification is what makes passkeys capable of 2FA, and knowing when the RP requires user validation to be performed is a prerequisite to performing it when it's needed (unless you always perform it, but KeePassXC doesn't).
  • Fixing this would involve changes in the browser extension.
• KeePassXC ignores any user presence requirement when the RP has supplied allowed credentials and there is only one matching entry. In that case, it does not perform the user presence test but tells the RP that it did.
  • This is a security issue, as I've mentioned above.
  • Fixing this would involve changes in KeePassXC.
• KeePassXC never performs user verification (even if the RP requires it), but if the RP did not explicitly discourage user verification, KeePassXC tells the RP that user verification was performed
  • This is a security issue as it means that while an RP might think that 2FA has been performed, it's actually only been 1FA, and that might not be good enough in relatively high-risk situations (especially given that it looks like an attacker could phish that single factor).
  • Fixing this would involve changes in KeePassXC.
  • One approach might be to update BrowserPasskeysConfirmationDialog to prompt for and validate database unlock credentials when user verification is required, though I don't know if KeePassXC can already do that kind of thing without locking the database (which could be a bit annoying if you're also using it for other things).

If my understanding is correct, then I think it would be dangerous to users to include KeePassXC's passkey support in a stable release in its current state.

I think that there are also some other potential issues or areas for improvement:

• Some operations that should be performed in the client (the browser extension) are being performed in the authenticator (KeePassXC), which might be a security issue if either of them can find themselves talking to untrusted code instead. Otherwise it's not an issue itself, but the discrepancies may hide other issues.
  • Improving on this would involve changes to the payloads sent between the browser extension and KeePassXC, so it would be good to clean those interfaces up before passkey support is released, at which point it becomes a breaking change.
• There are a variety of other discrepancies that I don't think impact privacy or security but may impact UX and compatibility.
  • I think these are relatively low priority and shouldn't impact any decision to release passkey support.

While I checked against the Level 1 spec, I did then compare Level 1 against Level 2 and I don't think the differences have meaningful privacy or security impacts. It would be good to be compliant with Level 2 for compatibility reasons (e.g. AuthenticatorAttestationResponse.getTransports() isn't implemented by the browser extension), but I think initially targeting Level 1 is reasonable. I didn't compare against Level 3.

Finally, it's not spec-related, but the property name KPEX_PASSKEY_GENERATED_USER_ID is misleadingly inaccurate, KPEX_PASSKEY_CREDENTIAL_ID would be more appropriate.

• It's confusing because the WebAuthn spec isn't consistent about the field names for the user handle: sometimes it's userHandle and sometimes it's id, and KeePassXC also has a PKEX_PASSKEY_USER_HANDLE property, so it looks like KeePassXC is storing two different user handles when that's not actually what's happening.

The following is a breakdown of the discrepancies I've noticed while going through the spec and the code.

Detailed breakdown

Create credential

In the client

https://www.w3.org/TR/2019/REC-webauthn-1-20190304/#createCredential

This is supposed to be implemented in the client, so I've been looking through keepassxc-browser and occasionally going into keepassxc to see if some checks were put there instead.

Discrepancies are:

• Step 4 - timeout is not checked, corrected or set to a client-specific default
• Step 5 - caller origin is not checked to see if it's opaque
• Step 6 - Effective domain is not checked
• Step 7 - Effective domain / RP ID check is not performed correctly, the browser plugin only checks - if the current hostname ends with the RP ID
• Step 9 - Unsupported credential types are not filtered out
• Step 10 - Check for presence of only unsupported credential types is not performed
• Step 12 - The extensions list is not checked
• Step 13 - collectedClientData is not built, though something that's supposed to be it is built - much later in BrowserPasskeys::buildClientDataJson()
• Step 14 - clientDataJSON is not created
• Step 15 - clientDataJSON is not hashed
• Step 16 - signal is not checked
• Step 17 - no set of authenticator requests, it's just one authenticator (though there is the - native implementation fallback), so probably not really an issue
• Step 18 - no set of authenticators, but again probably not really an issue
• Step 20
  • If lifetimeTimer expires - no lifetime timer
  • If the user exercises a user agent user-interface option to cancel the process - it doesn't look like the browser extension offers such an option, so this may not be relevant
  • If the options.signal is present and its aborted flag is set to true - signal is not checked
  • If an authenticator becomes available on this client device
    • Step 1 - authenticatorAttachment and requireResidentKey isn't checked. userVerification is checked but not as described
    • Step 2 - userVerification is not set as described
    • Step 3 - userPresence is not set
    • Step 6 - the data sent to the authenticator is not as described
  • If an authenticator ceases to be available on this client device - doesn't seem to be handled, may not be relevant?
  • If any authenticator returns a status indicating that the user cancelled the operation - there is error handling, seems sufficient since there's only one authenticator
  • If any authenticator returns an error status equivalent to "InvalidStateError" - it doesn't look like the exception is raised
  • If any authenticator returns an error status not equivalent to "InvalidStateError" - this is different to the above to avoid leaking information, but the extension doesn't seem to report any such errors anyway
  • If any authenticator indicates success
    • Step 2 - the struct is not created as described, the only inputs come from the authenticator, which is probably technically a security issue (though because the browser extension and KeePassXC have the same authors they can trust each other in practice)
    • Step 3.1 - attestationConveyancePreferenceOption is not checked
    • Step 3.4 - the object created doesn't quite look like a PublicKeyCredential, it's missing id, type and the internal slots aren't represented in any way (aside from [[identifier]], which is covered by rawId)
    • Step 4 - not really relevant as there's only one authenticator
• Step 21 - No exception thrown, seems like information is leaked due to that

Of these issues, I think the effective domain and RP ID checks represent privacy and possibly security issues, and the handling of data returned by the authenticator may be a security issue if it's possible that it's not KeePassXC that the browser extension is talking to. The rest seem to be more about UX and compatibility than security or privacy.

In the authenticator

https://www.w3.org/TR/2019/REC-webauthn-1-20190304/#authenticatormakecredential

These steps are supposed to happen in the authenticator, so I've been looking through KeePassXC's code for them.

Before the procedure starts, the spec notes that:

Before performing this operation, all other operations in progress in the authenticator session MUST be aborted by running the authenticatorCancel operation.

That doesn't seem to happen, which might have security implications (e.g. if a site can coordinate actions to happen at the same time and so get KeePassXC into a confused state where it performs the wrong operations on the wrong data).

• Step 1 - There are a couple of validation checks, but not to the level described. Could be an issue if KeePassXC could be contacted by anything other than the official browser extension.
• Step 2 - this validation isn't performed, instead it looks like if KeePassXC doesn't support one of the algorithms requested then it'll just used ES256, which is theoretically a compatibility issue (I've only seen RS256 and ES256 in the wild) but I don't think it's a security issue.
• Step 3 - The descriptor ID is looked up, but the RP ID and type don't get checked and there is no user consent check done
• Step 4 - Not relevant, KeePassXC can store resident keys
• Step 5 - Not relevant because KeePassXC can perform user verification (but doesn't, though that's a later issue)
• Step 6 - The user consent dialog doesn't display the RP name or user display name, but these only SHOULD be displayed, and they're less important than what does get displayed, so that's fine. However, the dialog does not perform user verification if it is required. This is a security issue as it downgrades 2FA to 1FA without the RP's knowledge, and so weakens the security of the user's online account.
• Step 7.3 - credentialSource is built without the type, but that's always public-key so not really an issue
• Step 7.4 - KeePassXC doesn't have a credentials map in the way that is described, but the functionality seems to be equivalent
• Step 8 - Error handling isn't clear, particularly if generating a key pair fails, as looks like it'll just silently store an empty key?
• Step 11 - The credential ID length is hardcoded to be 16 bytes, which is fine, but it would be better to get this from the credential ID that gets passed into BrowserPasskeys::buildAttestationObject().

The handling of step 3 might be a privacy risk, but I'm not confident about that. However, the user consent dialog in step 6 not performing user verification when required is definitely a security issue. The rest of the discrepancies seem like minor issues.

Get assertion

In the client

https://www.w3.org/TR/2019/REC-webauthn-1-20190304/#getAssertion

Steps 1 to 6 are the same as for passkey create, and have the same discrepancies.

• Step 7 - RP ID is set to the hostname, which isn't necessarily the effective domain, and if already set RP ID is not validated correctly.
• Step 8,9 - there doesn't seem to be any extensions handling, which is fine but it would be good to have a comment or no-op function signalling that it's intentional.
• Step 10 - collectedClientData is not built
• Step 11 - collectedClientData is not serialised as JSON
• Step 12 - clientDataJSON is not hashed
• Step 13 - no signal handling
• Step 16 - not really relevant since there's just one authenticator
• Step 17 - no timer
• Step 18
  • If lifetimeTimer expires - no lifetime timer
  • If the user exercises a user agent user-interface option to cancel the process - it doesn't look like the browser extension offers such an option, so this may not be relevant
  • If the signal is present and its aborted flag is set to true - signal is not checked
  • If issuedRequests is empty, options.allowCredentials is not empty, and no authenticator will become available for any public key credentials therein - doesn't seem to be handled, but I guess that the functionality wouldn't get this far if the connection with KeePassXC was broken?
  • If an authenticator becomes available on this client device
    • Step 1 - userVerification is checked but not as described, though KeePassXC is capable of user verification so this would always pass.
    • Step 2 - userVerification is not set as described
    • Step 3 - userPresence is not set
    • Step 4.1 to 4.5 - The logic for allowCredentials not being empty is handled differently, because no functionality has been built to query KeePassXC for a list of matching credentials.
    • Step 4.6,4.7 - The transports handling is different, but that's due to the above discrepancy. Given that all credentials in KeePassXC will have the same transports, I don't think it's significant. The parameters passed to the authenticator don't match what is described.
  • If an authenticator ceases to be available on this client device - doesn't seem to be handled, may not be relevant?
  • If any authenticator returns a status indicating that the user cancelled the operation - there is error handling, seems sufficient since there's only one authenticator
  • If any authenticator returns an error status - there is error handling, seems sufficient since there's only one authenticator
  • If any authenticator indicates success
    • Step 2 - the struct is not created as described, the only inputs come from the authenticator, which is probably technically a security issue (though because the browser extension and KeePassXC have the same authors they can trust each other in practice)
    • Step 3 - the object created doesn't quite look like a PublicKeyCredential, it's missing id, type, getClientExtensionResults and the internal slots aren't represented in any way (aside from [[identifier]], which is covered by rawId)
    • Step 4 - not really relevant as there's only one authenticator

Step 7's discrepancy seems likely to cause privacy/security issues, and the handling of data returned by the authenticator may be a security issue if it's possible that it's not KeePassXC that the browser extension is talking to. The rest seem to be more about UX and compatibility than security or privacy.

In the authenticator

https://www.w3.org/TR/2019/REC-webauthn-1-20190304/#authenticatorgetassertion

This starts with the same note about cancelling operations as when creating a passkey, so the same concerns apply here.

• Step 1 - There are a couple of validation checks, but not to the level described. Could be an issue if KeePassXC could be contacted by anything other than the official browser extension.
• Step 3 - the implementation also falls back to allowing entries which have a user handle set, which isn't relevant (this might be because the filtering happens even if the allowed credentials list is empty?). It also filters credentials by RP ID, which again doesn't happen yet (though that's in step 5)
• Step 7 - The handling of user verification is wrong, and the dialog doesn't perform user verification but KeePassXC may set the UV bit later. This is a security issue as it secretly downgrades 2FA to 1FA. There's also a case where no user presence check performed even if it's required, but KeePassXC always sets the UP bit later.
• Step 8 - there doesn't seem to be any extensions handling, which is fine as no extensions are supported, but a comment or no-op function for it would be good.
• Step 10 - as mentioned, KeePassXC may essentially lie to RPs when it sets the flags in the attestation object
• Step 11 - the clientDataJSON is built and hashed here instead of in the client, which may be a security issue if the client could be untrusted. I'm not familiar with Botan's API so can't comment on whether it's used correctly, but I assume so given that RPs can verify signatures successfully.
• Step 12 - Error handling isn't clear, particularly if creating a signature fails, as then it'll just send an empty signature back?
• Step 13 - More data is returned to the client than required, and the signature ID is always sent, but those don't seem to be issues.

Missing APIs

https://www.w3.org/TR/2019/REC-webauthn-1-20190304/#storeCredential https://www.w3.org/TR/2019/REC-webauthn-1-20190304/#preventSilentAccessCredential

These look like nice-to-haves for compatibility.

It also seems like the browser plugin should be overriding PublicKeyCredential.isUserVerifyingPlatformAuthenticatorAvailable() so that it returns true when KeePassXC is available.

https://www.w3.org/TR/2019/REC-webauthn-1-20190304/#op-lookup-credsource-by-credid

This one looks like the interface that should be implemented so that the browser extension can be updated to follow the specified steps for handling allowed credentials as part of the client get assertion procedure, as mentioned in the client get assertion section above.

https://www.w3.org/TR/2019/REC-webauthn-1-20190304/#op-cancel

As mentioned in both sections above, it looks like authenicatorCancel is important to have, but maybe it's not necessary with how KeePassXC and the browser extension communicate and operate together, I can't tell.

[droidmonkey]

Why do you think that the passkey confirmation dialog doesn't suffice as a user presence check? That part has me very confused.

[Ortham]

Why do you think that the passkey confirmation dialog doesn't suffice as a user presence check? That part has me very confused.

Oh, that's not what I meant, if the confirmation dialog is displayed then that is sufficient. I meant that it's possible for KeePassXC to skip displaying the dialog here:

https://github.com/keepassxreboot/keepassxc/blob/a472ef8a931dd64990d7f735394e949638e880d5/src/browser/BrowserService.cpp#L715-L717

If that return takes place, no dialog is displayed, but the response authenticator data still includes the user presence flag.

[droidmonkey]

However, the request said to discourage user presence check. We could certainly ignore that request or have a setting for the user to "always require confirmation of passkey requests".

[Ortham]

However, the request said to discourage user presence check.

Yes, but it doesn't matter that user verification is discouraged, user presence is still required. The only variable is if user verification is needed, and because KeePassXC is capable of verifying users, user verification is effectively required unless it's explicitly discouraged, see:

during client create credential:

userPresence becomes requireUserPresence (and the same for userVerification) during authenticator make credential:

and for client get credential it's the same:

and again for authenticator get credential:

(Apologies for the screenshots but it's hard to reference these deeply nested sections and copy/paste doesn't preserve the indentation.)

EDIT: User verification doesn't need to mean typing out the database's master password, the database quick unlock (Touch ID / Windows Hello) would be sufficient.

[varjolintu]

Just for curiosity: have you reviewed any other password manager's implementation of Passkeys?

[Ortham]

Just for curiosity: have you reviewed any other password manager's implementation of Passkeys?

No, the only other open-source one with passkey support that I'm aware of is Bitwarden (every time I think of it I mix its name up with Bitkeeper and Bitdefender...), and I haven't looked at its code.

I only had a look at KeePassXC's code because I was already looking forward to the next release having passkey support and wanted to try out the snapshot against the toy RP I've been writing, and found it didn't work due to the other issue I filed, and then I noticed user verification wasn't working properly, so I started investigating. (I currently use the original Keepass on my PC, though I did use KeepassXC on a work mac a few years ago, so I'm not familiar with all its extra features.)

[varjolintu]

KeePassXC does not perform domain and RP ID validation correctly, which I think cause privacy and security issues:

• Privacy example: I'm pretty sure that it would be possible for one .github.io site to gather information about what credentials a user has for other .github.io sites.
• Security example: If bank.com uses passkeys, an attacker could create a phishing site at fakebank.com and use an RP ID > of bank.com and I think KeePassXC would provide the user's bank.com assertion, which the attacker could then use to sign > into the user's bank account.
• Fixing this would involve changes to the browser extension to implement the correct logic.

@Ortham This assumption is false. The origin used in CollectedClientData comes from the browser's window.location.origin variable, not from the public key data. Unless you are using a malformed browser that can override window.location.origin from the internal API's. So a fake site would create an invalid clientDataJSON value.

I agree though, that checks related to the origin should be added to the KeePassXC side also.

[Ortham]

@Ortham This assumption is false. The origin used in CollectedClientData comes from the browser's window.location.origin variable, not from the public key data. Unless you are using a malformed browser that can override window.location.origin from the internal API's. So a fake site would create an invalid clientDataJSON value.

Sorry, I don't follow your logic. The browser doesn't need to be malformed, if you go to https://fakebank.com/ then your origin is fakebank.com. If that page fires a create or get credential request it can supply an RP ID of bank.com, and I think KeePassXC would currently accept it because the extension currently only checks that the RP ID is a suffix of the origin:

        if (!pkOptions.rp.id) {
            pkOptions.rp.id = window.location.hostname;
            pkOptions.rp.name = window.location.hostname;
        } else if (!window.location.hostname.endsWith(pkOptions.rp.id)) {
            throw new DOMException('Site domain differs from RP ID', DOMException.SecurityError);
        }

(From here.)

I've actually got what I think is the correct validation procedure for the origin and RP ID implemented (though I still need to write tests for it), so I could open a draft PR with that.

[varjolintu]

Sorry, I don't follow your logic. The browser doesn't need to be malformed, if you go to https://fakebank.com/ then your origin is fakebank.com. If that page fires a create or get credential request it can supply an RP ID of bank.com, and I think KeePassXC would currently accept it because the extension currently only checks that the RP ID is a suffix of the origin:

What I mean is that even if that check would fail (I'll fix that), authenticating from a different origin will create invalid data and the authentication will not succeed. I just tested this with a hardcoded https://fakewebauthn.io origin value during get:

[Ortham]

Yeah, but isn't that error coming from the RP? An attacker's RP would of course expect the fake origin.

[Ortham]

I've dumped an example of the changes I'd like to make to the browser extension into this commit.

• There are still a bunch of TODOs and FIXMEs (sameOriginWithAncestors being an important one I didn't even pick up on before)
  • I'm still not sure how sameOriginWithAncestors should be set, so I've set it so the code will always error for now, and the timer and non-success responses from the authenticator still have a lot of questions around them.
• there's a little inefficiency with isDomain checks happening multiple times, but that's probably fine
• there are no tests and I've not even manually tested anything
• this does change what gets sent to KeePassXC, so that'll need to be updated too, but I think bringing the interface into line with the spec is worth the breaking change.

With those qualifiers in mind, I'd like some feedback on the approach I've taken before I do much more.

[varjolintu]

@Ortham I'd rather do a simple origin vs. rpId check in the extension side and everything else in KeePassXC side. I'd like an idea that the extension itself just passes the information to KeePassXC, which then parses it, and sends back a proper response or an error that is handled in the extension.

After all, there's a possibility a 3rd party client will use the Browser API with KeePassXC and that client does not follow the same checks.

[Ortham]

If you do that then you are explicitly not compliant with the WebAuthn standard though, and by doing so you're risking introducing privacy and/or security vulnerabilities. I would not take that approach unless I was certain that I knew better than the spec authors.

After all, there's a possibility a 3rd party client will use the Browser API with KeePassXC and that client does not follow the same checks.

That's part of why the logic is separated between the client and authenticator in the spec though! (Another part of it is to be privacy-preserving.) Right now the implementation puts too much trust in KeePassXC itself, and KeePassXC puts too much trust in the client. The authenticator is intentionally only supposed to be given the data that it needs to perform its role, and the client is not supposed to use anything from the authenticator that it could not obtain itself.

---

The spec authors have provided a step-by-step description of exactly what an implementor must do, that's why it says

When this method is invoked, the user agent MUST execute the following algorithm

The current implementation has done a very poor job of following those steps. If it was just a case of misunderstanding the spec, then that would be fine (it's not an easy spec to read, I myself don't understand parts of it), but the more I hear, the more it sounds like you're intentionally non-compliant. Which, again, would be fine, if deviations from the spec were documented and justified, but it looks they're not.

I've checked the code and raised the issues that I've found, and nobody has correctly pointed out that I'm wrong about any of them (if I am, great, I'd love to be wrong!). I've offered to do my best to address the issues, and I've given you an example of what my approach might look like (I'm open to changes and discussion on it).

Please take a look at the commit I linked to above and decide how you want to proceed: if you broadly agree with me, great, we can go from there and work out the details. If not, then I think it's probably best if I just stick to using the original Keepass.

[varjolintu]

I've checked the code and raised the issues that I've found, and nobody has correctly pointed out that I'm wrong about any of them (if I am, great, I'd love to be wrong!). I've offered to do my best to address the issues, and I've given you an example of what my approach might look like (I'm open to changes and discussion on it).

I haven't had time to go through it yet, except for the first part.

[varjolintu]

If you do that then you are explicitly not compliant with the WebAuthn standard though, and by doing so you're risking introducing privacy and/or security vulnerabilities. I would not take that approach unless I was certain that I knew better than the spec authors.

Could you elaborate what privacy and/or security vulnerabilities this method would introduce in theory? The whole idea of the extension is to pass information between KeePassXC, and trying to keep the logic at minimum at the extension side.

[Ortham]

Could you elaborate what privacy and/or security vulnerabilities this method would introduce in theory? The whole idea of the extension is to pass information between KeePassXC, and trying to keep the logic at minimum at the extension side.

I don't really know, it's only been two weeks since I first started reading the spec, so I'm sure there are things that the authors have thought of that I haven't. In general I'd have thought that because the client and the authenticator both run locally (maybe not the same hardware but both owned by the user and in physical proximity) then they sit within the same trust boundary, but then defense in depth is a thing and restricting the capabilities and access to data of different subsystems can be part of that.

The spec does point to a FIDO security reference but I've only just skimmed it now, and it's not an easy read.

Thinking about it myself, if an authenticator does everything that the client is supposed to, barring the basic collection of data like the origin, then that reveals the following information to the authenticator that it doesn't need:

• the origin (it could be narrowed down given the authenticator has RP ID, but not known for sure)
• whether or not the request is running in an iframe
• the challenge
• the level of attestation required
• the criteria for authenticator selection
• the client extensions requested

The challenge seems harmless, given that it's supposed to be a single-use token, the others could be used to infringe on privacy, and the attestation and authenticator selection data could be used to weaken the strength of the authentication (e.g. an authenticator could decide that it'll pretend it can't do user verification when creating or using passkeys for a particular competitor's website, perhaps being able to pass it off as a problem with the website and inflicting reputational damage).

Going the other way, if the client takes the client data from the authenticator then the authenticator could make up whatever it wants in that data to match the signature it's generated, which seems like it might be open to some kind of MITM attack? Though when it gets to the RP the RP would then see the signature wouldn't match the client data JSON sent by the client...

I feel like those are pretty weak examples, but my intent when writing that comment was basically that unless you know that you know better than the spec authors, why risk doing something different? I'm certainly not a security expert, and thorough analysis takes time and effort: just following the spec is easier and less risky.

[varjolintu]

Going the other way, if the client takes the client data from the authenticator then the authenticator could make up whatever it wants in that data to match the signature it's generated, which seems like it might be open to some kind of MITM attack?

At this point every password manager is vulnerable to this because there's no API provided by browsers that any 3rd party client could use freely (and it would follow the specs automatically). This means every extension injects a script to a web page that captures the WebAuthn API calls. Which opens the possibility that any fraudulent browser extension could do the same.

[Ortham]

Which opens the possibility that any fraudulent browser extension could do the same.

True, at least macOS apparently has APIs for integrating third-party passkey providers, and according to passkeys.dev Windows is planning the same (but who knows when that'll happen...).

[droidmonkey]

I think you fundamentally misunderstand the role of the browser extension. It is NOT the client in the spec, that is actually the browser itself. The browser communicates with the RP and brokers the necessary data to complete the authentication process. The browser extension merely passes data from the browser to KeePassXC for processing (ie, the authenticator). KeePassXC acts very similar to the Windows OS, for example. In the case of Windows OS as the authenticator, the browser itself passes the necessary data to windows since it's built into the browser code base.

[Ortham]

I think you fundamentally misunderstand the role of the browser extension. It is NOT the client in the spec, that is actually the browser itself.

I had a read and a think but I'm still confused by that classification. The WebAuthn spec defines four components: the RP talks to the client platform, which talks to the authenticator. The client platform is composed of the client and the client device (the hardware). So the browser extension is either part of the client or part of the authenticator. It's pretty clear that KeePassXC itself is at least part of the authenticator. The client is defined as:

The browser extension replaces the browser's own implementation of CredentialsContainer (though there is that fallback to it if talking to KeePassXC fails), and therefore needs to provide the internal create and get methods, and in doing so it talks to the authenticator. That seems to pretty clearly sit within the client's responsibilities to me.

If the browser extension was part of the authenticator, then the browser would be able to perform all of the client logic in the spec and then call the browser extension using its existing implementation of CredentialsContainer and the associated internal functions. It obviously can't do that, because as @varjolintu said there's no API to register the extension to be called as an authenticator.

It would also be fine in that case for the browser extension to have full access to the private keys that are stored in KeePassXC, because there would be no logical distinction between the two pieces of software under WebAuthn's model, but that would violate the expectation that the private key is never exposed to any other party, because as the browser extension runs within the browser, the latter is able to read the former's memory, so any private key sent to the browser extension gets leaked to the browser, which we can agree is not part of the authenticator. It is just an expectation, but I think that if we can avoid violating it, we should.

(The spec also says that even the owner of the authenticator should not be able to see the private key, but I think that's fundamentally not possible without secure hardware being involved, so we can't avoid violating that expectation.)

To look at it another way, in my mind the client/authenticator boundary is most strongly defined by access to the private keys, because that's the most sensitive data in play. Taking that into account, it's best to draw that boundary as close to the actual storage of those private keys as possible, to limit how much code has access to them, and therefore the authenticator is limited to KeePassXC itself.

I'm not saying that the browser extension is the whole client, just that the combination of browser and extension is the client.

[droidmonkey]

and therefore needs to provide the internal create and get methods

No, that is the role of the authenticator. The client merely brokers with the authenticator. The client does not create, store, or retrieve credentials. Replacing the API call so we can point to our own authenticator is just a requirement due to no support for third party authenticators.

The browser extension is merely a bridge between the browser client and the KeePassXC authenticator. I think we do a little too much in the browser extension at this point, so I agree with @varjolintu we need to move more code into KeePassXC if anything.

[Ortham]

@droidmonkey I feel like we're completely talking past each other at this point.

No, that is the role of the authenticator.

I think I was unclear, when I was referring to the create and get methods, I meant [[Create]](origin, options, sameOriginWithAncestors) and [[DiscoverFromExternalSource]](origin, options, sameOriginWithAncestors), as mentioned in the screenshot I posted. Those are client methods:

When this method is invoked, the user agent MUST execute the following algorithm:

(As in my screenshot above, the user agent is at least partly the client.)

There are corresponding authenticatorMakeCredential and authenticatorGetAssertion methods that the authenticator provides, and which the client methods call. I wasn't speaking about them.

The client does not create, store, or retrieve credentials.

I agree (though you could argue that it retrieves them from the authenticator, but that's splitting hairs).

Replacing the API call so we can point to our own authenticator is just a requirement due to no support for third party authenticators.

Yes, but the API call is part of the client, therefore by replacing it you're replacing part of the client.

The browser extension is merely a bridge between the browser client and the KeePassXC authenticator.

There is no space for a bridge in the WebAuthn model! Everything involved in communication between the client, the client device and the authenticator must be one of those logical entities, or you're not using the WebAuthn model. I've already explained at length why I believe that the browser extension sits within the client, so I won't repeat myself.

I think we do a little too much in the browser extension at this point, so I agree with @varjolintu we need to move more code into KeePassXC if anything.

This makes absolutely no sense to me, it's completely inconsistent with your statement that the extension is not part of the client. If KeePassXC is the authenticator and the extension is not part of the client, then neither of them should be performing any of the steps described in [[Create]](origin, options, sameOriginWithAncestors) or [[DiscoverFromExternalSource]](origin, options, sameOriginWithAncestors), because those are performed by the client. If your understanding of the model is correct, you should be moving more code out of KeePassXC and the extension, because they're currently doing some of what the client is supposed to do.

If those steps (like origin and RP ID validation) aren't going to be performed in KeePassXC or the extension, what's going to perform them? The browser can't, because you need to replace its implementation of those steps due to the lack of third-party authenticator integration APIs.

I'm trying to see where you're coming from, in case this is just a difference of opinion, but your interpretation of the spec is inconsistent with itself, with some of the expectations of the spec, and with generic practices that aid security such as separation of concerns and the principle of least privilege.

[varjolintu]

If those steps (like origin and RP ID validation) aren't going to be performed in KeePassXC or the extension, what's going to perform them? The browser can't, because you need to replace its implementation of those steps due to the lack of third-party authenticator integration APIs.

The whole idea is to perform everything possible in KeePassXC's side. So KeePassXC itself will work as a client and an authenticator. The extension just passes the information and reports errors to the user.

[Ortham]

The whole idea is to perform everything possible in KeePassXC's side. So KeePassXC itself will work as a client and an authenticator. The extension just passes the information and reports errors to the user.

OK, if that's the case then let's say (for the sake of example, I'm not suggesting this) that the client's steps are implemented in the BrowserAction and BrowserService classes and the authenticator's methods are provided by the BrowserPasskeys class. With that approach, the client would encompass the browser, the extension, BrowserAction and BrowserService, and the authenticator would be BrowserPasskeys and all the code sitting behind it.

I don't like that approach, partly because it does not align the logical boundary between client and passkey to the process boundaries that exist in the implementation, but I can't think of any specific issues that it would cause.

However, you would have to change how the client's timer is handled, and adapt the branching logic in the spec for when the client talks to the authenticator. It's used to preserve privacy by not revealing the difference between there being no authenticator, no passkey or a user cancelling or denying the use of the authenticator (see here for some description of it). It's also used because the logically-separate entities may be physically separated, so communication between them may be inter-process or inter-device and is not guaranteed to be successful (whereas everything before talking to the authenticator can be done without interruption, e.g. up until [[Create]] section 20).

If the interface between client and authenticator sits within KeePassXC, then the logic that accounts for that separation doesn't apply, because KeePassXC will always be able to communicate with itself. Instead, you've introduced a fallible communication link within a single logical entity (which is the client, and the fallible communication link is between the browser extension and KeePassXC), which is... messy. In any case, you'd need to adapt the logic for handling authenticator availability to instead handle the availability of KeePassXC's part of the client.

So again, you're going off-spec and introducing complications that require more thought and careful implementation, and doing so unnecessarily. You've both said that you want to do more in KeePassXC: why? As I've said before, I feel like it's pretty important to have a clearly-expressed and documented motivation for any discrepancies, but if you've provided that then I'm sorry but I've missed it.

Finally, at this point I feel like it might be worth pointing out that you've already tried doing your own thing, and that seems to have caused serious security and privacy issues which would not be present had you followed the spec. Why double down on going off-spec unecessarily when the evidence available suggests that it leads to worse outcomes?

[varjolintu]

You've both said that you want to do more in KeePassXC: why?

Again: because we want the extension side to be very simple. One reason for this is that any 3rd party client a developer wants to use with KeePassXC via Browser API would have to include identical checks or there's similar issues all over again. It's only common sense to include as much implementation to KeePassXC side as possible.

Why double down on going off-spec unecessarily when the evidence available suggests that it leads to worse outcomes?

I'm already working on with the fixes.

[Ortham]

Again: because we want the extension side to be very simple. One reason for this is that any 3rd party client a developer wants to use with KeePassXC via Browser API would have to include identical checks or there's similar issues all over again.

Right, you did say that in an earlier comment, sorry I forgot. I disagree that it's KeePassXC's responsibility, but I understand that including the client functionality in KeePassXC does guard against it being done incorrectly elsewhere.

I'm already working on with the fixes.

Is there a branch I can look at to see the changes?

[varjolintu]

Is there a branch I can look at to see the changes?

Not yet. I need to make changes to both sides, and will put PR's online when things are starting to look better.

[droidmonkey]

@Ortham I see what you are saying now after sleeping on it. KeePassXC is basically forced to combine two roles of the client (Create/Retrieve) and the Authenticator roles. This is due to the lack of support for third party authenticators. Where I think we diverge is the view on the role of the extension, but Varjolintu helped out there. The browser itself still plays an important role as the "base client". It is unclear to me if we must implement the privacy timeout rules or if the browser does that on the backend regardless of what the overriding client does.

[Ortham]

It is unclear to me if we must implement the privacy timeout rules or if the browser does that on the backend regardless of what the overriding client does.

I assume the timeout rules need reimplementing because they basically control how long it could take navigator.credentials.get() and navigator.credentials.create() to return, and the extension is replacing the whole of navigator.credentials. I don't know for sure though, it's the sort of thing that could be tested once the headline issues are resolved.

In fact, I'll probably continue with my branch as a learning exercise, with the understanding that you're not interested in that approach, as it may help clarify some of those points and serve as a comparison against what @varjolintu is doing.

[varjolintu]

@Ortham Your previous commit has already been helpful! Thanks.

[Ortham]

I've deleted the keepassxc-browser fork that I linked to above, as it got very messy. Instead, I started from scratch and implemented a WebAuthn client and authenticator together as a browser extension here. The implementation has pretty good coverage of WebAuthn Level 2 and a few bits from Level 3, and I've tested it against Google and GitHub and various test sites (most notably https://webauthn.io/ and https://webauthntest.identitystandards.io) to cover a lot of combinations and they all now seem to work.

Hopefully it's useful as a point of comparison: it's my interpretation of the spec and how that met reality. It's fully self-contained (no non-dev dependencies), and having everything in one place should make it easier to follow (though I still think having the client and authenticator together is undesirable, and putting as much of the client into in-page JS as I have is probably a bad idea).

Putting it together I did realise I'd previously misinterpreted the spec in a few places, and there are a few places where rigorously following the spec doesn't really work or I think leaves the implementation worse off, at least when doing it in browser JS: in some of those cases I deviated, but mostly I tried to do what the spec says even though it makes the code a bit of a mess in places. I wouldn't advise taking the exact approach I have.

I still don't really understand what attachment or transports a software authenticator on the same device is supposed to have, but I can at least confirm that the timeout logic does need to be reimplemented.

At least if you're doing more in KeePassXC you won't need to provide a CBOR encoder and decoder, though they weren't too bad - I got caught out more by needing to DER-encode ECDSA signatures. Another thing to watch out for is AuthenticatorAttestationResponse.getPublicKey() - I found that some sites fail if you don't have it implemented, and that means you need to DER-encode the public key as SPKI. I did that using the Web Crypto API, going from a JWK, but you may want or need to take a different approach.

[varjolintu]

Putting it together I did realise I'd previously misinterpreted the spec in a few places, and there are a few places where rigorously following the spec doesn't really work or I think leaves the implementation worse off, at least when doing it in browser JS: in some of those cases I deviated, but mostly I tried to do what the spec says even though it makes the code a bit of a mess in places. I wouldn't advise taking the exact approach I have.

This is the reason I did not modify some places in my fix branch (not public yet). For now I have done the following (all tests pass now, and I have written a lot of new ones, especially concerning RP ID and Origin checks):

• BrowserService no longer does the checks by itself
• A new class BrowserPasskeysClient constructs the relevant objects, acting as a client.
• A new helper class PasskeyUtils includes the actual checks and parses the objects (prevents BrowserPasskeysClient not to bloat)
• BrowserPasskeys is pretty much intact, but some functions have been moved to PasskeyUtils.

About User Presence and User Verification. If a database is open, that should suffice as User Present. But still, I want to force User Verification to be done always, because these two things are hard do distinquish, especially when speaking about password managers. I'd assume every password manager there is asks just a some kind of confirmation which works as User Verification. We can add TouchID/Hello support later.

It would definitely be an optimal situation if there was an open API in the browser itself that every password manager could use directly, without implementing everything again.

[Ortham]

If a database is open, that should suffice as User Present.

I don't think that's true, if it was then you could unlock your DB when you start up your PC and a website could then not require User Verification and create or use passkeys without your knowledge until the DB was locked - imagine Facebook secretly signing you in without your knowledge to track you across the web (not that they need you to be signed in to do that).

I think the dialog box that KeePassXC currently displays (the one that asks things like "do you want to use this passkey" and "do you want to create this passkey") is sufficient for User Presence, since the user needs to click a button for the process to continue. There's just that one case I spotted where the dialog is skipped but shouldn't be.

But still, I want to force User Verification to be done always

If it's not too complicated, I'd go with respecting the discouraged option, as needing to enter your password every time could get old pretty fast, especially if your DB password is very long. E.g. user presence might be enough for something like GitHub's sudo mode, since you're already logged in and it doesn't require 2FA when not using a passkey.

I'd assume every password manager there is asks just a some kind of confirmation

I just tested out Bitwarden and 1Password, and first unlocked their vaults before trying to create a passkey requiring User Verification, and neither of them prompted me for my password during the creation flow, they only showed a dialog allowing me to accept or reject the request, which I think only counts as User Presence. IIRC Dashlane did ask me to re-enter my password, I tested that some time last week.

[varjolintu]

If it's not too complicated, I'd go with respecting the discouraged option, as needing to enter your password every time could get old pretty fast, especially if your DB password is very long. E.g. user presence might be enough for something like GitHub's sudo mode, since you're already logged in and it doesn't require 2FA when not using a passkey.

It's just the confirmation dialog. No password is asked again.

I just tested out Bitwarden and 1Password, and first unlocked their vaults before trying to create a passkey requiring User Verification, and neither of them prompted me for my password during the creation flow, they only showed a dialog allowing me to accept or reject the request, which I think only counts as User Presence. IIRC Dashlane did ask me to re-enter my password, I tested that some time last week.

It should be noted that KeePassXC asks access to the Passkey credential when loading the web page, and the Passkey confirmation dialog is shown after that. So the user must separately accept access to the entry itself before any verification can be done.

[varjolintu]

The new PR's are online: https://github.com/keepassxreboot/keepassxc-browser/pull/2121 https://github.com/keepassxreboot/keepassxc/pull/10318