Some _FORTIFY_SOURCE far-fetched warnings are… funny!

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 
#include 
#include 

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;
}

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 
#include 
#include 

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;
}

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 
#include 
#include 

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;
}

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 report 2002:5e5f which 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 hostent array 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 hostent result 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.

2 thoughts on “Some _FORTIFY_SOURCE far-fetched warnings are… funny!

  1. The documentation says that gethostbyname can return AF_INET6, but quick testing did not bear that out. It definitely prefers AF_INET hosts for me when the name has both A and AAAA. I lack a host that has only AAAA to test.Your note about NULLs is a misreading of the documentation, I think. Recall that h_addr is a deprecated alias for h_addr_list[0]. As I read the man page, the value of h_length is the size of an individual returned record (4 for A, 16 for AAAA) and h_addr_list is a pointer to an array of pointers. The array consists of some number of pointers to addresses, with a NULL pointer as sentinel. There does not appear to be any specific guarantee about what bytes will follow the bytes of the returned address. In practice, the resolver seems to lay the records in right next to each other, such that h_addr_list[0] + h_length == h_addr_list[1].As for what is present in the stack after the variable, that depends on how the code is built. Stack smash protectors place a cookie between the return value and the variables of the function. Some SSP implementations also arrange for the array(s) to be highest in memory, so that scalar variables cannot be rewritten by a buffer overflow. I have also read of SSP implementations that copy some or all of the function parameters into shadow scalar variables during the prolog, so that an overflow cannot rewrite arguments either.

    Like

  2. Yes, @gethostbyname@ in GLIBC (2.1x) does seem to only return AF_INET (IPv4) addresses and thus does not create trouble with the function as is. This does not really mean much because libtirpc is supposed to be portable.As for this:

    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;

    I admit I was both wrong in expectations and not clear of what I was expecting either; as the documentation notes, the @h_addr_list@ array is structured as one of these:<typo:code lang=”c”>{ NULL }{ “xffx00xffx00”, NULL }{ “x00xffx00xff”, “xffx00xffx00″, NULL }</typo:code>Which is “An array of pointers to network addresses for the host (in network byte order), terminated by a NULL pointer.” as the documentation reports. So technically once again, it is a lot implementation-dependent. But the data the returned pointer comes from is static data, which means there is no allocation going on; in such a situation, I was (and am) quite sure that for any implementation the returned strings are taken from the same character array.This is definitely true for GLIBC, and it can be tested with a very simple program, not far from the examples I used in the post:<typo:code lang=”c”>#include <sys socket.h=””>#include <netdb.h>#include <string.h>#include <stdio.h>int main(int argc, char *argv[]) { struct hostent *hp; char **haddr; int i = 0; if ( argc != 2 || (hp = gethostbyname(argv[1])) == NULL || hp->h_length != 4) return -1; haddr = hp->h_addr_list; while( *haddr ) { printf(“%d %p %u.%u.%u.%un”, ++i, *haddr, (uint8_t)(*haddr)[0], (uint8_t)(*haddr)[1], (uint8_t)(*haddr)[2], (uint8_t)(*haddr)[3]); haddr++; } return 0;}/* Example run:% ./test3 http://www.google.com1 0xff8070 74.125.43.992 0xff8074 74.125.43.1473 0xff8078 74.125.43.1044 0xff807c 74.125.43.1065 0xff8080 74.125.43.1036 0xff8084 74.125.43.105*/</typo:code>This is also suggested by the GLIBC2-only extension @gethostbyname_r@ function that takes a character array buffer to store the returned values. The same buffer is used also to store the host canonical name and its aliases.Now what you’re right on, is that there is neither guarantee nor practical example on what follows the last entry. For some reason I assumed that it wasn’t an extremely stupid implementation and it had at least a NULL worth of padding between the two; seems like that’s not the case though, and indeed the @h_aliases@ values follow:<typo:code>(gdb) rStarting program: /home/flame/mytmpfs/test3 http://www.google.com1 0x602070 74.125.43.992 0x602074 74.125.43.1053 0x602078 74.125.43.1064 0x60207c 74.125.43.1045 0x602080 74.125.43.1476 0x602084 74.125.43.103Breakpoint 1, main (argc=2, argv=0x7fffffffd218) at test3.c:2828 return 0;(gdb) print *((char[4]*)0x602088)$4 = “www.”(gdb) print *hp->h_aliases$5 = 0x602088 “http://www.google.com“</typo:code>Where does this leave us? Well, not really any further than we were before; from one side, causing the situation on GLIBC seems to be impossible thanks to @gethostbyname@ never returning anything beside IPv4 addresses of size four; on the other hand the vague definition of the functions allows for unsafe layout of the structure’s data.For what it’s worth I didn’t entirely dream up about having a NULL pointer after the list of characters; that safeguard _is_ there for the @h_name@ value; although now that I think a bit more about it, it might just be aligning for AF_INET6 values (the base canonical name for Google is already padding to 16 bytes included the final NULL, so it would align to 32-bit without requiring further padding).

    Like

Leave a Reply

Fill in your details below or click an icon to log in:

WordPress.com Logo

You are commenting using your WordPress.com account. Log Out /  Change )

Google+ photo

You are commenting using your Google+ account. Log Out /  Change )

Twitter picture

You are commenting using your Twitter account. Log Out /  Change )

Facebook photo

You are commenting using your Facebook account. Log Out /  Change )

w

Connecting to %s