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

Extract in/out HID report sizes from descriptors #112

Merged
merged 2 commits into from
Jul 23, 2020

Conversation

fmeum
Copy link
Contributor

@fmeum fmeum commented Apr 29, 2020

The U2F/CTAP specifications for the HID transport allow arbitrary
sizes for the HID input/output reports, not just the standard 64 bytes.
This commit adds the general logic needed to deal with varying report
sizes and implements the relevant descriptor parsing for Linux.

The U2F/CTAP specifications for the HID transport allow arbitrary
sizes for the HID input/output reports, not just the standard 64 bytes.
This commit adds the general logic needed to deal with varying report
sizes and implements the relevant descriptor parsing for Linux.
@fmeum fmeum mentioned this pull request Jun 18, 2020
@jcjones jcjones self-requested a review June 29, 2020 16:40
Copy link
Contributor

@jcjones jcjones 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 this, @FabianHenneke! Can you give me an example of a device that provides variable size HID reports?

@@ -5,17 +5,17 @@
// Allow dead code in this module, since it's all packet consts anyways.
#![allow(dead_code)]

pub const HID_RPT_SIZE: usize = 64;
Copy link
Contributor

Choose a reason for hiding this comment

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

This is used by all the other platforms as well, so we'll have to find/replace across the repo.

@fmeum
Copy link
Contributor Author

fmeum commented Jun 29, 2020

Thank you for this, @FabianHenneke! Can you give me an example of a device that provides variable size HID reports?

Dynamic or at least non-standard HID report sizes are used by authenticators that implement the HID protocol via Bluetooth. I am the developer of such an authenticator, WearAuthn, and I know that @rstoica is working on such an authenticator due to #32. While Bluetooth as a HID transport is not part of the FIDO specs, it is supported by the Windows WebAuthn API and by Chrome on Linux, macOS and Chrome OS. It would be great if Firefox could also become compatible on non-Windows platforms.

I will take another look at the PR and take care of the HID_RPT_SIZE issue.

@jcjones
Copy link
Contributor

jcjones commented Jun 29, 2020

Thank you for this, @FabianHenneke! Can you give me an example of a device that provides variable size HID reports?

Dynamic or at least non-standard HID report sizes are used by authenticators that implement the HID protocol via Bluetooth. I am the developer of such an authenticator, WearAuthn, and I know that @rstoica is working on such an authenticator due to #32. While Bluetooth as a HID transport is not part of the FIDO specs, it is supported by the Windows WebAuthn API and by Chrome on Linux, macOS and Chrome OS. It would be great if Firefox could also become compatible on non-Windows platforms.

That seems very doable. I just want to be able to test if I can. I'll see about getting something running WearOS if I can.

I will take another look at the PR and take care of the HID_RPT_SIZE issue.

Thanks. I put a skeleton fix in 5d28484 - I spent a short while this morning trying to figure out how to get the same data out of iokit but it needs more research than I could pull off at the time.

@rstoica
Copy link

rstoica commented Jun 30, 2020

Thank you for this, @FabianHenneke! Can you give me an example of a device that provides variable size HID reports?

Dynamic or at least non-standard HID report sizes are used by authenticators that implement the HID protocol via Bluetooth. I am the developer of such an authenticator, WearAuthn, and I know that @rstoica is working on such an authenticator due to #32. While Bluetooth as a HID transport is not part of the FIDO specs, it is supported by the Windows WebAuthn API and by Chrome on Linux, macOS and Chrome OS. It would be great if Firefox could also become compatible on non-Windows platforms.

That seems very doable. I just want to be able to test if I can. I'll see about getting something running WearOS if I can.

If Android OS would be easier to get a hang on for the necessary testing you are welcome to try WioKey (Early Access) as well. Source code is not yet out, but it uses the same underlying Bluetooth HID within the Android Bluetooth stack.

I will take another look at the PR and take care of the HID_RPT_SIZE issue.

Thanks. I put a skeleton fix in 5d28484 - I spent a short while this morning trying to figure out how to get the same data out of iokit but it needs more research than I could pull off at the time.

@jcjones
Copy link
Contributor

jcjones commented Jun 30, 2020

If Android OS would be easier to get a hang on for the necessary testing you are welcome to try WioKey (Early Access) as well. Source code is not yet out, but it uses the same underlying Bluetooth HID within the Android Bluetooth stack.

Thanks - a good idea. My general Android test device (a Samsung S7 edge) is apparently incompatible, but I'll give it a try on my other devices tomorrow.

I don't have to have running code to finish this review and accept. Would just be nice to be able to work it out for MacOS and Windows, too! I should have this patch into the next release of authenticator-rs, probably in Firefox 80.

@rstoica
Copy link

rstoica commented Jun 30, 2020

Thanks - a good idea. My general Android test device (a Samsung S7 edge) is apparently incompatible, but I'll give it a try on my other devices tomorrow.

Yes, unfortunately not all of the devices have the Bluetooth HID profile enabled, but most of the Android 9+ should support it. The other Bluetooth LE GATT solution is currently blacklisted by Google.

I don't have to have running code to finish this review and accept. Would just be nice to be able to work it out for MacOS and Windows, too! I should have this patch into the next release of authenticator-rs, probably in Firefox 80.

Current code version (without this PR) works out of the box on Windows machines as it goes through Windows Hello and the drivers pick up the Bluetooth automatically as an input device anyway. I don't have now a MacOS to check particularly the Mozilla browser, but I can do it maybe in the next days. I know though that it works on Mac with other browser clients as another colleague tested it already, but don't recall exactly the state for Firefox and don't want to pass misinformation. I will update here this comment once I find out.

@fmeum
Copy link
Contributor Author

fmeum commented Jun 30, 2020

Yes, unfortunately not all of the devices have the Bluetooth HID profile enabled, but most of the Android 9+ should support it.

It also doesn't work with OnePlus and (most) Nokia phones. More recent Samsung phones should work, but Pixels are ideal.

The other Bluetooth LE GATT solution is currently blacklisted by Google.

Unfortunately, it is even blacklisted in the Android Open Source Project directly. Without a custom ROM, it will thus remain disabled on most phones indefinitely. @jcjones This is why Bluetooth HID is the only workable solution for third-party FIDO apps on phones.

Current code version (without this PR) works out of the box on Windows machines as it goes through Windows Hello and the drivers pick up the Bluetooth automatically as an input device anyway. I don't have now a MacOS to check particularly the Mozilla browser, but I can do it maybe in the next days. I know though that it works on Mac with other browser clients as another colleague tested it already, but don't recall exactly the state for Firefox and don't want to pass misinformation. I will update here this comment once I find out.

I have added support for dynamic HID sizes on macOS to libfido2 in Yubico/libfido2#180 (and Windows in Yubico/libfido2#182, but as @rstoica said, Windows Hello makes this rather uninteresting). Maybe that code can be helpful, as I may not have time to work on it myself.

@jcjones
Copy link
Contributor

jcjones commented Jul 17, 2020

As an update, I should be able to take this for the next Firefox cycle, starting end-of-month.

@jcjones jcjones merged commit 8f26464 into mozilla:master Jul 23, 2020
jcjones added a commit that referenced this pull request Jul 27, 2020
jcjones added a commit that referenced this pull request Sep 10, 2020
Major changes:
* Move to Rust 2018 edition #125
* Remove dependency on boxfnonce #121
* Reworked error handling #130
* Added a higher-level AuthenticatorService that can use multiple backends #123
* Changed the C API to use the new AuthenticatorService #123
* Added a Status channel for backends #122
* Now obtaining HID report sizes from the descriptors #112
* Add authenticator USB and Firmware details to the C API #93
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants