Subject: bug#23482: [PATCH 22.1] Fix buffer overflow in x-send-client-message (Bug#23482).

From: Lars Ingebrigtsen <[email-masked]>
Date: Tue, 11 Aug 2020 17:34:05 +0200

Kalle Olavi Niemitalo <[email-masked]> writes:

> The docstring already said that excessive values are ignored,
> but they instead overflowed the buffer.
>
> This does not seem a security vulnerability though, because Emacs fully
> trusts Emacs Lisp code, and if some Emacs Lisp code sends client
> messages based on untrusted data, then that's already a bug of its own.
>
> 2016-05-08  Kalle Olavi Niemitalo  <[email-masked]>
>
> 	* xselect.c (x_fill_property_data): Add parameter NELEMENTS_MAX.
> 	* xterm.h (x_fill_property_data): Update prototype.
> 	* xselect.c (Fx_send_client_event): Update call.  This fixes
> 	  a buffer overflow in event.xclient.data.
> 	* xfns.c (Fx_change_window_property): Update call.

Sorry; it doesn't seem like you got a response to this patch at the
time.

To recap: The following will crash Emacs, so don't eval it:

(x-send-client-message nil nil nil "foo" 32 (make-list 100 0))

I can confirm that this problem is still present in Emacs 28, and that
Kalle's patch fixes it.  It looks pretty straight-forward, but does
anybody have any comments here?  I've included the re-spun patch for
Emacs 28 below.

> This patch is for Emacs 22.1 and includes the prominent notices
> required by clause 2a of GPLv2. 

I'm not sure what that means?

> I do not intend to assign copyright to the FSF.

It's less than ten lines, so that shouldn't be necessary.

diff --git a/src/xfns.c b/src/xfns.c
index 09dcbbfb92..0203c1324f 100644
--- a/src/xfns.c
+++ b/src/xfns.c
@@ -5890,7 +5890,8 @@ DEFUN ("x-change-window-property", Fx_change_window_property,
       elsize = element_format == 32 ? sizeof (long) : element_format >> 3;
       data = xnmalloc (nelements, elsize);
 
-      x_fill_property_data (FRAME_X_DISPLAY (f), value, data, element_format);
+      x_fill_property_data (FRAME_X_DISPLAY (f), value, data, nelements,
+                            element_format);
     }
   else
     {
diff --git a/src/xselect.c b/src/xselect.c
index 48d6215a7b..5234bccbd9 100644
--- a/src/xselect.c
+++ b/src/xselect.c
@@ -2276,23 +2276,28 @@ x_check_property_data (Lisp_Object data)
 
    DPY is the display use to look up X atoms.
    DATA is a Lisp list of values to be converted.
-   RET is the C array that contains the converted values.  It is assumed
-   it is big enough to hold all values.
+   RET is the C array that contains the converted values.
+   NELEMENTS_MAX is the number of values that will fit in RET.
+   Any excess values in DATA are ignored.
    FORMAT is 8, 16 or 32 and denotes char/short/long for each C value to
    be stored in RET.  Note that long is used for 32 even if long is more
    than 32 bits (see man pages for XChangeProperty, XGetWindowProperty and
    XClientMessageEvent).  */
 
 void
-x_fill_property_data (Display *dpy, Lisp_Object data, void *ret, int format)
+x_fill_property_data (Display *dpy, Lisp_Object data, void *ret,
+		      int nelements_max, int format)
 {
   unsigned long val;
   unsigned long  *d32 = (unsigned long  *) ret;
   unsigned short *d16 = (unsigned short *) ret;
   unsigned char  *d08 = (unsigned char  *) ret;
+  int nelements;
   Lisp_Object iter;
 
-  for (iter = data; CONSP (iter); iter = XCDR (iter))
+  for (iter = data, nelements = 0;
+       CONSP (iter) && nelements < nelements_max;
+       iter = XCDR (iter), nelements++)
     {
       Lisp_Object o = XCAR (iter);
 
@@ -2593,7 +2598,9 @@ x_send_client_event (Lisp_Object display, Lisp_Object dest, Lisp_Object from,
   event.xclient.window = to_root ? FRAME_OUTER_WINDOW (f) : wdest;
 
   memset (event.xclient.data.l, 0, sizeof (event.xclient.data.l));
+  /* event.xclient.data can hold 20 chars, 10 shorts, or 5 longs.  */
   x_fill_property_data (dpyinfo->display, values, event.xclient.data.b,
+                        5 * 32 / event.xclient.format,
                         event.xclient.format);
 
   /* If event mask is 0 the event is sent to the client that created
diff --git a/src/xterm.h b/src/xterm.h
index bc10043c54..db8d584781 100644
--- a/src/xterm.h
+++ b/src/xterm.h
@@ -1207,6 +1207,7 @@ x_mutable_colormap (Visual *visual)
 extern void x_fill_property_data (Display *,
                                   Lisp_Object,
                                   void *,
+				  int,
                                   int);
 extern Lisp_Object x_property_data_to_lisp (struct frame *,
                                             const unsigned char *,

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no