More libXt problems

["Alexander O. Yuriev" <alex@yuriev.com>]

Hi,

	It looks like libXt is a real mess... This is another one from
bugtraq. As usually, comments are welcome as well as the collective
discussion of what to do for the summary

------- Forwarded Message

Return-Path: owner-bugtraq@NETSPACE.ORG
Received: from brimstone.netspace.org (brimstone.netspace.org [128.148.157.143])
	by ding.mailhub.com (8.8.5/8.8.5) with ESMTP id VAA03392
	for <alex@YURIEV.COM>; Thu, 29 May 1997 21:11:36 -0400 (EDT)
Received: from netspace.org ([128.148.157.6]) by brimstone.netspace.org with ESMTP id <33766-27325>; Thu, 29 May 1997 18:42:07 -0400
Received: from NETSPACE.ORG by NETSPACE.ORG (LISTSERV-TCP/IP release 1.8c) with
          spool id 4006219 for BUGTRAQ@NETSPACE.ORG; Thu, 29 May 1997 18:29:06
          -0400
Received: from brimstone.netspace.org (brimstone [128.148.157.143]) by
          netspace.org (8.8.5/8.8.2) with ESMTP id SAA10608 for
          <BUGTRAQ@netspace.org>; Thu, 29 May 1997 18:28:52 -0400
Received: from netspace.org ([128.148.157.6]) by brimstone.netspace.org with
          ESMTP id <32818-27327>; Thu, 29 May 1997 18:31:37 -0400
Approved-By: aleph1@UNDERGROUND.ORG
Received: from phobos.illtel.denver.co.us (abelits@phobos.illtel.denver.co.us
          [207.33.75.1]) by netspace.org (8.8.5/8.8.2) with ESMTP id RAA03614
          for <BUGTRAQ@NETSPACE.ORG>; Thu, 29 May 1997 17:31:21 -0400
Received: from localhost (abelits@localhost) by phobos.illtel.denver.co.us
          (8.8.5/8.6.9) with SMTP id OAA01937 for <BUGTRAQ@NETSPACE.ORG>; Thu,
          29 May 1997 14:37:39 -0700
MIME-Version: 1.0
Content-Type: TEXT/PLAIN; charset=US-ASCII
Message-ID: <Pine.LNX.3.95.970529113609.136D-100000@phobos.illtel.denver.co.us>
Date: 	Thu, 29 May 1997 14:37:39 -0700
Reply-To: Alex Belits <abelits@phobos.illtel.denver.co.us>
Sender: Bugtraq List <BUGTRAQ@NETSPACE.ORG>
From: Alex Belits <abelits@phobos.illtel.denver.co.us>
Subject:      libX11 / libXt buffer overflows patches
To: BUGTRAQ@NETSPACE.ORG

  I have installed X11R6.3 (with fix-01) and found that buffer overflows
caused by environment variables are still there, just in different places.
(I have argued about validity of David Luyer's test program for them some
time ago, and I was wrong -- libraries use getenv(3), and the allocation
of buffer is correct in the test). Also, less dangerous, but annoying NULL
pointer + small offset dereference causes most of programs to dump core on
startup if opening display failed.

  Xrm bug is there, too, and I have made my version of its patch  --
one-line fix by David Hedley works with his test, but xc/lib/X11/Xrm.c has
other places where pointers can step over buffers boundaries (some parts
of code that are related to value parsing have boundaries checking, others
don't). Also some library functions make rather unobvious assumptions
about buffers that user supplied to them, but in themselves they look
safe.

Patches:

For environment buffer overflows and NULL display:
- ---8<---
diff -u old-xc/lib/X11/lcFile.c xc/lib/X11/lcFile.c
- --- old-xc/lib/X11/lcFile.c     Sat Sep 28 13:46:16 1996
+++ xc/lib/X11/lcFile.c Wed May 28 09:13:36 1997
@@ -267,10 +267,14 @@
     xlocaledir(dir,BUFSIZE);
     n = _XlcParsePath(dir, args, 256);
     for(i = 0; i < n; ++i){
- -       if ((2 + (args[i] ? strlen (args[i]) : 0) +
- -           strlen (locale_alias)) < BUFSIZE) {
- -           sprintf(buf, "%s/%s", args[i], locale_alias);
- -           name = _XlcResolveName(lc_name, buf, LtoR);
+        if (args[i]){
+           if ((2 + strlen (args[i]) +
+               strlen (locale_alias)) < BUFSIZE) {
+               sprintf(buf, "%s/%s", args[i], locale_alias);
+               name = _XlcResolveName(lc_name, buf, LtoR);
+           } else {
+                name = NULL;
+           }
        }
        if(name != NULL){
            break;
@@ -278,12 +282,25 @@
     }

     if(name != NULL){
- -       strcpy(buf, name);
+        if(strlen(name) < BUFSIZE - 1){
+           strcpy(buf, name);
+        } else {
+            fprintf(stderr,
+"Warning: locale \"%s\" is too long, ignored\n", name);
+            *buf = '\0';
+        }
        Xfree(name);
     }else{
- -       strcpy(buf, lc_name);
+        if(strlen(lc_name) < BUFSIZE - 1){
+           strcpy(buf, lc_name);
+        } else {
+            fprintf(stderr,
+"Warning: locale \"%s\" is too long, ignored\n", lc_name);
+            *buf = '\0';
+        }
     }
     if(full_name != NULL){
+/* This argument always has BUFSIZE bytes allocated if not NULL */
        strcpy(full_name, buf);
     }

@@ -296,25 +313,49 @@
        if(territory) *territory = '\0';
        if(codeset) *codeset = '\0';

- -       name_p = buf;
- -       ptr = language;
- -       while (1) {
- -           if (*name_p == '_') {
- -               if (ptr)
- -                   *ptr = '\0';
- -               ptr = territory;
- -           } else if (*name_p == '.') {
- -               if (ptr)
- -                   *ptr = '\0';
- -               ptr = codeset;
- -           } else {
- -               if (ptr)
- -                   *ptr++ = *name_p;
- -               if (*name_p == '\0')
- -                   break;
- -           }
- -           name_p++;
- -       }
+        name_p = buf;
+        ptr = strchr(name_p, '_');
+        if(!ptr) ptr = strchr(name_p, '.');
+        if(!ptr) ptr = name_p + strlen(name_p);
+/* 128 because it's the size of that buffer */
+        if(ptr - name_p < 128) {
+            if(language) {
+                memcpy(language,name_p, ptr - name_p);
+                language[ptr - name_p]=0;
+            }
+        }else{
+            fprintf(stderr,
+"Warning: language name in locale \"%s\" is too long, ignored\n", buf);
+        }
+        if(*ptr == '_') {
+            name_p = ptr + 1;
+            ptr = strchr(name_p, '.');
+            if(!ptr) ptr = name_p + strlen(name_p);
+/* 128 because it's the size of that buffer */
+            if(ptr - name_p < 128) {
+               if(territory) {
+                    memcpy(territory, name_p, ptr - name_p);
+                    territory[ptr - name_p]=0;
+               }
+            }else{
+                fprintf(stderr,
+"Warning: territory name in locale \"%s\" is too long, ignored\n", buf);
+            }
+        }
+        if(*ptr == '.') {
+            name_p = ptr+1;
+            ptr = name_p + strlen(name_p);
+/* 128 because it's the size of that buffer */
+            if(ptr - name_p < 128) {
+                if(codeset) {
+                    memcpy(codeset, name_p, ptr-name_p);
+                    codeset[ptr - name_p]=0;
+               }
+            }else{
+                fprintf(stderr,
+"Warning: codeset name in locale \"%s\" is too long, ignored\n", buf);
+            }
+        }
     }

     return (buf[0] != '\0') ? 1 : 0;
diff -u old-xc/lib/Xt/Initialize.c xc/lib/Xt/Initialize.c
- --- old-xc/lib/Xt/Initialize.c  Wed Dec  4 07:25:29 1996
+++ xc/lib/Xt/Initialize.c      Wed May 28 07:24:15 1997
@@ -938,7 +938,8 @@
                     argc_in_out, &argv_in_out, fallback_resources);

     LOCK_APP(app_con);
- -    XtSetArg(args[num], XtNscreen, DefaultScreenOfDisplay(dpy)); num++;
+    if(dpy)
+        XtSetArg(args[num], XtNscreen, DefaultScreenOfDisplay(dpy)); num++;
     XtSetArg(args[num], XtNargc, saved_argc);                   num++;
     XtSetArg(args[num], XtNargv, argv_in_out);                  num++;

diff -u old-xc/lib/Xt/Intrinsic.c xc/lib/Xt/Intrinsic.c
- --- old-xc/lib/Xt/Intrinsic.c   Sat Sep 28 13:54:39 1996
+++ xc/lib/Xt/Intrinsic.c       Tue May 27 04:44:01 1997
@@ -1109,6 +1109,7 @@
     char           *start;
     char           *end;
     int             len;
+    int             buflen;
 #ifdef SKIPCOUNT
     int                    n;
 #endif
@@ -1129,8 +1130,10 @@
 #endif
         if (end = strchr (start, ENDCHAR)) {
             len = end - start;
- -            strncpy(buf, start, len);
- -            *(buf + len) = '\0';
+            buflen=len;
+            if(buflen>=MAXLOCALE) buflen=MAXLOCALE-1;
+            strncpy(buf, start, buflen+1);
+            *(buf + buflen) = '\0';
 #ifdef WHITEFILL
            for (start = buf; start = strchr(start, ' '); )
                *start++ = '-';
- --->8---


For Xrm buffer overflows:
- ---8<---
- --- old-xc/lib/X11/Xrm.c        Thu Jun  8 20:20:39 1995
+++ xc/lib/X11/Xrm.c    Thu May 29 14:07:49 1997
@@ -1179,10 +1179,11 @@
         * storing characters and converting this to a Quark.
         *
         * If the number of quarks is greater than LIST_SIZE - 1.  This
- -        * function will trash your memory.
+        * function was able to trash your memory, so now checks for
+        * boundaries added.
         *
         * If the length of any quark is larger than BUFSIZ this function
- -        * will also trash memory.
+        * was also able trash memory, so boundaries checks are added, too.
         */

        t_bindings = bindings;
@@ -1191,26 +1192,29 @@
        sig = 0;
        ptr = buffer;
        *t_bindings = XrmBindTightly;
- -       for(;;) {
+       while(ptr - buffer < BUFSIZ && t_quarks - quarks < LIST_SIZE - 1) {
+
            if (!is_binding(bits)) {
- -               while (!is_EOQ(bits)) {
+               while (!is_EOQ(bits) && (ptr - buffer < BUFSIZ)) {
                    *ptr++ = c;
                    sig = (sig << 1) + c; /* Compute the signature. */
                    bits = next_char(c, str);
                }
- -
- -               *t_quarks++ = _XrmInternalStringToQuark(buffer, ptr - buffer,
- -                                                       sig, False);
+               if(t_quarks-quarks < LIST_SIZE - 1)
+                   *t_quarks++ = _XrmInternalStringToQuark(buffer, ptr - buffer,
+                                                           sig, False);

                if (is_separator(bits))  {
                    if (!is_space(bits))
                        break;

                    /* Remove white space */
- -                   do {
- -                       *ptr++ = c;
- -                       sig = (sig << 1) + c; /* Compute the signature. */
- -                   } while (is_space(bits = next_char(c, str)));
+                   if(ptr - buffer < BUFSIZ) {
+                       do {
+                           *ptr++ = c;
+                           sig = (sig << 1) + c; /* Compute the signature. */
+                       } while (is_space(bits = next_char(c, str)) && (ptr - buffer < BUFSIZ));
+                   }

                    /*
                     * The spec doesn't permit it, but support spaces
@@ -1223,10 +1227,12 @@
                    continue;
                }

- -               if (c == '.')
- -                   *(++t_bindings) = XrmBindTightly;
- -               else
- -                   *(++t_bindings) = XrmBindLoosely;
+               if(t_bindings - bindings < LIST_SIZE - 1) {
+                   if (c == '.')
+                       *(++t_bindings) = XrmBindTightly;
+                   else
+                       *(++t_bindings) = XrmBindLoosely;
+               }

                sig = 0;
                ptr = buffer;
- --->8---

- --
Alex

------- End of Forwarded Message


--
TO UNSUBSCRIBE FROM THIS MAILING LIST: e-mail the word "unsubscribe" to
debian-private-request@lists.debian.org . 
Trouble?  e-mail to templin@bucknell.edu .