GNU bug report logs - #59544
[PATCH] Fixed lib-src/etags.c command execute vulnerability

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 59544 in the body.
You can then email your comments to 59544 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: "lux" <lx <at> shellcodes.org>
To: "bug-gnu-emacs" <bug-gnu-emacs <at> gnu.org>
Cc: lux <lx <at> shellcodes.org>
Subject: [PATCH] Fixed lib-src/etags.c command execute vulnerability
Date: Thu, 24 Nov 2022 23:27:13 +0800

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

Hi, In ctags (Emacs <= 28.2.50) has a command execute vulnerability.

When using the -u parameter, ctags will execute external shell commands by calling the system() function, if there are special file names, unexpected shell commands may be executed. The example is as follows:


$ ls
etags.c

$ /usr/local/bin/ctags *.c
$ touch "'| uname -a #.c"
$ /usr/local/bin/ctags -u *.c
Linux mypc 6.0.8-300.fc37.x86_64 #1 SMP PREEMPT_DYNAMIC Fri Nov 11 15:09:04 UTC 2022 x86_64 x86_64 x86_64 GNU/Linux


^C/usr/local/bin/ctags: failed to execute shell command


The vulnerability occurs in the following code:


char *z = stpcpy (cmd, "mv ");&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp;
z = stpcpy (z, tagfile);&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp;
z = stpcpy (z, " OTAGS;grep -Fv '\t");&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp;
z = stpcpy (z, argbuffer[i].what);&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp;
z = stpcpy (z, "\t' OTAGS &gt;");&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp;
z = stpcpy (z, tagfile);&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp;
strcpy (z, ";rm OTAGS");&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp;
if (system (cmd) != EXIT_SUCCESS)&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; 
&nbsp; fatal ("failed to execute shell command");&nbsp; &nbsp; &nbsp;



Because the file name is not checked, the file name is used as a concatenated string:


mv tags OTAGS;grep -Fv '	'| uname -a #.c	' OTAGS &gt;tags;rm OTAGS


Email attachments are patches.

[Message part 2 (text/html, inline)]

[0001-lib-src-etags.c-Fix-ctags-command-execute-vulnerabil.patch (application/octet-stream, attachment)]

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: "lux" <lx <at> shellcodes.org>
Cc: 59544 <at> debbugs.gnu.org, lx <at> shellcodes.org
Subject: Re: bug#59544: [PATCH] Fixed lib-src/etags.c command execute
 vulnerability
Date: Thu, 24 Nov 2022 20:01:46 +0200

> Cc: lux <lx <at> shellcodes.org>
> From: "lux" <lx <at> shellcodes.org>
> Date: Thu, 24 Nov 2022 23:27:13 +0800
> 
> When using the -u parameter, ctags will execute external shell commands by calling the system() function,
> if there are special file names, unexpected shell commands may be executed. The example is as follows:
> 
> $ ls
> etags.c
> $ /usr/local/bin/ctags *.c
> $ touch "'| uname -a #.c"
> $ /usr/local/bin/ctags -u *.c
> Linux mypc 6.0.8-300.fc37.x86_64 #1 SMP PREEMPT_DYNAMIC Fri Nov 11 15:09:04 UTC 2022 x86_64
> x86_64 x86_64 GNU/Linux
> 
> ^C/usr/local/bin/ctags: failed to execute shell command
> 
> The vulnerability occurs in the following code:
> 
> char *z = stpcpy (cmd, "mv ");                   
> z = stpcpy (z, tagfile);                         
> z = stpcpy (z, " OTAGS;grep -Fv '\t");           
> z = stpcpy (z, argbuffer[i].what);               
> z = stpcpy (z, "\t' OTAGS >");                   
> z = stpcpy (z, tagfile);                         
> strcpy (z, ";rm OTAGS");                         
> if (system (cmd) != EXIT_SUCCESS)                
>   fatal ("failed to execute shell command");     
> 
> Because the file name is not checked, the file name is used as a concatenated string:
> 
> mv tags OTAGS;grep -Fv ' '| uname -a #.c ' OTAGS >tags;rm OTAGS
> 
> Email attachments are patches.

Thanks, but the solution you propose for this is too drastic: it in effect
rejects legitimate file names just because they have characters which look
"suspicious".  I think we need a more accurate test, which will not produce
false positives so easily.  Or maybe we need to ask the user for
confirmation instead of skipping the files with suspicious names.

Message #11 received at 59544 <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: 59544 <at> debbugs.gnu.org
Subject: Re: bug#59544: [PATCH] Fixed lib-src/etags.c command execute
 vulnerability
Date: Thu, 24 Nov 2022 10:12:31 -0800

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

> Thanks, but the solution you propose for this is too drastic: it in effect
> rejects legitimate file names just because they have characters which look
> "suspicious".  I think we need a more accurate test, which will not produce
> false positives so easily.  Or maybe we need to ask the user for
> confirmation instead of skipping the files with suspicious names.

I think we could escape the file name using single quotes, but AFAIU we
then need to escape single quote characters too, so that:

    '

becomes

    '\''

See here for why:
https://www.gnu.org/savannah-checkouts/gnu/bash/manual/bash.html#Single-Quotes

But would it not be better to rewrite etags.c to not use system(1) at
all?

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Stefan Kangas <stefankangas <at> gmail.com>
Cc: 59544 <at> debbugs.gnu.org, lx <at> shellcodes.org
Subject: Re: bug#59544: [PATCH] Fixed lib-src/etags.c command execute
 vulnerability
Date: Thu, 24 Nov 2022 20:38:04 +0200

> From: Stefan Kangas <stefankangas <at> gmail.com>
> Date: Thu, 24 Nov 2022 10:12:31 -0800
> Cc: 59544 <at> debbugs.gnu.org
> 
> But would it not be better to rewrite etags.c to not use system(1) at
> all?

That's a possibility, yes.  Although I doubt that people are still using
ctags that comes with Emacs (this code fragment runs only in ctags, not in
etags).

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

From: "lux" <lx <at> shellcodes.org>
To: "Eli Zaretskii" <eliz <at> gnu.org>,
 "Stefan Kangas" <stefankangas <at> gmail.com>
Cc: 59544 <59544 <at> debbugs.gnu.org>
Subject: Re: bug#59544: [PATCH] Fixed lib-src/etags.c command execute
 vulnerability
Date: Fri, 25 Nov 2022 11:45:37 +0800

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

> That's a possibility, yes.  Although I doubt that people are still using
> ctags that comes with Emacs (this code fragment runs only in ctags, not in
> etags).


Rewriting is a good solution, but there are three places in etags.c that use the system() function.

[Message part 2 (text/html, inline)]

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

From: "lux" <lx <at> shellcodes.org>
To: "Eli Zaretskii" <eliz <at> gnu.org>,
 "Stefan Kangas" <stefankangas <at> gmail.com>
Cc: 59544 <59544 <at> debbugs.gnu.org>
Subject: Re: bug#59544: [PATCH] Fixed lib-src/etags.c command execute
 vulnerability
Date: Fri, 25 Nov 2022 14:41:56 +0800

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

> That's a possibility, yes.  Although I doubt that people are still using
> ctags that comes with Emacs (this code fragment runs only in ctags, not in
> etags).


I rewrote this code, not use system(1).

[Message part 2 (text/html, inline)]

[0001-Fixed-lib-src-etags.c-command-execute-vulnerability.patch (application/octet-stream, attachment)]

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

From: Stefan Kangas <stefankangas <at> gmail.com>
To: lux <lx <at> shellcodes.org>
Cc: 59544 <59544 <at> debbugs.gnu.org>, Eli Zaretskii <eliz <at> gnu.org>
Subject: Re: bug#59544: [PATCH] Fixed lib-src/etags.c command execute
 vulnerability
Date: Thu, 24 Nov 2022 23:53:46 -0800

"lux" <lx <at> shellcodes.org> writes:

> I rewrote this code, not use system(1).

Thanks.

> From d6bc71f8640efe7caa2657a75c5aa4d8b4f0532c Mon Sep 17 00:00:00 2001
> From: lu4nx <lx <at> shellcodes.org>
> Date: Fri, 25 Nov 2022 14:38:29 +0800
> Subject: [PATCH] * Fixed lib-src/etags.c command execute vulnerability
>
> ---
>  lib-src/etags.c | 44 +++++++++++++++++++++++++++++++-------------
>  1 file changed, 31 insertions(+), 13 deletions(-)
>
> diff --git a/lib-src/etags.c b/lib-src/etags.c
> index f665f35fa6..1bb352f565 100644
> --- a/lib-src/etags.c
> +++ b/lib-src/etags.c
> @@ -1387,9 +1387,11 @@ main (int argc, char **argv)
>    /* From here on, we are in (CTAGS && !cxref_style) */
>    if (update)
>      {
> -      char *cmd =
> -	xmalloc (strlen (tagfile) + whatlen_max +
> -		 sizeof "mv..OTAGS;grep -Fv '\t\t' OTAGS >;rm OTAGS");
> +      FILE *otags_f, *tag_f;
> +      int buf_len;
> +      char *buf;
> +      char line[512];

Hmm, I'm not sure about the hard-coded 512 character line limit here.
ISTR that some people use much longer lines than that.

Could we do without it?

(As a matter of style, I would just declare the types at first use,
which limits their scope and makes the code easier to read.  But it's up
to you.)

> +
>        for (i = 0; i < current_arg; ++i)
>  	{
>  	  switch (argbuffer[i].arg_type)
> @@ -1400,17 +1402,33 @@ main (int argc, char **argv)
>  	    default:
>  	      continue;		/* the for loop */
>  	    }
> -	  char *z = stpcpy (cmd, "mv ");
> -	  z = stpcpy (z, tagfile);
> -	  z = stpcpy (z, " OTAGS;grep -Fv '\t");
> -	  z = stpcpy (z, argbuffer[i].what);
> -	  z = stpcpy (z, "\t' OTAGS >");
> -	  z = stpcpy (z, tagfile);
> -	  strcpy (z, ";rm OTAGS");
> -	  if (system (cmd) != EXIT_SUCCESS)
> -	    fatal ("failed to execute shell command");
> +
> +          otags_f = fopen ("OTAGS", "w");
> +          tag_f = fopen (tagfile, "r");
> +
> +          if (otags_f == NULL)
> +            pfatal ("OTAGS");
> +
> +          if (tag_f == NULL)
> +            pfatal (tagfile);
> +
> +          buf_len = strlen (argbuffer[i].what) + sizeof ("\t\t ") + 1;
> +          buf = xmalloc (buf_len);
> +          snprintf (buf, buf_len, "\t%s\t", argbuffer[i].what);
> +
> +          while (fgets (line, sizeof (line), tag_f) != NULL)

We should check ferror(tag_f), too and croak if there is a problem.

> +            {
> +              if (strstr (line, buf) == NULL)
> +                fputs (line, otags_f);

Missing error handling for fputs.

> +            }
> +
> +          fclose (otags_f);
> +          fclose (tag_f);

Should be:

if (fclose (otags_f) == EOF)
    pfatal (otags_f);
if (fclose (tag_f) == EOF)
    pfatal (tag_f);

> +
> +          rename ("OTAGS", tagfile);
> +          unlink ("OTAGS");

Please add error handling for both of these.

>  	}
> -      free (cmd);
> +

Nit: I don't think the empty line helps here?

>        append_to_tagfile = true;
>      }

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

From: "lux" <lx <at> shellcodes.org>
To: "Stefan Kangas" <stefankangas <at> gmail.com>
Cc: 59544 <59544 <at> debbugs.gnu.org>,
 Eli Zaretskii <eliz <at> gnu.org>
Subject: Re: bug#59544: [PATCH] Fixed lib-src/etags.c command execute
 vulnerability
Date: Fri, 25 Nov 2022 16:38:02 +0800

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

------------------ Original ------------------
From:                                                                                                                        "Stefan Kangas"                                                                                    <stefankangas <at> gmail.com&gt;;
Date:&nbsp;Fri, Nov 25, 2022 03:53 PM
To:&nbsp;"lux"<lx <at> shellcodes.org&gt;;
Cc:&nbsp;"Eli Zaretskii"<eliz <at> gnu.org&gt;;"59544"<59544 <at> debbugs.gnu.org&gt;;
Subject:&nbsp;Re: bug#59544: [PATCH] Fixed lib-src/etags.c command execute vulnerability



"lux" <lx <at> shellcodes.org&gt; writes:

&gt; I rewrote this code, not use system(1).

Thanks.

&gt; From d6bc71f8640efe7caa2657a75c5aa4d8b4f0532c Mon Sep 17 00:00:00 2001
&gt; From: lu4nx <lx <at> shellcodes.org&gt;
&gt; Date: Fri, 25 Nov 2022 14:38:29 +0800
&gt; Subject: [PATCH] * Fixed lib-src/etags.c command execute vulnerability
&gt;
&gt; ---
&gt;&nbsp; lib-src/etags.c | 44 +++++++++++++++++++++++++++++++-------------
&gt;&nbsp; 1 file changed, 31 insertions(+), 13 deletions(-)
&gt;
&gt; diff --git a/lib-src/etags.c b/lib-src/etags.c
&gt; index f665f35fa6..1bb352f565 100644
&gt; --- a/lib-src/etags.c
&gt; +++ b/lib-src/etags.c
&gt; @@ -1387,9 +1387,11 @@ main (int argc, char **argv)
&gt;&nbsp;&nbsp;&nbsp; /* From here on, we are in (CTAGS &amp;&amp; !cxref_style) */
&gt;&nbsp;&nbsp;&nbsp; if (update)
&gt;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; {
&gt; -&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; char *cmd =
&gt; -	xmalloc (strlen (tagfile) + whatlen_max +
&gt; -		 sizeof "mv..OTAGS;grep -Fv '\t\t' OTAGS &gt;;rm OTAGS");
&gt; +&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; FILE *otags_f, *tag_f;
&gt; +&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; int buf_len;
&gt; +&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; char *buf;
&gt; +&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; char line[512];

&gt; Hmm, I'm not sure about the hard-coded 512 character line limit here.
&gt; ISTR that some people use much longer lines than that.

Hi, do you have any suggestions? At present, I think hardcoding 512 is enough, thanks :-)

[Message part 2 (text/html, inline)]

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

From: Stefan Kangas <stefankangas <at> gmail.com>
To: lux <lx <at> shellcodes.org>
Cc: 59544 <59544 <at> debbugs.gnu.org>, Eli Zaretskii <eliz <at> gnu.org>
Subject: Re: bug#59544: [PATCH] Fixed lib-src/etags.c command execute
 vulnerability
Date: Fri, 25 Nov 2022 00:56:32 -0800

"lux" <lx <at> shellcodes.org> writes:

>> Hmm, I'm not sure about the hard-coded 512 character line limit here.
>> ISTR that some people use much longer lines than that.
>
> Hi, do you have any suggestions? At present, I think hardcoding 512 is
> enough, thanks :-)

No idea, really.  2^16?  2^20?

But why not allocate it dynamically, getting rid of any such arbitrary
limits?  AFAIU, grep (which we used before) doesn't have such limits, so
I fear that we otherwise risk introducing regressions.

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: "lux" <lx <at> shellcodes.org>
Cc: 59544 <at> debbugs.gnu.org, stefankangas <at> gmail.com
Subject: Re: bug#59544: [PATCH] Fixed lib-src/etags.c command execute
 vulnerability
Date: Fri, 25 Nov 2022 14:18:19 +0200

> From: "lux" <lx <at> shellcodes.org>
> Cc: "59544" <59544 <at> debbugs.gnu.org>
> Date: Fri, 25 Nov 2022 14:41:56 +0800
> 
> > That's a possibility, yes.  Although I doubt that people are still using
> > ctags that comes with Emacs (this code fragment runs only in ctags, not in
> > etags).
> 
> I rewrote this code, not use system(1).

Thanks.  I have a few comments:

> +
> +          otags_f = fopen ("OTAGS", "w");
> +          tag_f = fopen (tagfile, "r");

Please use "rb" and "wb" in these fopen calls, so as not to change the EOL
format of the original file.

> +          rename ("OTAGS", tagfile);
> +          unlink ("OTAGS");

We should not delete the original file if 'rename' fails; instead, we should
emit a warning and leave OTAGS alone, so that user could manually rename it.

Can you add a test of this feature to test/manual/etags/Makefile?

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Stefan Kangas <stefankangas <at> gmail.com>
Cc: 59544 <at> debbugs.gnu.org, lx <at> shellcodes.org
Subject: Re: bug#59544: [PATCH] Fixed lib-src/etags.c command execute
 vulnerability
Date: Fri, 25 Nov 2022 14:19:30 +0200

> From: Stefan Kangas <stefankangas <at> gmail.com>
> Date: Fri, 25 Nov 2022 00:56:32 -0800
> Cc: Eli Zaretskii <eliz <at> gnu.org>, 59544 <59544 <at> debbugs.gnu.org>
> 
> "lux" <lx <at> shellcodes.org> writes:
> 
> >> Hmm, I'm not sure about the hard-coded 512 character line limit here.
> >> ISTR that some people use much longer lines than that.
> >
> > Hi, do you have any suggestions? At present, I think hardcoding 512 is
> > enough, thanks :-)
> 
> No idea, really.  2^16?  2^20?
> 
> But why not allocate it dynamically, getting rid of any such arbitrary
> limits?  AFAIU, grep (which we used before) doesn't have such limits, so
> I fear that we otherwise risk introducing regressions.

Yes, reallocating dynamically as needed would be best.

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

From: "lux" <lx <at> shellcodes.org>
To: "Eli Zaretskii" <eliz <at> gnu.org>
Cc: 59544 <59544 <at> debbugs.gnu.org>,
 stefankangas <stefankangas <at> gmail.com>
Subject: Re: bug#59544: [PATCH] Fixed lib-src/etags.c command execute
 vulnerability
Date: Sat, 26 Nov 2022 00:02:01 +0800

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

> Can you add a test of this feature to test/manual/etags/Makefile?


Hi, I have refactored the code and added test cases, please help to review, thanks.

[Message part 2 (text/html, inline)]

[0001-Fixed-lib-src-etags.c-command-execute-vulnerability.patch (application/octet-stream, attachment)]

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

From: Stefan Kangas <stefankangas <at> gmail.com>
To: lux <lx <at> shellcodes.org>, Eli Zaretskii <eliz <at> gnu.org>
Cc: 59544 <59544 <at> debbugs.gnu.org>
Subject: Re: bug#59544: [PATCH] Fixed lib-src/etags.c command execute
 vulnerability
Date: Fri, 25 Nov 2022 16:43:40 -0800

"lux" <lx <at> shellcodes.org> writes:

> +          FILE *otags_f = fopen ("OTAGS", "wb");
> +          FILE *tag_f = fopen (tagfile, "rb");
> +
> +          if (otags_f == NULL)
> +              pfatal ("OTAGS");
               ^^

Two spurious spaces here.  Other than that, LGTM.

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

From: lux <lx <at> shellcodes.org>
To: Stefan Kangas <stefankangas <at> gmail.com>, Eli Zaretskii <eliz <at> gnu.org>
Cc: 59544 <59544 <at> debbugs.gnu.org>
Subject: Re: bug#59544: [PATCH] Fixed lib-src/etags.c command execute
 vulnerability
Date: Sat, 26 Nov 2022 10:30:01 +0800

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

> "lux" <lx <at> shellcodes.org> writes:
>
>> +          FILE *otags_f = fopen ("OTAGS", "wb");
>> +          FILE *tag_f = fopen (tagfile, "rb");
>> +
>> +          if (otags_f == NULL)
>> +              pfatal ("OTAGS");
>                 ^^
>
> Two spurious spaces here.  Other than that, LGTM.

Fixed, thank you :-)

[0001-Fixed-lib-src-etags.c-command-execute-vulnerability.patch (text/x-patch, attachment)]

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

From: lux <lx <at> shellcodes.org>
To: Stefan Kangas <stefankangas <at> gmail.com>, Eli Zaretskii <eliz <at> gnu.org>
Cc: 59544 <59544 <at> debbugs.gnu.org>
Subject: Re: bug#59544: [PATCH] Fixed lib-src/etags.c command execute
 vulnerability
Date: Sat, 26 Nov 2022 11:09:26 +0800

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

在 2022/11/26 08:43, Stefan Kangas 写道:
> Other than that, LGTM.

> +          char *buf = xmalloc (buf_len);

The buf variable is not released after use, I added free (buf)

[Message part 2 (text/html, inline)]

[0001-Fixed-lib-src-etags.c-command-execute-vulnerability.patch (text/x-patch, attachment)]

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

From: Stefan Kangas <stefankangas <at> gmail.com>
To: lux <lx <at> shellcodes.org>
Cc: 59544 <at> debbugs.gnu.org, Eli Zaretskii <eliz <at> gnu.org>
Subject: Re: bug#59544: [PATCH] Fixed lib-src/etags.c command execute
 vulnerability
Date: Sat, 26 Nov 2022 01:47:25 -0800

lux <lx <at> shellcodes.org> writes:

> 在 2022/11/26 08:43, Stefan Kangas 写道:
>
>  Other than that, LGTM.
>
>> +          char *buf = xmalloc (buf_len);
>
> The buf variable is not released after use, I added free (buf)

Thanks.  I think we should aim to push this security fix ASAP.

Eli, any additional comments on the patch?

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Stefan Kangas <stefankangas <at> gmail.com>
Cc: 59544 <at> debbugs.gnu.org, lx <at> shellcodes.org
Subject: Re: bug#59544: [PATCH] Fixed lib-src/etags.c command execute
 vulnerability
Date: Sat, 26 Nov 2022 12:14:56 +0200

> From: Stefan Kangas <stefankangas <at> gmail.com>
> Date: Sat, 26 Nov 2022 01:47:25 -0800
> Cc: Eli Zaretskii <eliz <at> gnu.org>, 59544 <at> debbugs.gnu.org
> 
> lux <lx <at> shellcodes.org> writes:
> 
> > 在 2022/11/26 08:43, Stefan Kangas 写道:
> >
> >  Other than that, LGTM.
> >
> >> +          char *buf = xmalloc (buf_len);
> >
> > The buf variable is not released after use, I added free (buf)
> 
> Thanks.  I think we should aim to push this security fix ASAP.
> 
> Eli, any additional comments on the patch?

Please don't push, the patch was posted just a few hours ago.  I have a lot
to do on my hands, and will get to reviewing this in due time.  We've lived
with this "security issue" for decades, so I see nothing here that justifies
"ASAP".

I find the tendency to rush with installing changes bad for the quality of
our code.  I always wait at least for a week before installing myself, and
suggest that you do the same.  Doing so lets others chime in and provide
valuable input and comments.

Thanks in advance.

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: lux <lx <at> shellcodes.org>
Cc: 59544 <at> debbugs.gnu.org, stefankangas <at> gmail.com
Subject: Re: bug#59544: [PATCH] Fixed lib-src/etags.c command execute
 vulnerability
Date: Sat, 26 Nov 2022 14:28:22 +0200

> Date: Sat, 26 Nov 2022 18:41:22 +0800
> Cc: 59544 <at> debbugs.gnu.org
> From: lux <lx <at> shellcodes.org>
> 
> > We've lived with this "security issue" for decades, so I see nothing here that justifies
> > "ASAP".
> Maybe someone found it, but didn't publish it?

Fixing it will not magically remove the problem from all the Emacs
installations out there, will it?  It will only help to people who track the
master branch and rebuild Emacs very frequently on top of that.

So the urgency of fixing it is not measured in hours anyway.

> for example, the lib-src/ntlib.c:
> 
> char *
> cuserid (char * s)
> {
>    char * name = getlogin ();
>    if (s)
>      return strcpy (s, name ? name : "");
>    return name;
> }
> 
> before calling the strcpy function, the memory size of the pointer s is 
> not checked, which may destroy the memory space. So, I want to replace 
> it with a safe function, any suggestions?

The above function doesn't seem to be called anywhere in Emacs, so making it
better is a waste of energy.  It should probably be removed.

Message #61 received at 59544 <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: 59544 <at> debbugs.gnu.org
Subject: Re: bug#59544: [PATCH] Fixed lib-src/etags.c command execute
 vulnerability
Date: Sat, 26 Nov 2022 05:03:02 -0800

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

> Fixing it will not magically remove the problem from all the Emacs
> installations out there, will it?  It will only help to people who track the
> master branch and rebuild Emacs very frequently on top of that.

Distributions often cherry-pick such patches and release security
updates for their stable distributions.

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: lux <lx <at> shellcodes.org>
Cc: 59544 <at> debbugs.gnu.org, stefankangas <at> gmail.com
Subject: Re: bug#59544: [PATCH] Fixed lib-src/etags.c command execute
 vulnerability
Date: Sat, 26 Nov 2022 15:21:49 +0200

> Date: Sat, 26 Nov 2022 11:09:26 +0800
> Cc: 59544 <59544 <at> debbugs.gnu.org>
> From: lux <lx <at> shellcodes.org>
> 
> +          linebuffer line;
> +          linebuffer_init (&line);
> +          while (readline_internal (&line, tag_f, tagfile) > 0)

This needs a minor adjustment: readline_internal removes the CR characters
from CR-LF end-of-lines, so I think using it unaltered will convert files
with DOS-style EOLs to Unix-style EOLs, because we write them with a single
newline at the end.  I think the best fix is to add one more argument to
readline_internal, which, if non-zero, will cause it to avoid removing the
CR characters.

> +ctags_update: CTAGS.good_update ${infiles}
> +	echo > CTAGS

This "echo" command creates an empty CTAGS file.  It would be better to have
at least a few lines there, so we could make sure the new code actually
removes some lines from its input.

Thanks.

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Stefan Kangas <stefankangas <at> gmail.com>
Cc: 59544 <at> debbugs.gnu.org, lx <at> shellcodes.org
Subject: Re: bug#59544: [PATCH] Fixed lib-src/etags.c command execute
 vulnerability
Date: Sat, 26 Nov 2022 16:15:47 +0200

> From: Stefan Kangas <stefankangas <at> gmail.com>
> Date: Sat, 26 Nov 2022 05:03:02 -0800
> Cc: 59544 <at> debbugs.gnu.org
> 
> Eli Zaretskii <eliz <at> gnu.org> writes:
> 
> > Fixing it will not magically remove the problem from all the Emacs
> > installations out there, will it?  It will only help to people who track the
> > master branch and rebuild Emacs very frequently on top of that.
> 
> Distributions often cherry-pick such patches and release security
> updates for their stable distributions.

That still leaves quite a few people outside the change.

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: lux <lx <at> shellcodes.org>
Cc: 59544 <at> debbugs.gnu.org, stefankangas <at> gmail.com
Subject: Re: bug#59544: [PATCH] Fixed lib-src/etags.c command execute
 vulnerability
Date: Sat, 26 Nov 2022 16:17:55 +0200

> Date: Sat, 26 Nov 2022 21:42:48 +0800
> Cc: stefankangas <at> gmail.com, 59544 <at> debbugs.gnu.org
> From: lux <lx <at> shellcodes.org>
> 
> On 2022/11/26 21:21, Eli Zaretskii wrote:
> >> Date: Sat, 26 Nov 2022 11:09:26 +0800
> >> Cc: 59544 <59544 <at> debbugs.gnu.org>
> >> From: lux <lx <at> shellcodes.org>
> >>
> >> +          linebuffer line;
> >> +          linebuffer_init (&line);
> >> +          while (readline_internal (&line, tag_f, tagfile) > 0)
> > This needs a minor adjustment: readline_internal removes the CR characters
> > from CR-LF end-of-lines, so I think using it unaltered will convert files
> > with DOS-style EOLs to Unix-style EOLs, because we write them with a single
> > newline at the end.  I think the best fix is to add one more argument to
> > readline_internal, which, if non-zero, will cause it to avoid removing the
> > CR characters.
> 
> Hmm.. but doing this doesn't have much to do with fixing the this 
> vulnerability, since
> 
> the previous hardcoded one doesn't work on Windows, so can it be changed 
> later?

I'd prefer to fix them together.  It's very simple, isn't it?

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: lux <lx <at> shellcodes.org>
Cc: 59544 <at> debbugs.gnu.org, stefankangas <at> gmail.com
Subject: Re: bug#59544: [PATCH] Fixed lib-src/etags.c command execute
 vulnerability
Date: Sat, 26 Nov 2022 16:30:12 +0200

> Date: Sat, 26 Nov 2022 21:06:16 +0800
> Cc: stefankangas <at> gmail.com, 59544 <at> debbugs.gnu.org
> From: lux <lx <at> shellcodes.org>
> 
> > The above function doesn't seem to be called anywhere in Emacs, so making it
> > better is a waste of energy.  It should probably be removed.
> 
> ok I cleaned up.

Thanks, installed.

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: lux <lx <at> shellcodes.org>
Cc: 59544 <at> debbugs.gnu.org, stefankangas <at> gmail.com
Subject: Re: bug#59544: [PATCH] Fixed lib-src/etags.c command execute
 vulnerability
Date: Sat, 26 Nov 2022 16:49:56 +0200

> Date: Sat, 26 Nov 2022 22:26:22 +0800
> Cc: stefankangas <at> gmail.com, 59544 <at> debbugs.gnu.org
> From: lux <lx <at> shellcodes.org>
> 
> Yes, but I think it violates the original author's intention, and it 
> seems that there is no occasion to use this parameter in etags?
> 
> /*
>   * Read a line of text from `stream' into `lbp', excluding the
>   * newline or CR-NL, if any.  Return the number of characters read from
>   * `stream', which is the length of the line including the newline.
>   *
>   * On DOS or Windows we do not count the CR character, if any before the
>   * NL, in the returned length; this mirrors the behavior of Emacs on those
>   * platforms (for text files, it translates CR-NL to NL as it reads in the
>   * file).

The above is about the character counts written in TAGS tables, which are
produced by etags, not by ctags.  Files produced by crags only count lines,
not characters.  So the above comment is not relevant to ctags.

More importantly, the original tags file could have been written by a
utility other than our ctags, and I don't think we should change the EOL
format of such a file when we update it.

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: lux <lx <at> shellcodes.org>
Cc: 59544 <at> debbugs.gnu.org, stefankangas <at> gmail.com
Subject: Re: bug#59544: [PATCH] Fixed lib-src/etags.c command execute
 vulnerability
Date: Sat, 26 Nov 2022 19:11:29 +0200

> Date: Sun, 27 Nov 2022 01:03:10 +0800
> Cc: stefankangas <at> gmail.com, 59544 <at> debbugs.gnu.org
> From: lux <lx <at> shellcodes.org>
> 
> I'm going to add the xxx parameter. If it is true, the line break will 
> be kept. Is this logic consistent with what you expressed? thank you 
> very much.

Much simpler:

static ptrdiff_t
readline_internal (linebuffer *lbp, FILE *stream, char const *filename, bool leave_cr)
{
 [...]
      if (c == '\n')
	{
	  if (!leave_cr && p > buffer && p[-1] == '\r')
	    {
 [...]

IOW, when this flag is TRUE, don't process \r specially.

Thanks.

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

From: lux <lx <at> shellcodes.org>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 59544 <at> debbugs.gnu.org, stefankangas <at> gmail.com
Subject: Re: bug#59544: [PATCH] Fixed lib-src/etags.c command execute
 vulnerability
Date: Sun, 27 Nov 2022 11:05:37 +0800

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

On 2022/11/27 01:11, Eli Zaretskii wrote:
>> Date: Sun, 27 Nov 2022 01:03:10 +0800
>> Cc: stefankangas <at> gmail.com, 59544 <at> debbugs.gnu.org
>> From: lux <lx <at> shellcodes.org>
>>
>> I'm going to add the xxx parameter. If it is true, the line break will
>> be kept. Is this logic consistent with what you expressed? thank you
>> very much.
> Much simpler:
>
> static ptrdiff_t
> readline_internal (linebuffer *lbp, FILE *stream, char const *filename, bool leave_cr)
> {
>   [...]
>        if (c == '\n')
> 	{
> 	  if (!leave_cr && p > buffer && p[-1] == '\r')
> 	    {
>   [...]
>
> IOW, when this flag is TRUE, don't process \r specially.
>
> Thanks.

Ok,  edited and added test case,  and move the patch code to the new 
function `clean_matched_file_tag`.

Thanks.

[0001-Fixed-ctags-local-command-execute-vulnerability.patch (text/x-patch, attachment)]

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

From: lux <lx <at> shellcodes.org>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 59544 <at> debbugs.gnu.org, stefankangas <at> gmail.com
Subject: Re: bug#59544: [PATCH] Fixed lib-src/etags.c command execute
 vulnerability
Date: Sun, 27 Nov 2022 14:35:01 +0800

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

Sorry, this is new patch, I fix a bug.

The `rename` function, if trying to rename a file across filesystem or 
device, it's not work, and give the EXDEV erorr:

    Invalid cross-device link

So, I add a new function `do_move_file`: if the rename fails, copy a new 
file, and unlink source file.

[0001-Fixed-ctags-local-command-execute-vulnerability.patch (text/x-patch, attachment)]

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: lux <lx <at> shellcodes.org>
Cc: 59544 <at> debbugs.gnu.org, stefankangas <at> gmail.com
Subject: Re: bug#59544: [PATCH] Fixed lib-src/etags.c command execute
 vulnerability
Date: Sun, 27 Nov 2022 16:15:38 +0200

> Date: Sun, 27 Nov 2022 14:35:01 +0800
> Cc: stefankangas <at> gmail.com, 59544 <at> debbugs.gnu.org
> From: lux <lx <at> shellcodes.org>
> 
> The `rename` function, if trying to rename a file across filesystem or 
> device, it's not work, and give the EXDEV erorr:
> 
>      Invalid cross-device link
> 
> So, I add a new function `do_move_file`: if the rename fails, copy a new 
> file, and unlink source file.

Thanks, I installed this.

But something is wrong with the 2 new tests: they fail.  I replaced the
"good" files with the ones I get on my system, but the test fails on another
system.  Could you please look into the test failures and find a fix?

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: lx <at> shellcodes.org
Cc: 59544 <at> debbugs.gnu.org, stefankangas <at> gmail.com
Subject: Re: bug#59544: [PATCH] Fixed lib-src/etags.c command execute
 vulnerability
Date: Sun, 27 Nov 2022 16:31:45 +0200

> Cc: 59544 <at> debbugs.gnu.org, stefankangas <at> gmail.com
> Date: Sun, 27 Nov 2022 16:15:38 +0200
> From: Eli Zaretskii <eliz <at> gnu.org>
> 
> But something is wrong with the 2 new tests: they fail.  I replaced the
> "good" files with the ones I get on my system, but the test fails on another
> system.  Could you please look into the test failures and find a fix?

It looks like at least some of the differences are due to different order of
entries in the tags files?  Maybe this is something related to sort order?

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: lux <lx <at> shellcodes.org>
Cc: 59544-done <at> debbugs.gnu.org, stefankangas <at> gmail.com
Subject: Re: bug#59544: [PATCH] Fixed lib-src/etags.c command execute
 vulnerability
Date: Sun, 27 Nov 2022 20:07:54 +0200

> Date: Sun, 27 Nov 2022 23:44:07 +0800
> From: lux <lx <at> shellcodes.org>
> 
> On Sun, 27 Nov 2022 16:15:38 +0200
> Eli Zaretskii <eliz <at> gnu.org> wrote:
> 
> > But something is wrong with the 2 new tests: they fail.  I replaced
> > the "good" files with the ones I get on my system, but the test fails
> > on another system.  Could you please look into the test failures and
> > find a fix?
> 
> Hi, I think because the order of the tag data of the files generated by
> different OS environments is different.
> 
> I sorted the file using the sort command, test ok.
> 
> ctags_update: CTAGS.good_update ${infiles}
> 	head -n 100 CTAGS.good_update > CTAGS
> 	tail -n 100 CTAGS.good_update >> CTAGS
> 	${RUN} ${CTAGS_PROG} -o CTAGS -u ${ARGS}
> 	diff -u --suppress-common-lines --width=80 <(sort
> CTAGS.good_update) <(sort CTAGS)
> 
> 	cp crlf CTAGS
> 	${RUN} ${CTAGS_PROG} -o CTAGS -u ${ARGS}
> 	diff -u --suppress-common-lines --width=80 <(sort
> 	CTAGS.good_crlf) <(sort CTAGS)

Thanks, I installed a variant of this using more portable commands.

And with that, I'm closing this bug.

Previous Next

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