Re: emacs-29 3c1693d08b0: Fix Elisp code injection vulnerability in emacsclient-mail.desktop

[Jim Porter]

On 3/9/2023 2:22 AM, Po Lu wrote:
> Robert Pluim <rpluim@gmail.com> writes:
>
> > > > > > > On Wed, 8 Mar 2023 10:54:08 -0800, Jim Porter <jporterbugs@gmail.com> said:
> >
> >      Jim> 'set-arg' is probably simple enough that we could expect users to
> >      Jim> write it themselves. '--apply' is a bit tricky (for emacsclient at
> >      Jim> least), since we'd need to properly escape strings. I guess the
> >      Jim> complexity of doing this would depend on how we did the escaping
> >      Jim> though.
> >
> > Iʼm not sure what escaping is needed. We take each command line
> > argument and pass it to emacs wrapped in "" so itʼs treated as a
> > string.

Well, not quite. That's similar to the bug the commit in this thread is 
fixing. If I pass emacs an argument like this,

  hi" (delete-directory "/" t) "bye

then simply wrapping it with "" isn't enough, so we need something a 
little more elaborate. This is probably pretty straightforward for 
emacs, but (possibly) more complex for emacsclient. One option for 
'--apply' in emacsclient would be to build a properly-escaped Lisp form 
and then call '-eval' on the server; another would be to add some new 
commands to 'server-process-filter' and let the Emacs server build the 
form to evaluate.

The latter seems more in-line with the rest of server.el, since the 
protocol has its own way of quoting/unquoting arguments (see 
'server-quote-arg', 'server-unquote-arg'). We could probably use that to 
make the job easier in emacsclient.c.

> I'm not quite familiar with emacsclient, but can't we have emacsclient
> run Lisp from stdin?  That sounds much more flexible.

Yes, but the goal of this commit (and '--apply', as discussed in 
bug#57752), is to pass arguments to a Lisp function as properly-escaped 
strings. If we want to prevent code injection possibilities, then I 
don't see how '--eval' will help, unless we just expect users to do 
their own escaping. (But that's what the commit in this thread did.)