Re: [PATCH] tftpd: DoS vuln
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 - 1.8
+++ tftpd.c 15 Mar 2013 00:36:40 -
@@ -168,7 +168,7 @@
voidtftp(struct tftp_client *, struct tftphdr *, size_t);
voidtftp_open(struct tftp_client *, const char *);
voidnak(struct tftp_client *, int);
-void oack(struct tftp_client *);
+intoack(struct tftp_client *);
voidoack_done(int, short, void *);
voidsendfile(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
Re: [PATCH] tftpd: DoS vuln
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 - 1.8
+++ tftpd.c 15 Mar 2013 00:36:40 -
@@ -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);
-voidoack(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
Re: [PATCH] tftpd: DoS vuln
What would that be used for? Look: client_free() frees
client-options, client-file AND client. So even if
you null client-options, client does no longer exist
as it has been freed. We should never use client after
calling client_free().
On 03/15/13 01:09, Ted Unangst wrote:
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 - 1.8
+++ tftpd.c 15 Mar 2013 00:36:40 -
@@ -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);
-voidoack(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
Re: [PATCH] tftpd: DoS vuln
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.
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 - 1.8
+++ tftpd.c 15 Mar 2013 00:36:40 -
@@ -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
Re: [PATCH] tftpd: DoS vuln
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 - 1.8
+++ tftpd.c 15 Mar 2013 00:36:40 -
@@ -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
