Re: ircii-pana (BitchX) 74p4 overflow - exploit/fix

[dark () XS4ALL NL (Richard Braakman)]

Michal Zalewski wrote:

> --- newio.c.orig        Tue Nov 18 04:49:28 1997
> +++ newio.c     Mon May 25 13:25:58 1998
> @@ -296,7 +296,7 @@
>         {
>                 if (((str[cnt] = ioe->buffer[ioe->read_pos++])) == '\n')
>                         break;
> -               cnt++;
> +               if (++cnt>=BIG_BUFFER_SIZE) ioe->read_pos=ioe->write_pos;
>         }
>
>         /*

As you said, this only addresses the specific problem.  The real
problem is that dgets() has no way to know its callers buffer size.
It is sometimes called with buffers that are considerably smaller than
BIG_BUFFER_SIZE, and in one case with a buffer whose size can vary at
run time.

The proper patch is to give dgets another parameter to specify the
caller's buffer size.  Fortunately there are only about a dozen
calls to it.

I'm afraid I couldn't test this patch much, because I'm behind a
masquerading host and finger daemons don't like me.  So in the grand
tradition of "Well, it compiles", I decided to send it here for
review.  (The alternative would be to sit on it until after the
Linux Expo.)

diff -ur ircii-pana-0.74p2/include/newio.h ircii-pana-0.74p2-patched/include/newio.h
--- ircii-pana-0.74p2/include/newio.h   Fri Oct 17 19:56:26 1997
+++ ircii-pana-0.74p2-patched/include/newio.h   Wed May 27 21:23:29 1998
@@ -10,7 +10,7 @@

 extern         int     dgets_errno;

-       int     dgets                   (char *, int, int);
+       int     dgets                   (char *, int, int, int);
        int     new_select              (fd_set *, fd_set *, struct timeval *);
        int     new_open                (int);
        int     new_close               (int);
diff -ur ircii-pana-0.74p2/source/dcc.c ircii-pana-0.74p2-patched/source/dcc.c
--- ircii-pana-0.74p2/source/dcc.c      Sat Jan  3 22:54:55 1998
+++ ircii-pana-0.74p2-patched/source/dcc.c      Wed May 27 21:29:24 1998
@@ -1736,7 +1736,7 @@
        int     sra;
        char    tmp[MAX_DCC_BLOCK_SIZE + 1];
        char    tmp2[MAX_DCC_BLOCK_SIZE + 1];
-       char    *s = NULL, *bufptr = NULL;
+       char    *s = NULL;
        char    *buf = NULL;
        u_32int_t       bytesread;
        int     type = Client->flags & DCC_TYPES;
@@ -1769,9 +1769,8 @@
                return;
        }
         s = Client->buffer;
-        bufptr = tmp;

-       bytesread = dgets(bufptr, Client->read, 1);
+       bytesread = dgets(tmp, sizeof(tmp), Client->read, 1);
        switch (bytesread)
        {
                case -1:
@@ -1946,7 +1945,8 @@
                        bufptr += len;
                }
         }
-       switch(bytesread = dgets(bufptr, Client->read, 0))
+       switch(bytesread = dgets(bufptr, sizeof(tmp) - (bufptr - tmp),
+                                Client->read, 0))
        {
                case -1:
                {
@@ -3804,7 +3804,6 @@
 int on = 1, data = -1, s1 = -1;
 char *a, *p;
 DCC_list *Client = NULL;
-char *bufptr;
        if (client->in_ftp)
        {
                put_it("%s", convert_output_format("$G %gFTP%n already transfering a file.", NULL, NULL));
@@ -3834,12 +3833,11 @@
        dcc_printf(client->read, "stor %s\n", args);

        memset(tmp, 0, sizeof(tmp));
-       bufptr = tmp;
        while (1)
        {
-               if (dgets(bufptr, client->read, 1) == -1 && dgets_errno > 0)
+               if (dgets(tmp, sizeof(tmp), client->read, 1) == -1 && dgets_errno > 0)
                        goto error_ftpsend;
-               if (*bufptr == '5')
+               if (tmp[0] == '5')
                        goto error_ftpsend;
                else if (strstr(tmp, "BINARY mode data connection"))
                        break;
@@ -3879,7 +3877,7 @@
 int len = sizeof(struct sockaddr_in);
 char tmp[2048];
 int on = 1, data = -1, s1 = -1;
-char *a, *p, *bufptr;
+char *a, *p;
 DCC_list *Client = NULL;
 off_t filesize = 0;
 char *filename = NULL;
@@ -3914,12 +3912,11 @@
        dcc_printf(client->read, "retr %s\n", args);

        memset(tmp, 0, sizeof(tmp));
-       bufptr = tmp;
        while (1)
        {
-               if (dgets(bufptr, client->read, 1) == -1 && dgets_errno > 0)
+               if (dgets(tmp, sizeof(tmp), client->read, 1) == -1 && dgets_errno > 0)
                        goto error_ftp;
-               if (*bufptr == '5')
+               if (tmp[0] == '5')
                        goto error_ftp;
                else if (strstr(tmp, "BINARY mode data connection"))
                {
@@ -4220,7 +4217,6 @@
 {
 char tmp[MAX_DCC_BLOCK_SIZE * 4 + 1];
 unsigned int bytesread = 0;
-char *bufptr;
        if ((client->flags & DCC_TYPES) == DCC_FTPGET)
        {
                DCC_list *new = NULL;
@@ -4259,8 +4255,7 @@
                client->flags &= ~DCC_WAIT;
                return;
        }
-       bufptr = tmp;
-       bytesread = dgets(bufptr, client->read, 1);
+       bytesread = dgets(tmp, sizeof(tmp), client->read, 1);
        switch (bytesread)
        {
                case -1:
diff -ur ircii-pana-0.74p2/source/exec.c ircii-pana-0.74p2-patched/source/exec.c
--- ircii-pana-0.74p2/source/exec.c     Thu Dec  4 08:35:30 1997
+++ ircii-pana-0.74p2-patched/source/exec.c     Wed May 27 21:35:36 1998
@@ -630,7 +630,7 @@
        char    exec_buffer[IO_BUFFER_SIZE + 1];
        int     len;

-       switch ((len = dgets(exec_buffer, *fd, 0)))     /* No buffering! */
+       switch ((len = dgets(exec_buffer, sizeof(exec_buffer), *fd, 0)))        /* No buffering! */
        {
                case -1:        /* Something died */
                {
diff -ur ircii-pana-0.74p2/source/functions.c ircii-pana-0.74p2-patched/source/functions.c
--- ircii-pana-0.74p2/source/functions.c        Tue Dec 30 01:12:24 1997
+++ ircii-pana-0.74p2-patched/source/functions.c        Wed May 27 21:46:33 1998
@@ -4501,11 +4501,9 @@
        if (socket_num != -1 && (new = (CloneList *)find_in_list((List **)&clones, ltoa(socket_num), 0)) )
        {
                char buffer[IRCD_BUFFER_SIZE+1];
-               char *str;
                char *s;
                *buffer = 0;
-               str = buffer;
-               switch(dgets(str, socket_num, 0))
+               switch(dgets(buffer, sizeof(buffer), socket_num, 0))
                {
                        case -1:
                                socket_num = 0;
diff -ur ircii-pana-0.74p2/source/misc.c ircii-pana-0.74p2-patched/source/misc.c
--- ircii-pana-0.74p2/source/misc.c     Sun Jan  4 00:39:50 1998
+++ ircii-pana-0.74p2-patched/source/misc.c     Wed May 27 21:36:30 1998
@@ -1293,8 +1293,7 @@
                if (!new->warn && FD_ISSET(new->socket_num, rd))
                {
                        char buffer[IRCD_BUFFER_SIZE + 1];
-                       char *str = buffer;
-                       switch(dgets(str, new->socket_num, 1))
+                       switch(dgets(buffer, sizeof(buffer), new->socket_num, 1))
                        {
                                case -1:
                                {
@@ -2269,7 +2268,7 @@
 char tmpstr[BIG_BUFFER_SIZE+1];
 register unsigned char *p = tmpstr;
        *tmpstr = 0;
-       switch(dgets(tmpstr, s, 0))
+       switch(dgets(tmpstr, sizeof(tmpstr), s, 0))
        {
                case -1:
                        break;
diff -ur ircii-pana-0.74p2/source/newio.c ircii-pana-0.74p2-patched/source/newio.c
--- ircii-pana-0.74p2/source/newio.c    Tue Nov 18 04:49:28 1997
+++ ircii-pana-0.74p2-patched/source/newio.c    Wed May 27 21:25:44 1998
@@ -103,7 +103,7 @@
  *     >0 -- If a full, newline terminated line was available, the length
  *           of the line is returned.
  */
-int dgets (char *str, int des, int buffer)
+int dgets (char *str, int max, int des, int buffer)
 {
        int     cnt = 0, c;
        MyIO    *ioe;
@@ -290,9 +290,13 @@
                return 0;
        }
        /*
+        * Leave room for terminating 0
+        */
+       max--;
+       /*
         * Slurp up the data that is available into 'str'.
         */
-       while (ioe->read_pos < ioe->write_pos)
+       while (ioe->read_pos < ioe->write_pos && cnt <= max)
        {
                if (((str[cnt] = ioe->buffer[ioe->read_pos++])) == '\n')
                        break;
diff -ur ircii-pana-0.74p2/source/screen.c ircii-pana-0.74p2-patched/source/screen.c
--- ircii-pana-0.74p2/source/screen.c   Wed May 27 21:47:19 1998
+++ ircii-pana-0.74p2-patched/source/screen.c   Wed May 27 21:37:08 1998
@@ -1723,7 +1723,7 @@
                        FD_SET(new->fdin, &fd_read);
                        select(NFDBITS, &fd_read, NULL, NULL, &timeout);

-                       if (dgets(buffer, new->fdin, 0) < 1)
+                       if (dgets(buffer, sizeof(buffer), new->fdin, 0) < 1)
                        {
                                new->fdin = new_close(new->fdin);
                                kill_screen(new);
@@ -1815,7 +1815,7 @@

                        if (dumb_mode)
                        {
-                               if (dgets(buffer, screen->fdin, 0))
+                               if (dgets(buffer, sizeof(buffer), screen->fdin, 0))
                                {
                                        *(buffer + strlen(buffer) - 1) = '\0';
                                        if (get_int_var(INPUT_ALIASES_VAR))
diff -ur ircii-pana-0.74p2/source/server.c ircii-pana-0.74p2-patched/source/server.c
--- ircii-pana-0.74p2/source/server.c   Sun Dec 28 21:51:22 1997
+++ ircii-pana-0.74p2-patched/source/server.c   Wed May 27 21:37:50 1998
@@ -209,11 +209,9 @@
                if (((des = server_list[i].read) > -1) && FD_ISSET(des, rd))
                {
                        int     junk;
-                       char    *bufptr;
                        errno   = 0;
                        last_server = from_server = i;
-                       bufptr = buffer;
-                       junk = dgets(bufptr, des, 1);
+                       junk = dgets(buffer, sizeof(buffer), des, 1);
                        switch (junk)
                        {
                                case 0: /* timeout */
@@ -1166,7 +1164,7 @@
                        default:
                                if (FD_ISSET(des, &rd))
                                {
-                                       if (!dgets(buffer, des, 0))
+                                       if (!dgets(buffer, sizeof(buffer), des, 0))
                                                flushing = 0;
                                }
                                break;
@@ -1176,7 +1174,7 @@
        FD_ZERO(&rd);
        FD_SET(des, &rd);
        if (new_select(&rd, NULL, &timeout) > 0)
-               dgets(buffer, des, 1);
+               dgets(buffer, sizeof(buffer), des, 1);
 }