bug#28350: enriched.el code execution

[charles@aurox.ch (Charles A. Roelli)]

[-- Attachment #1: Type: text/plain, Size: 2391 bytes --]

> Date: Sat, 09 Sep 2017 19:55:37 +0300
> From: Eli Zaretskii <eliz@gnu.org>
> CC: 28350@debbugs.gnu.org
>
> > > > +See Info node `(elisp)Display Property' for the use of these
> > > > +display specifications."
> > > > +  (ignore-errors
> > > > +    (or (stringp prop)
> > >             ^^^^^^^^^^^^
> > > What about an image spec (including a slice spec)?
> > 
> > Okay, I see that image specs can be safe.  But are they all safe?
> 
> I think they are.  Does anyone know different?

I read over the documentation some more and they do look alright.

> > And I don't understand how a slice spec is used together with an image
> > spec.  Is the slice spec used inside of IMAGE-PROPS, i.e. as you might
> > gather from the manual:
> > 
> > ‘(image . IMAGE-PROPS)’
> >      This kind of display specification is an image descriptor (*note
> >      Images).  When used as a display specification, it means to
> >      display the image instead of the text that has the display
> >      specification.
> > 
> > ‘(slice X Y WIDTH HEIGHT)’
> >      This specification together with ‘image’ specifies a “slice” (a
> >      partial area) of the image to display. 
> > 
> > ?
> 
> AFAIU, like this:
> 
>   ((slice X Y WIDTH HEIGHT) (image . IMAGE-PROPS))
> 
> You can see examples of this in image.el and image-mode.el.

Thanks.  I forgot that the display property can be set to a list or
vector of display specifications.  I've updated the patch to reflect
this:

+        (and (seqp prop) (seq-every-p 'enriched-display-prop-safe-p prop)))))

and I've added slice/image specifications.

> > At this point it seems that unsafe display specs are more the
> > exception than the rule, so it might make sense to define the
> > `enriched-display-prop-safe-p' function by excluding the unsafe
> > specifications instead of including the safe ones.  What do you
> > think?
> 
> I'm not sure.  The display spec can be complex, so to make sure none
> of these exceptions sneak through, you will have to recursively unpack
> the spec data structure and examine each of the elements, which smells
> too similar to emulating 'eval'.  No?

Thank you.  I've kept the current approach.  Please see again the
attached patch.

Also, should the left-fringe/right-fringe display specifications be
considered safe?  They seem innocuous.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Prevent-code-execution-by-text-enriched-files-Bug-28.patch --]
[-- Type: text/x-patch, Size: 4453 bytes --]

From baf533eeddc185a0e65c641022f7be2be2cbcb09 Mon Sep 17 00:00:00 2001
From: "Charles A. Roelli" <charles@aurox.ch>
Date: Sat, 9 Sep 2017 14:03:58 +0200
Subject: [PATCH] Prevent code execution by text/enriched files (Bug#28350)

* lisp/textmodes/enriched.el (enriched-allow-unsafe-display-props):
New customizable option.
(enriched-display-prop-safe-p): New function.
(enriched-decode-display-prop): Use the new function to prevent unsafe
display properties from being applied.
---
 lisp/textmodes/enriched.el | 84 +++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 83 insertions(+), 1 deletion(-)

diff --git a/lisp/textmodes/enriched.el b/lisp/textmodes/enriched.el
index 7ace2a5..74a1229 100644
--- a/lisp/textmodes/enriched.el
+++ b/lisp/textmodes/enriched.el
@@ -147,6 +147,20 @@ enriched-mode-hook
   :type 'hook
   :group 'enriched)
 
+(defcustom enriched-allow-unsafe-display-props nil
+  "Variable determining whether to decode arbitrary display properties.
+
+Enriched mode recognizes display properties of text stored using
+an extension command to the text/enriched format, \"x-display\".
+These properties must, by default, satisfy
+`enriched-display-prop-safe-p' (q.v.), otherwise they are not
+applied.  Customize this option to t to turn off this safety
+feature.  Note, however, that applying unsafe display properties
+can execute arbitrary Lisp code."
+  :risky t
+  :type 'boolean
+  :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,6 +517,74 @@ enriched-decode-display-prop
 		  (error nil)))))
     (unless prop
       (message "Warning: invalid <x-display> parameter %s" param))
-    (list start end 'display prop)))
+    (if (enriched-display-prop-safe-p prop)
+        (list start end 'display prop)
+      (message "Warning: unsafe <x-display> parameter %s not applied" param)
+      (list start end))))
+
+(defun enriched-display-prop-safe-p (prop)
+  "Return t if display property PROP is safe to apply to text.
+
+This function always returns t when
+`enriched-allow-unsafe-display-props' is set to t.
+
+A safe display property is either:
+
+  - a string,
+
+  - an image display specification, (image . image-props), where
+    IMAGE-PROPS is a property list,
+
+  - a slice display specification, (slice x y width height),
+    where X and Y are integers, and WIDTH and HEIGHT are either
+    integers or floats,
+
+  - a space display specification, (space . props), where PROPS
+    is a property list,
+
+  - a space-width display specification, (space-width factor),
+    where FACTOR is an integer or a float,
+
+  - a margin display specification, ((margin right-margin) spec)
+    or ((margin left-margin) spec), where SPEC is a string or an
+    image display specification as above,
+
+  - a height display specification, (height spec), where SPEC is
+    of the form (+ n), (- n) or n, and N is an integer or a
+    float,
+
+  - a raise display specification, (raise factor), where
+    FACTOR is an integer or a float,
+
+  - or a list/vector containing safe display specifications, as
+    above.
+
+See Info node `(elisp)Display Property' for the use of these
+display specifications."
+  (ignore-errors
+    (or enriched-allow-unsafe-display-props
+        (stringp prop)
+        (and (consp prop) (eq (car prop) 'image))
+        (and (consp prop)
+             (eq (car prop) 'slice)
+             (integerp (elt prop 1)) ; x
+             (integerp (elt prop 2)) ; y
+             (numberp (elt prop 3))  ; width
+             (numberp (elt prop 4))) ; height
+        (and (consp prop) (eq (car prop) 'space))
+        (and (eq (car prop) 'space-width) (numberp (cadr prop)))
+        (and (consp (car prop))
+             (eq (caar prop) 'margin)
+             (or (eq (cadar prop) 'right-margin)
+                 (eq (cadar prop) 'left-margin))
+             (enriched-display-prop-safe-p (cadr prop)))
+        (and (eq (car prop) 'height)
+             (or (numberp (cadr prop))
+                 (and (listp (cadr prop))
+                      (or (eq (elt (cadr prop) 0) '+) (elt (cadr prop) 0) '-)
+                      (integerp (elt (cadr prop) 1)))))
+        (and (eq (car prop) 'raise)
+             (numberp (cadr prop)))
+        (and (seqp prop) (seq-every-p 'enriched-display-prop-safe-p prop)))))
 
 ;;; enriched.el ends here
-- 
2.9.4