-
Notifications
You must be signed in to change notification settings - Fork 36
clientData sending invalid tokenBinding status #29
Comments
@frankh what site is this for? I thought chrome dropped support for the token binding standard anyways..? |
I've done some more research and it seems that token-binding is not supported anywhere It's a private project that's requiring this flag, i'll bug them to remove the requirement. |
Actually, it does look like this is a bug According to the spec[1], the only valid values for this field are "supported" and "present" - so "not-supported" causes it to be rejected, but skipping the field entirely works. I think the fix is to simply remove the field from the clientData here Line 164 in f9599d4
[1] https://www.w3.org/TR/webauthn/#dom-collectedclientdata-tokenbinding |
Created a PR here #30 |
It seems this was valid at some point, but is no longer, see: w3c/webauthn#914 |
While the PR looks good wrt to the spec, I wonder if this will break other sites that maybe also don’t realize this spec change (especially sites that support U2F only). Have you tested this change with sites like Google, Dropbox, etc...? |
I just tested with Google and Dropbox and can confirm they still work. It also seems that my Yubikey 5C doesn't send this field in clientData, so anything that supports that should work with this change |
Hm I would also test with a site that is U2F only i.e maybe GitHub & GitLab (Google and Dropbox both are webauthn). Might need to set/send the field differently based upon the protocol but I'm not sure. |
Can confirm github works too (this is good for me upping all my security 😄 ) |
hi, just wondering if you're considering merging this? |
Sorry for the delay, I had merged this a little while back. |
edit: My original comment was wrong, see #29 (comment)
I tried to use krypton as a u2f key on a site but it kept rejected it. I eventually realised that it's because the site requires token binding (see https://fidoalliance.org/fido-technote-the-growing-role-of-token-binding/)
It seems krypton doesn't support this yet, is it on the roadmap?
The text was updated successfully, but these errors were encountered: