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 user verification to webauthn challenges #44442

Closed
wants to merge 3 commits into from

Conversation

p1gp1g
Copy link
Contributor

@p1gp1g p1gp1g commented Mar 24, 2024

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

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

@solracsf solracsf added the 3. to review Waiting for reviews label Mar 24, 2024
@solracsf solracsf added this to the Nextcloud 29 milestone Mar 24, 2024
@Altahrim Altahrim mentioned this pull request Mar 25, 2024
@skjnldsv skjnldsv mentioned this pull request Mar 28, 2024
81 tasks
This was referenced Apr 4, 2024
@blizzz blizzz modified the milestones: Nextcloud 29, Nextcloud 30 Apr 8, 2024
@p1gp1g
Copy link
Contributor Author

p1gp1g commented Apr 9, 2024

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.

Copy link
Contributor

github-actions bot commented May 1, 2024

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!

@blizz

This comment was marked as off-topic.

Copy link
Member

@st3iny st3iny left a comment

Choose a reason for hiding this comment

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

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
Copy link
Contributor Author

p1gp1g commented May 2, 2024

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
Copy link
Member

blizzz commented May 2, 2024

@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

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

Signed-off-by: S1m <git@sgougeon.fr>
Signed-off-by: Richard Steinmetz <richard@steinmetz.cloud>
@st3iny
Copy link
Member

st3iny commented May 2, 2024

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.

Copy link
Member

@st3iny st3iny left a comment

Choose a reason for hiding this comment

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

Tested and works. Remaining feedback has been addressed.

@st3iny st3iny removed the request for review from JohannesGGE May 2, 2024 15:34
@p1gp1g
Copy link
Contributor Author

p1gp1g commented Jul 18, 2024

Hello, is anything blocking this PR ?

Copy link
Member

@ChristophWurst ChristophWurst left a comment

Choose a reason for hiding this comment

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

Thanks a lot

This drowned in my notifications, sorry

Left some minor feedback

@ChristophWurst ChristophWurst added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Jul 18, 2024
This was referenced Jul 30, 2024
@Altahrim Altahrim mentioned this pull request Aug 5, 2024
@skjnldsv skjnldsv modified the milestones: Nextcloud 30, Nextcloud 31 Aug 6, 2024
@p1gp1g
Copy link
Contributor Author

p1gp1g commented Aug 14, 2024

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
Copy link
Member

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
Copy link
Member

Running in #47253

@p1gp1g
Copy link
Contributor Author

p1gp1g commented Aug 15, 2024

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

@nickvergessen
Copy link
Member

@nickvergessen
Copy link
Member

THanks for the contribution @p1gp1g and sorry for the miss

@p1gp1g
Copy link
Contributor Author

p1gp1g commented Aug 15, 2024

No problem for the miss, and thank you !

@skjnldsv
Copy link
Member

skjnldsv commented Aug 16, 2024

@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! 🌞

@skjnldsv skjnldsv removed this from the Nextcloud 31 milestone Aug 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4. to release Ready to be released and/or waiting for tests to finish enhancement feedback-requested
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: Passwordless authentication does not ask for PIN when using security keys.
9 participants