More xterm problems

[dark@xs4all.nl (Richard Braakman)]

I started to look for other potential security problems in xterm.
Simply grepping for strcpy turned up three situations where someone
might overwrite the stack by setting X resources.  Whether they can be
exploited hinges on the maximum string length that xrdb allows.  If
this is less than 1000, there should be no danger.

I only recently began learning about how to deal with security in
setuid programs.  So please bear with me if I'm making mistakes.

The problems are in VTInitI18N() and HandleKeymapChange() in charproc.c.
I have included their code below, with line numbers, for reference. 
I took the code from the XFree86 3.3 source tree on ftp.funet.fi.

In each case, a string supplied as a resource (thus, by the user) is
copied directly into an array allocated on the stack, with no length
checks.  From what I've heard, this is the classic "buffer overrun".

Two of them are in VTInitI18N(), line numbers 3791 and 3826.  Note
that this function is only compiled if XtSpecificationRelease >= 6, so
older systems are not affected.  The resources used are inputMethod
and preeditType, both of the xterm widget.  The first of these has
an additional complication, as the code performs some parsing on
the inputMethod string and assumes that the parse result will fit
in a 32-byte buffer.  (line 3801).

The third is in HandleKeymapChange(), line 4374.  This function is
registered as an action callback ("keymap") for the xterm widget, thus
the param[0] string it receives is also supplied by the user.  This
string is copied without length checks, using sprintf, to an array
declared on the stack.

For both functions the fix is simple.  In VTInitI18N(), slightly
rewrite the loops so that they do not have to modify the resource
string, and thus remove the need for a stack-allocated buffer to
hold a copy.  The secondary problem at line 3801 is resolved by
truncating the parse result where necessary.

In HandleKeymapChange(), add a maximum string length to the sprintf
format string, making it impossible for the parameter to overflow the
buffer.

I do not foresee any compatibility problems with these truncations,
because the buffer sizes were already generous, and exceeding them was
already likely to cause a crash.  If you want to be sure, bump the
buffer sizes a bit.

I have included a diff for these changes at the end of this mail.
This diff is intended for illustration only, since I have not tested
it except to check that the result compiles and runs normally.  I am
not familiar enough with keymap support or the I18N extensions to test
it properly.

A spot check of the xrdb sources revealed no builtin limit on the
string length.  I have not experimented because... well, because it's
11 am and I haven't slept yet.

If these are true security problems, I'll keep quiet about them until
you give me the ok.  Feel free to forward this mail wherever
appropriate.

I hope this helps,

  Richard Braakman
(now off to bed)

=== function charproc.c:VTInitI18N included for reference ===
  3768  #if XtSpecificationRelease >= 6
  3769  static void VTInitI18N()
  3770  {
  3771      int         i;
  3772      char       *p,
  3773                 *s,
  3774                 *ns,
  3775                 *end,
  3776                  tmp[1024],
  3777                  buf[32];
  3778      XIM         xim = (XIM) NULL;
  3779      XIMStyles  *xim_styles;
  3780      XIMStyle    input_style = 0;
  3781      Boolean     found;
  3782
  3783      term->screen.xic = NULL;
  3784
  3785      if (!term->misc.open_im) return;
  3786
  3787      if (!term->misc.input_method || !*term->misc.input_method) {
  3788          if ((p = XSetLocaleModifiers("@im=none")) != NULL && *p)
  3789              xim = XOpenIM(XtDisplay(term), NULL, NULL, NULL);
  3790      } else {
  3791          strcpy(tmp, term->misc.input_method);
  3792          for(ns=s=tmp; ns && *s;) {
  3793              while (*s && isspace(*s)) s++;
  3794              if (!*s) break;
  3795              if ((ns = end = strchr(s, ',')) == 0)
  3796                  end = s + strlen(s);
  3797              while (isspace(*end)) end--;
  3798              *end = '\0';
  3799
  3800              strcpy(buf, "@im=");
  3801              strcat(buf, s);
  3802              if ((p = XSetLocaleModifiers(buf)) != NULL && *p
  3803                  && (xim = XOpenIM(XtDisplay(term), NULL, NULL, NULL)) != NULL)
  3804                  break;
  3805
  3806              s = ns + 1;
  3807          }
  3808      }
  3809
  3810      if (xim == NULL && (p = XSetLocaleModifiers("@im=none")) != NULL && *p)
  3811          xim = XOpenIM(XtDisplay(term), NULL, NULL, NULL);
  3812
  3813      if (!xim) {
  3814          fprintf(stderr, "Failed to open input method\n");
  3815          return;
  3816      }
  3817
  3818      if (XGetIMValues(xim, XNQueryInputStyle, &xim_styles, NULL)
  3819          || !xim_styles) {
  3820          fprintf(stderr, "input method doesn't support any style\n");
  3821          XCloseIM(xim);
  3822          return;
  3823      }
  3824
  3825      found = False;
  3826      strcpy(tmp, term->misc.preedit_type);
  3827      for(s = tmp; s && !found;) {
  3828          while (*s && isspace(*s)) s++;
  3829          if (!*s) break;
  3830          if ((ns = end = strchr(s, ',')) != 0)
  3831              ns++;
  3832          else
  3833              end = s + strlen(s);
  3834          while (isspace(*end)) end--;
  3835          *end = '\0';
  3836
  3837          if (!strcmp(s, "OverTheSpot")) {
  3838              input_style = (XIMPreeditPosition | XIMStatusArea);
  3839          } else if (!strcmp(s, "OffTheSpot")) {
  3840              input_style = (XIMPreeditArea | XIMStatusArea);
  3841          } else if (!strcmp(s, "Root")) {
  3842              input_style = (XIMPreeditNothing | XIMStatusNothing);
  3843          }
  3844          for (i = 0; (unsigned short)i < xim_styles->count_styles; i++)
  3845              if (input_style == xim_styles->supported_styles[i]) {
  3846                  found = True;
  3847                  break;
  3848              }
  3849
  3850          s = ns;
  3851      }
  3852      XFree(xim_styles);
  3853
  3854      if (!found) {
  3855          fprintf(stderr, "input method doesn't support my preedit type\n");
  3856          XCloseIM(xim);
  3857          return;
  3858      }
  3859
  3860      /*
  3861       * This program only understands the Root preedit_style yet
  3862       * Then misc.preedit_type should default to:
  3863       *          "OverTheSpot,OffTheSpot,Root"
  3864       *
  3865       *  /MaF
  3866       */
  3867      if (input_style != (XIMPreeditNothing | XIMStatusNothing)) {
  3868          fprintf(stderr,"This program only supports the 'Root' preedit type\n");
  3869          XCloseIM(xim);
  3870          return;
  3871      }
  3872
  3873      term->screen.xic = XCreateIC(xim, XNInputStyle, input_style,
  3874                                        XNClientWindow, term->core.window,
  3875                                        XNFocusWindow, term->core.window,
  3876                                        NULL);
  3877
  3878      if (!term->screen.xic) {
  3879          fprintf(stderr,"Failed to create input context\n");
  3880          XCloseIM(xim);
  3881      }
  3882
  3883      return;
  3884  }
  3885  #endif
=== ===

=== function charproc.c:HandleKeymapChange included for reference ===
  4351  /* ARGSUSED */
  4352  static void HandleKeymapChange(w, event, params, param_count)
  4353      Widget w;
  4354      XEvent *event;
  4355      String *params;
  4356      Cardinal *param_count;
  4357  {
  4358      static XtTranslations keymap, original;
  4359      static XtResource key_resources[] = {
  4360          { XtNtranslations, XtCTranslations, XtRTranslationTable,
  4361                sizeof(XtTranslations), 0, XtRTranslationTable, (XtPointer)NULL}
  4362      };
  4363      char mapName[1000];
  4364      char mapClass[1000];
  4365
  4366      if (*param_count != 1) return;
  4367
  4368      if (original == NULL) original = w->core.tm.translations;
  4369
  4370      if (strcmp(params[0], "None") == 0) {
  4371          XtOverrideTranslations(w, original);
  4372          return;
  4373      }
  4374      (void) sprintf( mapName, "%sKeymap", params[0] );
  4375      (void) strcpy( mapClass, mapName );
  4376      if (islower(mapClass[0])) mapClass[0] = toupper(mapClass[0]);
  4377      XtGetSubresources( w, (XtPointer)&keymap, mapName, mapClass,
  4378                         key_resources, (Cardinal)1, NULL, (Cardinal)0 );
  4379      if (keymap != NULL)
  4380          XtOverrideTranslations(w, keymap);
  4381  }
=== ===

--- xterm-old/charproc.c	Sun May 25 14:29:00 1997
+++ xterm-fix/charproc.c	Mon Aug 11 10:43:32 1997
@@ -3773,7 +3773,6 @@
 	       *s,
 	       *ns,
 	       *end,
-		tmp[1024],
 	  	buf[32];
     XIM		xim = (XIM) NULL;
     XIMStyles  *xim_styles;
@@ -3788,17 +3787,18 @@
 	if ((p = XSetLocaleModifiers("@im=none")) != NULL && *p)
 	    xim = XOpenIM(XtDisplay(term), NULL, NULL, NULL);
     } else {
-	strcpy(tmp, term->misc.input_method);
-	for(ns=s=tmp; ns && *s;) {
+	for(ns=s=term->misc.input_method; ns && *s;) {
 	    while (*s && isspace(*s)) s++;
 	    if (!*s) break;
 	    if ((ns = end = strchr(s, ',')) == 0)
 		end = s + strlen(s);
 	    while (isspace(*end)) end--;
-	    *end = '\0';
 
 	    strcpy(buf, "@im=");
-	    strcat(buf, s);
+	    if (end - s >= sizeof(buf) - 4)
+		end = s + (sizeof(buf) - 5);
+	    strncat(buf, s, end - s);
+
 	    if ((p = XSetLocaleModifiers(buf)) != NULL && *p
 		&& (xim = XOpenIM(XtDisplay(term), NULL, NULL, NULL)) != NULL)
 		break;
@@ -3823,8 +3823,7 @@
     }
 
     found = False;
-    strcpy(tmp, term->misc.preedit_type);
-    for(s = tmp; s && !found;) {
+    for(s = term->misc.preedit_type; s && !found;) {
 	while (*s && isspace(*s)) s++;
 	if (!*s) break;
 	if ((ns = end = strchr(s, ',')) != 0)
@@ -3832,13 +3831,12 @@
 	else
 	    end = s + strlen(s);
 	while (isspace(*end)) end--;
-	*end = '\0';
 
-	if (!strcmp(s, "OverTheSpot")) {
+	if (!strncmp(s, "OverTheSpot", end - s)) {
 	    input_style = (XIMPreeditPosition | XIMStatusArea);
-	} else if (!strcmp(s, "OffTheSpot")) {
+	} else if (!strncmp(s, "OffTheSpot", end - s)) {
 	    input_style = (XIMPreeditArea | XIMStatusArea);
-	} else if (!strcmp(s, "Root")) {
+	} else if (!strncmp(s, "Root", end - s)) {
 	    input_style = (XIMPreeditNothing | XIMStatusNothing);
 	}
 	for (i = 0; (unsigned short)i < xim_styles->count_styles; i++)
@@ -4371,7 +4369,7 @@
 	XtOverrideTranslations(w, original);
 	return;
     }
-    (void) sprintf( mapName, "%sKeymap", params[0] );
+    (void) sprintf( mapName, "%900sKeymap", params[0] );
     (void) strcpy( mapClass, mapName );
     if (islower(mapClass[0])) mapClass[0] = toupper(mapClass[0]);
     XtGetSubresources( w, (XtPointer)&keymap, mapName, mapClass,


--
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 .