search

duo-labs / py_webauthn

Pythonic WebAuthn 🐍
https://duo-labs.github.io/py_webauthn
BSD 3-Clause "New" or "Revised" License
856 stars 171 forks source link

_verify_token_binding_id does not actually verify the tokenBinding.id property #93

Closed chrisdlangton closed 3 years ago

chrisdlangton commented 3 years ago

https://github.com/duo-labs/py_webauthn/blob/master/webauthn/webauthn.py#L1285

It only checks the tokenBinding.status and never uses the token_binding_id variable or verify the id as the function name and documentations suggests.

Perhaps the implementer should implement the verification? If so, the function is inappropriate and should be renamed to describe the functionality it provides (and be documented to describe this too). In it's current form it misleads the implementer into believing they are using the function to actually do verification and the intent of the function implies in it's documentation this is also the function - however it does not..

So what is the proper use by implementers? What is the function actually meant to do? Is this a defect? Is it named and documented incorrectly and it is working as intended (but very differently from the name and description)?

MasterKale commented 3 years ago

Support for tokenBinding across browsers has been..."inconsistent"...to date. As such it is currently being considered for deletion from L3 of the WebAuthn spec.

Due to this I don't plan on supporting tokenBinding in the library revamp I'm preparing (see here), nor is it likely that any work will be done on the current iteration of the library to refactor this code...

chrisdlangton commented 3 years ago

@MasterKale I see, reading the related material and doing some research of my own I believe I understand the perceived issue.

From the reports it seems that there are some implementations on the client devices that are inconsistent, however many (namely Yubikey) adhere to the spec and the server is capable of authenticating the client identity / public key in a way that would avoid certain MitM and replay attacks.

Therefore a decision to remvoe sserver-side verification, without a replacement mechanism designed to meet or exceed the intent (the security characteristics) of the removed functionality, seems immature at best, negligent when we start discussing the spec. Because the reason of removal is because the client devices we are attempting to authenticate are failing to meet the spec NOT that teh spec is incorrect..

chrisdlangton commented 3 years ago

Adding a compromise, if you chose to continue with deprecation (hopefully with a sufficiently secure replacement feature) then perhaps you might follow a normal deprecation strategy?

  1. log out a deprecation notice when implementations call _verify_token_binding_id
  2. state in the deprecation notice that token binding was not verified as the implementer expected
  3. offer a date / version when the _verify_token_binding_id will be removed.
  4. advise what functionality to use instead for the same (or better) security characteristics

Note: Logging does not functionally change this code. Also code comments do not functionally change the code, so it is probably decent to link to the decision rationale in the code.

MasterKale commented 3 years ago

The newly-released v1.0.0 features an attempt to parse "tokenBinding" in clientDataJSON, passing on it if it's malformed (based on experience with responses from actual older security keys). Validation of token binding status will only take place if the status was valid during parsing, otherwise that check is skipped.

If you see room for improvement with this new mechanism I welcome any PR's to enhance it.