bug#37656: 27.0.50; Arbitrary code execution with special `mode:'

bug#37656: 27.0.50; Arbitrary code execution with special `mode:'

[adam plaice]

Since the bug allows an attacker to execute arbitrary code if the
victim opens a payload file, and hence opening any file from an
untrusted source becomes dangerous, it seems to be rather
serious.

The bug relies on the fact that flymake-mode can execute arbitrary
code, that minor modes (in particular, flymake-mode) can be set with
local variables (with `mode:') and that when a minor-mode is set in
this way, the major-mode is not unset. (See the linked bug or below
for details.)

I'm not sure whether I should be bringing greater attention to it,
but given that it's already in the open, and malicious actors can
find it (or just come up with it themselves, as it's not a particularly
complex idea), increasing the likelihood of getting it fixed hopefully
outweighs the disadvantages.

I'd offer to provide a patch, but I'm neither very proficient with
Emacs lisp, nor a security expert.  I also haven't signed any copyright
papers.


Some thoughts on potential solutions (from a well-intentioned, but
possibly misguided layman):

AFAICT the easiest way to prevent this specific bug would be to
prevent more than one mode being set by the file and directory
local-variables machinery.

Perhaps also only allowing major modes to be set with `mode' in local
variables (and only allowing minor-modes to be set with `eval', as is
already encouraged in the manual), might decrease the "attack surface"
for similar such attacks.

I'm not sure whether any major modes are "unsafe" (in the way flymake
is), but possibly it might make sense to mark major modes as safe,
similarly to the way variables are, though that would be a far more
extensive change.

Thank you,
Adam

PS Should Emacs have some policies on reporting security issues? I
was encouraged (via an earlier e-mail exchange) to post the bug to
debbugs, as normal, but it might perhaps be useful if the process
(specifically for security vulnerabilities, not bugs in general) were
mentioned in the manual.

> https://debbugs.gnu.org/cgi/bugreport.cgi?bug=37656
>
> * To reproduce:
>
> 1. Create a file, say `~/foobar', (it could have an arbitrary
> extension) with the following contents:
>
> -*- mode: emacs-lisp; mode: flymake -*-
>
> (eval-when-compile
>   (with-temp-file "~/emacs_flymake_security_bug"
>       (insert "Could have also executed any code.")))
>
> 2. Open the file with emacs:
>
> emacs -Q ~/foobar
>
> 3. Inspect ~/emacs_flymake_security_bug:
>
> cat ~/emacs_flymake_security_bug
>
> * Expected result
>
> ~/emacs_flymake_security_bug does not exist.
>
> * Actual result
>
> ~/emacs_flymake_security_bug does exist.
>
> * Further information
>
> This relies on the "deprecated" feature of allowing `mode: ' to be
> repeated more than once, to also specify minor modes.  Just having:
>
> -*- mode: flymake -*-
>
> in, say, `~/foobar.el' would not trigger the security bug.  There may,
> however, be alternative ways of triggering it, that I haven't come up
> with.
>
>
> This was "inspired" by a very similar bug (concerning an external
> package, editorconfig), described here:
>
> https://illikainen.dev/blog/2019-10-06-editorconfig
>
> Thank you and best regards,
> Adam
>
>

bug#37656: 27.0.50; Arbitrary code execution with special `mode:'

[Stefan Kangas]

adam plaice <plaice.adam+lists@gmail.com> writes:

> Since the bug allows an attacker to execute arbitrary code if the
> victim opens a payload file, and hence opening any file from an
> untrusted source becomes dangerous, it seems to be rather
> serious.

Thanks for raising this here.  I agree that this is serious, and we
should treat it accordingly.

The below patch seems to fix it by disabling the feature it exploits.

A workaround is to add this to your init file:
(setq enable-local-variables nil)

Best regards,
Stefan Kangas


diff --git a/lisp/files.el b/lisp/files.el
index 40807617fa..550227b21a 100644
--- a/lisp/files.el
+++ b/lisp/files.el
@@ -3068,7 +3068,7 @@ set-auto-mode
           (if (save-excursion (search-forward ":" end t))
               ;; Find all specifications for the `mode:' variable
               ;; and execute them left to right.
-          (while (let ((case-fold-search t))
+        (when (let ((case-fold-search t))
                        (or (and (looking-at "mode:")
                                 (goto-char (match-end 0)))
                            (re-search-forward "[ \t;]mode:" end t)))

bug#37656: 27.0.50; Arbitrary code execution with special `mode:'

[Stefan Kangas]

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

Stefan Kangas <stefan@marxist.se> writes:
> The below patch seems to fix it by disabling the feature it exploits.

Here is a more complete patch.  Does it look like the right fix?

I think the relevant node in the documentation is:
(info "(emacs)Choosing Modes")

Best regards,
Stefan Kangas

[-- Attachment #2: 0001-Remove-support-for-more-than-one-mode-in-file-local-.patch --]
[-- Type: application/octet-stream, Size: 2032 bytes --]

From d640efe970ed1fdd5f1262d09bfd6d564c8e7f80 Mon Sep 17 00:00:00 2001
From: Stefan Kangas <stefankangas@gmail.com>
Date: Wed, 16 Oct 2019 00:44:56 +0200
Subject: [PATCH] Remove support for more than one mode in file local variables

* lisp/files.el (set-auto-mode): Remove support for specifying
more than one major mode in file local variables.  (Bug#37656)
* etc/NEWS: Announce it.
---
 etc/NEWS      |  7 +++++++
 lisp/files.el | 13 +++++++------
 2 files changed, 14 insertions(+), 6 deletions(-)

diff --git a/etc/NEWS b/etc/NEWS
index d06f0a5952..d0cec05143 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -197,6 +197,13 @@ the new version of the file again.)
 \f
 * Changes in Emacs 27.1
 
+---
+** File local variables can now specify only one major mode.
+Previously, it was possible to specify more than one major mode using
+file local variables.  A security issue was discovered where an
+attacker could exploit this feature using a specially crafted file,
+which could lead to arbitrary code execution when the file was opened.
+
 ** emacsclient
 
 +++
diff --git a/lisp/files.el b/lisp/files.el
index 40807617fa..41c1ba60ed 100644
--- a/lisp/files.el
+++ b/lisp/files.el
@@ -3066,12 +3066,13 @@ set-auto-mode
 	   try-locals
 	   (setq end (set-auto-mode-1))
 	   (if (save-excursion (search-forward ":" end t))
-	       ;; Find all specifications for the `mode:' variable
-	       ;; and execute them left to right.
-	       (while (let ((case-fold-search t))
-			(or (and (looking-at "mode:")
-				 (goto-char (match-end 0)))
-			    (re-search-forward "[ \t;]mode:" end t)))
+	       ;; Find the specification of the `mode:' variable and
+	       ;; execute it.  We never want to execute more than one
+	       ;; mode for security reasons.  (Bug#37656)
+	       (when (let ((case-fold-search t))
+		       (or (and (looking-at "mode:")
+				(goto-char (match-end 0)))
+			   (re-search-forward "[ \t;]mode:" end t)))
 		 (skip-chars-forward " \t")
 		 (let ((beg (point)))
 		   (if (search-forward ";" end t)
-- 
2.23.0

bug#37656: 27.0.50; Arbitrary code execution with special `mode:'

[Stefan Kangas]

Stefan Kangas <stefan@marxist.se> writes:

> > The below patch seems to fix it by disabling the feature it exploits.
>
> Here is a more complete patch.  Does it look like the right fix?

flymake.el was first added to Emacs in version 22.1:
4bcbcb9df3 2004-05-29 Eli Zaretskii New file.

The "multiple mode specification feature" dates back to:
9fa7bfe524 1993-09-11 Richard M. Stallman
    (hack-local-variables-prop-line): Ignore any specification
    for `mode:', since set-auto-mode has already handled it.
    (set-auto-mode): Clean up.  Handle more than one `mode:' spec in -*-.

The code that my proposed patch changes has stayed untouched since
this 1993 commit.  If we agree that disabling this feature is the
solution here, a backported security fix should therefore hopefully be
a one liner all the way back to version 22.1.

Best regards,
Stefan Kangas

bug#37656: 27.0.50; Arbitrary code execution with special `mode:'

[Adam Plaice]

> Here is a more complete patch.  Does it look like the right fix?

This indeed fixes the issue! Thanks for dealing with it so quickly! (Though
I'm obviously not qualified to say whether it's _the_ right fix for this.)

>  I think the relevant node in the documentation is:
> (info "(emacs)Choosing Modes")

That, and part of:
(info "(emacs)Specifying File Variables")


Unfortunately, I've realised that a similar problem can be introduced
with directory variables. (Should I file separate bug for this as it's
closely related but not quite the same?) This requires at least two
files, so it's not quite as serious:

In .dir-locals.el:

((nil . ((mode . flymake))))

In, say, foobar, in the same directory:

-*- mode: emacs-lisp -*-

(eval-when-compile
  (with-temp-file "~/emacs_flymake_security_bug"
    (insert "Could have also executed any code.")))


(Some other, equivalent arrangements (e.g. (mode . emacs-lisp) directly in
.dir-locals.el), or simply an .el extension, also "work".)

According to the manual (info "(emacs)Directory Variables"):

> The special ‘mode’ element specifies the minor mode to be
> enabled.  So ‘(mode . auto-fill)’ specifies that the minor mode
> ‘auto-fill-mode’ needs to be enabled.

so in this case setting the minor mode _is_ the intended/documented behaviour,
which might make resolving the bug harder.

(OTOH (info "(emacs)Directory Variables") also states:

> You can specify the variables ‘mode’, ‘eval’, and ‘unibyte’ in your
> ‘.dir-locals.el’, and they have the same meanings as they would have in
> file local variables.

while (info "(emacs)Specifying File Variables") says:

> The special variable/value pair ‘mode:
> MODENAME;’, if present, specifies a major mode.

so there's some inconsistency on what `mode' in .dir-locals.el is actually
"supposed" to specify — a major mode, a minor mode or either.)

Thanks,
Adam

bug#37656: 27.0.50; Arbitrary code execution with special `mode:'

[Phil Sainty]

On 2019-10-16 11:55, Stefan Kangas wrote:
> Here is a more complete patch.  Does it look like the right fix?

I don't think so.  If we're removing the multiple 'mode' feature, then
`set-auto-mode' says the following about it:

     ;; Once we drop the deprecated feature where mode: is also allowed 
to
     ;; specify minor-modes (ie, there can be more than one "mode:"), we 
can
     ;; remove this section and just let (hack-local-variables t) handle 
it.
     ;; Find a -*- mode tag.

bug#37656: 27.0.50; Arbitrary code execution with special `mode:'

[Eli Zaretskii]

> From: Adam Plaice <plaiceadam@gmail.com>
> Date: Wed, 16 Oct 2019 02:35:58 +0200
> Cc: 37656@debbugs.gnu.org, Emacs developers <emacs-devel@gnu.org>
> 
> Unfortunately, I've realised that a similar problem can be introduced
> with directory variables.

Indeed, and I expect the same problem to pop up in other places.

Which is why I think the problem should be solved in those modes which
allow execution of arbitrary code via file-local variables without any
security precautions or other limitations, at least under user
control.

> (Should I file separate bug for this as it's closely related but not
> quite the same?)

No, it's the same problem, and I don't like the proposed solution for
the reasons explained above.  I think we need a different solution.

bug#37656: 27.0.50; Arbitrary code execution with special `mode:'

[Eli Zaretskii]

> From: Stefan Kangas <stefan@marxist.se>
> Date: Wed, 16 Oct 2019 01:17:51 +0200
> Cc: 37656@debbugs.gnu.org, Emacs developers <emacs-devel@gnu.org>
> 
> The "multiple mode specification feature" dates back to:
> 9fa7bfe524 1993-09-11 Richard M. Stallman
>     (hack-local-variables-prop-line): Ignore any specification
>     for `mode:', since set-auto-mode has already handled it.
>     (set-auto-mode): Clean up.  Handle more than one `mode:' spec in -*-.
> 
> The code that my proposed patch changes has stayed untouched since
> this 1993 commit.  If we agree that disabling this feature is the
> solution here, a backported security fix should therefore hopefully be
> a one liner all the way back to version 22.1.

This feature was described as "deprecated", but where and why did we
deprecate it?

bug#37656: 27.0.50; Arbitrary code execution with special `mode:'

[Adam Plaice]

> Please don't cross-post bug reports.  Please use only one of these two
> lists for any discussions, preferably the bug list.

Sorry!

> This feature was described as "deprecated", but where and why did we
> deprecate it?

I think bug#8613 is where the decision was made.  The deprecation is
mentioned in files.el and the manual warns against using `mode:' for
minor modes in:
(info "(emacs) Specifying File Variables")

Adam

bug#37656: 27.0.50; Arbitrary code execution with special `mode:'

[Eli Zaretskii]

> From: Adam Plaice <plaiceadam@gmail.com>
> Date: Wed, 16 Oct 2019 13:51:57 +0200
> Cc: Stefan Kangas <stefan@marxist.se>, 37656@debbugs.gnu.org
> 
> > This feature was described as "deprecated", but where and why did we
> > deprecate it?
> 
> I think bug#8613 is where the decision was made.  The deprecation is
> mentioned in files.el and the manual warns against using `mode:' for
> minor modes in:
> (info "(emacs) Specifying File Variables")

OK, thanks.

However, I don't think that removing the feature will solve the more
general problem in this bug report.

bug#37656: 27.0.50; Arbitrary code execution with special `mode:'

[Phil Sainty]

> > -*- mode: emacs-lisp; mode: flymake -*-
> > This relies on the "deprecated" feature of allowing `mode: '
> > to be repeated more than once, to also specify minor modes.
> > Just having: -*- mode: flymake -*- [...] would not trigger
> > the security bug.


On 17/10/19 6:09 AM, Eli Zaretskii wrote:
> I don't think that removing the feature will solve the more
> general problem in this bug report.


In particular it seems there is no point in removing the deprecated
method of calling a minor mode using local variables because, after
testing, the *approved* method of calling a minor mode via local
variables causes the same behaviour.  i.e.:

-*- mode: emacs-lisp; eval:(flymake-mode 1); -*-


So the deprecated approach isn't actually a factor here.


-Phil

bug#37656: 27.0.50; Arbitrary code execution with special `mode:'

[Eli Zaretskii]

> Cc: Adam Plaice <plaiceadam@gmail.com>, 37656@debbugs.gnu.org,
>  stefan@marxist.se
> From: Phil Sainty <psainty@orcon.net.nz>
> Date: Thu, 17 Oct 2019 08:09:04 +1300
> 
> On 17/10/19 6:09 AM, Eli Zaretskii wrote:
> > I don't think that removing the feature will solve the more
> > general problem in this bug report.
> 
> 
> In particular it seems there is no point in removing the deprecated
> method of calling a minor mode using local variables because, after
> testing, the *approved* method of calling a minor mode via local
> variables causes the same behaviour.  i.e.:
> 
> -*- mode: emacs-lisp; eval:(flymake-mode 1); -*-
> 
> 
> So the deprecated approach isn't actually a factor here.

Right, thanks for confirming.

The question is: can we do something in core to prevent these
problems, or does the solution have to be in the individual minor
modes?

bug#37656: 27.0.50; Arbitrary code execution with special `mode:'

[Adam Plaice]

> So the deprecated approach isn't actually a factor here.

FWIW bug#8613 included a discussion of adding an optional `:risky'
argument to define-minor-mode.  If RISKY were absent (or nil) then the
relevant minor mode function would have its `safe-local-eval-function'
property set to t.  (Why a `:risky' argument rather than a `:safe'
one, would have been preferable, is discussed in the bug.)  In the end,
this was not implemented, (and the alternative approach of treating
modes as a special case in `hack-one-local-variable-eval-safep', was
taken).  It was decided to not be needed yet, as the case of an
unsafe minor mode was considered hypothetical.

> I think it goes further than just flymake support for Elisp: flymake
> support for other major modes may also end up running arbitrary code
> (tho it will depend on the specifics).

The advantage of being able to mark minor modes as "risky" would be
that it might help solve the issue for all flymake backends and for
any third-party minor modes which are unsafe, with minimal changes
needed for such backends/modes.

Adam