bug#66390: `man' allows to inject arbitrary shell code

[Eli Zaretskii]

> Date: Sun, 8 Oct 2023 10:37:33 +0700
> Cc: 66390@debbugs.gnu.org
> From: Max Nikulin <manikulin@gmail.com>
> 
> On 08/10/2023 01:26, Eli Zaretskii wrote:
> > 
> > So the problem _is_ with the shell?  If so, the best way of avoiding
> > these problems is not invoke 'man' via the shell, but via call-process
> > and its ilk instead.
> 
> It will be great if it is possible to avoid shell in the middle. However
> - man.el uses pipes with sed and awk to post-process output of man 
> executable.
> - if support of remote man files is considered then it is even more hard 
> to avoid shell. SSH assumes shell commands.

Even if sometimes the shell cannot be avoided (which has yet to be
established, AFAIU), it's not an argument against avoiding it where
possible, because that solves any security issues, definitely those
you brought up.

> I had in mind using at least `shell-quote-argument'.

That doesn't work with 'man', which has its own ideas about quoting,
besides shell-related quoting.

> The issues of sanitizing outputs in callers
> - If there was a safe function in man.el then callers code would be more 
> simple, so it would be less probable to introduce bugs in such code.
> - behavior of the `man' emacs command is *underspecified*, so it is hard 
> to provide safe argument for it. Some parenthesis are allowed as in 
> "man(1)" others may be interpreted by shell.
> - `shell-quote-argument' in callers would rely on man.el implementation 
> details at best or may even lead to undefined behavior since I see have 
> no way to bypass some processing of the argument of the `man' emacs command.

Reiterating what you already said doesn't help to have a productive
discussion.

> Execution a part of `man' emacs command argument by shell is a surprise 
> to the user any case. Ideally elisp code should prevent it and man.el 
> should emit an error.

IMO, this ideal cannot be reached in practice, let alone kept for any
length of time.  Systems are adding strangely-named man pages all the
time.  We had quite a few bug reports about that during the recent
years.

> Attempts to call of `man' from other packages is an open door for 
> security vulnerabilities.

Then perhaps those other packages shouldn't call 'man'.