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

Update documentation to avoid PIN bypass #372

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

tcannonfodder
Copy link
Contributor

This fixes #350, which pointed out a bug in certain browser/device combinations that allow bypassing the user's PIN if the user_verfication: true flag is not set.

https://hwsecurity.dev/2020/08/webauthn-pin-bypass/

tcannonfodder and others added 3 commits September 20, 2022 14:40
This fixes cedarcode#350, which pointed out a bug in certain browser/device combinations that allow bypassing the user's PIN if the `user_verfication: true` flag is not set.

https://hwsecurity.dev/2020/08/webauthn-pin-bypass/
Similar to cedarcode@98cb710, this helps ensure that clients do not allow PIN bypasses for older browser/device combinations
@tcannonfodder
Copy link
Contributor Author

I feel like that in order to help with the migration to passkeys, the docs should setup so that user_verification is required throughout.

@brauliomartinezlm
Copy link
Member

Sorry for the delay. Will take a look ASAP

Copy link
Contributor

@santiagorodriguez96 santiagorodriguez96 left a comment

Choose a reason for hiding this comment

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

Nice! Thank you @tcannonfodder! And sorry for the late response 😥

I think this is a good idea to add some documentation on how to require user_verification for the cases where developers use this gem in a password-less login!

Although adding user_verification: true to the verify method is correct, we should also add some documentation about how to set the user_verification in the PublicKeyCredential::Options to be required in the initiation phase of both Credential Authentication and Registration ceremonies – see the docs. That is because, as for a password-less login you will be requiring UV, you will want the authenticators that don't support UV to not be eligible to be used instead of letting them be used and then raise a verification error. In order to do that you can do:

  • In the initiation phase of the Credential Registration ceremony:
options = WebAuthn::Credential.options_for_create(
  ...,
  authenticator_selection: { user_verification: "required" }
)
  • In the initiation phase of the Credential Authentication ceremony:
options = WebAuthn::Credential.options_for_get(
  ...,
  user_verification: "required"
)

We had an old PR opened in webauthn-rails-demo-app – which implements a password-less login using webauthn – for adding user verification which I revisited and merged after seeing this issue/PR. You can check it to see how we did to add this and maybe even try it if you want 🙂

@@ -237,7 +237,8 @@ begin
webauthn_credential.verify(
session[:authentication_challenge],
public_key: stored_credential.public_key,
sign_count: stored_credential.sign_count
sign_count: stored_credential.sign_count,
user_verification: true, # needed for passwordless verification
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should also add this to creation options in the initiation phase of the Credential Registration ceremony 🙂

```ruby
credential_with_assertion.verify(
session[:authentication_challenge],
public_key: stored_credential.public_key,
sign_count: stored_credential.sign_count
sign_count: stored_credential.sign_count,
user_verification: true # needed for passwordless verification
Copy link
Contributor

Choose a reason for hiding this comment

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

We should also add this for PublicKeyCredentialWithAttestation#verify 🙂

Also, we could also add it to the params of this method in line 381.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants