So the new Portage releases finally kills when sure-enough overflows are built and that’s for me good news: we’ll finally get to fix these problems rather than ignoring them until they hit a security issue.
But then, Kumba reported a failure in libtirpc, which I was the last one to play with, to fix NFS with Kerberos — as you can understand, it was something I felt I had to fix myself, since I might as well have been the cause.
Interestingly, the problem I could reproduce on my user system, but not on the tinderbox; but that was enough for me to have an idea where to start, so I finally was able to reduce the problem to this synthetic test case out of the libtirpc 0.2.1 sources:
#include <sys/socket.h>
#include <netdb.h>
#include <string.h>
int main(int argc, char *argv[]) {
struct sockaddr_in addr = {
.sin_family = AF_INET,
.sin_port = 0
};
struct hostent *hp;
if ((hp = gethostbyname(argv[1])) == NULL)
return -1;
if (hp->h_length > sizeof(addr))
hp->h_length = sizeof(addr);
memcpy(&addr.sin_addr.s_addr, hp->h_addr, (size_t)hp->h_length);
return 0;
}Code language: PHP (php)Interestingly enough, if you compile it with the minimum flags required for _FORTIFY_SOURCE diagnostics to be emitted – gcc -O2 -Wall test.c – you won’t find any warning in your output; there isn’t a sure-fire overflow in that case. So let’s look deeper at the code.
On the highest level you can look at the code, there just isn’t space for any overflow: we’re calling gethostbyname() which happens to only return IPv4 addresses, which are always 32-bit; so the h_length member of hostent is never going to be anything else than 4 (size in bytes of the address). So the code is, by any mean, safe.
If you look at the actual code much harder, you can notice that there is an extra failsafe: in any case you’re going to copy at most sizeof(addr) bytes… and here’s the catch; addr is a sockaddr structure, and as all of the other sockaddr structures, start with a single, 16-bit value indicating the actual address family. Additionally, what you’re actually going to copy is the IP address itself, (addr.sin_addr.s_addr which is neither the first nor the second member of addr, but it’s following the actual port. This does not only mean that the offset of s_addr is 8, but also that the size of the complete structure is 8, not 4, so the example code above can be rewritten as:
#include <sys/socket.h>
#include <netdb.h>
#include <string.h>
int main(int argc, char *argv[]) {
struct sockaddr_in addr = {
.sin_family = AF_INET,
.sin_port = 0
};
struct hostent *hp;
if ((hp = gethostbyname(argv[1])) == NULL)
return -1;
memcpy(&addr.sin_addr.s_addr, hp->h_addr, MAX(hp->h_length, 8));
return 0;
}Code language: PHP (php)It should be now much easier to identify where the problem is: you can have hp->h_length to be 4 (or less — more to follow) or it is between 5 and 8 inclusive. But once again, GCC is not complaining it with -O2, because it is not surely an overflow for the compiler, as the value is actually variable: it can be included in either the 0-8 range, or between 0x80000000-0xFFFFFFFF (the negative range). What is causing it to understand there is a dangerous situation? An old friend of mine, actually: -ftracer.
The description from the man page is not really something you look out for, but here it is:
Perform tail duplication to enlarge superblock size. This transformation simplifies the control flow of the function allowing other optimizations to do better job.
What it means is that it somehow allows different branches to duplicate their code, allowing for further optimisations of corner cases. I have reported before that this was causing a build failure in MPlayer last year, because a segment of handwritten inline asm was using global labels rather than local labels, and while it was present on an already-joined branch, gcc -ftracer was splitting it in two to optimise it better; if a single copy of the asm was there, the global labels worked just as fine, but once it was inlined twice, you’d have a conflict of labels. It was trivial to fix but took a while to track it down.
Now, thanks to the help of -S -O2 -ftracer -fverbose-asm I could tell what was going on in the code by looking at the generated asm; if I try to translate that back to C code (you don’t really want to look at x86 asm code when trying to track down a logic bug!), the result would be something very similar to this (it would be closer if I used goto instructions, but I’d rather not):
#include <sys/socket.h>
#include <netdb.h>
#include <string.h>
int main(int argc, char *argv[]) {
struct sockaddr_in addr = {
.sin_family = AF_INET,
.sin_port = 0
};
struct hostent *hp;
if ((hp = gethostbyname(argv[1])) == NULL)
return -1;
if (hp->h_length >= sizeof(addr))
memcpy(&addr.sin_addr.s_addr, hp->h_addr, sizeof(addr));
else
memcpy(&addr.sin_addr.s_addr, hp->h_addr, (size_t)hp->h_length);
return 0;
}Code language: PHP (php)And indeed, if you build that code with gcc -O2 -Wall, it will report the same overflow warning.
My first look at fixing this was only fixing the obvious problem: it should have compared h_length with addr.sin_addr.s_addr rather than just addr, so it would compare to 4, not 8… and that’s what I first committed; then I actually thought a bit more at the code, and noted that there are quite a few problems with code such as that:
- if the address is reported as IPv6 (although I admit I’m not sure if
gethostbyname()does that at all!), there is no way that the such-reduced address works at all; for my own IPv6 subnet, it would report2002:5e5fwhich is not even telling my full IPv4 address (it is a 6-to-4 mapping); - if the address has a size shorter than 32-bit, it’ll cause an overflow on the other side, as more data is copied from the
hostentarray than is present — do note that it it’s not a security problem or a possible buffer overflow, as the returned array is NULL-terminated, so it either reads garbage from the following entries, or it reads the 0-bytes of the NULL termination; any recent architecture has 32-bit sized NULLs as far as I know; - everything can break if the
hostentresult is actually providing a negative length; in such case you’d be reading a huge number of bytes, and causing a segfault at one point or another; I don’t think that case is going to happen ever though, so it shouldn’t be of concern.
Given these details, I updated the patch during the same sync cycle, so that if the length is not the same between the hostent and addr.sin_addr.s_addr, the getrpcport() call will simply fail.
Is the bug exploitable? I don’t think so but I wouldn’t bet my life on it.
I’m not extremely sure if it’s the client or the server to call getrpcport(), but it is going to ask the question to a nameserver and, from there, it means that you’re working on untrusted data. If gethostbyname() reports AF_INET6 hosts as well, an attacker could rewrite the four bytes below the addr object; if the code is built with optimisations turned on, that’s the only object on the stack, and I have sincerely no idea whether it’s the parameters or the return value that’s present at that point on the stack; if it’s the parameters, it shouldn’t really matter at all, return value might be a bit trickier, but on 64-bit systems it wouldn’t be able to do much good beside possibly causing either server or client to crash by jumping on an non-executable memory area.
At any rate, I hope you now have a bit more idea of why I’m interested that this stuff is actually looked at properly, and not simply ignored.