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

[Robert Pluim <rpluim@gmail.com>]

>>>>> On Wed, 08 Mar 2023 08:15:48 +0100, Ulrich Mueller <ulm@gentoo.org> said:

>>>>> On Wed, 08 Mar 2023, Po Lu wrote:
    >> Ulrich Mueller <ulm@gentoo.org> writes:
    >>> Then the desktop file won't work, obviously. The problem is that
    >>> ${PARAMETER//PATTERN/STRING} substitution is not available in POSIX
    >>> parameter expansion. So with POSIX sh, an external program (e.g. sed)
    >>> would have to be called.
    >>> 
    >>> The long term solution (suggested by Stefan Monnier) might be to add
    >>> a --funcall option to emacsclient. Then there would be no need for a
    >>> shell wrapper, in the first place.
    >>> 
    >>> Should the Makefile skip installation of emacsclient-mail.desktop
    >>> when bash isn't available on the system?

Then youʼd require every distro builder to have bash available in
their build setup

    >> Could we install this change not on emacs-29, but on master?

    >> I don't think the problem it solves is severe, nor a regression from
    >> Emacs 28.  It is rather a minor nusiance with certain URLs.

    Ulrich> Seriously? It is a vulnerability that allows remote injection of
    Ulrich> arbitrary Elisp code through a crafted "mailto" URI.

Itʼs certainly not a regression, but it is fairly serious. We could
mitigate it somewhat by adding '--funcall', I guess.

The last time --funcall was discussed, there was no consensus on how
arguments should be handled, so Iʼve just gone ahead and implemented
one variant. We could add any restrictions we like on the server side,
it currently just disallows direct `eval'

Not for emacs-29, I think.

Robert
-- 

diff --git c/lib-src/emacsclient.c i/lib-src/emacsclient.c
index 698bf9b50ae..d408af6658f 100644
--- c/lib-src/emacsclient.c
+++ i/lib-src/emacsclient.c
@@ -113,6 +113,9 @@ #define DEFAULT_TIMEOUT (30)
 /* True means don't print values returned from emacs. --suppress-output.  */
 static bool suppress_output;
 
+/* True means arg is a function to be called.  --funcall.  */
+static bool funcall;
+
 /* True means args are expressions to be evaluated.  --eval.  */
 static bool eval;
 
@@ -168,6 +171,7 @@ #define DEFAULT_TIMEOUT (30)
   { "no-wait",	no_argument,	   NULL, 'n' },
   { "quiet",	no_argument,	   NULL, 'q' },
   { "suppress-output", no_argument, NULL, 'u' },
+  { "funcall",	no_argument,	   NULL, 'l' },
   { "eval",	no_argument,	   NULL, 'e' },
   { "help",	no_argument,	   NULL, 'H' },
   { "version",	no_argument,	   NULL, 'V' },
@@ -191,7 +195,7 @@ #define DEFAULT_TIMEOUT (30)
 /* Short options, in the same order as the corresponding long options.
    There is no '-p' short option.  */
 static char const shortopts[] =
-  "nqueHVtca:F:w:"
+  "nquleHVtca:F:w:"
 #ifdef SOCKETS_IN_FILE_SYSTEM
   "s:"
 #endif
@@ -548,6 +552,10 @@ decode_options (int argc, char **argv)
 	    }
 	  break;
 
+	case 'l':
+	  funcall = true;
+	  break;
+
 	case 'e':
 	  eval = true;
 	  break;
@@ -689,6 +697,7 @@ print_help_and_exit (void)
 ", "\
 -F ALIST, --frame-parameters=ALIST\n\
 			Set the parameters of a new frame\n\
+-l, --funcall    	Evaluate the FILE argument as function to call\n\
 -e, --eval    		Evaluate the FILE arguments as ELisp expressions\n\
 -n, --no-wait		Don't wait for the server to return\n\
 -w, --timeout=SECONDS	Seconds to wait before timing out\n\
@@ -2067,11 +2076,28 @@ main (int argc, char **argv)
   if (create_frame && !tty)
     send_to_emacs (emacs_socket, "-window-system ");
 
+  bool funcall_sent = false;
+
   if (optind < argc)
     {
       for (int i = optind; i < argc; i++)
 	{
 
+	  if (funcall)
+            {
+	      /* funcall applies the function to all subsequent
+		 arguments, unlike eval, which evaluates each
+		 expression individually.  */
+	      if (!funcall_sent)
+		{
+		  send_to_emacs (emacs_socket, "-funcall ");
+		  funcall_sent = true;
+		}
+              quote_argument (emacs_socket, argv[i]);
+              send_to_emacs (emacs_socket, " ");
+              continue;
+            }
+
 	  if (eval)
             {
               /* Don't prepend cwd or anything like that.  */
@@ -2139,7 +2165,7 @@ main (int argc, char **argv)
   send_to_emacs (emacs_socket, "\n");
 
   /* Wait for an answer. */
-  if (!eval && !tty && !nowait && !quiet && 0 <= process_grouping ())
+  if (!funcall && !eval && !tty && !nowait && !quiet && 0 <= process_grouping ())
     {
       printf ("Waiting for Emacs...");
       skiplf = false;
diff --git c/lisp/server.el i/lisp/server.el
index eaf24a770e4..e5ea2b14b3b 100644
--- c/lisp/server.el
+++ i/lisp/server.el
@@ -1295,6 +1295,29 @@ server-process-filter
                                proc))
                  (setq filepos nil))
 
+                ;; -funcall FUNCTION:  Call the function, and pass it any subsequent command line args
+                ("-funcall"
+                 (if use-current-frame
+                     (setq use-current-frame 'always))
+                 (let ((fun (pop args-left))
+                       args real-fun)
+                   (if coding-system
+                       (setq fun (decode-coding-string fun coding-system)))
+                   (when fun
+                     (setq real-fun (intern-soft fun))
+                     (when (and (fboundp real-fun)
+                                (not (eq real-fun 'eval)))
+                       (setq args (mapcar
+                                   (lambda (a)
+                                     (if coding-system
+                                         (setq a (decode-coding-string a coding-system)))
+                                     a)
+                                   args-left))
+                       (setq args-left nil)
+                       (push (lambda () (apply real-fun args))
+                             commands))))
+                 (setq filepos nil))
+
                 ;; -eval EXPR:  Evaluate a Lisp expression.
                 ("-eval"
                  (if use-current-frame