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

Previous Next

Full log

Message #27 received at 28350 <at> debbugs.gnu.org (full text, mbox):

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

> Date: Sat, 09 Sep 2017 14:23:54 +0200
> From: charles <at> aurox.ch (Charles A. Roelli)
> CC: 28350 <at> debbugs.gnu.org
> 
> Thank you.  Does the attached patch look OK?  I've used the file
> enriched-test-safe-props.txt (also attached) to test that safe
> properties are still applied.

Thank you for working on this.  I have some comments:

> --- 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.

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

> +        (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.

> +        (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.

Previous Next

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