bug#28350: enriched.el code execution

bug#28350: enriched.el code execution

[Charles A. Roelli]

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

Enriched mode implements an extension command to the text/enriched
format called "x-display", which stores "display" text properties.
It was added awhile ago:

  commit d9e28c1ca1d95f51a05d052dcf1fe06888d52476
  Author: Gerd Moellmann <gerd@gnu.org>
  Date:   Wed Jul 21 21:43:03 1999 +0000

  (enriched-translations): Add `display' and "x-display".
  (enriched-handle-display-prop): New.
  (enriched-decode-display-prop): New.

It's possible to use this extension command to transparently execute
arbitrary code in an Emacs process that opens a text/enriched file.
For example, if you open a file containing the following contents:

Content-Type: text/enriched
Text-Width: 70

<x-display><param>(when (message "hello world") nil)</param>test</x-display>

Then "hello world" will be printed in the echo area whenever the
"test" text is displayed (which is immediate).  Note that the
s-expression between the <param> tags needs to conform to a "display"
spec: but since there are a few display specs that can execute code,
it's not difficult to craft a file that could have bad effects (shell
commands work, for example).  Additionally, such a file can be
compressed with gzip (thus hiding the contents), and when it is
opened, Emacs will automatically decompress it and apply the display
properties.  Attached is an example file (enriched-bug-example.txt)
that turns the mode line red as soon as you open it.  It works in
23.4, 24.5, 25.2 and master (and possibly earlier versions -- I
haven't tested).

Other extensions in `enriched-translations' of enriched.el may have
similar issues (I don't understand them all, so I hope somebody else
can make sure).


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: enriched-bug-example.txt --]
[-- Type: text/enriched, Size: 157 bytes --]

Content-Type: text/enriched
Text-Width: 70

<x-display><param>(when (set-face-attribute (quote mode-line) nil :background "red") nil)</param>test</x-display>

bug#28350: enriched.el code execution

[Charles A. Roelli]

If anyone wants a fix to apply locally, the following s-expression
prevents the display parameter from being used by Enriched mode
(tested in Emacs 23+):

(eval-after-load "enriched"
  '(defun enriched-decode-display-prop (start end &optional param)
     (list start end)))

As for a fix to apply to master: I'd like to keep "x-display" if we
can agree on some "safe" predicate that the given parameter would have
to satisfy.  Looking at the list of display specifications that are
available, it seems that simple string, margin text, space-width,
height (only in the (+ n), (- n) and n cases) and raise specifications
should be okay.  Does anybody else have an opinion about this?

bug#28350: enriched.el code execution

[Eli Zaretskii]

> Date: Wed, 06 Sep 2017 21:25:18 +0200
> From: charles@aurox.ch (Charles A. Roelli)
> 
> As for a fix to apply to master: I'd like to keep "x-display" if we
> can agree on some "safe" predicate that the given parameter would have
> to satisfy.  Looking at the list of display specifications that are
> available, it seems that simple string, margin text, space-width,
> height (only in the (+ n), (- n) and n cases) and raise specifications
> should be okay.  Does anybody else have an opinion about this?

I agree that the cases you have shown are safe.

Thanks.

bug#28350: enriched.el code execution

[Charles A. Roelli]

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

> Date: Thu, 07 Sep 2017 05:34:34 +0300
> From: Eli Zaretskii <eliz@gnu.org>
> CC: 28350@debbugs.gnu.org
> 
> > Date: Wed, 06 Sep 2017 21:25:18 +0200
> > From: charles@aurox.ch (Charles A. Roelli)
> > 
> > As for a fix to apply to master: I'd like to keep "x-display" if we
> > can agree on some "safe" predicate that the given parameter would have
> > to satisfy.  Looking at the list of display specifications that are
> > available, it seems that simple string, margin text, space-width,
> > height (only in the (+ n), (- n) and n cases) and raise specifications
> > should be okay.  Does anybody else have an opinion about this?
> 
> I agree that the cases you have shown are safe.
> 
> Thanks.

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.


[-- 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: 2524 bytes --]

From 1c58b3e76a80a342c2f7e96d91214fe49678f471 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-display-prop-safe-p): New
function.
(enriched-decode-display-prop): Use it to prevent unsafe display
properties from being applied.
---
 lisp/textmodes/enriched.el | 43 ++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 42 insertions(+), 1 deletion(-)

diff --git a/lisp/textmodes/enriched.el b/lisp/textmodes/enriched.el
index 7ace2a5..f496259 100644
--- 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))))
+
+(defun enriched-display-prop-safe-p (prop)
+  "Return t if display property PROP is safe to apply to text.
+
+A safe display property is either:
+
+  - a string,
+
+  - 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,
+
+  - a height display specification, (height spec), where SPEC is
+    of the form (+ n), (- n) or n, and N is an integer,
+
+  - or a raise display specification, (raise factor), where
+    FACTOR is an integer.
+
+See Info node `(elisp)Display Property' for the use of these
+display specifications."
+  (ignore-errors
+    (or (stringp prop)
+        (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)))
+        (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)))))
+        (and (eq (car prop) 'raise)
+             (integerp (cadr prop))))))
 
 ;;; enriched.el ends here
-- 
2.9.4


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: enriched-test-safe-props.txt --]
[-- Type: text/enriched, Size: 367 bytes --]

Content-Type: text/enriched
Text-Width: 70

<x-display><param>"replace"</param>test</x-display>

<x-display><param>(space-width 5)</param>large spaces</x-display>

<x-display><param>((margin left-margin) "string")</param>marginal text</x-display>

<x-display><param>(height 3)</param>tall text</x-display>

<x-display><param>(raise 5)</param>raised text</x-display>

bug#28350: enriched.el code execution

[Eli Zaretskii]

> Date: Sat, 09 Sep 2017 14:23:54 +0200
> From: charles@aurox.ch (Charles A. Roelli)
> CC: 28350@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.

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?

bug#28350: enriched.el code execution

[Eli Zaretskii]

> Date: Sat, 09 Sep 2017 17:57:10 +0200
> From: charles@aurox.ch (Charles A. Roelli)
> 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?

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

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

Thanks.

bug#28350: enriched.el code execution

[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

bug#28350: enriched.el code execution

[Eli Zaretskii]

> Date: Sat, 09 Sep 2017 22:37:29 +0200
> From: charles@aurox.ch (Charles A. Roelli)
> CC: 28350@debbugs.gnu.org
> 
> Thank you.  I've kept the current approach.  Please see again the
> attached patch.

Some minor nits below.

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

Yes, I think so.  And your patch already does allow them, doesn't it?

> +(defcustom enriched-allow-unsafe-display-props nil
> +  "Variable determining whether to decode arbitrary display properties.

"If non-nil allow to evaluate arbitrary forms in display properties."

> +  :risky t
> +  :type 'boolean
> +  :group 'enriched)

Please add :version here.  Please also add a short NEWS entry.

It would be good to have tests for this, but doing that is much less
urgent than fixing the vulnerability, so please feel free to do so as
a separate commit (unless you already have the tests ready).

Otherwise, looks good to me.  Thanks.

bug#28350: enriched.el code execution

[Glenn Morris]

Eli Zaretskii wrote:

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

FWIW, there is 'unsafep'.

bug#28350: enriched.el code execution

[Eli Zaretskii]

> From: Glenn Morris <rgm@gnu.org>
> Cc: charles@aurox.ch (Charles A. Roelli),  28350@debbugs.gnu.org
> Date: Mon, 11 Sep 2017 12:32:38 -0400
> 
> FWIW, there is 'unsafep'.

Thanks.  Did that pass any audits from security experts?

bug#28350: enriched.el code execution

[Paul Eggert]

Thanks for reporting this bug. Since it is a serious security hole I have 
installed a patch by Lars Ingebrigtsen that temporarily disables the problematic 
translations, and that also changes Gnus to not call enriched-decode. For the 
emacs-25 branch the patch is here:

https://git.savannah.gnu.org/cgit/emacs.git/commit/?h=emacs-25&id=9ad0fcc54442a9a01d41be19880250783426db70

and for the master branch the patch is here:

https://git.savannah.gnu.org/cgit/emacs.git/commit/?id=19584f13b1e2e4a778602a8302619ef5c675e68b

As this patch is merely a workaround to close the security hole, I am not 
marking the underlying bug as fixed.

Thank you for reporting the problem.

bug#28350: enriched.el code execution

[Charles A. Roelli]

> From: Paul Eggert <eggert@cs.ucla.edu>
> Date: Sat, 9 Sep 2017 15:43:30 -0700
> 
> Thanks for reporting this bug. Since it is a serious security hole I have 
> installed a patch by Lars Ingebrigtsen that temporarily disables the problematic 
> translations, and that also changes Gnus to not call enriched-decode. For the 
> emacs-25 branch the patch is here:
> 
> https://git.savannah.gnu.org/cgit/emacs.git/commit/?h=emacs-25&id=9ad0fcc54442a9a01d41be19880250783426db70
> 
> and for the master branch the patch is here:
> 
> https://git.savannah.gnu.org/cgit/emacs.git/commit/?id=19584f13b1e2e4a778602a8302619ef5c675e68b
> 
> As this patch is merely a workaround to close the security hole, I am not 
> marking the underlying bug as fixed.
> 
> Thank you for reporting the problem.

Thanks for these fixes.  I have some comments:

> branch: master
> commit 19584f13b1e2e4a778602a8302619ef5c675e68b
> Author: Lars Ingebrigtsen <larsi@gnus.org>
> Commit: Paul Eggert <eggert@cs.ucla.edu>
>
> [...]
>
> --- 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"?  If not, I can still re-add them at a
later time.

> branch: emacs-25
> commit b6389930146882a77c22901a4357e287826fc7ff
> Author: Paul Eggert <eggert@cs.ucla.edu>
> Commit: Paul Eggert <eggert@cs.ucla.edu>
>
> [...]
>
> +** Enriched text mode no longer supports the 'FUNCTION' and 'display'
> +translations, and Gnus no longer processes enriched text when
> +inlining.  This fixes bugs introduced in Emacs 19.29.  To work around
> +these bugs in Emacs versions 19.29 through 25.2, append the following
> +to your ~/.emacs file:
> +
> +  (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)))

But it may not work in Emacs earlier than 23 (I can't test it).

bug#28350: enriched.el code execution

[Paul Eggert]

Charles A. Roelli wrote:

> Do we know that "x-color" and/or "x-bg-color" are vulnerable to a
> similar misuse as "x-display"?  If not, I can still re-add them at a
> later time.

Eli asked the same question privately. I don't know the code myself; perhaps 
Lars could say.

>> +  (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)))
> 
> But it may not work in Emacs earlier than 23 (I can't test it).

It should work, since eval-after-load predates Emacs 19.29.  Though it assumes 
that x-display is the only problem here.

bug#28350: enriched.el code execution

[Eli Zaretskii]

> From: Paul Eggert <eggert@cs.ucla.edu>
> Date: Sun, 10 Sep 2017 14:46:59 -0700
> Cc: larsi@gnus.org, 28350@debbugs.gnu.org
> 
> >    (eval-after-load "enriched"
> >      '(defun enriched-decode-display-prop (start end &optional param)
> >         (list start end)))
> > 
> > But it may not work in Emacs earlier than 23 (I can't test it).
> 
> It should work, since eval-after-load predates Emacs 19.29.  Though it assumes 
> that x-display is the only problem here.

x-display _is_ the only problem, because only it allows arbitrary Lisp
forms.

bug#28350: enriched.el code execution

[Eli Zaretskii]

> Date: Mon, 11 Sep 2017 05:39:27 +0300
> From: Eli Zaretskii <eliz@gnu.org>
> Cc: larsi@gnus.org, charles@aurox.ch, 28350@debbugs.gnu.org
> 
> > From: Paul Eggert <eggert@cs.ucla.edu>
> > Date: Sun, 10 Sep 2017 14:46:59 -0700
> > Cc: larsi@gnus.org, 28350@debbugs.gnu.org
> > 
> > >    (eval-after-load "enriched"
> > >      '(defun enriched-decode-display-prop (start end &optional param)
> > >         (list start end)))
> > > 
> > > But it may not work in Emacs earlier than 23 (I can't test it).
> > 
> > It should work, since eval-after-load predates Emacs 19.29.  Though it assumes 
> > that x-display is the only problem here.
> 
> x-display _is_ the only problem, because only it allows arbitrary Lisp
> forms.

I eventually decided to provide a simpler patch, see below.  The
original changes unnecessarily removed the capability to encode
display properties while saving Enriched Mode text, something that
doesn't have any security issues (because the vulnerability is on the
receiving end).  I also prefer not to remove the offending code, but
instead to comment it out, as I believe this is more in the tradition
of Free Software to let people eyeball what we did.  Finally, I
rewrote the NEWS entry to be more accurate wrt the actual change.

Nicolas is working on the release as we speak, so if someone has
suggestions, or objections, or something else important to say about
the patch, please speak up.

I'd like to take this opportunity to thank all those who worked and
continue working on fixing this vulnerability.


2017-09-11  Eli Zaretskii  <eliz@gnu.org>

	* etc/NEWS: Document the vulnerability and its resolution.
	Include a workaround.  Suggested by Charles A. Roelli
	<charles@aurox.ch>.

	* lisp/gnus/mm-view.el (mm-inline-text): Disable decoding of
	"enriched" and "richtext" MIME objects.  Suggested by Lars
	Ingebrigtsen <larsi@gnus.org>.

	* lisp/textmodes/enriched.el (enriched-decode-display-prop): Don't
	produce 'display' properties.  (Bug#28350)


--- lisp/textmodes/enriched.el~0	2017-02-03 12:25:44.000000000 +0200
+++ lisp/textmodes/enriched.el	2017-09-11 17:31:35.943569900 +0300
@@ -503,6 +503,9 @@
 		  (error nil)))))
     (unless prop
       (message "Warning: invalid <x-display> parameter %s" param))
-    (list start end 'display prop)))
+    ;; Disabled in Emacs 25.3 to avoid execution of arbitrary Lisp
+    ;; forms in display properties stored within enriched text.
+    ;; (list start end 'display prop)))
+    (list start end)))
 
 ;;; enriched.el ends here


--- lisp/gnus/mm-view.el~0	2017-02-03 12:25:44.000000000 +0200
+++ lisp/gnus/mm-view.el	2017-09-11 16:56:58.804519400 +0300
@@ -383,10 +383,12 @@
 	(goto-char (point-max))))
     (save-restriction
       (narrow-to-region b (point))
-      (when (member type '("enriched" "richtext"))
-        (set-text-properties (point-min) (point-max) nil)
-	(ignore-errors
-	  (enriched-decode (point-min) (point-max))))
+      ;; Disabled in Emacs 25.3 to avoid execution of arbitrary Lisp
+      ;; forms in display properties supported by enriched.el.
+      ;; (when (member type '("enriched" "richtext"))
+      ;;   (set-text-properties (point-min) (point-max) nil)
+      ;; 	(ignore-errors
+      ;; 	  (enriched-decode (point-min) (point-max))))
       (mm-handle-set-undisplayer
        handle
        `(lambda ()


--- etc/NEWS~0	2017-02-21 11:08:27.000000000 +0200
+++ etc/NEWS	2017-09-11 17:21:06.994252400 +0300
@@ -16,6 +16,32 @@
 with a prefix argument or by typing C-u C-h C-n.
 
 \f
+* Changes in Emacs 25.3
+
+This is an emergency release to fix a security vulnerability in Emacs.
+
+** Security vulnerability related to Enriched Text mode is removed.
+
+*** Enriched Text mode has its support for decoding 'x-display' disabled.
+This feature allows saving 'display' properties as part of text.
+Emacs 'display' properties support evaluation of arbitrary Lisp forms
+as part of instantiating the property, so decoding 'x-display' is
+vulnerable to executing arbitrary malicious Lisp code included in the
+text (e.g., sent as part of an email message).
+
+This vulnerability was introduced in Emacs 19.29.  To work around that
+in Emacs versions before 25.3, append the following to your ~/.emacs
+init file:
+
+  (eval-after-load "enriched"
+    '(defun enriched-decode-display-prop (start end &optional param)
+       (list start end)))
+
+*** Gnus no longer supports "richtext" and "enriched" inline MIME objects.
+This support was disabled to avoid evaluation of arbitrary Lisp code
+contained in email messages and news articles.
+
+\f
 * Changes in Emacs 25.2
 
 This is mainly a bug-fix release, but there are some other changes.

bug#28350: enriched.el code execution

[Eli Zaretskii]

> 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");
 

bug#28350: enriched.el code execution

[Charles A. Roelli]

> Date: Mon, 11 Sep 2017 18:18:01 +0300
> From: Eli Zaretskii <eliz@gnu.org>
> 
> 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.

Thanks for suggesting this; it's much cleaner than sanitizing the
display specification from Lisp.  Looks good to me.

bug#28350: enriched.el code execution

[Eli Zaretskii]

> Date: Mon, 11 Sep 2017 20:44:19 +0200
> From: charles@aurox.ch (Charles A. Roelli)
> CC: eggert@cs.ucla.edu, larsi@gnus.org, 28350@debbugs.gnu.org
> 
> > 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.
> 
> Thanks for suggesting this; it's much cleaner than sanitizing the
> display specification from Lisp.  Looks good to me.

Thanks, I will wait for a few days before pushing.

Thanks again for all your work on this grave issue.

bug#28350: enriched.el code execution

[Eli Zaretskii]

> Date: Mon, 11 Sep 2017 22:07:26 +0300
> From: Eli Zaretskii <eliz@gnu.org>
> Cc: larsi@gnus.org, eggert@cs.ucla.edu, 28350@debbugs.gnu.org
> 
> > Date: Mon, 11 Sep 2017 20:44:19 +0200
> > From: charles@aurox.ch (Charles A. Roelli)
> > CC: eggert@cs.ucla.edu, larsi@gnus.org, 28350@debbugs.gnu.org
> > 
> > > 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.
> > 
> > Thanks for suggesting this; it's much cleaner than sanitizing the
> > display specification from Lisp.  Looks good to me.
> 
> Thanks, I will wait for a few days before pushing.

Done.

Lars, I re-enabled support for enriched text in Gnus, as the
vulnerability is now removed.  Feel free to disable it again, if you
don't want Gnus users to be able to display enriched text, ever.

I'm marking the bug done.

bug#28350: enriched.el code execution

[Glenn Morris]

I submitted this to https://github.com/distributedweaknessfiling/ .
I see you sent it to http://seclists.org/oss-sec/2017/q3/422 .
Are you sure this issue affects Emacs 19.29, as stated there?
The x-display code is "only" present since 21.1, AFAICS.

bug#28350: enriched.el code execution

[Paul Eggert]

On 09/11/2017 08:33 AM, Glenn Morris wrote:
> I submitted this tohttps://github.com/distributedweaknessfiling/  .
> I see you sent it tohttp://seclists.org/oss-sec/2017/q3/422  .

Yes, I sent it to the oss-security mailing list, and it is archived here:

http://www.openwall.com/lists/oss-security/2017/09/11/1

> Are you sure this issue affects Emacs 19.29, as stated there?
> The x-display code is "only" present since 21.1, AFAICS.

Thanks for checking. When I wrote that, I looked for any of the text 
involved in Lars's patch. If a smaller patch will do, that might explain 
why you're seeing 21.1 rather than 19.29. We can mention 21.1 instead of 
19.29 in the 25.3 release, and I'll update etc/NEWS accordingly in 
emacs-25 and master once that comes out.

These days almost nobody is running Emacs older than 21.1, so the exact 
version number shouldn't matter to anybody other than software 
archaeologists.

bug#28350: enriched.el code execution

[Glenn Morris]

Paul Eggert wrote:

> We can mention 21.1 instead of 19.29 in the 25.3 release, and I'll
> update etc/NEWS accordingly in emacs-25 and master once that comes
> out.

Too late. :(


For the record:

commit d9e28c1
Date:   Wed Jul 21 21:43:03 1999 +0000

    (enriched-translations): Add `display' and "x-display".

git describe --contains --tags d9e28c1
emacs-pretest-21.0.90~7452

> These days almost nobody is running Emacs older than 21.1, so the exact 
> version number shouldn't matter to anybody other than software 
> archaeologists.

Indeed not, but 19.29 v 21.1 is "staggeringly bad" v "extremely bad" for me.

bug#28350: enriched.el code execution

[Paul Eggert]

On 09/11/2017 02:16 PM, Glenn Morris wrote:
> Too late. :(

Yes, that horse left the barn. To close the barn door, I changed 
emacs-25's etc/NEWS (and master's etc/NEWS.25) to say "21.1" rather than 
"19.29" for when the bug was introduced, so that we look just "extremely 
bad" rather than "staggeringly bad" in the latest source code.