Yubico / libfido2

Provides library functionality for FIDO2, including communication with a device over USB or NFC.
Other
602 stars 153 forks source link

Way to interrupt blocked calls #56

Closed Silvercast closed 5 years ago

Silvercast commented 5 years ago

According to the documentation, fido_dev_make_cred and fido_dev_get_assert are synchronous and will block if necessary. I wonder if there is anyway to interrupt the API calls when they are blocking. The use case is quite simple:

  1. Plug in a FIDO2 key to an USB port
  2. call one of the API
  3. The key will start flashing/blinking
  4. Don't activate the key but pull it out
  5. The API is still stuck.

In some cases the HID communication is even stuck even after I plug the device out and retry. Thanks

martelletto commented 5 years ago

hi, is this happening on macos?

-p.

Silvercast commented 5 years ago

yes, mainly on macOS. I guess it's because the way we implement HID code. We're on official version 1.2.0.

martelletto commented 5 years ago

ok, i will see what can be done in hid_osx.c.

-p.

martelletto commented 5 years ago

does the diff below fix the problem for you?

diff --git src/hid_osx.c src/hid_osx.c
index faf05d4..a26e206 100644
--- src/hid_osx.c
+++ src/hid_osx.c
@@ -339,6 +339,16 @@ read_callback(void *context, IOReturn result, void *dev, IOHIDReportType type,
    }
 }

+static void
+removal_callback(void *context, IOReturn result, void *sender)
+{
+   (void)context;
+   (void)result;
+   (void)sender;
+
+   CFRunLoopStop(CFRunLoopGetCurrent());
+}
+
 int
 hid_read(void *handle, unsigned char *buf, size_t len, int ms)
 {
@@ -356,6 +366,7 @@ hid_read(void *handle, unsigned char *buf, size_t len, int ms)

    IOHIDDeviceRegisterInputReportCallback(dev->ref, buf, len,
        &read_callback, NULL);
+   IOHIDDeviceRegisterRemovalCallback(dev->ref, &removal_callback, dev);
    IOHIDDeviceScheduleWithRunLoop(dev->ref, CFRunLoopGetCurrent(),
        dev->loop_id);

@@ -364,6 +375,7 @@ hid_read(void *handle, unsigned char *buf, size_t len, int ms)
    while (r != kCFRunLoopRunHandledSource);

    IOHIDDeviceRegisterInputReportCallback(dev->ref, buf, len, NULL, NULL);
+   IOHIDDeviceRegisterRemovalCallback(dev->ref, NULL, NULL);
    IOHIDDeviceUnscheduleFromRunLoop(dev->ref, CFRunLoopGetCurrent(),
        dev->loop_id);

-p.

Silvercast commented 5 years ago

Sorry for long delay, the patch helps a tiny bit but I notice the signal is still jammed after a few times plugging out/in. On my test it's usually the 3rd time + . Switching between different keys also has the same issue. I'm testing against two Yubico keys on macOS fyi.

martelletto commented 5 years ago

On 09/09/2019 15:22, Silvercast wrote:

Sorry for long delay, the patch helps a tiny bit but I notice the signal is still jammed after a few times plugging out/in. On my test it's usually the 3rd time + . Switching between different keys also has the same issue. I'm testing against two Yubico keys on macOS fyi.

ok, thanks for the feedback. two questions:

1) are you able to reproduce the problem using one of the bundled examples, e.g. examples/cred.c? so that we have a common reference point. 2) there seems to be a delay of ~3 seconds between key detection and availability in macos; the key is observed by libfido2, but talking to it results in an i/o error. could it be related?

-p.

Silvercast commented 5 years ago

hi, our issue is a the combination of 3 factors:

I've tested examles/cred.c and it works well when plugging out/in devices. However I notice a large delay of detecting new keys when the program is killed (with Ctrl-C).

So the question still holds: how we can interrupt blocked API calls without getting random delay between tries.

martelletto commented 5 years ago

hi!

thanks once again for the feedback.

i believe we are looking at multiple issues here.

  • a delay of ~3 seconds between key detection and availability in macOS; the key is observed by libfido2, but talking to it results in an i/o error as you said

the initial delay is happening inside macos's USB subsystem. you will see it whenever a key is connected; it does not seem to matter whether the key has been previously seen by the OS.

to get some numbers, i wrote a small proof of concept (happy to share) with a looped call to fido_dev_info_manifest() sandwiched between two calls to gettimeofday(). what i observed was that, if the yubikey is in otp + fido + ccid mode, the initial delay is around 5 seconds. if the key is in fido-only mode, the delay goes down to ~3 seconds.

  • missing call of fido_dev_close when fido_dev_open returns something different than FIDO_OK

this is definitely a bug. from the application's perspective, a device should only be considered open iff fido_dev_open() returns FIDO_OK. i will look into it; thanks for the heads-up!

I've tested examles/cred.c and it works well when plugging out/in devices. However I notice a large delay of detecting new keys when the program is killed (with Ctrl-C).

if the device is disconnected, you will be subjected to the initial delay upon reinsertion.

however, if you ctrl-c during a pending operation and try to re-initiate communication with the key, you will hit an interesting case.

currently, there is no API call in libfido2 to cancel a pending operation. i've taken note and will implement a fido_dev_cancel() in the next version of libfido2.

-p.

martelletto commented 5 years ago

(the fido_dev_open() issue was addressed in d5978f2)

Silvercast commented 5 years ago

Awesome ! For now I put on a walkaround of fido_dev_open by calling fido_dev_close in my code because our application is very close to release. Look forward to next release of libfido2. Thanks.