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

Previous Next

Full log

View this message in rfc822 format

From: charles <at> aurox.ch (Charles A. Roelli)
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 28350 <at> debbugs.gnu.org
Subject: bug#28350: enriched.el code execution
Date: Sat, 09 Sep 2017 17:57:10 +0200

Thanks for the feedback.

> Date: Sat, 09 Sep 2017 16:45:40 +0300
> From: Eli Zaretskii <eliz <at> gnu.org>
> CC: 28350 <at> debbugs.gnu.org
> Reply-to: Eli Zaretskii <eliz <at> 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?

Previous Next

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