trussed-dev / usbd-ctaphid

Apache License 2.0
0 stars 3 forks source link

Wrong usage page in descriptor #1

Open szszszsz opened 1 year ago

szszszsz commented 1 year ago

The FIDO_USAGE_DATA_OUT constant is probably equal to 0x21, and not to 0x04. The exact value is not specified in the FIDO CTAPHID spec from the brief look. Because of the wrong value, the fido2luks application cannot find authenticator using this library (as it searches for 0x21 as the usage page).

Nitrokey FIDO2 has this set to 0x21 as well.

https://github.com/trussed-dev/usbd-ctaphid/blob/2f29e0126ff32d34094fabefd58910d586d42e24/src/class.rs#L157-L158

Connected:

robin-nitrokey commented 1 year ago

I would argue that this is a problem with the ctap crate. According to the spec, devices should be identified by the device usage page and usage:

A unique Usage Page is defined (0xF1D0) for the FIDO alliance and under this realm, a CTAPHID Usage is defined as well (0x01). During CTAPHID device discovery, all HID devices present in the system are examined and devices that match this usage pages and usage are then considered to be CTAPHID devices.

https://fidoalliance.org/specs/fido-v2.1-ps-20210615/fido-client-to-authenticator-protocol-v2.1-ps-20210615.html#usb-discovery

We correctly set these values:

https://github.com/trussed-dev/usbd-ctaphid/blob/2f29e0126ff32d34094fabefd58910d586d42e24/src/class.rs#L132-L138

It seems to me that the ctap crate reads the complete device descriptor and uses the last usage value.

https://github.com/ArdaXi/ctap/blob/0fa8d420c9e4e0714d1f29ef458101b5c83bf93a/src/hid_linux.rs#L27-L88

But this happens to be the endpoint usage, not the device usage. A better approach would be to use the first value. (This probably could still fail for composite devices, but let’s ignore that for the moment.)

So I think this should be fixed in ctap by a) checking for usage 0x01 instead of 0x21 and b) properly parsing the device usage (instead of the endpoint usage). The current implementation only works by accident with devices that set the last endpoint usage to 0x21. If we would change it here, we would just hide the actual problem.

robin-nitrokey commented 1 year ago

I’ve prepared a simple patch for ctap, at least for testing: https://github.com/ArdaXi/ctap/pull/5