good point.
On 16/03/2013, at 10:17 PM, Maxime Villard <rusty...@gmx.fr> wrote:
> Le 15/03/2013 12:17, David Gwynne a écrit :
>>
>> On 15/03/2013, at 9:02 AM, Maxime Villard <rusty...@gmx.fr> 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
>>>
>>
>>
>>
>
