In article <[email protected]>, Maxime Villard <[email protected]> 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 >> > >
