On 15/03/2013, at 9:02 AM, Maxime Villard <[email protected]> wrote:
> Hi,
> there is a huge bug in the tftp daemon.
sure, but then there's also a huge bug in your fix by your own definition of
huge.
when oack fails it frees the client struct and everything hanging off it. now
you avoid the unconditional free of of the client options straight after oack
is called by going to the error label if oack fails, which calls nak, which
then uses client and tries to free it again.
nice find though. i'll put a fix in shortly which will look very much like
yours.
dlg
>
> -- /usr/src/usr.sbin/tftpd/tftpd.c --
> In tftp_open() on line 870, the daemon checks for options
> (OACK) and handle them through the oack() function. Then,
> it frees and NULLs the variable client->options.
>
> oack() - l.1390 - does some stuff, and if an error happens,
> client_free() is called with the client structure, which
> frees this structure, including client->options, but does
> not null it.
>
> So, when returning after an error, client->options is freed
> twice, causing a double-free and a crash.
>
> I succeeded in making the server crash, by changing the
> tsize and blksize options before transferring a small file
> in localhost. In fact, if you look on line 1432, you'll see
> that you just have to close the socket to make the server
> crash, just after sending the OACK packet.
>
> Typically, I just have to do:
> $ tftp localhost
> tftp> tsize 2
> Tsize option on.
> tftp> blksize 10
> tftp> get test
> [...wait ~ 10 sec...]
> tftpd in free(): error: chunk is already free 0x1a658ca65f40
>
> Here is a patch. I switched oack() to int, so that we can
> handle errors and call nak() to alert the client before
> closing the connection. nak() frees the client structure,
> so all goes well.
>
> Ok/Comments?
>
>
> Index: tftpd.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/tftpd/tftpd.c,v
> retrieving revision 1.8
> diff -u -r1.8 tftpd.c
> --- tftpd.c 13 Jul 2012 02:31:46 -0000 1.8
> +++ tftpd.c 15 Mar 2013 00:36:40 -0000
> @@ -168,7 +168,7 @@
> void tftp(struct tftp_client *, struct tftphdr *, size_t);
> void tftp_open(struct tftp_client *, const char *);
> void nak(struct tftp_client *, int);
> -void oack(struct tftp_client *);
> +int oack(struct tftp_client *);
> void oack_done(int, short, void *);
>
> void sendfile(struct tftp_client *);
> @@ -876,8 +876,8 @@
> goto error;
>
> if (client->options) {
> - oack(client);
> -
> + if (oack(client) == -1)
> + goto error;
> free(client->options);
> client->options = NULL;
> } else if (client->opcode == WRQ) {
> @@ -1386,7 +1386,7 @@
> /*
> * Send an oack packet (option acknowledgement).
> */
> -void
> +int
> oack(struct tftp_client *client)
> {
> struct opt_client *options = client->options;
> @@ -1436,10 +1436,10 @@
> oack_done, client);
>
> event_add(&client->sev, &client->tv);
> - return;
> + return 0;
>
> error:
> - client_free(client);
> + return -1;
> }
>
> int
>
