After reading your description, I was expecting the patch to include a line
setting options to null after freeing it. Whatever else we do, shouldn't we do
that too?
On Fri, Mar 15, 2013 at 00:02, Maxime Villard wrote:
> Hi,
> there is a huge bug in the tftp daemon.
>
> -- /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
