fido_dev_set_sigmask is broken on macOS
riastradh opened this issue · 9 comments
What version of libfido2 are you using?
1.12.0, git main
What operating system are you running?
macOS
What application are you using in conjunction with libfido2?
https://github.com/riastradh/fidocrypt
How does the problem manifest itself?
fido_dev_set_sigmask unconditionally fails with FIDO_ERR_INTERNAL on macOS, requiring a special case for macOS.
Most likely there is a way to change the sigmask during a CFRunLoop in macOS so that it atomically unblocks signals, but I'm not familiar enough with it to say.
Simply not using fido_dev_set_sigmask doesn't work, because then the signal will never be unblocked, so the effect of the signal will be lost. So the best alternative is to never block signals in the first place, which leads to a race condition where the effect of the signal may be lost depending on timing, or may not be lost if it is delivered during the blocking syscall underlying fido_hid_read/write.
In this case, the signal handler doesn't even need to do anything except guarantee that the next blocking syscall, or current one if it has already begun, will fail promptly with EINTR.
Is the problem reproducible?
yes
What are the steps that lead to the problem?
Call fido_dev_set_sigmask on macOS.
Does the problem happen with different authenticators?
yes
Please include the output of fido2-token -L.
N/A
Please include the output of fido2-token -I.
N/A
Please include the output of FIDO_DEBUG=1.
N/A
Hi Team, Is there any update for this issue? Thanks. I met the same issue. I saw hid_osx.c has empty implementation for this function.
Hi all,
Most likely there is a way to change the sigmask during a CFRunLoop in macOS so that it atomically unblocks signals, but I'm not familiar enough with it to say.
While I can't claim to be an expert on CFRunLoops either, I have not been able to find an equivalent mechanism. If anyone knows of one, contributions are welcome!
An alternative approach might be to have a fido_dev_interrupt operation or something, which uses a second file descriptor and select(2) or similar under the hood, or an equivalent mechanism with CFRunLoop -- even if CFRunLoop somehow doesn't have a reliable way to update the signal mask while it sleeps, surely it can multiplex I/O. Using an extra file descriptor isn't great, but it's better than an application hanging in the event of a race.
This would require changing all the HID back ends to wait with select(2) for either (a) I/O on the device, or (b) a notification from fido_dev_interrupt.
Ideally, libfido2 would provide operations that compose with other I/O multiplexing by performing state transitions when I/O is ready, rather than operating as blocking procedure calls; then you could write the blocking procedure calls as a combination of select(2) and libfido2 state transitions, so the application can have its own mechanism to interrupt the I/O loop that doesn't require signals/threads/whatever. But it's a lot of work to restructure things that way, of course.
Could something like main...martelletto:libfido2:osx_sigmask-ci work?
Possible -- I'm not familiar enough with the CFRunLoop APIs to say. The main questions are:
- Suppose the signal is delivered between the call to sigprocmask on line 552 and when CFRunLooprunInMode goes to sleep internally. Will this prevent the thread from going to sleep at all?
- Suppose the signal is delivered while the thread has already gone to sleep. Will the signal delivery wake the thread up?
From the documentation:
Therefore, execution of the block occurs the next time the run loop wakes up to handle another input source. If you want the work performed right away, you must explicitly wake up that thread using the CFRunLoopWakeUp function.
This suggests to me that the block won't run until the run loop is woken up by some other input source, which at this point can only be the FIDO device or timeout, not a signal -- and the point is to respond to a signal promptly without waiting to time out.
But maybe I misunderstood the documentation -- like I said, I'm not familiar with the CFRunLoop API, so I probably missed something.
Note: This logic would need to use pthread_sigmask instead of sigprocmask, so that it doesn't affect other threads' signal masks -- after all, the purpose here is to allow multiple threads to concurrently do synchronous I/O on different devices so that the first one to succeed can cancel all the other ones.
If it turns out this doesn't work, and CFRunLoop just has no way to atomically update the signal mask during sleep, another idea that occurred to me is to introduce one more API function that applications must invoke in order for a signal handler to wake libfido2 I/O on all platforms, say fido_dev_interrupt:
- On everything except macOS, fido_dev_interrupt can be a no-op, because
ppollalready atomically updates the signal mask and returns EINTR if interrupted by a signal. - On macOS, fido_dev_interrupt can (a) cause any subsequent CFRunLoopRunInMode to return immediately, or (b) cause any sleeping CFRunLoopRunInMode to wake and fail with EINTR.
By requiring fido_dev_interrupt to run in a signal handler in the same thread as the libfido2 I/O function (like fido_dev_make_cred, fido_dev_get_assert), there's no need to allocate an extra file descriptor in every device on platforms other than macOS.
This way, the macOS logic can non-atomically update the signal mask without introducing a race. The code would look something like this:
struct hid_osx {
...
sig_atomic_t interrupted;
}
/* fido_hid_read */
...
if ((error = pthread_sigmask(SIG_SETMASK, ctx->sigmaskp, &omask)) != 0)
return -1;
/* At this point, fido_dev_interrupt will prevent CFRunLoop from sleeping, or wake it if asleep. */
IOHIDDeviceScheduleWithRunLoop(ctx->ref, CFRunLoopGetCurrent(),
ctx->loop_id);
...
CFRunLoopRunInMode(ctx->loop_id, (double)ms/1000.0, true);
IOHIDDeviceUnscheduleFromRunLoop(ctx->ref, CFRunLoopGetCurrent(),
ctx->loop_id);
if ((error = pthread_sigmask(SIG_SETMASK, &omask, NULL)) != 0)
return -1;
if (ctx->interrupted) {
ctx->interrupted = 0;
return -1;
}
if ((r = read(ctx->report_pipe[0], buf, len)) == -1) {
...
To implement fido_dev_interrupt on macOS, my first guess was to use CFRunLoopStop or CFRunLoopWakeUp, but it looks like they're not safe to use in a signal handler, if https://opensource.apple.com/source/CF/CF-1153.18/CFRunLoop.c.auto.html is any indication. So, the macOS version could use a second pipe, which fido_dev_interrupt writes a single byte to, and which is also registered as an input source for the run loop so fido_hid_read/write are woken by it. Writing to a pipe is safe to use in a signal handler. Or, the report_pipe could be multiplexed for both purposes, to avoid wasting extra file descriptors -- just extend each record transmitted through report_pipe by one byte so you can distinguish report input from interruption.
Possible -- I'm not familiar enough with the CFRunLoop APIs to say. The main questions are:
- Suppose the signal is delivered between the call to sigprocmask on line 552 and when CFRunLooprunInMode goes to sleep internally. Will this prevent the thread from going to sleep at all?
- Suppose the signal is delivered while the thread has already gone to sleep. Will the signal delivery wake the thread up?
I think so (on both questions), since we're already executing the loop at point. But I honestly have no idea. I'm equally unfamiliar with the CFRunLoop API.
From the documentation:
Therefore, execution of the block occurs the next time the run loop wakes up to handle another input source. If you want the work performed right away, you must explicitly wake up that thread using the CFRunLoopWakeUp function.
This suggests to me that the block won't run until the run loop is woken up by some other input source, which at this point can only be the FIDO device or timeout, not a signal -- and the point is to respond to a signal promptly without waiting to time out.
But maybe I misunderstood the documentation -- like I said, I'm not familiar with the CFRunLoop API, so I probably missed something.
My understanding is that the call to CFRunLoopRunInMode triggers the loop. At least that's what I observe if I comment out the call to IOHIDDeviceScheduleWithRunLoop and invoke the loop with only a CFRunLoopPerformBlock that triggers a call to a function that prints to stdio, with no resource associated with the loop.
Note: This logic would need to use pthread_sigmask instead of sigprocmask, so that it doesn't affect other threads' signal masks -- after all, the purpose here is to allow multiple threads to concurrently do synchronous I/O on different devices so that the first one to succeed can cancel all the other ones.
Ack.
If it turns out this doesn't work, and CFRunLoop just has no way to atomically update the signal mask during sleep, another idea that occurred to me is to introduce one more API function that applications must invoke in order for a signal handler to wake libfido2 I/O on all platforms, say fido_dev_interrupt:
- On everything except macOS, fido_dev_interrupt can be a no-op, because
ppollalready atomically updates the signal mask and returns EINTR if interrupted by a signal.- On macOS, fido_dev_interrupt can (a) cause any subsequent CFRunLoopRunInMode to return immediately, or (b) cause any sleeping CFRunLoopRunInMode to wake and fail with EINTR.
By requiring fido_dev_interrupt to run in a signal handler in the same thread as the libfido2 I/O function (like fido_dev_make_cred, fido_dev_get_assert), there's no need to allocate an extra file descriptor in every device on platforms other than macOS.
This way, the macOS logic can non-atomically update the signal mask without introducing a race. The code would look something like this:
struct hid_osx { ... sig_atomic_t interrupted; } /* fido_hid_read */ ... if ((error = pthread_sigmask(SIG_SETMASK, ctx->sigmaskp, &omask)) != 0) return -1; /* At this point, fido_dev_interrupt will prevent CFRunLoop from sleeping, or wake it if asleep. */ IOHIDDeviceScheduleWithRunLoop(ctx->ref, CFRunLoopGetCurrent(), ctx->loop_id); ... CFRunLoopRunInMode(ctx->loop_id, (double)ms/1000.0, true); IOHIDDeviceUnscheduleFromRunLoop(ctx->ref, CFRunLoopGetCurrent(), ctx->loop_id); if ((error = pthread_sigmask(SIG_SETMASK, &omask, NULL)) != 0) return -1; if (ctx->interrupted) { ctx->interrupted = 0; return -1; } if ((r = read(ctx->report_pipe[0], buf, len)) == -1) { ...To implement fido_dev_interrupt on macOS, my first guess was to use CFRunLoopStop or CFRunLoopWakeUp, but it looks like they're not safe to use in a signal handler, if https://opensource.apple.com/source/CF/CF-1153.18/CFRunLoop.c.auto.html is any indication. So, the macOS version could use a second pipe, which fido_dev_interrupt writes a single byte to, and which is also registered as an input source for the run loop so fido_hid_read/write are woken by it. Writing to a pipe is safe to use in a signal handler. Or, the report_pipe could be multiplexed for both purposes, to avoid wasting extra file descriptors -- just extend each record transmitted through report_pipe by one byte so you can distinguish report input from interruption.
I'm not sure I understand the extension approach. Would the distinction happen based on the value of the extra byte?
Possible -- I'm not familiar enough with the CFRunLoop APIs to say. The main questions are:
- Suppose the signal is delivered between the call to sigprocmask on line 552 and when CFRunLooprunInMode goes to sleep internally. Will this prevent the thread from going to sleep at all?
- Suppose the signal is delivered while the thread has already gone to sleep. Will the signal delivery wake the thread up?
I think so (on both questions), since we're already executing the loop at point. But I honestly have no idea. I'm equally unfamiliar with the CFRunLoop API.
I'm struggling to see how (1) could work because normally, if a signal interrupts a running thread (and the handler does nothing), there are no side effects on subsequent system calls like preventing sleep. So I think there's still a race condition here, unless something about the CFRunLoop API remembers something about signals that system calls normally don't.
If (2) works (and it may depend on how CFRunLoopRunInMode works internally, and whether it has an EINTR loop), I suspect that the subsequent read will fail with EAGAIN (because there's nothing actually ready to read from ctx->report_pipe[0]) rather than EINTR (indicating that a signal was delivered). But I guess in this code path that might not matter -- it would get translated to -1 inside libfido2 either way.
I'm not sure I understand the extension approach. Would the distinction happen based on the value of the extra byte?
Yes -- say, write 0x00 for a regular report, or 0x01 for an interrupt. Just another way to transmit two types of information: two pipes, vs one pipe and an extra bit in each message.
Let's try solve this from another way, by inspecting the src/CMakeLists.txt we can see src/hid_unix.c was not included, and the reason is Darwin does not provide ppoll() function which causing undefined reference.
// src/hid_unix.c:69
int
fido_hid_unix_wait(int fd, int ms, const fido_sigset_t *sigmask)
{
...
if ((r = ppoll(&pfd, 1, ms > -1 ? &ts : NULL, sigmask)) < 1) {
if (r == -1)
fido_log_error(errno, "%s: ppoll", __func__);
return (-1);
}
return (0);
}While fido_dev_set_sigmask implementation under src/dev.c requires an address of fido_hid_read being stored to io.read of a pointer fido_dev_t *dev which going to be used by other functions, Darwin fido_hid_read shall match the behavior that other implementations do, which be something like
// src/hid_freebsd.c:270
int
fido_hid_read(void *handle, unsigned char *buf, size_t len, int ms)
{
struct hid_freebsd *ctx = handle;
ssize_t r;
if (len != ctx->report_in_len) {
fido_log_debug("%s: len %zu", __func__, len);
return (-1);
}
if (fido_hid_unix_wait(ctx->fd, ms, ctx->sigmaskp) < 0) {
fido_log_debug("%s: fd not ready", __func__);
return (-1);
}
if ((r = read(ctx->fd, buf, len)) == -1) {
fido_log_error(errno, "%s: read", __func__);
return (-1);
}
if (r < 0 || (size_t)r != len) {
fido_log_debug("%s: %zd != %zu", __func__, r, len);
return (-1);
}
return ((int)r);
}freebsd/openbsd/netbsd/linux has literally same implementation except the ctx struct, that all calls fido_hid_unix_wait which defines under src/hid_unix.c, the source file not included while targeting Darwin. causing a null pointer being passed to fido_dev_set_sigmask's dev->io.read, then there's no need to deal with a null pointer on macOS with final fido_hid_set_sigmask.
So the critical issue is the implementation of ppoll on macOS. Luckily I have been participate epoll-shim which actually emulated a ppoll based on poll and sigprocmask/pthread_sigmask, by grabbing this implementation we can then achieve fido_hid_open and fido_hid_read like how other unix like systems do.
Let's try solve this from another way, by inspecting the
src/CMakeLists.txtwe can seesrc/hid_unix.cwas not included, and the reason is Darwin does not provideppoll()function which causing undefined reference.
I don't think that's why libfido2 doesn't use hid_unix.c on macOS. Rather, macOS exposes access to devices through an entirely different API, IOKit and CoreFoundation, which don't expose file descriptors that can be used with ppoll at all (and internally, they probably work with Mach ports and a different I/O multiplexing system call).
Luckily I have been participate epoll-shim which actually emulated a
ppollbased onpollandsigprocmask/pthread_sigmask, by grabbing this implementation we can then achievefido_hid_openandfido_hid_readlike how other unix like systems do.
This implementation of ppoll may enable programs to compile, but they will be broken if you try to use it -- and, worse, the brokenness will manifest only in rare circumstances of race conditions with signal delivery, making it extremely difficult to diagnose.
The problem is that there is a window in your ppoll implementation when signals are unblocked but control is still in userland, where the signal handler has no opportunity to cause the blocking system call to fail promptly with EINTR:
137 if (sigmask != NULL) {
138 (void)pthread_sigmask(SIG_SETMASK, sigmask, &origmask);
139 }
140
141 int ready = real_poll(fds2 ? fds2 : fds, nfds + !!(fds2), timeout_ms);
If a signal is delivered at line 140, it can set a flag or print a message or dance a jig, but the logic it interrupted has no way to know that a signal was delivered and that it must not block in real_poll. That's the whole point of fido_dev_set_sigmask -- it makes the libfido2 I/O operations atomically unblock signals and wait for I/O as a single action, so that if a signal is delivered it is guaranteed that the wait for I/O will return promptly rather than hang indefinitely.