6 ms·

Every time I pay enough attention to an OpenSSH release that I trick myself into reading some of the code, I'm left smh in disbelief that I actually rely on thi

Every time I pay enough attention to an OpenSSH release that I trick myself into reading some of the code, I'm left smh in disbelief that I actually rely on this software still.

jabiko 5y ago
Can you tell why that is? Did you find the OpenSSH source code especially bad? I am under the belief that OpenSSH is a really high quality, security focused software project.
- traceroute66 5y ago
  > Can you tell why that is? Yeah, I too find it a really bizarre comment to make, especially as it is not substantiated with any detail whatsoever. OpenSSH comes from the house of OpenBSD and LibreSSL. I am no security guru, but as I understand it, all three have a well deserved security reputation, and the core maintainers run a tight ship. We've already covered in recent weeks on HN how OpenSSH has maintained its robustness in the face of an increasingly hostile cyber world.[1] So, if you are going to imply that OpenSSH code is shit, back it up with hard facts. [1] https://news.ycombinator.com/item?id=30830749 https://news.ycombinator.com/item?id=30830749
  - pengaru 5y ago
    If you think this [0] is quality workmanship, I never want to work on your team. I haven't even exhaustively looked through this code, only took a quick glance at clientloop.c [1] and was curious about how the channels were integrated into the poll loop. This is a mess, and I'm pretty sure I already see a bug. [0] https://github.com/openssh/openssh-portable/blob/master/channels.c#L2636-L2743 https://github.com/openssh/openssh-portable/blob/master/chan... [1] https://github.com/openssh/openssh-portable/blob/master/clientloop.c#L1306-L1388 https://github.com/openssh/openssh-portable/blob/master/clie... Edit: I'm not looking at a future CVE, but I've definitely found a bug. For those of you tossing up low-effort straw men, actually go grok the code. The way they've integrated the poll loop into all the actual handling of the returned events is a sprawling mess, and the bug I've found is a direct result of that.
    - paskozdilar 5y ago
      I don't see anything so obviously wrong with [0]. I guess I'll never make it into your team ¯\_(ツ)_/¯
    - artogahr 5y ago
      Out of curiosity, can you explain what's wrong with that code? I dabbled in C a few years back but I'm nowhere near competent enough to find anything wrong with it.
      - faho 5y ago
        I'm also no C guru, but this seems to dereference a pointer before checking if it's in-bounds: struct pollfd *pfd = &pfds[p]; if (fd == -1) return; if (p == -1 || (u_int)p >= npfd) fatal_f("channel %d: bad pfd %d (max %u)", c->self, p, npfd); dump_channel_poll(__func__, what, c, p, pfd); If p is < 0 or >= npfd (the number of elements in pfds), this should read out of bounds of the pfds array, and the check happens too late. This is similar to a famous Linux kernel bug from ages ago, where a !NULL check was optimized away in such a situation, leading to an exploit: https://lwn.net/Articles/342330/ https://lwn.net/Articles/342330/ In this case I'm not aware if something like that happens, or if it happens to be harmless.
        aaronmdjones 5y ago
        This: struct pollfd *pfd = &pfds[p]; ... does not dereference pfds. It looks that way, but you need to account for the address-of operator before it. What it's actually doing is: struct pollfd *pfd = pfds + p; ... and thus it works even if pfds is NULL or if p is out of bounds. The following check before using pfd is correct.
        faho 5y ago
        Ah, okay. Personally I'd still want to do the check before, because someone could still be adding something in-between the assignment and the check, but if that's the order it's resolved in it seems to be working now. (I'm guessing they declare and assign the variable first because of C version constraints - wasn't that something that changed in C99?)
  - adastra22 5y ago
    > I am no security guru, but as I understand it, all three have a well deserved security reputation, and the core maintainers run a tight ship. Among us security gurus, OpenSSL has a really bad reputation. It's a giant pile of spaghetti code with a huge history of critical system vulnerabilities stemming from trying to be everything to everybody. Bitcoin is in the process of writing replacement functionality in order to remove OpenSSL entirely from its dependency tree because they got burned by major bugs multiple times in the past. A lot of modern alternatives like libsodium list "not OpenSSL" as a selling point. The situation is so bad that multiple organizations have forked OpenSSL to create stripped down alternatives (BoringSSL, LibreSSL) that are more likely to be secure. I know less about the OpenSSH code base specifically, except that it came out of the same culture and the same developers. I wouldn't use it in new projects unless I had no other choice.
    - zokier 5y ago
      > I know less about the OpenSSH code base specifically, except that it came out of the same culture and the same developers ummm what? Afaik openssh and openssl have no major common history? openssh comes from openbsd, openssl has always been independent project.
    - brynet 5y ago
      OpenSSH and LibreSSL are OpenBSD projects. OpenSSL has nothing to do with OpenBSD.
      - adastra22 5y ago
        Thank you for the correction!
daneel_w 5y ago
It's one of the most deployed pieces of software on the planet, and for that reason has a million eyes on it, yet there doesn't seem to be any alarming concern about the source quality. Maybe it's all right after all...
- tialaramex 5y ago
  Maybe. You've expressed the same sentiment as you'd have seen for OpenSSL just a few months before everybody decided OpenSSL was unacceptable and they'd fork it (producing BoringSSL, LibreSSL and so on). OpenBSD has some very peculiar preferences, some of which are in step with modern thinking (e.g. they like sandboxes, privilege dropping and defence in depth very much) but some very much are not (they like C despite its lack of Type Safety, Memory Safety, and generally just Safety) SecSH (the SSH v2 standard) is a fairly clean modern encrypted protocol design. You do a key exchange, you bring up an encrypted link and then everything else happens inside this, alleviating many of your security concerns, the same way TLS 1.3 works (but TLS 1.3 is much more recent). But the OpenSSH codebase dates back to even before SSHv2, it's basically the original SSH codebase, from back when encrypted remote shells was an entirely new idea, the mid-1990s, the SSH codebase went proprietary and OpenSSH is the fork. That's some crufty old code, and in C that's not an insignificant problem because problems in code you never read has potentially drastic consequences for unrelated code in the same binary.
  - jamespo 5y ago
    The reason they use C is as you said it's a fork. Being able to compile to almost anything is another factor I'd expect.
    - tialaramex 5y ago
      That would be an obvious reason why their SSH implementation is in C. But for example their ACME implementation (to get certificates from e.g. Let's Encrypt) is also in C, even though various more famous ACME clients that were started earlier and have better features are in more modern languages like Python. That C ACME client complies with OpenBSD's other preferences, it is privilege separated resulting in a bunch of separate programs running to handle different privileges for example, but it is written in C and lacks some obvious security features that aren't platform specific. Thus on OpenBSD a separate program handles your private key data from the one talking to the network. But, if you used a more capable ACME implementation for other platforms written in say, bash, a completely separate piece of software even on another machine can be talking to the network since that's just how the certificate process works. OpenBSD does not support this (last time I looked).
deleted 5y ago
[deleted]
aborsy 5y ago
OpenSSH is very well regarded in security community, has seen no major vulnerability for a long time.

Every time I pay enough attention to an OpenSSH release that I trick myself into reading some of the code, I'm left smh in disbelief that I actually rely on this software still.

jabiko 5y ago

Can you tell why that is? Did you find the OpenSSH source code especially bad? I am under the belief that OpenSSH is a really high quality, security focused software project.

traceroute66 5y ago
> Can you tell why that is? Yeah, I too find it a really bizarre comment to make, especially as it is not substantiated with any detail whatsoever. OpenSSH comes from the house of OpenBSD and LibreSSL. I am no security guru, but as I understand it, all three have a well deserved security reputation, and the core maintainers run a tight ship. We've already covered in recent weeks on HN how OpenSSH has maintained its robustness in the face of an increasingly hostile cyber world.[1] So, if you are going to imply that OpenSSH code is shit, back it up with hard facts. [1] https://news.ycombinator.com/item?id=30830749 https://news.ycombinator.com/item?id=30830749
- pengaru 5y ago
  If you think this [0] is quality workmanship, I never want to work on your team. I haven't even exhaustively looked through this code, only took a quick glance at clientloop.c [1] and was curious about how the channels were integrated into the poll loop. This is a mess, and I'm pretty sure I already see a bug. [0] https://github.com/openssh/openssh-portable/blob/master/channels.c#L2636-L2743 https://github.com/openssh/openssh-portable/blob/master/chan... [1] https://github.com/openssh/openssh-portable/blob/master/clientloop.c#L1306-L1388 https://github.com/openssh/openssh-portable/blob/master/clie... Edit: I'm not looking at a future CVE, but I've definitely found a bug. For those of you tossing up low-effort straw men, actually go grok the code. The way they've integrated the poll loop into all the actual handling of the returned events is a sprawling mess, and the bug I've found is a direct result of that.
  - paskozdilar 5y ago
    I don't see anything so obviously wrong with [0]. I guess I'll never make it into your team ¯\_(ツ)_/¯
  - artogahr 5y ago
    Out of curiosity, can you explain what's wrong with that code? I dabbled in C a few years back but I'm nowhere near competent enough to find anything wrong with it.
    - faho 5y ago
      I'm also no C guru, but this seems to dereference a pointer before checking if it's in-bounds: struct pollfd *pfd = &pfds[p]; if (fd == -1) return; if (p == -1 || (u_int)p >= npfd) fatal_f("channel %d: bad pfd %d (max %u)", c->self, p, npfd); dump_channel_poll(__func__, what, c, p, pfd); If p is < 0 or >= npfd (the number of elements in pfds), this should read out of bounds of the pfds array, and the check happens too late. This is similar to a famous Linux kernel bug from ages ago, where a !NULL check was optimized away in such a situation, leading to an exploit: https://lwn.net/Articles/342330/ https://lwn.net/Articles/342330/ In this case I'm not aware if something like that happens, or if it happens to be harmless.
      - aaronmdjones 5y ago
        This: struct pollfd *pfd = &pfds[p]; ... does not dereference pfds. It looks that way, but you need to account for the address-of operator before it. What it's actually doing is: struct pollfd *pfd = pfds + p; ... and thus it works even if pfds is NULL or if p is out of bounds. The following check before using pfd is correct.
        faho 5y ago
        Ah, okay. Personally I'd still want to do the check before, because someone could still be adding something in-between the assignment and the check, but if that's the order it's resolved in it seems to be working now. (I'm guessing they declare and assign the variable first because of C version constraints - wasn't that something that changed in C99?)
- adastra22 5y ago
  > I am no security guru, but as I understand it, all three have a well deserved security reputation, and the core maintainers run a tight ship. Among us security gurus, OpenSSL has a really bad reputation. It's a giant pile of spaghetti code with a huge history of critical system vulnerabilities stemming from trying to be everything to everybody. Bitcoin is in the process of writing replacement functionality in order to remove OpenSSL entirely from its dependency tree because they got burned by major bugs multiple times in the past. A lot of modern alternatives like libsodium list "not OpenSSL" as a selling point. The situation is so bad that multiple organizations have forked OpenSSL to create stripped down alternatives (BoringSSL, LibreSSL) that are more likely to be secure. I know less about the OpenSSH code base specifically, except that it came out of the same culture and the same developers. I wouldn't use it in new projects unless I had no other choice.
  - zokier 5y ago
    > I know less about the OpenSSH code base specifically, except that it came out of the same culture and the same developers ummm what? Afaik openssh and openssl have no major common history? openssh comes from openbsd, openssl has always been independent project.
  - brynet 5y ago
    OpenSSH and LibreSSL are OpenBSD projects. OpenSSL has nothing to do with OpenBSD.
    - adastra22 5y ago
      Thank you for the correction!

daneel_w 5y ago

It's one of the most deployed pieces of software on the planet, and for that reason has a million eyes on it, yet there doesn't seem to be any alarming concern about the source quality. Maybe it's all right after all...

tialaramex 5y ago
Maybe. You've expressed the same sentiment as you'd have seen for OpenSSL just a few months before everybody decided OpenSSL was unacceptable and they'd fork it (producing BoringSSL, LibreSSL and so on). OpenBSD has some very peculiar preferences, some of which are in step with modern thinking (e.g. they like sandboxes, privilege dropping and defence in depth very much) but some very much are not (they like C despite its lack of Type Safety, Memory Safety, and generally just Safety) SecSH (the SSH v2 standard) is a fairly clean modern encrypted protocol design. You do a key exchange, you bring up an encrypted link and then everything else happens inside this, alleviating many of your security concerns, the same way TLS 1.3 works (but TLS 1.3 is much more recent). But the OpenSSH codebase dates back to even before SSHv2, it's basically the original SSH codebase, from back when encrypted remote shells was an entirely new idea, the mid-1990s, the SSH codebase went proprietary and OpenSSH is the fork. That's some crufty old code, and in C that's not an insignificant problem because problems in code you never read has potentially drastic consequences for unrelated code in the same binary.
- jamespo 5y ago
  The reason they use C is as you said it's a fork. Being able to compile to almost anything is another factor I'd expect.
  - tialaramex 5y ago
    That would be an obvious reason why their SSH implementation is in C. But for example their ACME implementation (to get certificates from e.g. Let's Encrypt) is also in C, even though various more famous ACME clients that were started earlier and have better features are in more modern languages like Python. That C ACME client complies with OpenBSD's other preferences, it is privilege separated resulting in a bunch of separate programs running to handle different privileges for example, but it is written in C and lacks some obvious security features that aren't platform specific. Thus on OpenBSD a separate program handles your private key data from the one talking to the network. But, if you used a more capable ACME implementation for other platforms written in say, bash, a completely separate piece of software even on another machine can be talking to the network since that's just how the certificate process works. OpenBSD does not support this (last time I looked).

deleted 5y ago

[deleted]

aborsy 5y ago

OpenSSH is very well regarded in security community, has seen no major vulnerability for a long time.