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

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

[Clément Pit-Claudel]

Besides this particular problem, whose fix might just be to remove an already deprecated feature, there's the more general issue that many people enable Flymake and Flycheck unconditionally in prog-mode and derivatives.

Flycheck has that same code execution problem, and it's been a known design issue for a while.  I think the right fix is to make a safe ELisp macroexpander (we try call checkers for languages that execute code at compile time with appropriate flags, but such flags are not always available; locally, I have a list of paths to enable Flycheck on, and I disable it otherwise).

Clément.

On 2019-10-15 17:05, adam plaice wrote:
> 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
>>
>>
> 
> 

Re: 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

Re: 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

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

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

[Eli Zaretskii]

> From: adam plaice <plaice.adam+lists@gmail.com>
> Date: Tue, 15 Oct 2019 23:05:01 +0200
> Cc: 37656@debbugs.gnu.org
> 
> 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.

Please don't cross-post bug reports.  Please use only one of these two
lists for any discussions, preferably the bug list.  Doing it on both
tends to cause some of the responses to be archived in one list,
others in another, and makes the discussion confused, confusing, and
complicated to follow.

I suggest to continue discussing this only on the bug list.

Thanks in advance.

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

[Eli Zaretskii]

> From: Stefan Kangas <stefan@marxist.se>
> Date: Wed, 16 Oct 2019 00:55:08 +0200
> Cc: 37656@debbugs.gnu.org, Emacs developers <emacs-devel@gnu.org>
> 
> Here is a more complete patch.  Does it look like the right fix?

As explained on the bug list (and let's please continue discussing
this there), I don't think this is the right solution.

Thanks.