Yubico / libfido2

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

hidraw: fallback to HID_NAME #769

Closed jo-bitsch closed 7 months ago

jo-bitsch commented 8 months ago

if we can't get the vendor/product string of the USB attributes, fallback to the HID_NAME string collected from the uevent.

This will be useful in linux when you provide a virtual fido2 device via /dev/uhid that emulates a USB connected HID device.

jo-bitsch commented 8 months ago

There was a off by one in the buffer size for the string, which the pipeline runner caught. I updated the PR accordingly.

jo-bitsch commented 8 months ago

There are 2 warning reported by the pipelines.

One is:

/home/runner/work/libfido2/libfido2/src/hid_linux.c:177:23: warning: Null pointer passed to 1st parameter expecting 'nonnull' [core.NonNullParamChecker]
  177 |                 char* first_space = strchr(hid_name, ' ');
      |                                     ^~~~~~~~~~~~~~~~~~~~~

This should be resolved by adding a hid_name != NULL to the previous condition.

The other is:

/home/runner/work/libfido2/libfido2/src/hid_linux.c:94:14: warning: Potential memory leak [unix.Malloc]
   94 |         while ((p = strsep(&cp, "\n")) != NULL && *p != '\0') {
      |                     ^~~~~~

I didn't change this line in my patch though.

This is in the context of s = cp = strdup(uevent) where later free(s); is called. I'm therefore not sure if this is a false positive. I'll think a bit more about how to best solve this.

jo-bitsch commented 7 months ago

My bad, it was in my patch after all, just the warning was slightly misleading to me. Once I ran

scan-build-17 --use-cc=clang-17 cmake -DCMAKE_BUILD_TYPE=Debug ..

and then

 scan-build-17 --use-cc=clang-17 --status-bugs make -C build-Debug

similar to how the pipeline ran the code, the potential memory leak became apparent.

I updated the PR accordingly.

jo-bitsch commented 7 months ago

I really appreciate the static analysis in the pipeline.

jo-bitsch commented 7 months ago

The fuzzer discovered another issue, which I think is a false positive. Nevertheless I slightly rewrote that line in copy_info to make it clear to the compiler that di->manufacturer and di->product cannot be NULL.

jo-bitsch commented 7 months ago

Yes, that is exactly the point: Make the listing of connected authenticators more human-friendly for virtual hid device (using /dev/uhid) or non-USB connected HID devices (if compiled with FIDO_HID_ANY)

jo-bitsch commented 7 months ago

I updated the PR to follow your 2 recommendations.

LDVG commented 7 months ago

Paging in @martelletto: I see that something similar to this has both been added to and removed from libfido2 previously. Do you have any additional context?

LDVG commented 7 months ago

We merged your changes with some additional tweaks on top. Thank you for your contribution!