TFTPD: Buffer Overflow(s)

[Maxime Villard <max%M00nBSD.net@localhost>]

Hi,
one week ago, I was comparing OpenBSD's TFTPD with NetBSD's one when
I stumbled across several buffer overflows that may occur when dealing
with specially crafted packets. I'm gonna explain quickly what it
consists in.

 - - src/libexec/tftpd/tftpd.c - -

Let's say you send this packet:

        /* header */
        "\x00\x01" // OACK packet
        "merde"    // file name
        "\x00"     // blank
        /* tm */
        "netascii" // transfert mode
        "\x00"     // blank
        /* blksize option */
        "blksize"  // option
        "\x00"     // blank
        "\x39"     // value = 9
        "\x00"     // blank

The packet will be handled that way:

 at l.86 : "char oackbuf[PKTSIZE];"  <-- Here, PKTSIZE = SEGSIZE + 4 = 516.
 at l.669: get_options() is called with 'oackbuf' and 'alen'. Here, alen = 2.
 at l.600: get_options() parses the received packet and calls the appropriate
           handler. With our packet, blk_handler() will be called with 'oackbuf'
           and 'alen' as arguments.
 at l.453: "strcpy(ack + *ackl, "blksize");
            *ackl += 8;"
           Here, ack=oackbuf and ackl=alen. So, each time blk_handler() is
           called, 'alen' is incremented of 8.

Now, what if you send this packet:

        /* header */
        "\x00\x01" // OACK packet
        "merde"    // file name
        "\x00"     // blank
        /* tm */
        "netascii" // transfert mode
        "\x00"     // blank
        /* blksize option */
        "blksize"  // option
        "\x00"     // blank
        "\x39"     // value = 9
        "\x00"     // blank
        /* blksize option, AGAIN */
        "blksize"  // option
        "\x00"     // blank
        "\x39"     // value = 9
        "\x00"     // blank

?

blk_handler() will be called two times, and ackl will be incremented two times.
So, if you send a big packet with lots of blksize options in it, 'alen' will be
incremented lots of times, so that it will end up being larger than 
sizeof(oackbuf).
At the end, you could get something like :  strcpy(ack + 5000, blksize);

Exploit here:

        http://M00nBSD.net/garbage/TFTPD-NetBSD.c

You can patch locally your TFTPD to see how big the overflow is:

-----

Index: tftpd.c
===================================================================
RCS file: /cvsroot/src/libexec/tftpd/tftpd.c,v
retrieving revision 1.39
diff -u -r1.39 tftpd.c
--- tftpd.c     29 Aug 2011 20:41:07 -0000      1.39
+++ tftpd.c     28 Jun 2013 08:15:05 -0000
@@ -450,6 +450,8 @@
        }
 
        tftp_blksize = bsize;
+       if (oackbuf == ack) syslog(LOG_ERR, "OK, same var: sizeof(ack) = 
%lu\n", sizeof(oackbuf));
+       syslog(LOG_ERR, "strcpy'ing in ack + %d\n", *ackl);
        strcpy(ack + *ackl, "blksize");
        *ackl += 8;
        l = sprintf(ack + *ackl, "%lu", bsize);

-----

With that, my tty tells me:
        OK, same var: sizeof(ack) = 516
        strcpy'ing in ack + 24992

It seems that it could also occur in other places, but I did not investigate
further.

Last thing: I think it would be better if the filename was validated BEFORE
parsing the options. With the previous exploit, the file specified by 'filename'
does not even need to be present on the server. I suggest moving the block at
l.687 above l.666, with some changes.

Maxime