10 ms·

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 sof

by 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?)
        aaronmdjones 5y ago
        Yes; it's C99 that let you declare variables anywhere in a function, rather than only before any statements. So it could be rewritten as: if (fd == -1) return; if (p == -1 || (u_int)p >= npfd) fatal_f("channel %d: bad pfd %d (max %u)", c->self, p, npfd); struct pollfd *pfd = &pfds[p]; dump_channel_poll(__func__, what, c, p, pfd); This is how I would have written it (I exclusively write C99), but the OpenSSH developers target a lot more platforms that may not have reliable C99 compilers, and are understandably reticent to rely upon the GNU extensions to C89 that would also let them do this.
  - gkhartman 5y ago
    Have you created a new issue ticket for it on GitHub? If I'd spotted a bug, and felt strongly enough to comment on HN about it, opening a new ticket would be my next move.
    - pengaru 5y ago
      Hi Greg, that's the first thing I tried to do. Alas they have issues disabled on the github repo; https://github.com/openssh/openssh-portable/issues https://github.com/openssh/openssh-portable/issues redirects to /pulls. So I cloned the repo to start looking more closely at the code in a real editor with an eye towards making a PR. Fortunately the nature of the bug I've found seems unlikely to be a security issue, it's a busy-loop CPU-burning bug in clientloop.c's poll() integration.
      - gkhartman 5y ago
        Good point on the /issues redirect. They have a separate bug tracker it seems [0]. Thanks for taking the time to look into it. This sort of thing is what makes open source software great. P.S. I'm not Greg (added it to my profile->about section to clarify). [0] https://www.openssh.com/report.html https://www.openssh.com/report.html
- 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!

by 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?)
        aaronmdjones 5y ago
        Yes; it's C99 that let you declare variables anywhere in a function, rather than only before any statements. So it could be rewritten as: if (fd == -1) return; if (p == -1 || (u_int)p >= npfd) fatal_f("channel %d: bad pfd %d (max %u)", c->self, p, npfd); struct pollfd *pfd = &pfds[p]; dump_channel_poll(__func__, what, c, p, pfd); This is how I would have written it (I exclusively write C99), but the OpenSSH developers target a lot more platforms that may not have reliable C99 compilers, and are understandably reticent to rely upon the GNU extensions to C89 that would also let them do this.
- gkhartman 5y ago
  Have you created a new issue ticket for it on GitHub? If I'd spotted a bug, and felt strongly enough to comment on HN about it, opening a new ticket would be my next move.
  - pengaru 5y ago
    Hi Greg, that's the first thing I tried to do. Alas they have issues disabled on the github repo; https://github.com/openssh/openssh-portable/issues https://github.com/openssh/openssh-portable/issues redirects to /pulls. So I cloned the repo to start looking more closely at the code in a real editor with an eye towards making a PR. Fortunately the nature of the bug I've found seems unlikely to be a security issue, it's a busy-loop CPU-burning bug in clientloop.c's poll() integration.
    - gkhartman 5y ago
      Good point on the /issues redirect. They have a separate bug tracker it seems [0]. Thanks for taking the time to look into it. This sort of thing is what makes open source software great. P.S. I'm not Greg (added it to my profile->about section to clarify). [0] https://www.openssh.com/report.html https://www.openssh.com/report.html
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!