GNU bug report logs - #3230
dired-actual-switches is risky

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 3230 in the body.
You can then email your comments to 3230 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> emacsbugs.donarmstrong.com (full text, mbox):

From: Leo <sdl.web <at> gmail.com>
To: emacs-pretest-bug <at> gnu.org
Subject: 23.0.93; Make dired-actual-switches safe local variable?
Date: Wed, 06 May 2009 15:20:57 +0100

Hi there,

The dired-x manual gives an example in using local variables for dired
buffers. However, the variable dired-actual-switches has not been marked
as safe local variable. I think this is an oversight.

,----[ (info "(dired-x)Local Variables") ]
| For example, if the user puts
| 
|      Local Variables:
|      dired-actual-switches: "-lat"
|      dired-omit-mode: t
|      End:
`----

Best wishes,

Leo

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

From: Glenn Morris <rgm <at> gnu.org>
To: Leo <sdl.web <at> gmail.com>
Cc: 3230 <at> debbugs.gnu.org
Subject: Re: 23.0.93; Make dired-actual-switches safe local variable?
Date: Wed, 23 Feb 2011 21:08:26 -0500

retitle 3230 dired-actual-switches is risky
stop

Leo wrote:

> The dired-x manual gives an example in using local variables for dired
> buffers. However, the variable dired-actual-switches has not been marked
> as safe local variable. I think this is an oversight.

As it stands, it emphatically should NOT be marked safe. Example:

cat <<EOF >| .dired
Local Variables:
dired-actual-switches: "-l ; touch /tmp/OHDEAR"
End:
EOF

rm -f /tmp/OHDEAR

emacs -Q -l dired-x
M-x dired /path/to/dir/*.el     ; wildcard is important
answer "y" to question about possibly unsafe local variable

ls /tmp/OHDEAR

Oh dear, arbitrary shell command executed with permissions of the user
running Emacs.

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

From: Leo <sdl.web <at> gmail.com>
To: Glenn Morris <rgm <at> gnu.org>
Cc: 3230 <at> debbugs.gnu.org
Subject: Re: bug#3230: 23.0.93; Make dired-actual-switches safe local variable?
Date: Thu, 24 Feb 2011 12:46:40 +0800

On 2011-02-24 10:08 +0800, Glenn Morris wrote:
> As it stands, it emphatically should NOT be marked safe. Example:
[...]
>
> Oh dear, arbitrary shell command executed with permissions of the user
> running Emacs.

Looks like a bug in the way dired-actual-switches is used. Should we
devise a function to check every switch in dired-actual-switches is
actual a switch?

(defun dired-actual-switches-p (switches)
  (assert (stringp switches))
  (mapc
   (lambda (switch)
     (assert (eq (aref switch 0) ?-)))
   (split-string switches nil t)))

(put 'dired-actual-switches 'safe-local-variable 'dired-actual-switches-p)

Leo

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

From: Leo <sdl.web <at> gmail.com>
To: Glenn Morris <rgm <at> gnu.org>
Cc: 3230 <at> debbugs.gnu.org
Subject: Re: bug#3230: 23.0.93; Make dired-actual-switches safe local variable?
Date: Thu, 24 Feb 2011 14:51:16 +0800

On 2011-02-24 12:46 +0800, Leo wrote:
> (defun dired-actual-switches-p (switches)
>   (assert (stringp switches))
>   (mapc
>    (lambda (switch)
>      (assert (eq (aref switch 0) ?-)))
>    (split-string switches nil t)))

Sorry, it should not err. I originally named the function
dired-check-switches, then changed it to be a predicate in the last
minute.

(defun dired-actual-switches-p (switches)
  (and (stringp switches)
       (catch 'exit
         (mapc (lambda (switch)
                 (unless (eq (aref switch 0) ?-)
                   (throw 'exit nil)))
               (split-string switches nil t))
         t)))

Leo

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

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Leo <sdl.web <at> gmail.com>
Cc: 3230 <at> debbugs.gnu.org, Glenn Morris <rgm <at> gnu.org>
Subject: Re: bug#3230: 23.0.93; Make dired-actual-switches safe local variable?
Date: Thu, 24 Feb 2011 09:57:04 -0500

> (defun dired-actual-switches-p (switches)
>   (and (stringp switches)
>        (catch 'exit
>          (mapc (lambda (switch)
>                  (unless (eq (aref switch 0) ?-)
>                    (throw 'exit nil)))
>                (split-string switches nil t))
>          t)))

Hmm, what about "-l;reboot" ?
BTW, writing a predicate is the right thing to so, and the predicate
should then go to safe-local-variable.  I'd recommend something simple
like

  (defun dired-safe-switches-p (switches)
    (string-match "\\`[- [[:alnum:]]]+\\'" switches))

Hopefully that one is safe (tho maybe we should check string-length to
avoid attacks playing on overflow).  And if it proves too restrictive,
we can make it a bit more permissive once we encounter a particular
example that warrants it.
    

        Stefan

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

From: Leo <sdl.web <at> gmail.com>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 3230 <at> debbugs.gnu.org, Glenn Morris <rgm <at> gnu.org>
Subject: Re: bug#3230: 23.0.93; Make dired-actual-switches safe local variable?
Date: Fri, 25 Feb 2011 22:38:21 +0800

On 2011-02-24 22:57 +0800, Stefan Monnier wrote:
> Hmm, what about "-l;reboot" ?

Thanks.

> BTW, writing a predicate is the right thing to so, and the predicate
> should then go to safe-local-variable.  I'd recommend something simple
> like
>
>   (defun dired-safe-switches-p (switches)
>     (string-match "\\`[- [[:alnum:]]]+\\'" switches))

A typo: should be [:alnum:].

> Hopefully that one is safe (tho maybe we should check string-length to
> avoid attacks playing on overflow).  And if it proves too restrictive,
> we can make it a bit more permissive once we encounter a particular
> example that warrants it.
>
>         Stefan

I think I like this. Glenn, would you agree to this?

If this is accepted, I have one use case that can be easily done:

I have one directory where I file regularly. I like that directory to be
sorted by time instead of by name in dired. This change will allow me to
set dired-actual-switches to achieve that.

Leo

Message #29 received at 3230-done <at> debbugs.gnu.org (full text, mbox):

From: Glenn Morris <rgm <at> gnu.org>
To: 3230-done <at> debbugs.gnu.org
Subject: Re: bug#3230: 23.0.93; Make dired-actual-switches safe local variable?
Date: Mon, 28 Feb 2011 22:25:59 -0500

Version: 24.1

Stefan Monnier wrote:

> should then go to safe-local-variable.  I'd recommend something simple
> like
>
>   (defun dired-safe-switches-p (switches)
>     (string-match "\\`[- [[:alnum:]]]+\\'" switches))

Added.

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

From: Juanma Barranquero <lekktu <at> gmail.com>
To: 3230 <at> debbugs.gnu.org, rgm <at> gnu.org
Cc: 3230-done <at> debbugs.gnu.org
Subject: Re: bug#3230: 23.0.93; Make dired-actual-switches safe local variable?
Date: Tue, 1 Mar 2011 05:31:34 +0100

On Tue, Mar 1, 2011 at 04:25, Glenn Morris <rgm <at> gnu.org> wrote:

>>   (defun dired-safe-switches-p (switches)
>>     (string-match "\\`[- [[:alnum:]]]+\\'" switches))

This is a poster case for using `string-match-p'.

    Juanma

Previous Next

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