3 ms·

It’s also worth reading the release notes https://www.openssh.com/releasenotes.html https://www.openssh.com/releasenotes.html This is actually an interesting v

by fanf2 2y ago

It’s also worth reading the release notes https://www.openssh.com/releasenotes.html https://www.openssh.com/releasenotes.html This is actually an interesting variant of a signal race bug. The vulnerability report says, “OpenBSD is notably not vulnerable, because its SIGALRM handler calls syslog_r(), an async-signal-safer version of syslog() that was invented by OpenBSD in 2001.” So a signal-safety mitigation encouraged OpenBSD developers to put non-trivial code inside signal handlers, which becomes unsafe when ported to other systems. They would have avoided this bug if they had done one of their refactoring sweeps to minimize the amount of code in signal handlers, according to the usual wisdom and common unix code guidelines.

djmdjm 2y ago
Theo de Raadt made an, I think, cogent observation about this bug and how to prevent similar ones: no signal handler should call any function that isn't a signal-safe syscall. The rationale is that, over time, it's too way easy for any transitive call (where it's not always clear that it can be reached in signal context) to pick up some call that isn't async signal safe.
- fanf2 2y ago
  Exactly, yes :-) Signal handlers have so many hazards it's vital to keep them as simple as possible.
  - growse 2y ago
    I'm not overly familiar with the language and tooling ecosystem, but how trivial is this to detect on a static analysis?
    - kccqzy 2y ago
      Quite easy.
  - rwmj 2y ago
    A rule I try to follow: either set a global variable or write to a self pipe (using the write syscall), and handle the signal in the main loop.
    - cesarb 2y ago
      > either set a global variable IIRC, the rule is also that said global variable must have the type "volatile sig_atomic_t".
  - lokar 2y ago
    You should read the sigsev handler at google, it’s great, doing all kinds of things. Of course it’s going to crash at some point anyway….
- ralferoo 2y ago
  I'm kind of surprised advocating calling any syscall other than signal to add the handler back again. It's been a long time since I looked at example code, but back in the mid 90s, everything I saw (and so informed my habits) just set a flag, listened to the signal again if it was something like SIGUSR1 and then you'd pick up the flag on the next iteration of your main loop. Maybe that's also because I think of a signal like an interrupt, and something you want to get done as soon as possible to not cause any stalls to the main program. I notice that nowadays signalfd() looks like a much better solution to the signal problem, but I've never tried using it. I think I'll give it a go in my next project.
  - qhwudbebd 2y ago
    In practice when I tried it, I wasn't sold on signalfd's benefits over the 90s style self-pipe, which is reliably portable too. Either way, being able to handle signals in a poll loop is much nicer than trying to do any real work in an async context.
  - formerly_proven 2y ago
    This isn't the case for OpenSSH but because a lot of environments (essentially all managed runtimes) actually do this transparently for you when you register a signal "handler" it might be that less people are aware that actual signal handlers require a ton of care. On the other hand "you can't even call strcmp in a signal handler or you'll randomly corrupt program state" used to be a favorite among practicing C lawyers.
    - lilyball 2y ago
      Why can't you call strcmp? I think a general practice of "only call functions that are explicitly blessed as async-signal-safe" is a good idea, which means not calling strcmp as it hasn't been blessed, but surely it doesn't touch any global (or per-thread) state so how can it corrupt program state? Update: according to https://man7.org/linux/man-pages/man7/signal-safety.7.html https://man7.org/linux/man-pages/man7/signal-safety.7.html strcmp() actually is async-signal-safe as of POSIX.1-2008 TC2.
INTPenis 2y ago
So it's very likely that some young sysadmin or intern that will have to patch for this vuln was not even born when OpenBSD implemented the solution.
- creshal 2y ago
  n>=1, one of our juniors is indeed younger than the OpenBSD fix and dealing with this bug.

by fanf2 2y ago

djmdjm 2y ago

Theo de Raadt made an, I think, cogent observation about this bug and how to prevent similar ones: no signal handler should call any function that isn't a signal-safe syscall. The rationale is that, over time, it's too way easy for any transitive call (where it's not always clear that it can be reached in signal context) to pick up some call that isn't async signal safe.

fanf2 2y ago
Exactly, yes :-) Signal handlers have so many hazards it's vital to keep them as simple as possible.
- growse 2y ago
  I'm not overly familiar with the language and tooling ecosystem, but how trivial is this to detect on a static analysis?
  - kccqzy 2y ago
    Quite easy.
- rwmj 2y ago
  A rule I try to follow: either set a global variable or write to a self pipe (using the write syscall), and handle the signal in the main loop.
  - cesarb 2y ago
    > either set a global variable IIRC, the rule is also that said global variable must have the type "volatile sig_atomic_t".
- lokar 2y ago
  You should read the sigsev handler at google, it’s great, doing all kinds of things. Of course it’s going to crash at some point anyway….
ralferoo 2y ago
I'm kind of surprised advocating calling any syscall other than signal to add the handler back again. It's been a long time since I looked at example code, but back in the mid 90s, everything I saw (and so informed my habits) just set a flag, listened to the signal again if it was something like SIGUSR1 and then you'd pick up the flag on the next iteration of your main loop. Maybe that's also because I think of a signal like an interrupt, and something you want to get done as soon as possible to not cause any stalls to the main program. I notice that nowadays signalfd() looks like a much better solution to the signal problem, but I've never tried using it. I think I'll give it a go in my next project.
- qhwudbebd 2y ago
  In practice when I tried it, I wasn't sold on signalfd's benefits over the 90s style self-pipe, which is reliably portable too. Either way, being able to handle signals in a poll loop is much nicer than trying to do any real work in an async context.
- formerly_proven 2y ago
  This isn't the case for OpenSSH but because a lot of environments (essentially all managed runtimes) actually do this transparently for you when you register a signal "handler" it might be that less people are aware that actual signal handlers require a ton of care. On the other hand "you can't even call strcmp in a signal handler or you'll randomly corrupt program state" used to be a favorite among practicing C lawyers.
  - lilyball 2y ago
    Why can't you call strcmp? I think a general practice of "only call functions that are explicitly blessed as async-signal-safe" is a good idea, which means not calling strcmp as it hasn't been blessed, but surely it doesn't touch any global (or per-thread) state so how can it corrupt program state? Update: according to https://man7.org/linux/man-pages/man7/signal-safety.7.html https://man7.org/linux/man-pages/man7/signal-safety.7.html strcmp() actually is async-signal-safe as of POSIX.1-2008 TC2.

INTPenis 2y ago

So it's very likely that some young sysadmin or intern that will have to patch for this vuln was not even born when OpenBSD implemented the solution.

creshal 2y ago
n>=1, one of our juniors is indeed younger than the OpenBSD fix and dealing with this bug.