bug#28350: enriched.el code execution

[Charles A. Roelli]

Thanks for the feedback.

> Date: Sat, 09 Sep 2017 16:45:40 +0300
> From: Eli Zaretskii <eliz@gnu.org>
> CC: 28350@debbugs.gnu.org
> Reply-to: Eli Zaretskii <eliz@gnu.org>
> 
> > --- a/lisp/textmodes/enriched.el
> > +++ b/lisp/textmodes/enriched.el
> > @@ -503,6 +503,47 @@ 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))))
> 
> I think we will want to allow unsafe display properties, given a
> user's explicit permission.  So I think we need a defcustom that
> allows this, and then enriched-display-prop-safe-p should always
> return non-nil.

Agreed, I've added this.

> > +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?

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. 

?

> 
> > +        (and (eq (car prop) 'space-width)
> > +             (or (integerp (cadr prop)) (floatp (cadr prop))))
> > +        (and (consp (car prop))
> > +             (eq (caar prop) 'margin)
> > +             (or (eq (cadar prop) 'right-margin)
> > +                 (eq (cadar prop) 'left-margin))
> > +             (stringp (cadr prop)))
> 
> The margin display can also specify an image, not just a string, and I
> think that would be safe as well.

Okay, I'll apply the same procedure as we decide for the above image
spec.

> 
> > +        (and (eq (car prop) 'height)
> > +             (or (integerp (cadr prop))
> > +                 (and (listp (cadr prop))
> > +                      (or (eq (elt (cadr prop) 0) '+) (elt (cadr prop) 0) 
> > '-)
> > +                      (integerp (elt (cadr prop) 1)))))
>                           ^^^^^^^^
> I think this should be numberp, as the value could also safely be a
> float.
> 
> > +        (and (eq (car prop) 'raise)
> > +             (integerp (cadr prop))))))
>                  ^^^^^^^^
> The FACTOR in (raise FACTOR) can also be a float, so I think numberp
> is the correct predicate here.
> 
> And then what about (space . PROPS) type of display spec?  I think all
> of its variants are safe.

Okay, I've made these changes and added the `space' spec.  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?