Re: TFTPD: Buffer Overflow(s)

[christos%astron.com@localhost (Christos Zoulas)]

In article <51D4618B.8020103%M00nBSD.net@localhost>,
Maxime Villard  <max%M00nBSD.net@localhost> wrote:
>There is an error in the fix which has been committed. What
>if snprintf() returns -1 ?

Yes, fixed.

christos
>
>- - 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
>> 
>
>