The `risky-local-variable' blacklist

The `risky-local-variable' blacklist

[Davis Herring]

(Apologies in advance for a long message, but this is a long issue.)

While looking at diffs for `timeclock.el', I noticed the addition of a
risk-local-variable declaration for "timeclock-mode-string".  This is
certainly justified, but calls forth a bigger concern: is it wise to apply
a 'trust by default' policy when such innocuous-looking variables as that
mode-string can completely compromise a user's security (including
modifying configurations for further attacks)?  Not to mention the threat
posed by Lisp authors (who possibly write popular packages not picked over
by the maintainers) that accidentally endanger their users (unless and
until the author (if needed) learns about risky variables, recognizes the
risk, and changes the code, and the users upgrade)...

The danger is especially pertinent since `enable-local-variables' defaults 
to t and `normal-mode' overrides any paranoid setting of that variable 
when called interactively or without its (optional!) argument.

It seems much safer in the long term to allow only specific variables to
be set by a local-variables specification, especially since relatively few
variables are useful to set that way.  Of course, more things should be
possible with prompting, but such prompting should certainly (or at least
by default) be with `yes-or-no-p'.

Fortunately, this does not break existing code in notable ways, since the
`risky-local-variable' property would then be a no-op.  At worst, the user
encounters prompts for files which did not previously cause them.  This 
may be a bit confusing, but if the files really are legitimate, that will 
be evident at the time of the prompt.

Also, the protection versus honoring `eval' settings when root does little 
good since it does not apply in other cases; anyone interested in rooting 
via Emacs surely knows this.

Here is a list of local-variable settings used by the 21.3.1 lisp files:
calendar/calendar.el         byte-compile-dynamic    t
gnus/gnus-cite.el            (coding)                iso-latin-1
gnus/gnus-spec.el            (coding)                iso-latin-1
gnus/message.el              (coding)                iso-latin-1
gnus/nnultimate.el           (coding)                iso-latin-1
international/titdic-cnv.el  (coding)                (iso-2022-7bit)
play/landmark.el             outline-layout          (0 : -1 -1 0)
progmodes/ebrowse.el         (eval)                  (useless)
tildify.el                   (coding)                iso-latin-2
version.el                   version-control         never
allout.el                    outline-layout          (0 : -1 -1 0)
dired-x.el                   byte-compile-dynamic    t
icomplete.el                 outline-layout          (-2 :)
wid-edit.el                  byte-compile-dynamic    t

(titdic-cnv.el is adding that coding specification into a buffer.)

We see a few codings (which are safe), a few `outline-layout's (safe), a 
few `byte-compile-dynamic's (safe), a `version-control' (mostly safe; can 
risk minor data loss), and one (eval) that does nothing but `put' safe 
values on `lisp-indent-hook' properties.  From this sample, there does not 
appear to be much variety in (legitimate) local-variable settings; surely 
anything truly useful in a file will be defined either globally or by the 
mode for the file, and thus known and established by the time it is set.

Therefore, suggestions (all of them with some urgency):

* Drop `risky-local-variable' property: everything is risky unless 
blessed as safe.

* Do not make `compile-command' safe; the default value for M-x compile is
of absolutely no use if one must first make sure that it has not in fact
become "rm -rf ~/*" since you started Emacs.  Of course, the risk of 
double-tapping RET after "M-x compile" is notable as well.

* Do not provide options for trusting non-"safe" variables (much less
eval) without prompting -- at the very least, force prompting for root!

* Prompt, when prompting is necessary, with `yes-or-no-p' unless the user 
is not root and has asked for `y-or-n-p'.

* Provide a simple option to entirely prevent the setting or evaluation of
anything (except, say, mode and coding system) by a file, for use by the
paranoid.  Make this be the default for root.

* In conjunction with the option for wholesale supression of this feature, 
do not make `normal-mode' ignore these protections -- although with only
whitelisting and prompting, there is less for it to ignore.

* Fix the documentation of `enable-local-eval' so that it does not say 
that `normal-mode' acts as if it were t (fortunately it does not).

* Rename (or at least re-document) `ignored-local-variables': it does not
ignore anything but merely causes "riskiness".

* Refine the check in `hack-one-local-variable' for harmless properties: 
for instance, it appears to allow setting lisp-indent-hook to be a macro, 
or to be bytecode.  It would probably be better to have a safe-property 
property for safe property symbols, handled as is safe-local-variable.

* Do not use `enable-local-eval' as a local flag to prevent dangerous bugs 
in its handling; have `hack-one-local-variable' accept a flag FORCE which 
overrides all protections, and have it return t if FORCE was set or if the 
user accepted the risk when prompted.  (This might be better done by 
factoring the risk-checking and prompting out of 
`hack-one-local-variable'.)

* (The doozy, of course:) Mark all safe local variables as such.

I would be happy to work on all these projects, but desire opinions (on
the whole issue and on the suggested changes) and suggestions before 
undertaking such an endeavor.

Davis Herring

-- 
This product is sold by volume, not by mass.  If it seems too dense or too 
sparse, it means mass-energy conversion has occurred during shipping.

Re: The `risky-local-variable' blacklist

[Stefan]

> (Apologies in advance for a long message, but this is a long issue.)
> While looking at diffs for `timeclock.el', I noticed the addition of a
> risk-local-variable declaration for "timeclock-mode-string".  This is
> certainly justified, but calls forth a bigger concern: is it wise to apply
> a 'trust by default' policy when such innocuous-looking variables as that
> mode-string can completely compromise a user's security (including
> modifying configurations for further attacks)?

Actually, for mode-line variables, the situation is a bit more complex:
the lack of "risky-local-variable" annotation was not introducing any kind
of security hole because when we interpret a mode-line-string, we discard
any "dangerous" element (such as "eval") unless the variable is marked as
"risky".  I.e. either we check its safety via the "risky" annotation or we
assume it's dangerous and we only use known-safe elements.

So the "risky" annotation was only added in order to enable potentially
dangerous things like "eval" in that variable.


        Stefan

Re: The `risky-local-variable' blacklist

[Davis Herring]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1946 bytes --]

> Actually, for mode-line variables, the situation is a bit more complex:
> the lack of "risky-local-variable" annotation was not introducing any kind
> of security hole because when we interpret a mode-line-string, we discard
> any "dangerous" element (such as "eval") unless the variable is marked as
> "risky".  I.e. either we check its safety via the "risky" annotation or we
> assume it's dangerous and we only use known-safe elements.
> 
> So the "risky" annotation was only added in order to enable potentially
> dangerous things like "eval" in that variable.

We may be speaking at cross-purposes, but are you sure about that?

$ emacs -q lvtest   #lvtest is attached to this message
M-: java-mode-abbrev-table RET
>> cheese
M-: java-mode-syntax-table RET
>> (((nil)))

Both those variables are set sans prompting with the default security
settings; of course, those variables (and the nonsense values I supply)  
are harmless, but other random variables with no `risky' or `safe' 
indications might not be.

I realize now that my example about timeclock is silly, because
`timeclock-mode-string' wasn't dangerous before and isn't now, since it
has never been included in `mode-line-format' except as a symbol, and
those are not evaluated twice (so as to execute a form set as its value).

However, this is itself worthy of note: gm, when he applied the
`risky-local-variable' brand, had mis-diagnosed the danger of that
variable.  All it takes is for one person (whose code, whether part of
Emacs or not, is used widely) to misdiagnose (or forget to diagnose) a
variable in the other direction, and trouble ensues.

So I believe my original statements about the (potential and extreme)
danger of such things -- and the necessity of improvement in their control
-- stand.

Davis Herring

-- 
This product is sold by volume, not by mass.  If it seems too dense or too 
sparse, it means mass-energy conversion has occurred during shipping.

[-- Attachment #2: Local Variables Test --]
[-- Type: TEXT/PLAIN, Size: 122 bytes --]

//-*- mode: Java; java-mode-abbrev-table : cheese -*-

//Local variables:
//java-mode-syntax-table: (((nil)))
//End:

[-- Attachment #3: Type: text/plain, Size: 142 bytes --]

_______________________________________________
Emacs-devel mailing list
Emacs-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/emacs-devel

Re: The `risky-local-variable' blacklist

[Richard Stallman]

The problem with the change you've proposed is that we'd have to go
through and find check nearly all the variables in Emacs, and mark
most of them as ok to change.  That is a lot of work.

The default is already no for the kinds of variable names
that are typically used for dangerous variables, those that
hold commands, function names, expressions, etc.  Given that
Emacs users don't regularly get files in the mail and give
their local variables a chance to run, I don't think we have
enough of a danger to justify all that work.

    * Do not make `compile-command' safe;

This is a commonly used feature.  Simply eliminating it would make
users quite unhappy.

Here's an idea that might do the job and be acceptable.  Each time
Emacs sees a variable/value combination that is new for the current
user, it asks the user to confirm that combination.  Any given
combination only needs to be confirmed once by any given user.  This
could reduce the repetitive nuisance down to the point where people
will (1) accept the burden and (2) not zone out when they see the
questions.

What do you think?

    Also, the protection versus honoring `eval' settings when root does little 
    good since it does not apply in other cases; anyone interested in rooting 
    via Emacs surely knows this.

Sorry, I do not follow you here.

    * Do not use `enable-local-eval' as a local flag to prevent dangerous bugs 
    in its handling;

Why not?

Re: The `risky-local-variable' blacklist

[Stefan]

>> So the "risky" annotation was only added in order to enable potentially
>> dangerous things like "eval" in that variable.

> We may be speaking at cross-purposes, but are you sure about that?

You do not understand.  I was speaking very specifically about the
timeclock-mode-string.  My comments only apply to variables used as
mode-line-string.

> I realize now that my example about timeclock is silly, because
> `timeclock-mode-string' wasn't dangerous before and isn't now, since it
> has never been included in `mode-line-format' except as a symbol, and
> those are not evaluated twice (so as to execute a form set as its value).

I don't think you understand the reason why timeclock-mode-string was not
dangerous before: timeclock-mode-string can have a value of the form
(:eval <foo>) which means "evaluate <foo> to get the string to display".
So there's clearly something dangerous here.  Such evalution-in-mode-string
is new in Emacs-21, so most foo-mode-line-string variable have suddenly been
made dangerous.  Instead of going through all those vars and marking them
risky, Emacs-21 decided that "if the var is not marked `risky', then ignore
any of those new :eval special forms".  I.e. it's safe either way. but in
order to be able to use the new feature, you need to mark the var as
"risky".

I didn't discuss your general point about risky variables (because I mostly
agree with it).


        Stefan

Re: The `risky-local-variable' blacklist

[Davis Herring]

> The problem with the change you've proposed is that we'd have to go
> through and find check nearly all the variables in Emacs, and mark
> most of them as ok to change.  That is a lot of work.

I believe that few variables are really useful to set locally; that's why
I gave the list from the Emacs lisp/ directory.  There is no need, for
instance, to give a per-file value to `abbrev-all-caps', nor
`blink-cursor', `c-default-style', `c-tab-always-indent',
`christian-holidays', `comint-highlight-input' (to what file would this
even apply?), `comment-auto-fill-only-comments',
`compilation-ask-about-save', `confirm-kill-emacs', `crisp-mode',
`delete-exited-processes', `ediff-make-buffers-readonly-at-startup',
`eol-mnemonic-mac', `executable-chmod', nor many others (I obviously was
looking alphabetically).

In summary, in my Emacs (21.3.1) "emacs -q --no-site-file" yields only 498
non-risky user variables (as tested by `user-variable-p', `boundp', and
`risky-local-variable-p'), 48 of which are "-mode$" variables that
(according to the manual) should almost never be set by a file.  It is not
unreasonable to mark the most useful of these (and of those in the
regular-file major-mode lisp files, of which there are relatively few)  
`safe'; `files.el' already marks nearly 50 `risky'.  If any truly useful
ones were missed, they would be quickly noticed and easily accounted for.

I did say (sincerely) that I was willing to undertake the tedium
associated with this, including (perhaps) an easy way to report (to me,
say) a variable as obviously safe when a prompt is perceived as unneeded.  
While the probability of Emacs exploits this way is fairly small, the
resulting damage (to, among other things, people's opinions of Emacs)
would be notable, and none of us want to see that happen.

> The default is already no for the kinds of variable names
> that are typically used for dangerous variables, those that
> hold commands, function names, expressions, etc.  Given that
> Emacs users don't regularly get files in the mail and give
> their local variables a chance to run, I don't think we have
> enough of a danger to justify all that work.

(Note of course my offer of work above; notwithstanding that:) It may not
be, but some of the suggestions would still be useful (like making
`normal-mode' not override the user's preference; this would never do
worse than cause a re-prompting, which would be resolved by your
suggestion below).

>     * Do not make `compile-command' safe;
> 
> This is a commonly used feature.  Simply eliminating it would make
> users quite unhappy.

I am not advocating eliminating anything; Emacs would always -allow- the 
user to use such features.  It would simply no longer -assume- that such 
requests were safe except in very specific circumstances (variables 
blessed as safe, such as `tab-width').

> Here's an idea that might do the job and be acceptable.  Each time
> Emacs sees a variable/value combination that is new for the current
> user, it asks the user to confirm that combination.  Any given
> combination only needs to be confirmed once by any given user.  This
> could reduce the repetitive nuisance down to the point where people
> will (1) accept the burden and (2) not zone out when they see the
> questions.
>
> What do you think?

That sounds like it would work, but it also sounds like a lot of overhead 
(and more user-data to keep up with in one dot-file or another); since 
users could of course put `safe-local-variable' declarations into their 
.emacs, it might not be necessary.  Combined with the arbitrary filter 
feature (where the `safe-local-variable' property stores a check-function 
through which to run the file's value for the variable) and a simple 
interface (like that provided for disabled commands), my original 
suggestion would be almost entirely equivalent.

>     Also, the protection versus honoring `eval' settings when root does little 
>     good since it does not apply in other cases; anyone interested in rooting 
>     via Emacs surely knows this.
> 
> Sorry, I do not follow you here.

When emacs has UID 0, it will not evaluate `eval' "local variable 
settings" at all.  However, since (my claim is that) setting true 
variables with no "security properties" (neither `risky-local-variable' 
nor `safe-local-variable' set) is also dangerous, this protection is 
incomplete.  Anyone wishing to use Emacs to exploit a root account would 
be unlikely to use something obvious like `eval' anyway.

>     * Do not use `enable-local-eval' as a local flag to prevent dangerous bugs 
>     in its handling;
> 
> Why not?

Er, perhaps I miswrote.  I meant "Do not use `enable-local-eval' as a
dynamically-bound flag as well as a global user-option because that risks
confusion and bugs with a rather important variable."  However, this is
not at all the most important suggestion: the amount of code dealing with
that variable is small and easily audited.  The dual use just confused me
when I first read the code, and I imagine it might confuse someone
changing it as well.

Davis Herring

-- 
This product is sold by volume, not by mass.  If it seems too dense or too 
sparse, it means mass-energy conversion has occurred during shipping.

Re: The `risky-local-variable' blacklist

[Davis Herring]

> You do not understand.  I was speaking very specifically about the
> timeclock-mode-string.  My comments only apply to variables used as
> mode-line-string.

Ah -- the actual mode-line, not the "line in a file that sets the mode".  
Oops.

> I don't think you understand the reason why timeclock-mode-string was not
> dangerous before: timeclock-mode-string can have a value of the form
> (:eval <foo>) which means "evaluate <foo> to get the string to display".
> So there's clearly something dangerous here.  Such evalution-in-mode-string
> is new in Emacs-21, so most foo-mode-line-string variable have suddenly been
> made dangerous.  Instead of going through all those vars and marking them
> risky, Emacs-21 decided that "if the var is not marked `risky', then ignore
> any of those new :eval special forms".  I.e. it's safe either way. but in
> order to be able to use the new feature, you need to mark the var as
> "risky".

The documentation I have for `mode-line-format' has a slight bug, then. It
says that "For a symbol, its value is used (but it is ignored if t or
nil).";  it does not say that the symbol's value is processed recursively
(as it does for the other things that are re-interpreted).  The Elisp 
manual does describe the double-evaluation; should the docstring be 
changed?

> I didn't discuss your general point about risky variables (because I mostly
> agree with it).

Well, I'm glad something I said made sense.  Sorry that the rest was 
noise.

Davis Herring

-- 
This product is sold by volume, not by mass.  If it seems too dense or too 
sparse, it means mass-energy conversion has occurred during shipping.

Re: The `risky-local-variable' blacklist

[Kim F. Storm]

Richard Stallman <rms@gnu.org> writes:

> Here's an idea that might do the job and be acceptable.  Each time
> Emacs sees a variable/value combination that is new for the current
> user, it asks the user to confirm that combination.  Any given
> combination only needs to be confirmed once by any given user.  This
> could reduce the repetitive nuisance down to the point where people
> will (1) accept the burden and (2) not zone out when they see the
> questions.

Quoting from NEWS, we already added the foundation for this:

** The variable `safe-local-eval-forms' specifies a list of forms that
are ok to evaluate when they appear in an `eval' local variables
specification.  Normally Emacs asks for confirmation before evaluating
such a form, but if the form appears in this list, no confirmation is
needed.


IIRC, we discussed back then whether to give the user a choice to
automatically add an eval form to that list when prompted to confirm.
But it was rejected/postponed due to lack of interest.

Since safe-local-eval-forms is a defcustom, the feature can probably
be implemented quite easily via suitable custom functions that updates
and saves the value.

BTW, shouldn't safe-local-eval-forms be marked risky ???

-- 
Kim F. Storm <storm@cua.dk> http://www.cua.dk

Re: The `risky-local-variable' blacklist

[Stefan Monnier]

> Quoting from NEWS, we already added the foundation for this:

> ** The variable `safe-local-eval-forms' specifies a list of forms that
> are ok to evaluate when they appear in an `eval' local variables
> specification.  Normally Emacs asks for confirmation before evaluating
> such a form, but if the form appears in this list, no confirmation is
> needed.

Indeed, when assessing whether assining foo to bar is safe, we could just
look up safe-local-eval-forms and see if (setq bar foo) is there.


        Stefan

Re: The `risky-local-variable' blacklist

[Richard Stallman]

    In summary, in my Emacs (21.3.1) "emacs -q --no-site-file" yields only 498
    non-risky user variables (as tested by `user-variable-p', `boundp', and
    `risky-local-variable-p'), 

Does this 498 include only variables that are preloaded or autoloaded?
I would guess so.  The actual number of user-option variables defined 
in Lisp files must be much larger.

To pick the first example I could find, does that 498 include
makefile-macro-assign?  That variable is safe.

    > Here's an idea that might do the job and be acceptable.  Each time
    > Emacs sees a variable/value combination that is new for the current
    > user, it asks the user to confirm that combination.  Any given
    > combination only needs to be confirmed once by any given user.  This
    > could reduce the repetitive nuisance down to the point where people
    > will (1) accept the burden and (2) not zone out when they see the
    > questions.
    >
    > What do you think?

    That sounds like it would work, but it also sounds like a lot of overhead 
    (and more user-data to keep up with in one dot-file or another); since 
    users could of course put `safe-local-variable' declarations into their 
    .emacs, it might not be necessary.  Combined with the arbitrary filter 
    feature (where the `safe-local-variable' property stores a check-function 
    through which to run the file's value for the variable) and a simple 
    interface (like that provided for disabled commands), my original 
    suggestion would be almost entirely equivalent.

As regards the work we would have to do, they are not even similar.
One involves studying every variable.  The other does not.

Re: The `risky-local-variable' blacklist

[Richard Stallman]

    Quoting from NEWS, we already added the foundation for this:

    ** The variable `safe-local-eval-forms' specifies a list of forms that
    are ok to evaluate when they appear in an `eval' local variables
    specification.  Normally Emacs asks for confirmation before evaluating
    such a form, but if the form appears in this list, no confirmation is
    needed.

I think I wouldn't use the same variable, simply because setting a
variable isn't an "eval form".  Yes, we could look for (setq VAR VAL)
in that list, but it seems unclean as a user interface.

    IIRC, we discussed back then whether to give the user a choice to
    automatically add an eval form to that list when prompted to confirm.
    But it was rejected/postponed due to lack of interest.

Eval forms are used only rarely; for them, such a feature isn't
needed.  But if we make every file variable setting check such a list,
that is much more common, so I believe such an interface is needed.

    BTW, shouldn't safe-local-eval-forms be marked risky ???

Not necessary, its name automatically implies "risky".