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

[lux]

On Sun, 2023-10-08 at 08:28 +0300, Eli Zaretskii wrote:
> > 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'.
> 
> 
> 

Hi, 

There is indeed an code injection vulnerability issue here, for example:

  (man ";ls")    <-- The `ls' command will be executed.

I think the fix can start with the `Man-translate-references' function.

Here's my patch and the test cases.

0001-Fix-man.el-code-injection-vulnerability.patch
Description: Text Data