Another signal handler related bug

Posted Jul 10, 2024 16:19 UTC (Wed) by rweikusat2 (subscriber, #117920)
In reply to: Another signal handler related bug by make
Parent article: Another OpenSSH remote code execution vulnerability

> I think people should not write signal handlers at all, at least not on Linux; Linux has signalfd which is the only
> secure (and sane) way to handle signals.

signalfd requires the signals it's supposed to receive to be blocked to prevent default actions from being taken. There are at least three more ways to handle signals safely, ie, in such a way that code handling a signal can do whatever it wants without risking bad interactions with other code. They also required to set of handled signals to be blocked throughout ordinary program execution.

1. Define signal handlers via sigaction such that all handled signals are blocked during execution of any signal handlers. Only unblock signals as implicit effect of the p-type synchronous I/O mulitplexing call, ie, pselect, ppoll or epoll_pwait.

2. Define signal handlers as above, use signals for I/O readiness notification¹. Use a central loop which looks like this:

while (1) {
sigprocmask(SIG_SETMASK, &omask, NULL):
pause();
sigprocmask(SIG_BLOCK, &handled_set, NULL);
}

3. Use signals for I/O readiness notification and a loop built around sigwait, sigwaitinfo or sigtimedwait to receive signals.

¹ Linux supports signal-driven I/O for any number of file descriptors by providing a file descriptor and an I/O event type in struct siginfo.

Another signal handler related bug

Posted Jul 10, 2024 16:47 UTC (Wed) by rweikusat2 (subscriber, #117920) [Link]

Correcting some errors and omissions:

omask is supposed to be a sigset_t which was used to capture the signal mask the process was invoked with.

handled_set is supposed to be a sigset_t containing all signals which are supposed to be handled by the program.

Signal-driven I/O also requires using queued realtime signals which can be associated with a file descriptors via the F_SETSIG fcntl-operation. Otherwise, I/O events could get lost. Code employing this model also needs to handle possible signal queue overflows in some way.

Another signal handler related bug

Posted Jul 10, 2024 17:01 UTC (Wed) by make (subscriber, #62794) [Link] (3 responses)

1. is the old way of doing signals safely (before signalfd was invented), and I mentioned that here: https://lwn.net/Articles/981540/ - though I'm not sure if that's portable or Linux-only

2. still suffers from the race condition; SIG_SETMASK will immediately deliver all pending signals, and pause() will block forever, or am I missing something? (I've never used pause()) - and maybe a signal gets delivered between SIG_SETMASK and pause()

3. maybe you can use signals for I/O readiness, but what's the point? epoll/kqueue/... is so much easier; it has simpler semantics and fewer footguns (you named one example: signal queue overflows)

(None of this matters for OpenSSH, because they're using alarm() to implement a timeout for some blocking function which has no event loop. The SIGALRM handler simply exits the process.)

Another signal handler related bug

Posted Jul 10, 2024 18:03 UTC (Wed) by rweikusat2 (subscriber, #117920) [Link] (2 responses)

There's no race condition in 2) because the program (I've actually written programs in this way) is entirely signal driven. Once an event again occurs, a signal handler will be invoked which will interrupt the pause call as a side effect. The loop I posted is too complicated, though: It's sufficient to do the SIG_SETMASK before the loop which can just become

while (1) pause();

Queued realtime signals for I/O readiness notification is a somewhat older way than epoll to make synchronous I/O multiplexing scale better than possible with select/ poll because the complete interest set needs to be transferred to the kernel with every call of either. I used that once in a HTTP interception proxy supposed to provide content-filtering for an UTM appliance almost 20 years ago.

Another signal handler related bug

Posted Jul 10, 2024 18:24 UTC (Wed) by make (subscriber, #62794) [Link] (1 responses)

> There's no race condition in 2) because the program (...) is entirely signal driven

OK, I was confused by the two sigprocmask() calls which made no sense. If that's the whole main loop, there is indeed no race condition, because there is no race - not because the code is flawless.

> realtime signals for I/O readiness notification is a somewhat older way than epoll

I've never used that, that's why I asked: what's the point? I was wondering if I should learn about this or whether this is just obsolete stuff, and I should keep avoiding signals whenever possible.

Another signal handler related bug

Posted Jul 10, 2024 18:33 UTC (Wed) by rweikusat2 (subscriber, #117920) [Link]

>> There's no race condition in 2) because the program (...) is entirely signal driven
>OK, I was confused by the two sigprocmask() calls which made no sense. If that's the whole main loop, there is
> indeed no race condition, because there is no race - not because the code is flawless.

The code is correct with or without the superfluous sigprocmask calls in the loop as it doesn't rely on pause being interrupted. That's just something which may happen and the whole purpose of the loop is to work around that. Anything actually productive only happens because this or that signal handler got invoked by the kernel.