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.
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.
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/chan...
[1] 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.
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.
P.S. I'm not Greg (added it to my profile->about section to clarify).
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);
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/
In this case I'm not aware if something like that happens, or if it happens to be harmless.
As far as I know, C allows arbitrary pointer arithmetic (including pointing to an out-of-bounds address) as long as you don't read/write into the invalid memory. If the p check fails, nothing will be done with pfd.
If I'm wrong, please correct me.
struct pollfd *pfd = &pfds[p];
struct pollfd *pfd = pfds + p;
https://en.cppreference.com/w/c/language/operator_arithmetic...
Indirection through an invalid pointer value and passing an invalid pointer value to a deallocation function have undefined behavior. Any other use of an invalid pointer value has implementation-defined behavior.
Also note that you've quoted C++ reference. Just another example of where C++ stops being "compatible" with C.
Furthermore, we are not discussing operations on an invalid pointer value. We are talking about the addition operator operating with a pointer value and when its behaviour is defined.
Lastly, it is your reference here that is to a C++ language draft, not the C language.
EDIT (since I can't reply yet):
Official C standard documents are surprisingly hard to come by. The only place I've found one was genesis. Here's a direct quote about pointer arithmetic from ISO/IEC 9899: 1990, page 47:
---
When an expression that has integral type is added to or subtracted from a pointer, the result has the type of the pointer operand.
If the pointer operand points to an element of an array object, and the array is large enough, the result points to an element offset from the original element such that the difference of the subscripts of the resulting and original array elements equals the integral expression. In other words, if the expression P points to the i-th element of an array object, the expressions (P)+N (equivalently. N+(P) ) and (P)-N (where N has the value n) point to, respectively, the i+n-th and i-n-th elements of the array object, provided they exist.
Moreover, if the expression P points to the last element of an array object, the expression (P)+1 points one past the last element of the array object, and if the expression Q points one past the last element of an array object, the expression (Q)-1 points to the last element of the array object.
If both the pointer operand and the result point to elements of the same array object, or one past the last element of the array object, the evaluation shall not produce an overflow; otherwise, the behavior is undefined.
Unless both the pointer operand and the result point to elements of the same array object, or the pointer operand points one past the last element of an array object and the result points to an element of the same array object, the behavior is undefined if the result is used as an operand of the unary * operator.
Maybe I just suck at reading technical documents, but I can't figure out which part says that storing an invalid pointer is undefined behavior. Would you care to point it out for me?
If both the pointer operand and the result point to elements of the same array object, or one past the last element of the array object, the evaluation shall not produce an overflow; otherwise, the behavior is undefined.
C is a simple language, but it is hard to get it right sometimes.
Typically an invalid pointer is created by deallocating some memory, which transforms previously valid pointers to the memory into invalid pointers without the pointer values themselves changing their value.
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?)
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);
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);
}
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];
I guess I'll never make it into your team ¯\_(ツ)_/¯
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.
ummm what? Afaik openssh and openssl have no major common history? openssh comes from openbsd, openssl has always been independent project.