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

[Max Nikulin]

On 11/10/2023 10:08, lux wrote:
> On Tue, 2023-10-10 at 18:21 +0200, Andreas Schwab wrote:
> > On Okt 10 2023, lux wrote:
> >
> > > +        ;; see Bug#66390
> > > +       (mapconcat 'identity
> > > +                   (mapcar #'shell-quote-argument
> > > +                           (split-string ref " "))
> >
> > You need to split on arbitrary sequences of whitespace to not introduce
> > spurious empty arguments.
>
> Thanks, I've modified it to (split-string ref "\\s-+").

At this point spaces are supposed to be already normalized by the a bit 
buggy `Man-translate-cleanup' function.

I can not provide an example that is not handled by the suggested patch. 
I am not still feeling comfortable since it affects rather specific code 
path. Even the last line of this function might be more suitable.

Other considerations:

The patch changes behavior. Earler users had to escape characters to get 
reliable result, but it will break searches (I am in doubts if enough 
people will notice it):

   (man "-k \\[a-z\\]dparm")

Buffer names will have backslashes.

I do not like that tests for `system-type' are not the same in 
`shell-quote-argument' and in `Man-getpage-in-background'. I am afraid 
that in some cases improper style of escaping may be applied.

From my point of view, code that performs quoting should be close to 
the code that invokes shell otherwise risk of inconsistent changes 
increases. I admit, it requires more work than quick plumbing at the 
place where a minimal patch fixes the issue.