bug#3230: 23.0.93; Make dired-actual-switches safe local variable?

bug#3230: 23.0.93; Make dired-actual-switches safe local variable?

[Leo]

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

bug#3230: 23.0.93; Make dired-actual-switches safe local variable?

[Glenn Morris]

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.

bug#3230: 23.0.93; Make dired-actual-switches safe local variable?

[Leo]

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

bug#3230: 23.0.93; Make dired-actual-switches safe local variable?

[Leo]

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

bug#3230: 23.0.93; Make dired-actual-switches safe local variable?

[Stefan Monnier]

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

bug#3230: 23.0.93; Make dired-actual-switches safe local variable?

[Leo]

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

bug#3230: 23.0.93; Make dired-actual-switches safe local variable?

[Glenn Morris]

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.

bug#3230: 23.0.93; Make dired-actual-switches safe local variable?

[Juanma Barranquero]

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

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

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

    Juanma