GNU bug report logs - #61709
[PATCH] Security hardening: safely invoke `shell-command*' function.

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 61709 in the body.
You can then email your comments to 61709 AT debbugs.gnu.org in the normal way.

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

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

From: Xi Lu <lx <at> shellcodes.org>
To: bug-gnu-emacs <at> gnu.org
Cc: Xi Lu <lx <at> shellcodes.org>
Subject: [PATCH] Security hardening: safely invoke `shell-command*' function.
Date: Wed, 22 Feb 2023 22:35:54 +0800

* lisp/filesets.el:
(filesets-select-command, filesets-which-command,
filesets-spawn-external-viewer, filesets-run-cmd): Add `shell-quote-argument'
---
 lisp/filesets.el | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/lisp/filesets.el b/lisp/filesets.el
index 1b7e6ffa81f..96ac11bb40b 100644
--- a/lisp/filesets.el
+++ b/lisp/filesets.el
@@ -165,14 +165,15 @@ filesets-select-command
   "Select one command from CMD-LIST -- a string with space separated names."
   (let ((this (shell-command-to-string
 	       (format "which --skip-alias %s 2> %s | head -n 1"
-		       cmd-list null-device))))
+		       (shell-quote-argument cmd-list)
+                       (shell-quote-argument null-device)))))
     (if (equal this "")
 	nil
       (file-name-nondirectory (substring this 0 (- (length this) 1))))))
 
 (defun filesets-which-command (cmd)
   "Call \"which CMD\"."
-  (shell-command-to-string (format "which %s" cmd)))
+  (shell-command-to-string (format "which %s" (shell-quote-argument cmd))))
 
 (defun filesets-which-command-p (cmd)
   "Call \"which CMD\" and return non-nil if the command was found."
@@ -1264,9 +1265,11 @@ filesets-spawn-external-viewer
 		  (funcall vwr file)
 		  nil)
 		 (co-flag
-		  (shell-command-to-string (format "%s %s" vwr args)))
+		  (shell-command-to-string (shell-quote-argument
+                                            (format "%s %s" vwr args))))
 		 (t
-		  (shell-command (format "%s %s&" vwr args))
+		  (shell-command (shell-quote-argument
+                                  (format "%s %s&" vwr args)))
 		  nil))))
 	  (if co-flag
 	      (progn
@@ -1578,7 +1581,7 @@ filesets-run-cmd
 				   " "))
 				 (cmd (concat fn " " args)))
 			    (filesets-cmd-show-result
-			     cmd (shell-command-to-string cmd))))
+			     cmd (shell-command-to-string (shell-quote-argument cmd)))))
 			 ((symbolp fn)
 			  (apply fn
 			         (mapcan (lambda (this)
-- 
2.39.2

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Xi Lu <lx <at> shellcodes.org>
Cc: 61709 <at> debbugs.gnu.org
Subject: Re: bug#61709: [PATCH] Security hardening: safely invoke
 `shell-command*' function.
Date: Wed, 22 Feb 2023 17:29:23 +0200

> Cc: Xi Lu <lx <at> shellcodes.org>
> From: Xi Lu <lx <at> shellcodes.org>
> Date: Wed, 22 Feb 2023 22:35:54 +0800
> 
>  (defun filesets-which-command-p (cmd)
>    "Call \"which CMD\" and return non-nil if the command was found."
> @@ -1264,9 +1265,11 @@ filesets-spawn-external-viewer
>  		  (funcall vwr file)
>  		  nil)
>  		 (co-flag
> -		  (shell-command-to-string (format "%s %s" vwr args)))
> +		  (shell-command-to-string (shell-quote-argument
> +                                            (format "%s %s" vwr args))))
>  		 (t
> -		  (shell-command (format "%s %s&" vwr args))
> +		  (shell-command (shell-quote-argument
> +                                  (format "%s %s&" vwr args)))
>  		  nil))))

These two cannot be right: you are quoting several separate
command-line arguments.

>  	  (if co-flag
>  	      (progn
> @@ -1578,7 +1581,7 @@ filesets-run-cmd
>  				   " "))
>  				 (cmd (concat fn " " args)))
>  			    (filesets-cmd-show-result
> -			     cmd (shell-command-to-string cmd))))
> +			     cmd (shell-command-to-string (shell-quote-argument cmd)))))
>  			 ((symbolp fn)
>  			  (apply fn
>  			         (mapcan (lambda (this)

I think this is also wrong: cmd is not a single word.

In general, you cannot quote arbitrary parts of a shell command, you
can only quote each command-line argument separately.

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

From: lux <lx <at> shellcodes.org>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 61709 <at> debbugs.gnu.org
Subject: Re: bug#61709: [PATCH] Security hardening: safely invoke
 `shell-command*' function.
Date: Thu, 23 Feb 2023 21:17:12 +0800

[Message part 1 (text/plain, inline)]

On Wed, 2023-02-22 at 17:29 +0200, Eli Zaretskii wrote:
> > Cc: Xi Lu <lx <at> shellcodes.org>
> > From: Xi Lu <lx <at> shellcodes.org>
> > Date: Wed, 22 Feb 2023 22:35:54 +0800
> > 
> >  (defun filesets-which-command-p (cmd)
> >    "Call \"which CMD\" and return non-nil if the command was
> > found."
> > @@ -1264,9 +1265,11 @@ filesets-spawn-external-viewer
> >                   (funcall vwr file)
> >                   nil)
> >                  (co-flag
> > -                 (shell-command-to-string (format "%s %s" vwr
> > args)))
> > +                 (shell-command-to-string (shell-quote-argument
> > +                                            (format "%s %s" vwr
> > args))))
> >                  (t
> > -                 (shell-command (format "%s %s&" vwr args))
> > +                 (shell-command (shell-quote-argument
> > +                                  (format "%s %s&" vwr args)))
> >                   nil))))
> 
> These two cannot be right: you are quoting several separate
> command-line arguments.
> 
> >           (if co-flag
> >               (progn
> > @@ -1578,7 +1581,7 @@ filesets-run-cmd
> >                                    " "))
> >                                  (cmd (concat fn " " args)))
> >                             (filesets-cmd-show-result
> > -                            cmd (shell-command-to-string cmd))))
> > +                            cmd (shell-command-to-string (shell-
> > quote-argument cmd)))))
> >                          ((symbolp fn)
> >                           (apply fn
> >                                  (mapcan (lambda (this)
> 
> I think this is also wrong: cmd is not a single word.
> 
> In general, you cannot quote arbitrary parts of a shell command, you
> can only quote each command-line argument separately.
> 
> 
> 

You're right, thank you. I rewrited this patch.

Let me briefly explain this patch:

1. I think `filesets-select-command' not need fixed, because it not
used, and I cleaned up relevant old comments in `filesets-external-
viewers'.

2. Using `shell-quote-argument' to replace `filesets-quote' and
`(format "%S" ...)'. Because in the shell, double quotation marks can
still execute unexpected code, such as $(), `command` and $var.

[0001-Security-hardening-safely-invoke-shell-command-funct.patch (text/x-patch, attachment)]

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: lux <lx <at> shellcodes.org>
Cc: 61709 <at> debbugs.gnu.org
Subject: Re: bug#61709: [PATCH] Security hardening: safely invoke
 `shell-command*' function.
Date: Thu, 23 Feb 2023 17:58:58 +0200

> From: lux <lx <at> shellcodes.org>
> Cc: 61709 <at> debbugs.gnu.org
> Date: Thu, 23 Feb 2023 21:17:12 +0800
> 
> You're right, thank you. I rewrited this patch.
> 
> Let me briefly explain this patch:
> 
> 1. I think `filesets-select-command' not need fixed, because it not
> used, and I cleaned up relevant old comments in `filesets-external-
> viewers'.
> 
> 2. Using `shell-quote-argument' to replace `filesets-quote' and
> `(format "%S" ...)'. Because in the shell, double quotation marks can
> still execute unexpected code, such as $(), `command` and $var.

Thanks.  I hesitate installing this because I don't myself use
filesets, and we don't have tests for it.  So I'm not sure how to
ensure that we don't break this package.

Does someone else here use filesets?

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

From: lux <lx <at> shellcodes.org>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 61709 <at> debbugs.gnu.org
Subject: Re: bug#61709: [PATCH] Security hardening: safely invoke
 `shell-command*' function.
Date: Mon, 05 Feb 2024 14:13:28 +0800

[Message part 1 (text/plain, inline)]

On Wed, 2023-02-22 at 17:29 +0200, Eli Zaretskii wrote:
> > Cc: Xi Lu <lx <at> shellcodes.org>
> > From: Xi Lu <lx <at> shellcodes.org>
> > Date: Wed, 22 Feb 2023 22:35:54 +0800
> > 
> >  (defun filesets-which-command-p (cmd)
> >    "Call \"which CMD\" and return non-nil if the command was found."
> > @@ -1264,9 +1265,11 @@ filesets-spawn-external-viewer
> >  		  (funcall vwr file)
> >  		  nil)
> >  		 (co-flag
> > -		  (shell-command-to-string (format "%s %s" vwr args)))
> > +		  (shell-command-to-string (shell-quote-argument
> > +                                            (format "%s %s" vwr args))))
> >  		 (t
> > -		  (shell-command (format "%s %s&" vwr args))
> > +		  (shell-command (shell-quote-argument
> > +                                  (format "%s %s&" vwr args)))
> >  		  nil))))
> 
> These two cannot be right: you are quoting several separate
> command-line arguments.
> 
> >  	  (if co-flag
> >  	      (progn
> > @@ -1578,7 +1581,7 @@ filesets-run-cmd
> >  				   " "))
> >  				 (cmd (concat fn " " args)))
> >  			    (filesets-cmd-show-result
> > -			     cmd (shell-command-to-string cmd))))
> > +			     cmd (shell-command-to-string (shell-quote-
> > argument cmd)))))
> >  			 ((symbolp fn)
> >  			  (apply fn
> >  			         (mapcan (lambda (this)
> 
> I think this is also wrong: cmd is not a single word.
> 
> In general, you cannot quote arbitrary parts of a shell command, you
> can only quote each command-line argument separately.
> 
> 
> 

This patch went unaddressed for a long time, so just to be on the safe side, I
only remove the `filesets-select-command' function.

[0001-Removed-the-filesets-select-command-which-was-unused.patch (text/x-patch, attachment)]

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

From: Stefan Kangas <stefankangas <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>, lux <lx <at> shellcodes.org>
Cc: 61709-done <at> debbugs.gnu.org
Subject: Re: bug#61709: [PATCH] Security hardening: safely invoke
 `shell-command*' function.
Date: Mon, 5 Feb 2024 02:29:55 -0500

Version: 30.1

Eli Zaretskii <eliz <at> gnu.org> writes:

>> From: lux <lx <at> shellcodes.org>
>> Cc: 61709 <at> debbugs.gnu.org
>> Date: Thu, 23 Feb 2023 21:17:12 +0800
>>
>> You're right, thank you. I rewrited this patch.
>>
>> Let me briefly explain this patch:
>>
>> 1. I think `filesets-select-command' not need fixed, because it not
>> used, and I cleaned up relevant old comments in `filesets-external-
>> viewers'.
>>
>> 2. Using `shell-quote-argument' to replace `filesets-quote' and
>> `(format "%S" ...)'. Because in the shell, double quotation marks can
>> still execute unexpected code, such as $(), `command` and $var.

Thank you for paying attention to these issues.

Pushed to master as commit 7756e9c7361, and closing the bug.

> Thanks.  I hesitate installing this because I don't myself use
> filesets, and we don't have tests for it.  So I'm not sure how to
> ensure that we don't break this package.
>
> Does someone else here use filesets?

Let's hope that if it breaks something, someone will report a bug.  :-/

Previous Next

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