GNU bug report logs - #28350
CVE-2017-14482: enriched.el code execution

Previous Next

Full log

Message #57 received at 28350 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: charles <at> aurox.ch (Charles A. Roelli)
Cc: larsi <at> gnus.org, eggert <at> cs.ucla.edu, 28350 <at> debbugs.gnu.org
Subject: Re: bug#28350: enriched.el code execution
Date: Mon, 11 Sep 2017 18:18:01 +0300

> Date: Sun, 10 Sep 2017 20:54:13 +0200
> From: charles <at> aurox.ch (Charles A. Roelli)
> Cc: larsi <at> gnus.org, 28350 <at> debbugs.gnu.org
> 
> > --- a/lisp/textmodes/enriched.el
> > +++ b/lisp/textmodes/enriched.el
> > @@ -117,12 +117,7 @@ expression, which is evaluated to get the string to insert.")
> >  		   (full        "flushboth")
> >  		   (center      "center"))
> >      (PARAMETER     (t           "param")) ; Argument of preceding annotation
> > -    ;; The following are not part of the standard:
> > -    (FUNCTION      (enriched-decode-foreground "x-color")
> > -		   (enriched-decode-background "x-bg-color")
> 
> Do we know that "x-color" and/or "x-bg-color" are vulnerable to a
> similar misuse as "x-display"?

They are not.  They are converted to face properties, whose values
don't support Lisp evaluation.

> > +  (provide 'enriched)
> > +  (defun enriched-mode (&optional arg))
> > +  (defun enriched-decode (from to))
> 
> This fix is very safe, at the cost of disabling Enriched mode.  Could
> we do any better?  I had suggested the following (in
> https://debbugs.gnu.org/cgi/bugreport.cgi?bug=28350#16):
> 
>   (eval-after-load "enriched"
>     '(defun enriched-decode-display-prop (start end &optional param)
>        (list start end)))

You are right, and I therefore asked Nicolas to put that as a
workaround into NEWS of Emacs 25.3.

Anyway, I think I have a better idea for how to fix this on master.
And I'm very sorry that this idea didn't come to me earlier, before
you invested all these efforts in your patch.  (I'm also surprised
that no one here had beaten me up to this idea.)

Here's the idea: we introduce a new form of a display property:

  ('disable-eval SPEC)

where SPEC is anything supported in a display property.  Then we
change the implementation of display property in xdisp.c such that
when the above form is seen, we disable Lisp evaluation while walking
SPEC and producing display from it in the display engine.  Such a
patch to xdisp.c appears below.  Then enriched.el will have to either
add disable-eval wrapper around any display property, or not add it if
the user customized enriched.el to enable such evaluation.

This has the advantage of allowing every possible value of display
properties that's safe, while positively disabling any Lisp evaluation
while we process such properties that came from enriched text.  So we
are free from the need to figure out which forms of a display spec can
or cannot invoke Lisp.

WDYT?

Here's the patch:

--- src/xdisp.c~2	2017-09-07 11:16:30.503455400 +0300
+++ src/xdisp.c	2017-09-11 17:29:00.507991400 +0300
@@ -876,9 +876,9 @@ static int face_before_or_after_it_pos (
 static ptrdiff_t next_overlay_change (ptrdiff_t);
 static int handle_display_spec (struct it *, Lisp_Object, Lisp_Object,
 				Lisp_Object, struct text_pos *, ptrdiff_t, bool);
-static int handle_single_display_spec (struct it *, Lisp_Object,
-                                       Lisp_Object, Lisp_Object,
-                                       struct text_pos *, ptrdiff_t, int, bool);
+static int handle_single_display_spec (struct it *, Lisp_Object, Lisp_Object,
+				       Lisp_Object, struct text_pos *,
+				       ptrdiff_t, int, bool, bool);
 static int underlying_face_id (struct it *);
 
 #define face_before_it_pos(IT) face_before_or_after_it_pos (IT, true)
@@ -4731,6 +4731,14 @@ handle_display_spec (struct it *it, Lisp
 		     ptrdiff_t bufpos, bool frame_window_p)
 {
   int replacing = 0;
+  bool enable_eval = true;
+
+  /* Support (disable-eval PROP) which is used by enriched.el.  */
+  if (CONSP (spec) && EQ (XCAR (spec), Qdisable_eval))
+    {
+      enable_eval = false;
+      spec = XCAR (XCDR (spec));
+    }
 
   if (CONSP (spec)
       /* Simple specifications.  */
@@ -4754,7 +4762,8 @@ handle_display_spec (struct it *it, Lisp
 	{
 	  int rv = handle_single_display_spec (it, XCAR (spec), object,
 					       overlay, position, bufpos,
-					       replacing, frame_window_p);
+					       replacing, frame_window_p,
+					       enable_eval);
 	  if (rv != 0)
 	    {
 	      replacing = rv;
@@ -4772,7 +4781,8 @@ handle_display_spec (struct it *it, Lisp
 	{
 	  int rv = handle_single_display_spec (it, AREF (spec, i), object,
 					       overlay, position, bufpos,
-					       replacing, frame_window_p);
+					       replacing, frame_window_p,
+					       enable_eval);
 	  if (rv != 0)
 	    {
 	      replacing = rv;
@@ -4785,7 +4795,8 @@ handle_display_spec (struct it *it, Lisp
     }
   else
     replacing = handle_single_display_spec (it, spec, object, overlay, position,
-					    bufpos, 0, frame_window_p);
+					    bufpos, 0, frame_window_p,
+					    enable_eval);
   return replacing;
 }
 
@@ -4830,6 +4841,8 @@ display_prop_end (struct it *it, Lisp_Ob
    don't set up IT.  In that case, FRAME_WINDOW_P means SPEC
    is intended to be displayed in a window on a GUI frame.
 
+   Enable evaluation of Lisp forms only if ENABLE_EVAL_P is true.
+
    Value is non-zero if something was found which replaces the display
    of buffer or string text.  */
 
@@ -4837,7 +4850,7 @@ static int
 handle_single_display_spec (struct it *it, Lisp_Object spec, Lisp_Object object,
 			    Lisp_Object overlay, struct text_pos *position,
 			    ptrdiff_t bufpos, int display_replaced,
-			    bool frame_window_p)
+			    bool frame_window_p, bool enable_eval_p)
 {
   Lisp_Object form;
   Lisp_Object location, value;
@@ -4855,6 +4868,8 @@ handle_single_display_spec (struct it *i
       spec = XCDR (spec);
     }
 
+  if (!NILP (form) && !EQ (form, Qt) && !enable_eval_p)
+    form = Qnil;
   if (!NILP (form) && !EQ (form, Qt))
     {
       ptrdiff_t count = SPECPDL_INDEX ();
@@ -4903,7 +4918,7 @@ handle_single_display_spec (struct it *i
 		    steps = - steps;
 		  it->face_id = smaller_face (it->f, it->face_id, steps);
 		}
-	      else if (FUNCTIONP (it->font_height))
+	      else if (FUNCTIONP (it->font_height) && enable_eval_p)
 		{
 		  /* Call function with current height as argument.
 		     Value is the new height.  */
@@ -4924,7 +4939,7 @@ handle_single_display_spec (struct it *i
 		  new_height = (XFLOATINT (it->font_height)
 				* XINT (f->lface[LFACE_HEIGHT_INDEX]));
 		}
-	      else
+	      else if (enable_eval_p)
 		{
 		  /* Evaluate IT->font_height with `height' bound to the
 		     current specified height to get the new height.  */
@@ -32164,6 +32179,10 @@ They are still logged to the *Messages* 
   DEFSYM (Qfontified, "fontified");
   DEFSYM (Qfontification_functions, "fontification-functions");
 
+  /* Name of the symbol which disables Lisp evaluation in 'display'
+     properties.  This is used by enriched.el.  */
+  DEFSYM (Qdisable_eval, "disable-eval");
+
   /* Name of the face used to highlight trailing whitespace.  */
   DEFSYM (Qtrailing_whitespace, "trailing-whitespace");
 

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.