Hacker News Reader

Comment by fanf2 | Hacker News Reader

It’s also worth reading the release notes 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.

View on HN

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.

Exactly, yes :-) Signal handlers have so many hazards it's vital to keep them as simple as possible.

I'm not overly familiar with the language and tooling ecosystem, but how trivial is this to detect on a static analysis?

Quite easy.

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.

> either set a global variable

IIRC, the rule is also that said global variable must have the type "volatile sig_atomic_t".

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….

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.

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.

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.

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 strcmp() actually is async-signal-safe as of POSIX.1-2008 TC2.

That's the point. They weren't added until TC2 in 2016.

Right, it wasn't promised to be safe until then. That doesn't mean it was definitively unsafe before, just that you couldn't rely on it being safe. My question is how would a function like strcmp() actually end up being unsafe in practice given the trivial nature of what it does.

> surely it doesn't touch any global (or per-thread) state

Not necessarily. An implementation might choose to e.g. use some kind of cache similar to what the JVM does with interned strings, and then a function like strcmp() might behave badly if it happened to run while that cache was halfway through being rebuilt.

Contrived example, never seen in reality.

You could be surprised.

For example, recently I wanted to call `gettid()` in a signal handler. Which I guessed was just a simple wrapper around the syscall.

However, it seems this can cache the thread ID in thread local storage (can't remember exact details).

I switched to making a syscall instead.

Well, this is possible for sure, but not for primitive functions, such as strcmp. It just does not happen in practice.

For functions like strcmp, I think they must be signal safe, to be POSIX compliant.

https://man7.org/linux/man-pages/man7/signal-safety.7.html

If it's on this list I generally trust it is safe.

I guess my point is, that if it's not, even a simple function may appear safe, but could do surprising things.

The JVM does it in reality, I can't see why a C runtime wouldn't.

It in theory may, but no one does.

A function like strcmp() cannot assume that if it sees the same pointer multiple times that this pointer contains the same data, so there's no opportunity for doing any sort of caching of results. The JVM has a lot more flexibility here in that it's working with objects, not raw pointers to arbitrary memory.

> A function like strcmp() cannot assume that if it sees the same pointer multiple times that this pointer contains the same data

For arbitrary pointers no. But it could special-case e.g. string constants in the source code and/or pointers returned by some intern function (which is also how the JVM does it - for arbitrary strings, even though they're objects, it's always possible that the object has been GCed and another string allocated at the same location).

> but surely it doesn't touch any global (or per-thread) state so how can it corrupt program state?

On x86 and some other (mostly extinct) architectures that have string instructions, the string functions are usually best implemented using those (you might get a generation where there's a faster way and then microcode catches back up). And specifically (not just?) on x86 there was/is some confusion about who should or would restore some of the flags that control what these do. So you could end up with e.g. a memcpy or some other string instruction being interrupted by a signal handler and then it would continue doing what it did, but in the opposite direction, giving you wrong results or even resulting in buffer overflows (imagine interrupting a 1 MB memcpy that just started and then resuming it in the opposite direction).

Make no sense to me. OS restores all the registers incl. flags after leaving the signal handler. Besides, your scenario is not related to the handler _itself_ calling memcpy; it is about interrupting the main code. And it never ever destroys flags.

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.

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

Reply on news.ycombinator.com