From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.org!.POSTED!not-for-mail From: Eli Zaretskii Newsgroups: gmane.emacs.bugs Subject: bug#28350: enriched.el code execution Date: Mon, 11 Sep 2017 18:18:01 +0300 Message-ID: <83y3pls1qu.fsf@gnu.org> References: <305e0573-2e10-cb15-4133-9bd72d33ea5e@cs.ucla.edu> Reply-To: Eli Zaretskii NNTP-Posting-Host: blaine.gmane.org X-Trace: blaine.gmane.org 1505325843 9666 195.159.176.226 (13 Sep 2017 18:04:03 GMT) X-Complaints-To: usenet@blaine.gmane.org NNTP-Posting-Date: Wed, 13 Sep 2017 18:04:03 +0000 (UTC) Cc: larsi@gnus.org, eggert@cs.ucla.edu, 28350@debbugs.gnu.org To: charles@aurox.ch (Charles A. Roelli) Original-X-From: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane.org@gnu.org Mon Sep 11 17:19:18 2017 Return-path: Envelope-to: geb-bug-gnu-emacs@m.gmane.org Original-Received: from lists.gnu.org ([208.118.235.17]) by blaine.gmane.org with esmtp (Exim 4.84_2) (envelope-from ) id 1drQUU-00032N-1E for geb-bug-gnu-emacs@m.gmane.org; Mon, 11 Sep 2017 17:19:14 +0200 Original-Received: from localhost ([::1]:58374 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1drQUX-0002Dt-Td for geb-bug-gnu-emacs@m.gmane.org; Mon, 11 Sep 2017 11:19:17 -0400 Original-Received: from eggs.gnu.org ([2001:4830:134:3::10]:47850) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1drQUM-00029g-Bf for bug-gnu-emacs@gnu.org; Mon, 11 Sep 2017 11:19:10 -0400 Original-Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1drQUI-0002mr-KK for bug-gnu-emacs@gnu.org; Mon, 11 Sep 2017 11:19:06 -0400 Original-Received: from debbugs.gnu.org ([208.118.235.43]:53226) by eggs.gnu.org with esmtps (TLS1.0:RSA_AES_128_CBC_SHA1:16) (Exim 4.71) (envelope-from ) id 1drQUI-0002mM-GK for bug-gnu-emacs@gnu.org; Mon, 11 Sep 2017 11:19:02 -0400 Original-Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1drQUI-000626-Am for bug-gnu-emacs@gnu.org; Mon, 11 Sep 2017 11:19:02 -0400 X-Loop: help-debbugs@gnu.org Resent-From: Eli Zaretskii Original-Sender: "Debbugs-submit" Resent-CC: bug-gnu-emacs@gnu.org Resent-Date: Mon, 11 Sep 2017 15:19:02 +0000 Resent-Message-ID: Resent-Sender: help-debbugs@gnu.org X-GNU-PR-Message: followup 28350 X-GNU-PR-Package: emacs X-GNU-PR-Keywords: security Original-Received: via spool by 28350-submit@debbugs.gnu.org id=B28350.150514310923147 (code B ref 28350); Mon, 11 Sep 2017 15:19:02 +0000 Original-Received: (at 28350) by debbugs.gnu.org; 11 Sep 2017 15:18:29 +0000 Original-Received: from localhost ([127.0.0.1]:33673 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1drQTh-00061E-Jj for submit@debbugs.gnu.org; Mon, 11 Sep 2017 11:18:29 -0400 Original-Received: from eggs.gnu.org ([208.118.235.92]:56602) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1drQTc-00060y-Jn for 28350@debbugs.gnu.org; Mon, 11 Sep 2017 11:18:24 -0400 Original-Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1drQTS-0001am-So for 28350@debbugs.gnu.org; Mon, 11 Sep 2017 11:18:15 -0400 Original-Received: from fencepost.gnu.org ([2001:4830:134:3::e]:33162) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1drQTS-0001ai-PX; Mon, 11 Sep 2017 11:18:10 -0400 Original-Received: from 84.94.185.246.cable.012.net.il ([84.94.185.246]:4336 helo=home-c4e4a596f7) by fencepost.gnu.org with esmtpsa (TLS1.2:RSA_AES_256_CBC_SHA1:256) (Exim 4.82) (envelope-from ) id 1drQTS-0003uP-0J; Mon, 11 Sep 2017 11:18:10 -0400 In-reply-to: (charles@aurox.ch) X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic] X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.18 Precedence: list X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic] X-Received-From: 208.118.235.43 X-BeenThere: bug-gnu-emacs@gnu.org List-Id: "Bug reports for GNU Emacs, the Swiss army knife of text editors" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane.org@gnu.org Original-Sender: "bug-gnu-emacs" Xref: news.gmane.org gmane.emacs.bugs:136908 > Date: Sun, 10 Sep 2017 20:54:13 +0200 > From: charles@aurox.ch (Charles A. Roelli) > Cc: larsi@gnus.org, 28350@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");