Stack allocated buffers, questionable logic and a generally terrible style as well as a complete lack of comments.
Don't take my word for it, see for yourself:
https://github.com/reyk/httpd/blob/master/httpd/server.c
The "new" is a bit off too, the copyright runs 2006-2015.
On a more flamebaity note, I don't know why you'd even want to write something like this in C. Writing a server for one of the most prevalent network protocols on the Internet in C, in 2015, just seems like masochism. This code reinvents so many wheels for the 1000th time in C code history, it's just tiresome to read. C++ would've reduced and simplified the code substantially and there are a growing number of other fine choices these days.
There is nothing wrong with using the stack for what it was designed for.
The function names are self-explanatory.
The "style" might not be yours, but it doesn't make it bad.
And as described in the slides, it was derived from relayd.
Look longer.
Let me give you some examples:
Forward declarations for functions that could be avoided by re-arranging the code, using both 0 and -1 to indicate error returns from functions, bits like:
s = fd == -1 ? socket(ss->ss_family, SOCK_STREAM, IPPROTO_TCP) : fd;
if (s == -1)
goto bad;
> There is nothing wrong with using the stack for what it was designed for.
The stack should not be used to allocate buffers intended to hold data written by other routines or read from untrusted sources for reasons that have become painfully clear over the last couple of years.
> The function names are self-explanatory.
Strong disagree about the function names being self explanatory, plenty of the functions have non-obvious side-effects. The 'style' is asking for trouble and as for it being derived from 'relayd' that pretty much confirms that this isn't something new (which is actually a good thing), but an adaptation of something old to a new role (such adaptations are security wise something to be very wary of, re-purposing old code is a great way to find out what edge cases were missed previously).
Far more symbols are exported than necessary.
> The "style" might not be yours, but it doesn't make it bad.
There's a return at the end of a function returning void for no reason.
#if 0'd old code that should simply be purged.
In the server_log code I think there may be a path to get a double free of 'ptr' where it is used first in the block with the while loop, then not reset to NULL and re-used in the second block and freed if it is not NULL (which it still is from the previous block...).
The style used obscures this possibility.
s = fd == -1 ? socket(ss->ss_family, SOCK_STREAM, IPPROTO_TCP) : fd;
if (s == -1)
goto bad;
if (fd == -1)
{
s = -1;
goto bad;
}
else
{
s = socket(ss->ss_family, SOCK_STREAM, IPPROTO_TCP);
}
if (fd != -1)
{
s = fd;
}
else
{
s = socket(ss->ss_family, SOCK_STREAM, IPPROTO_TCP);
if (s == -1)
{
goto bad;
}
}
Ternary operators should, in my opinion, be put to the 3 am test. If you think you'd get confused at looking at code that uses one at 3 am, then you've not written it properly and should clarify.
I couldn't find any function that returns 0 on failure. Or do you mean a null pointer?
>The stack should not be used to allocate buffers intended to hold data written by other routines for reasons that have become painfully clear over the last couple of years.
You want to see a zero tolerance policy for them? I think this is a bit overprotective. Their maximum size is usually known and all called functions take a size argument (snprintf(), strftime(), strlcpy(), etc.). There's a chance one could make a mistake here, but C is the wrong language to dynamically allocate everything. The probability to forget to free it, including in every error path, is much higher in my eyes. But then again, a memory leak is probably preferable than a buffer overflow (of course, both are bad). There are also already many heap allocated buffers.
>Looking a bit longer at the server_log code I think there may be a path to get a double free of 'ptr' where it is used first in the block with the while loop, then not reset to NULL and re-used in the second block and freed if it is not NULL (which it still is from the previous block...)
I don't think so, but that code really is dodgy. Also, `ptr != NULL` is unnecessary (just mentioning since OpenBSD/LibreSSL developers like to mention that too).
server_socket_getport
An unknown address family will return a '0'.
> You want to see a zero tolerance policy for them?
No, but if you use that mechanism then it is preferable to have all the functions hitting that buffer to be visible from the scope of the declaration of the buffer. Passing it on to other libraries can cause problems when/if those libraries' maintainers mess up. So if you're defensive about this then you can't be hurt that way.
> I don't think so,
Agreed, looking still longer it looks like it will always end up with a NULL in it after the while, but that's very ugly to put it mildly.
> Also, `ptr != NULL` is unnecessary (just mentioning since OpenBSD/LibreSSL developers like to mention that too).
Yep. And there are plenty of other nitpicks like that but I don't even care that much about any of those, I mostly care about the way that the code is laid out making it an excellent place to hide some really nasty bugs.
I found this interesting article: http://daniel.haxx.se/blog/2014/10/25/pretending-port-zero-i...
So since 0 can be okay and (in_port_t)-1 (=65535) is okay, it has to be changed to return int and take a in_port_t* to be acceptable for you. The return value may not even matter if only AF_INET/AF_INET6 as family is possible - a comment could be of help here.
In this particular case, returning 0 doesn't necessarily indicate failure. Binding a socket to port 0 means you're asking the operating system to pick an available port for you, which one might argue is a reasonably safe default for unknown address families.
Let it crash, as close as possible to the point of origin of a problem is a very good principle.
There's a trick you can use to make remembering to free it more likely. Some people call it "RAII in C", I like to think of it as "nested allocations and errors": When you allocate something, or do something else which one, can fail, and two, must be undone (freed, closed, etc.), do the allocation and deallocation in properly nested pairs, and use gotos to jump to the code which undoes the last successful allocation if some allocation goes wrong. Basically, you arrange your code in a stack, where the most recent successful allocation is undone first, so you can neatly jump down to the code which undoes all successful allocations and doesn't try to undo the unsuccessful ones.
For example:
int foo(void)
{
int ret = 0; /* Variable we return to indicate
success or failure. Defaults to 0,
which is success. */
FILE *inf;
FILE *outf;
if ((inf = fopen("foo", "rb")) == NULL) {
ret = -1;
goto inf_fail;
}
if ((outf = fopen("bar", "wb")) == NULL) {
ret = -2;
goto outf_fail;
}
/* Do the actual work, now that you know you have
all the resources you need. */
fclose(outf);
outf_fail:
fclose(inf); /* If we jump here, we know we
successfully opened inf,
but not outf. */
inf_fail: /* If we jump here, we didn't actually
open squat, so all we can do is return
the status variable we set above. */
return ret;
}
int foo(void)
{
int ret = -1; /* Variable we return to indicate
success or failure. Defaults to -1,
which is failure. */
FILE *inf;
FILE *outf;
if (!(inf = fopen("foo", "rb"))) {
ret = -1;
return ret;
}
do {
if (!(outf = fopen("bar", "wb"))) {
ret = -2;
break;
}
do {
/* Do the actual work, now that you know you have
all the resources you need.
Use break if something fails.
*/
ret = 0;
} while(0);
fclose(outf);
} while(0);
fclose(inf);
return ret;
}
int foo(void)
{
int ret = 0; /* Variable we return to indicate
success or failure. Defaults to 0,
which is success. */
FILE *inf = NULL;
FILE *outf = NULL;
if ((inf = fopen("foo", "rb")) == NULL) {
ret = -1;
goto foo_fail;
}
if ((outf = fopen("bar", "wb")) == NULL) {
ret = -2;
goto foo_fail;
}
/* Do the actual work, now that you know you have
all the resources you need. */
foo_fail:
if (outf) {
fclose(outf);
outf = NULL;
}
if (inf) {
fclose(inf);
inf = NULL;
}
return ret;
}
But hey, why bother reviewing the code you call, the docs say that it is secure and that makes it so, right?
To trust the OpenBSD's code with regards to security is more of a safe bet than trusting a lot of other organization's code. But of course, that's a relative statement: maybe it's still a horrible assumption.
Given that and the fact httpd is made by people which know and care about the issues you point, there is much street cred to be had exhibiting a buffer overflow on the stack of this program.
""" OpenSSH is developed by two teams. One team does strictly OpenBSD-based development, aiming to produce code that is as clean, simple, and secure as possible. We believe that simplicity without the portability "goop" allows for better code quality control and easier review. The other team then takes the clean version and makes it portable (adding the "goop") to make it run on many operating systems -- the so-called -p releases, ie "OpenSSH 4.0p1". """
Obviously, I'm quoting from OpenSSH's page here, but I believe it's probably the same philosophy on why they're writing code that relies on the security properties of OpenBSD.
As the presentation says, it's based on relayd, so the code is not all new, a lot of it is reused.