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

Extend FIDO2 BLE support also for Linux #700

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

akemnade
Copy link

@akemnade akemnade commented Jun 3, 2023

For Windows it was already added via gh#336,
so let's also add it for Linux.
Unpaired devices are ignored, the user has to pair independently of libfido use using the bluetooth manager provided by the desktop environment.

@akemnade akemnade force-pushed the btsup branch 7 times, most recently from 2790ef6 to be35428 Compare June 6, 2023 12:54
@LDVG
Copy link
Contributor

LDVG commented Jun 7, 2023

Thank you for your contribution! Before having a more in-depth look, may I ask what authenticator you are testing this against?

@akemnade
Copy link
Author

akemnade commented Jun 7, 2023

This was tested with:

eSecu FIDO2 pro (passkey printed on device, 20 byte control point size, fragmentation often needed, bluetooth turned off after a short time
of inactivity)
AirID2 Mini (full Numerical comparison for pairing, >200 byte control point size, so fragmentation rarely needed),

@akemnade
Copy link
Author

about the ci-failures:
cifuzz: it does not install dependencies correctly, and they are not specified in this repo, so I cannot do anything there easily.
The other fuzzer:

==16136==WARNING: MemorySanitizer: use-of-uninitialized-value
5096
    #0 0x7ff5d26bb286  (/lib/x86_64-linux-gnu/libsystemd.so.0+0x4b286) (BuildId: e45f7492c0f62251620378d7224ad0371a8d1f98)
5097
    #1 0x7ff5d26b2ae7 in sd_bus_start (/lib/x86_64-linux-gnu/libsystemd.so.0+0x42ae7) (BuildId: e45f7492c0f62251620378d7224ad0371a8d1f98)
5098
    #2 0x7ff5d26b2e88 in sd_bus_open_system_with_description (/lib/x86_64-linux-gnu/libsystemd.so.0+0x42e88) (BuildId: e45f7492c0f62251620378d7224ad0371a8d1f98)
5099
    #3 0x7ff5d26ba157 in sd_bus_default_system (/lib/x86_64-linux-gnu/libsystemd.so.0+0x4a157) (BuildId: e45f7492c0f62251620378d7224ad0371a8d1f98)

How to debug this? sd_bus_default_system just takes a pointer to a variable on the stack to write some output into this, so nothing scary. Can this be resolved to source code lines?

@LDVG
Copy link
Contributor

LDVG commented Jun 12, 2023

How to debug this? sd_bus_default_system just takes a pointer to a variable on the stack to write some output into this, so nothing scary. Can this be resolved to source code lines?

MemorySanitizer requires the whole application to be instrumented -- libsystemd included.

We'd likely need to mock a lot of these calls to sensibly fuzz your implementation.

src/bluetooth_linux.c Outdated Show resolved Hide resolved
src/bluetooth_linux.c Outdated Show resolved Hide resolved
src/bluetooth_linux.c Outdated Show resolved Hide resolved
src/bluetooth_linux.c Outdated Show resolved Hide resolved
src/bluetooth_linux.c Outdated Show resolved Hide resolved
Copy link
Contributor

@LDVG LDVG left a comment

Choose a reason for hiding this comment

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

This is starting to take shape!

I have left a set of comments which mostly aim to get this PR into a shape that is consistent with our existing code base. Note that some of the comments apply to more than one occurrence. I'd like to generally mention:

  • We try to follow a basic KNF/style(9) format for our code. While not critical, I left a couple of nitpicking comments on this.
  • Some of the functions have conditionals/loops that nest very deeply, we'd prefer if we could split some of the logic out to separate functions to alleviate this.
  • We'd like for most return values to be checked and bubbled up to the calling function. Where it makes sense, we also like to log a debug message indicating what went wrong.

Once again, I'd like to thank you for your work so far!

CMakeLists.txt Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
src/bluetooth.c Outdated Show resolved Hide resolved
src/bluetooth.c Outdated Show resolved Hide resolved
src/bluetooth_linux.c Outdated Show resolved Hide resolved
src/extern.h Outdated Show resolved Hide resolved
src/bluetooth.c Outdated Show resolved Hide resolved
src/bluetooth_linux.c Outdated Show resolved Hide resolved
src/bluetooth_linux.c Outdated Show resolved Hide resolved
@akemnade
Copy link
Author

akemnade commented Jul 6, 2023

CMakeFiles/fido2_shared.dir/ble_linux.c.o.d -o CMakeFiles/fido2_shared.dir/ble_linux.c.o -c /__w/libfido2/libfido2/src/ble_linux.c
327
In file included from /__w/libfido2/libfido2/src/ble_linux.c:2:
328
/usr/include/elogind/systemd/sd-bus.h:96:43: error: ISO C restricts enumerator values to range of 'int' [-Wpedantic]
329
   96 |         SD_BUS_CREDS_UNIQUE_NAME        = 1ULL << 31,
330
      |                                           ^~~~
331
make[2]: *** [src/CMakeFiles/fido2.dir/build.make:569: src/CMakeFiles/fido2.dir/ble_linux.c.o] Error 1

seems like a problem in alpine + libelogind in the ci run

Copy link
Contributor

@LDVG LDVG 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 the updates. I've added a set of new comments, and also left a set of (potentially?) unresolved comments from the previous round.

src/ble_linux.c Outdated Show resolved Hide resolved
src/ble_linux.c Outdated Show resolved Hide resolved
src/ble_linux.c Show resolved Hide resolved
src/ble.c Show resolved Hide resolved
src/ble_linux.c Outdated Show resolved Hide resolved
@akemnade akemnade force-pushed the btsup branch 2 times, most recently from 7e34f64 to 35baeda Compare July 6, 2023 17:50
For Windows it was already added via gh#336,
so let's also add it for Linux.
Unpaired devices are ignored, the user has to pair independently
of libfido use using the bluetooth manager provided by the desktop
environment.
@akemnade
Copy link
Author

akemnade commented Jul 7, 2023

opened an issue for the alpine trouble:
https://gitlab.alpinelinux.org/alpine/aports/-/issues/15102

By adding the include path, the alpine build got broken. Don't ask me questions...

@akemnade
Copy link
Author

A summary of the alpine problem:
later versions of sd-bus.h in systemd/elogind use extension keyword to allow 64bit enums even in pedantic mode. So this is solved in alpine edge already and next release in November. Without extension these things are tolerated in system includes (not something added via -I). E.g. debian does not add -Isomething in pkg-config --cflags for libsystemd, so that gets ignored, alpine does, but has a copy of the header files in /usr/include/systemd besides of /usr/include/elogind, so things are found in any case.

So what can we do now:

  • just accept the alpine problem until it gets fixed
  • remove addition of include path for systemd in CMakeLists.txt
  • remove -pedantic
  • any hacks in the alpine build process (like removing /usr/include/elogind in the alpine container)

@jo-bitsch
Copy link
Contributor

I'm very interested in that functionality and would be willing to help fixing the remaining issues.

The alpine issue referenced above should be fixed in the current stable (as of the new release of alpine v.3.19 released last December 7, 2023). As the pipeline refers to latest (

container: alpine:latest
) this might just work.

Unfortunately, I can't inspect the fuzzing issues, as the log expired.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants