Le 15/03/2013 12:17, David Gwynne a écrit :
>
> 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.
>
No. I removed the call to client_free() in oack():
- client_free(client);
+ return -1;
There is no double-free in my patch. But I saw you committed
fixes, so ok now.
> 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
>>
>
>
>
