Add user verification to webauthn challenges

[p1gp1g]

Require user verification if all tokens are registered with UV flag, else discourage it

• Resolves: [Bug]: Passwordless authentication does not ask for PIN when using security keys. #41599
• Previously work on Recommend a device PIN for passwordless auth #34389 (by @ChristophWurst)

Summary

TODO

• Recommend UV (user verification with a PIN) during device registration
• Require UV during authentication if all tokens are registered with UV
• Discourage UV during authentication if any token is registered without UV *
• Previously registered token are considered without UV to avoid locking out a user

* Currently, this is always discouraged

Checklist

• Code is properly formatted
• Sign-off message is added to all commits
• Tests (unit, integration, api and/or acceptance) are included
• Screenshots before/after for front-end changes
• Documentation (manuals or wiki) has been updated or is not required
• Backports requested where applicable (ex: critical bugfixes)

[p1gp1g]

Is it possible to add the security label to this PR (like #34389 had) ?

@blizzz: I see you've changed the milestone, I wonder if it's possible to reconsider. I know you have several requests in the queue and need to prioritize things. But currently using a hardware token for passwordless authentication weakens authentication security in a way: anyone with physical access to that token can get an authenticated session.

[github-actions]

Hello there,
Thank you so much for taking the time and effort to create a pull request to our Nextcloud project.

We hope that the review process is going smooth and is helpful for you. We want to ensure your pull request is reviewed to your satisfaction. If you have a moment, our community management team would very much appreciate your feedback on your experience with this PR review process.

Your feedback is valuable to us as we continuously strive to improve our community developer experience. Please take a moment to complete our short survey by clicking on the following link: https://cloud.nextcloud.com/apps/forms/s/i9Ago4EQRZ7TWxjfmeEpPkf6

Thank you for contributing to Nextcloud and we hope to hear from you soon!

[st3iny]

Thank you for the pull request 😃

I rebased, fixed conflicts, tested locally with 2 keys and made some necessary changes to the code.

My Changes

1. Made user_verification column nullable to make it compatible with all databases.
2. Bumped @since 29.0.0 to @since 30.0.0 etc.
3. Made some minor changes to avoid checking in already deprecated code.

Tests

It works good so far. I tested it with a key protected by a PIN (user verification) and one that has no pin. The PIN prompt is only shown when just the protected key is configured.

Bugs

However, there is one bug/caveat:

1. Set up one key A with a PIN and one key B without.
2. Observe the oc_webauthn table and see that user_verification is correctly set to 1 and 0 (A, B).
3. Log out and log in using a device.
4. Use the protected key (-> no PIN is prompted as expected).
5. Observe the oc_webauthn table and see that user_verification was overwritten to 0 and 0 (A, B).

The credential is saved and overwritten each time a log in is performed which causes the user verification to be reset. I stepped through the code and it seems that the Webauthn library calls our CredentialRepository::saveCredentialSource() method on each login. Note that this only happens when at least one key without a PIN is configured.

I think we should handle this case a bit better by either not overwriting during logins or finding another solution.

[p1gp1g]

Thanks for your review

One solution would be to change the authentication challenge from DISCOURAGED to PREFERRED when one of the key doesn't have UV.

It has the benefits to:

• Allow to update the UV of a key without having to re-register it
• Protect against someone who get a key which can use UV to directly use it in a browser: UV will be asked.

Downside:

• It can give a false sense of security. Users can think that the UV is required even if they have a non-secure key registered, while it is possible to bypass this UV for all keys.

I think the downside is dangerous. Not overwriting the key is probably a better solution

[blizzz]

@blizzz: I see you've changed the milestone, I wonder if it's possible to reconsider. I know you have several requests in the queue and need to prioritize things. But currently using a hardware token for passwordless authentication weakens authentication security in a way: anyone with physical access to that token can get an authenticated session.

If you think this is security related, please follow https://github.com/nextcloud/server/blob/master/SECURITY.md

cc @nickvergessen

[st3iny]

I think the downside is dangerous. Not overwriting the key is probably a better solution

This is the route I chose for now. Now, the user verification is never downgraded from true to false. It works without having the potential downside you mentioned.

Existing users won't be affected. Only new devices will be affected and if a user wants to benefit from UV they have to remove all existing keys and re-register them with UV.

[st3iny]

Tested and works. Remaining feedback has been addressed.

[p1gp1g]

Hello, is anything blocking this PR ?

[ChristophWurst]

Thanks a lot

This drowned in my notifications, sorry

Left some minor feedback

[p1gp1g]

So this PR is tagged with "to release" and won't be release ? This is sad because this is a security fix

Fortunately there is a workaround for admin who cares about their user security: #41599 (comment) and users should be aware to not use passkeys until this is merged or there is the workaround

[nickvergessen]

So this PR is tagged with "to release" and won't be release ?

Well I guess it just slipped, as CI didnt work for the fork and therefore the PR never turned green.

I will send a local PR so CI passes and then it should go in

[nickvergessen]

Running in #47253

[p1gp1g]

Thanks a lot for your feedback, I hope the CI will be OK

[nickvergessen]

• Merged for 31 in feat(webauthn): Add user verification to webauthn challenges #47253
• Backported for 30 in [stable30] feat(webauthn): Add user verification to webauthn challenges #47255

[nickvergessen]

THanks for the contribution @p1gp1g and sorry for the miss

[p1gp1g]

No problem for the miss, and thank you !

[skjnldsv]

@p1gp1g @Be-ing, sorry for the late reply!
We did some cleanup on old opened PR, but haven't replied to all of them yet (public holiday yesterday) 🙈

Thanks @nickvergessen for the help, glad to see this moving forward!!
Have a nice Friday then a nice weekend everyone! 🌞