5 ms·

From the diff introducing the bug [1], the issue according to the analysis is that the function was refactored from this: void sigdie(const char *fmt,...)

From the diff introducing the bug [1], the issue according to the analysis is that the function was refactored from this: void sigdie(const char *fmt,...) { #ifdef DO_LOG_SAFE_IN_SIGHAND va_list args; va_start(args, fmt); do_log(SYSLOG_LEVEL_FATAL, fmt, args); va_end(args); #endif _exit(1); } to this: void sshsigdie(const char *file, const char *func, int line, const char *fmt, ...) { va_list args; va_start(args, fmt); sshlogv(file, func, line, 0, SYSLOG_LEVEL_FATAL, fmt, args); va_end(args); _exit(1); } which lacks the #ifdef. What could have prevented this? More eyes on the pull request? It's wild that software nearly the entire world relies on for secure access is maintained by seemingly just two people [2]. [1] https://github.com/openssh/openssh-portable/commit/752250caabda3dd24635503c4cd689b32a650794 https://github.com/openssh/openssh-portable/commit/752250caa... [2] https://github.com/openssh/openssh-portable/graphs/contributors https://github.com/openssh/openssh-portable/graphs/contribut...

ghostpepper 2y ago
> It's wild that software nearly the entire world relies on for secure access is maintained by seemingly just two people obligatory xkcd https://xkcd.com/2347/ https://xkcd.com/2347/
unilynx 2y ago
It's always easy with hindsight to tell how to prevent something. In this case, a comment might have helped why the #ifdef was needed, eg void CloseAllFromTheHardWay(int firstfd) //Code here must be async-signal-safe! Locks may be in indeterminate state { struct rlimit lim; getrlimit(RLIMIT_NOFILE,&lim); for (int fd=(lim.rlim_cur == RLIM_INFINITY ? 1024 : lim.rlim_cur);fd>=firstfd;--fd) close(fd); } Although to be honest, getrlimit isn't actually on the list here: https://man7.org/linux/man-pages/man7/signal-safety.7.html https://man7.org/linux/man-pages/man7/signal-safety.7.html But I hope that removing the comment or modifying code with a comment about async-signal-safe might have been noticed in review. The code you quoted only has the mention SAFE_IN_SIGHAND to suggest that this code might need to be async-signal-safe
- loeg 2y ago
  The ifdef name was a big clue! "SIGHAND" is short for signal handler. Sure, there is an implicit connection here from "signal handler" to "all code must be async signal safe," but that association is pretty well known to most openssh authors and code reviewers. Oh well, mistakes happen.
devit 2y ago
One of these: 1. Using a proper programming language that doesn't allow you to setup arbitrary functions as signal handlers (since that's obviously unsafe on common libcs...) - e.g. you can't do that in safe Rust, or Java, etc. 2. Using a well-implemented libc that doesn't cause memory corruption when calling async-signal-unsafe functions but only deadlocks (this is very easy to achieve by treating code running in signals as a separate thread for thread-local storage access purposes), and preferably also doesn't deadlock (this requires no global mutexes, or the ability to resume interrupted code holding a mutex) 3. Thinking when changing and accepting code, not like the people who committed and accepted [1] which just arbitrarily removes an #ifdef with no justification 4. Using simple well-engineered software written by good programmers instead of OpenSSH
- bigiain 2y ago
  This is all great thinking. Except for those of us who live in a world where most of their OS and utilities and libraries were originally written decades before Rust existed, and often even before Java existed. And where "legacy" C code pretty much underpins everything running on the public internet and which you need to connect to. There's a very real risk that reimplementing every piece of code on a modern internet connected server in exciting new "safe" languages and libc-type things - by a bunch of "modern" programmers who do not have the learning experience of decades worth of mistakes - will end up with not just new implementation of old and fixed bugs and security problems, but also with new implementations that are incompatible in strange and hard to debug ways with every existing piece of software that uses SSH protocols as they are deployed in the field. I, for one, and not going to install and put into production version 1.0 of some new OpenSSH replacement written in Rust out Go or Java, which has a non zero chance of strange edge case bugs that are different when connecting to SSH on different Linux/BSD/Windows distros or versions, across different cpu architectures, and probably have subtly different bugs when connecting to different cloud hyperscalers.
- MyelinatedT 2y ago
  This comes across as quite scathing critique of an open source tool that has provided an extremely high standard of security and reliability over decades, despite being built on technologies that don’t offer the guardrails outlined in points (1) and (2). Point (3) seems like a personal attack on the developers/reviewer, who made human errors. Humans do in fact make mistakes, and the best build toolchain/test suite in the world won’t save you 100% of the time. Point (4) seems to imply that OpenSSH is not well-engineered, simple, or written by good programmers. While all of that is fairly subjective, it is (I feel) needlessly unkind. I’d invite you to recommend an alternative remote access technology with an equivalent track record of security and stability in this space — I’m not aware of any.
pja 2y ago
OpenBSD refactored their system to use async signal safe re-entrent syslog functions, so it’s possible that the author of this code simply assumed that it was safe to make this change, forgetting (or completely unaware) that other platforms (which the openBSD ssh devs don’t actually claim to support) were still using async unsafe functions.
theoma 2y ago
> What could have prevented this? More eyes on the pull request? It's wild that software nearly the entire world relies on for secure access is maintained by seemingly just two people [2]. It's open source. If you feel you could do a better job, then by all means, go ahead and fork it. You're not entitled to anything from open source developers. They're allowed to make mistakes, and they're allowed to have as many or as few maintainers/reviewers as they wish. https://gist.github.com/richhickey/1563cddea1002958f96e7ba9519972d9 https://gist.github.com/richhickey/1563cddea1002958f96e7ba95...
- squigz 2y ago
  Strangely aggressive and unproductive response. GP has valid points. They are not acting entitled in any way.
  - theoma 2y ago
    I disagree. An obvious interpretation of what is quoted is entitlement.
    - squigz 2y ago
      How would you say someone should express such concerns without coming off as entitled? Or do you think they're not valid concerns? Or do you really think that if someone has such concerns, their only recourse is to start contributing to the project? That project of course being one of the most security-sensitive projects one could imagine.
      - theoma 2y ago
        > How would you say someone should express such concerns without coming off as entitled? > Or do you really think that if someone has such concerns, their only recourse is to start contributing to the project? Yes, I think one way to not come off as entitled when being critical to volunteers is to also offer volunteer work yourself. And it's most helpful to provide feedback directly to the developers through their preferred means of communication. > Or do you think they're not valid concerns? Irrelevant what I think here, that's kind of the point. That's just my opinion. > That project of course being one of the most security-sensitive projects one could imagine. Agreed that the project is important. However, this is irrelevant, too, unless you're bolstering your "valid concerns" argument.

ghostpepper 2y ago

> It's wild that software nearly the entire world relies on for secure access is maintained by seemingly just two people obligatory xkcd https://xkcd.com/2347/ https://xkcd.com/2347/

unilynx 2y ago

It's always easy with hindsight to tell how to prevent something. In this case, a comment might have helped why the #ifdef was needed, eg void CloseAllFromTheHardWay(int firstfd) //Code here must be async-signal-safe! Locks may be in indeterminate state { struct rlimit lim; getrlimit(RLIMIT_NOFILE,&lim); for (int fd=(lim.rlim_cur == RLIM_INFINITY ? 1024 : lim.rlim_cur);fd>=firstfd;--fd) close(fd); } Although to be honest, getrlimit isn't actually on the list here: https://man7.org/linux/man-pages/man7/signal-safety.7.html https://man7.org/linux/man-pages/man7/signal-safety.7.html But I hope that removing the comment or modifying code with a comment about async-signal-safe might have been noticed in review. The code you quoted only has the mention SAFE_IN_SIGHAND to suggest that this code might need to be async-signal-safe

loeg 2y ago
The ifdef name was a big clue! "SIGHAND" is short for signal handler. Sure, there is an implicit connection here from "signal handler" to "all code must be async signal safe," but that association is pretty well known to most openssh authors and code reviewers. Oh well, mistakes happen.

devit 2y ago

One of these: 1. Using a proper programming language that doesn't allow you to setup arbitrary functions as signal handlers (since that's obviously unsafe on common libcs...) - e.g. you can't do that in safe Rust, or Java, etc. 2. Using a well-implemented libc that doesn't cause memory corruption when calling async-signal-unsafe functions but only deadlocks (this is very easy to achieve by treating code running in signals as a separate thread for thread-local storage access purposes), and preferably also doesn't deadlock (this requires no global mutexes, or the ability to resume interrupted code holding a mutex) 3. Thinking when changing and accepting code, not like the people who committed and accepted [1] which just arbitrarily removes an #ifdef with no justification 4. Using simple well-engineered software written by good programmers instead of OpenSSH

bigiain 2y ago
This is all great thinking. Except for those of us who live in a world where most of their OS and utilities and libraries were originally written decades before Rust existed, and often even before Java existed. And where "legacy" C code pretty much underpins everything running on the public internet and which you need to connect to. There's a very real risk that reimplementing every piece of code on a modern internet connected server in exciting new "safe" languages and libc-type things - by a bunch of "modern" programmers who do not have the learning experience of decades worth of mistakes - will end up with not just new implementation of old and fixed bugs and security problems, but also with new implementations that are incompatible in strange and hard to debug ways with every existing piece of software that uses SSH protocols as they are deployed in the field. I, for one, and not going to install and put into production version 1.0 of some new OpenSSH replacement written in Rust out Go or Java, which has a non zero chance of strange edge case bugs that are different when connecting to SSH on different Linux/BSD/Windows distros or versions, across different cpu architectures, and probably have subtly different bugs when connecting to different cloud hyperscalers.
MyelinatedT 2y ago
This comes across as quite scathing critique of an open source tool that has provided an extremely high standard of security and reliability over decades, despite being built on technologies that don’t offer the guardrails outlined in points (1) and (2). Point (3) seems like a personal attack on the developers/reviewer, who made human errors. Humans do in fact make mistakes, and the best build toolchain/test suite in the world won’t save you 100% of the time. Point (4) seems to imply that OpenSSH is not well-engineered, simple, or written by good programmers. While all of that is fairly subjective, it is (I feel) needlessly unkind. I’d invite you to recommend an alternative remote access technology with an equivalent track record of security and stability in this space — I’m not aware of any.

pja 2y ago

OpenBSD refactored their system to use async signal safe re-entrent syslog functions, so it’s possible that the author of this code simply assumed that it was safe to make this change, forgetting (or completely unaware) that other platforms (which the openBSD ssh devs don’t actually claim to support) were still using async unsafe functions.

theoma 2y ago

> What could have prevented this? More eyes on the pull request? It's wild that software nearly the entire world relies on for secure access is maintained by seemingly just two people [2]. It's open source. If you feel you could do a better job, then by all means, go ahead and fork it. You're not entitled to anything from open source developers. They're allowed to make mistakes, and they're allowed to have as many or as few maintainers/reviewers as they wish. https://gist.github.com/richhickey/1563cddea1002958f96e7ba9519972d9 https://gist.github.com/richhickey/1563cddea1002958f96e7ba95...

squigz 2y ago
Strangely aggressive and unproductive response. GP has valid points. They are not acting entitled in any way.
- theoma 2y ago
  I disagree. An obvious interpretation of what is quoted is entitlement.
  - squigz 2y ago
    How would you say someone should express such concerns without coming off as entitled? Or do you think they're not valid concerns? Or do you really think that if someone has such concerns, their only recourse is to start contributing to the project? That project of course being one of the most security-sensitive projects one could imagine.
    - theoma 2y ago
      > How would you say someone should express such concerns without coming off as entitled? > Or do you really think that if someone has such concerns, their only recourse is to start contributing to the project? Yes, I think one way to not come off as entitled when being critical to volunteers is to also offer volunteer work yourself. And it's most helpful to provide feedback directly to the developers through their preferred means of communication. > Or do you think they're not valid concerns? Irrelevant what I think here, that's kind of the point. That's just my opinion. > That project of course being one of the most security-sensitive projects one could imagine. Agreed that the project is important. However, this is irrelevant, too, unless you're bolstering your "valid concerns" argument.