4 ms·

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

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.
        mdaniel 5y ago
        For consideration, "declare variable" is just that; one could still choose to only assign to it after having done the validation (assuming fatal_f causes control flow termination) struct pollfd *pfd; if (p == -1 || (u_int)p >= npfd) fatal_f("channel %d: bad pfd %d (max %u)", c->self, p, npfd); pfd = &pfds[p]; right?
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

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.
      - mdaniel 5y ago
        For consideration, "declare variable" is just that; one could still choose to only assign to it after having done the validation (assuming fatal_f causes control flow termination) struct pollfd *pfd; if (p == -1 || (u_int)p >= npfd) fatal_f("channel %d: bad pfd %d (max %u)", c->self, p, npfd); pfd = &pfds[p]; right?

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