libX11 / libXt buffer overflows patches

Alex Belits (abelits@phobos.illtel.denver.co.us)
Thu, 29 May 1997 14:37:39 -0700

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