[Emacs-diffs] master a103dbe: Disable execution of unsafe Lisp by Enriched Text mode

[Eli Zaretskii]

branch: master
commit a103dbe36022cd2454eaeed96def1c777c049762
Author: Eli Zaretskii <address@hidden>
Commit: Eli Zaretskii <address@hidden>

    Disable execution of unsafe Lisp by Enriched Text mode
    
    * src/xdisp.c (handle_display_spec): If the display property is
    wrapped in 'disable-eval' form, disable Lisp evaluation while
    processing this property.
    (handle_single_display_spec): Accept new argument ENABLE_EVAL_P.
    If that argument is false, don't evaluate Lisp while processing
    display properties.
    
    * lisp/textmodes/enriched.el
    (enriched-allow-eval-in-display-props): New defcustom.
    (enriched-decode-display-prop): If
    enriched-allow-eval-in-display-props is nil, wrap the display
    property with 'disable-eval' to disable Lisp evaluation when the
    display property is processed for display.  (Bug#28350)
    * lisp/gnus/mm-view.el (mm-inline-text): Re-enable processing of
    enriched text.
    
    * doc/lispref/display.texi (Display Property): Document the
    'disable-eval' wrapping of 'display' properties.
    * doc/emacs/text.texi (Enriched Properties): Document
    'enriched-allow-eval-in-display-props'.
    
    * etc/NEWS: Describe the security issues with Enriched Text mode
    and their solution.
---
 doc/emacs/text.texi        | 17 +++++++++++++++++
 doc/lispref/display.texi   | 11 +++++++++++
 etc/NEWS                   | 22 ++++++++++++++++++++++
 lisp/gnus/mm-view.el       | 10 ++++------
 lisp/textmodes/enriched.el | 23 +++++++++++++++++++----
 src/xdisp.c                | 37 ++++++++++++++++++++++++++++---------
 6 files changed, 101 insertions(+), 19 deletions(-)

diff --git a/doc/emacs/text.texi b/doc/emacs/text.texi
index 3b54aa8..496b43c 100644
--- a/doc/emacs/text.texi
+++ b/doc/emacs/text.texi
@@ -2398,6 +2398,23 @@ these special properties from the text in the region.
 
   The @code{invisible} and @code{intangible} properties are not saved.
 
address@hidden enriched-allow-eval-in-display-props
address@hidden security, when displaying enriched text
+  Enriched mode also supports saving and restoring @code{display}
+properties (@pxref{Display Property,,,elisp, the Emacs Lisp Reference
+Manual}), which affect how text is displayed on the screen, and also
+allow displaying images and strings that come from sources other than
+buffer text.  The @code{display} properties also support execution of
+arbitrary Lisp forms as part of processing the property for display,
+thus providing a means to dynamically tailor the display to some
+conditions that can only be known at display time.  Since execution of
+arbitrary Lisp opens Emacs to potential attacks, especially when the
+source of enriched text is outside of Emacs or even outside of your
+system (e.g., if it was received in an email message), such execution
+is by default disabled in Enriched mode.  You can enable it by
+customizing the variable @code{enriched-allow-eval-in-display-props}
+to a address@hidden value.
+
 @node Text Based Tables
 @section Editing Text-based Tables
 @cindex table mode
diff --git a/doc/lispref/display.texi b/doc/lispref/display.texi
index 1dbc0bb..3dae984 100644
--- a/doc/lispref/display.texi
+++ b/doc/lispref/display.texi
@@ -4486,6 +4486,17 @@ for the @code{display} property, only one of the values 
takes effect,
 following the rules of @code{get-char-property}.  @xref{Examining
 Properties}.
 
address@hidden display property, unsafe evaluation
address@hidden security, and display specifications
+  Some of the display specifications allow inclusion of Lisp forms,
+which are evaluated at display time.  This could be unsafe in certain
+situations, e.g., when the display specification was generated by some
+external program/agent.  Wrapping a display specification in a list
+that begins with the special symbol @code{disable-eval}, as in
address@hidden@code{('disable-eval @var{spec})}}, will disable evaluation of any
+Lisp in @var{spec}, while still supporting all the other display
+property features.
+
   The rest of this section describes several kinds of
 display specifications and what they mean.
 
diff --git a/etc/NEWS b/etc/NEWS
index 016868d..ce82804 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -117,6 +117,28 @@ The effect is similar to that of "toolBar" resource on the 
tool bar.
 
 * Changes in Emacs 26.1
 
+** Security vulnerability related to Enriched Text mode is removed.
+
++++
+*** Enriched Text mode does not evaluate Lisp in 'display' properties.
+This feature allows saving 'display' properties as part of text.
+Emacs 'display' properties support evaluation of arbitrary Lisp forms
+as part of processing the property for display, so displaying Enriched
+Text could be vulnerable to executing arbitrary malicious Lisp code
+included in the text (e.g., sent as part of an email message).
+Therefore, execution of arbitrary Lisp forms in 'display' properties
+decoded by Enriched Text mode is now disabled by default.  Customize
+the new option 'enriched-allow-eval-in-display-props' to a non-nil
+value to allow Lisp evaluation in decoded 'display' properties.
+
+This vulnerability was introduced in Emacs 21.1.  To work around that
+in Emacs versions before 25.3, append the following to your ~/.emacs
+init file:
+
+  (eval-after-load "enriched"
+    '(defun enriched-decode-display-prop (start end &optional param)
+       (list start end)))
+
 +++
 ** Functions in 'write-contents-functions' can fully short-circuit the
 'save-buffer' process.  Previously, saving a buffer that was not
diff --git a/lisp/gnus/mm-view.el b/lisp/gnus/mm-view.el
index 86e2171..d7a41b8 100644
--- a/lisp/gnus/mm-view.el
+++ b/lisp/gnus/mm-view.el
@@ -362,12 +362,10 @@
        (goto-char (point-max))))
     (save-restriction
       (narrow-to-region b (point))
-      ;; Disabled in Emacs 25.3 to avoid execution of arbitrary Lisp
-      ;; forms in display properties supported by enriched.el.
-      ;; (when (member type '("enriched" "richtext"))
-      ;;   (set-text-properties (point-min) (point-max) nil)
-      ;;       (ignore-errors
-      ;;         (enriched-decode (point-min) (point-max))))
+      (when (member type '("enriched" "richtext"))
+        (set-text-properties (point-min) (point-max) nil)
+       (ignore-errors
+         (enriched-decode (point-min) (point-max))))
       (mm-handle-set-undisplayer
        handle
        `(lambda ()
diff --git a/lisp/textmodes/enriched.el b/lisp/textmodes/enriched.el
index d90c207..be5cd6b 100644
--- a/lisp/textmodes/enriched.el
+++ b/lisp/textmodes/enriched.el
@@ -147,6 +147,22 @@ them and their old values to `enriched-old-bindings'."
   :type 'hook
   :group 'enriched)
 
+(defcustom enriched-allow-eval-in-display-props nil
+  "If non-nil allow to evaluate arbitrary forms in display properties.
+
+Enriched mode recognizes display properties of text stored using
+an extension command to the text/enriched format, \"x-display\".
+These properties must not, by default, include evaluation of
+Lisp forms, otherwise they are not applied.  Customize this option
+to t to turn off this safety feature, and allow Enriched mode to
+apply display properties which evaluate arbitrary Lisp forms.
+Note, however, that applying unsafe display properties could
+execute malicious Lisp code, if that code came from an external source."
+  :risky t
+  :type 'boolean
+  :version "26.1"
+  :group 'enriched)
+
 (defvar enriched-old-bindings nil
   "Store old variable values that we change when entering mode.
 The value is a list of \(VAR VALUE VAR VALUE...).")
@@ -503,9 +519,8 @@ the range of text to assign text property SYMBOL with value 
VALUE."
                  (error nil)))))
     (unless prop
       (message "Warning: invalid <x-display> parameter %s" param))
-    ;; Disabled in Emacs 25.3 to avoid execution of arbitrary Lisp
-    ;; forms in display properties stored within enriched text.
-    ;; (list start end 'display prop)))
-    (list start end)))
+    (if enriched-allow-eval-in-display-props
+        (list start end 'display prop)
+      (list start end 'display (list 'disable-eval prop)))))
 
 ;;; enriched.el ends here
diff --git a/src/xdisp.c b/src/xdisp.c
index 8ca9037..dc5dbb0 100644
--- a/src/xdisp.c
+++ b/src/xdisp.c
@@ -876,9 +876,9 @@ static int face_before_or_after_it_pos (struct it *, bool);
 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)
@@ -4748,6 +4748,14 @@ handle_display_spec (struct it *it, Lisp_Object spec, 
Lisp_Object object,
                     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.  */
@@ -4771,7 +4779,8 @@ handle_display_spec (struct it *it, Lisp_Object spec, 
Lisp_Object object,
        {
          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;
@@ -4789,7 +4798,8 @@ handle_display_spec (struct it *it, Lisp_Object spec, 
Lisp_Object object,
        {
          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;
@@ -4802,7 +4812,8 @@ handle_display_spec (struct it *it, Lisp_Object spec, 
Lisp_Object object,
     }
   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;
 }
 
@@ -4847,6 +4858,8 @@ display_prop_end (struct it *it, Lisp_Object object, 
struct text_pos start_pos)
    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.  */
 
@@ -4854,7 +4867,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;
@@ -4872,6 +4885,8 @@ handle_single_display_spec (struct it *it, Lisp_Object 
spec, Lisp_Object object,
       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 ();
@@ -4920,7 +4935,7 @@ handle_single_display_spec (struct it *it, Lisp_Object 
spec, Lisp_Object object,
                    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.  */
@@ -4941,7 +4956,7 @@ handle_single_display_spec (struct it *it, Lisp_Object 
spec, Lisp_Object object,
                  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.  */
@@ -32204,6 +32219,10 @@ They are still logged to the *Messages* buffer.  */);
   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");