GNU bug report logs - #66390
`man' allows to inject arbitrary shell code

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 66390 in the body.
You can then email your comments to 66390 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: Maxim Nikulin <m.a.nikulin <at> gmail.com>
To: bug-gnu-emacs <at> gnu.org
Subject: `man' allows to inject arbitrary shell code
Date: Sat, 7 Oct 2023 19:47:04 +0700

man.el does not escape properly shell special characters when `man' is 
invoked with an argument to open particular manual page. As a result 
arbitrary shell code may be executed.

I do not consider it as a real issue when the `man' command is invoked 
by a user directly. However it is a security vulnerability when other 
packages calls `man' to open a specific page.

Consider an Org mode document with the following link and ol-man is loaded

  <man:File:\:UserDirs(3pm)>

In response to C-c C-o (`org-open-at-point') an error appears instead of 
formatted manual page

--- 8< ---
/usr/bin/sh: 1: Syntax error: "(" unexpected

process exited abnormally with code 2
--- >8 ---

Alternatively just evaluate

 (man "File:\\:UserDirs(3pm)")

A side note: I tried to add backslash due to an issue with ol-man that 
is to be fixed. A workaround in this particular case is to remove 
"(3pm)". Though the real problem is that special characters "()" are not 
quoted.

I would not consider the issue as a severe one unless some users who 
wish to open arbitrary Org files from the net

https://debbugs.gnu.org/cgi/bugreport.cgi?bug=58774#34
> Org files are native to Emacs, I wish to open Org files by using EWW.

man.el should prevent substitution of shell specials literally from 
`man' arguments into shell commands.

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Maxim Nikulin <m.a.nikulin <at> gmail.com>
Cc: 66390 <at> debbugs.gnu.org
Subject: Re: bug#66390: `man' allows to inject arbitrary shell code
Date: Sat, 07 Oct 2023 16:04:17 +0300

> From: Maxim Nikulin <m.a.nikulin <at> gmail.com>
> Date: Sat, 7 Oct 2023 19:47:04 +0700
> 
> man.el does not escape properly shell special characters when `man' is 
> invoked with an argument to open particular manual page. As a result 
> arbitrary shell code may be executed.
> 
> I do not consider it as a real issue when the `man' command is invoked 
> by a user directly. However it is a security vulnerability when other 
> packages calls `man' to open a specific page.
> 
> Consider an Org mode document with the following link and ol-man is loaded
> 
>    <man:File:\:UserDirs(3pm)>
> 
> In response to C-c C-o (`org-open-at-point') an error appears instead of 
> formatted manual page
> 
> --- 8< ---
> /usr/bin/sh: 1: Syntax error: "(" unexpected
> 
> process exited abnormally with code 2
> --- >8 ---
> 
> Alternatively just evaluate
> 
>   (man "File:\\:UserDirs(3pm)")

Why isn't it a problem with the command that invokes 'man', in this
case Org?

> man.el should prevent substitution of shell specials literally from 
> `man' arguments into shell commands.

I think callers of 'man' should prevent that instead.

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

From: Max Nikulin <manikulin <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 66390 <at> debbugs.gnu.org
Subject: Re: bug#66390: `man' allows to inject arbitrary shell code
Date: Sat, 7 Oct 2023 21:12:54 +0700

On 07/10/2023 20:04, Eli Zaretskii wrote:
>> From: Maxim Nikulin
>> Date: Sat, 7 Oct 2023 19:47:04 +0700
> 
>> man.el should prevent substitution of shell specials literally from
>> `man' arguments into shell commands.
> 
> I think callers of 'man' should prevent that instead.

If it is fixed in man.el then it is fixed for all callers. Otherwise 
every caller must have notion of structure of references to man pages 
instead of just treating them as opaque sequence of characters.

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Max Nikulin <manikulin <at> gmail.com>
Cc: 66390 <at> debbugs.gnu.org
Subject: Re: bug#66390: `man' allows to inject arbitrary shell code
Date: Sat, 07 Oct 2023 17:19:28 +0300

> Date: Sat, 7 Oct 2023 21:12:54 +0700
> Cc: 66390 <at> debbugs.gnu.org
> From: Max Nikulin <manikulin <at> gmail.com>
> 
> On 07/10/2023 20:04, Eli Zaretskii wrote:
> >> From: Maxim Nikulin
> >> Date: Sat, 7 Oct 2023 19:47:04 +0700
> > 
> >> man.el should prevent substitution of shell specials literally from
> >> `man' arguments into shell commands.
> > 
> > I think callers of 'man' should prevent that instead.
> 
> If it is fixed in man.el then it is fixed for all callers. Otherwise 
> every caller must have notion of structure of references to man pages 
> instead of just treating them as opaque sequence of characters.

Sorry, I disagree.  'man' is an interactive command, so it should not
second-guess the user who invokes it.  Commands that call 'man'
non-interactively should make sure they call 'man' with a valid
argument, especially when the argument comes from some file.

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

From: Max Nikulin <manikulin <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 66390 <at> debbugs.gnu.org
Subject: Re: bug#66390: `man' allows to inject arbitrary shell code
Date: Sat, 7 Oct 2023 21:29:12 +0700

On 07/10/2023 21:19, Eli Zaretskii wrote:
> 
> Sorry, I disagree.  'man' is an interactive command, so it should not
> second-guess the user who invokes it.  Commands that call 'man'
> non-interactively should make sure they call 'man' with a valid
> argument, especially when the argument comes from some file.

Does man.el provide a function that opens references to man pages, but 
that is safe in respect to shell specials?

Calling of shell commands belongs to implementation details of man.el 
and effectively you require that callers must be aware of it.

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Max Nikulin <manikulin <at> gmail.com>
Cc: 66390 <at> debbugs.gnu.org
Subject: Re: bug#66390: `man' allows to inject arbitrary shell code
Date: Sat, 07 Oct 2023 18:10:36 +0300

> Date: Sat, 7 Oct 2023 21:29:12 +0700
> Cc: 66390 <at> debbugs.gnu.org
> From: Max Nikulin <manikulin <at> gmail.com>
> 
> On 07/10/2023 21:19, Eli Zaretskii wrote:
> > 
> > Sorry, I disagree.  'man' is an interactive command, so it should not
> > second-guess the user who invokes it.  Commands that call 'man'
> > non-interactively should make sure they call 'man' with a valid
> > argument, especially when the argument comes from some file.
> 
> Does man.el provide a function that opens references to man pages, but 
> that is safe in respect to shell specials?
> 
> Calling of shell commands belongs to implementation details of man.el 
> and effectively you require that callers must be aware of it.

No, I just expect the callers to call 'man' with valid arguments.

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

From: Michael Albinus <michael.albinus <at> gmx.de>
To: Max Nikulin <manikulin <at> gmail.com>
Cc: Eli Zaretskii <eliz <at> gnu.org>, 66390 <at> debbugs.gnu.org
Subject: Re: bug#66390: `man' allows to inject arbitrary shell code
Date: Sat, 07 Oct 2023 17:37:33 +0200

Max Nikulin <manikulin <at> gmail.com> writes:

Hi,

>> Sorry, I disagree.  'man' is an interactive command, so it should
>> not
>> second-guess the user who invokes it.  Commands that call 'man'
>> non-interactively should make sure they call 'man' with a valid
>> argument, especially when the argument comes from some file.
>
> Does man.el provide a function that opens references to man pages, but
> that is safe in respect to shell specials?
>
> Calling of shell commands belongs to implementation details of man.el
> and effectively you require that callers must be aware of it.

I tend to agree with both :-) The caller of a shell command (`man ARGS') is
responsible for proper quoting of the arguments.

The function `Man-translate-references' tries to do it. For example, it
translates the argument "cat(1)" into "1 cat", which doesn't pose a
problem. The function should check stronger, and it should reject
arguments like "File:\\:UserDirs(3pm)". ol-man.el should be busy to
offer only valid arguments to `man' according to the man page man(1).

Oh man ...

Best regards, Michael.

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Michael Albinus <michael.albinus <at> gmx.de>
Cc: manikulin <at> gmail.com, 66390 <at> debbugs.gnu.org
Subject: Re: bug#66390: `man' allows to inject arbitrary shell code
Date: Sat, 07 Oct 2023 18:58:12 +0300

> From: Michael Albinus <michael.albinus <at> gmx.de>
> Cc: Eli Zaretskii <eliz <at> gnu.org>,  66390 <at> debbugs.gnu.org
> Date: Sat, 07 Oct 2023 17:37:33 +0200
> 
> The function `Man-translate-references' tries to do it. For example, it
> translates the argument "cat(1)" into "1 cat", which doesn't pose a
> problem. The function should check stronger, and it should reject
> arguments like "File:\\:UserDirs(3pm)".

Based on what would we reject such arguments?

And what kind of shell would we assume when rejecting that?

Once again, interactive invocations should let the user type whatever
she wants, and if that fails in strange ways, it's on the user, not on
us.

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

From: Michael Albinus <michael.albinus <at> gmx.de>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: manikulin <at> gmail.com, 66390 <at> debbugs.gnu.org
Subject: Re: bug#66390: `man' allows to inject arbitrary shell code
Date: Sat, 07 Oct 2023 18:55:01 +0200

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

Hi Eli,

>> The function `Man-translate-references' tries to do it. For example, it
>> translates the argument "cat(1)" into "1 cat", which doesn't pose a
>> problem. The function should check stronger, and it should reject
>> arguments like "File:\\:UserDirs(3pm)".
>
> Based on what would we reject such arguments?

On argument syntax for man. It is documented.

> And what kind of shell would we assume when rejecting that?

It isn't a problem of the shell. Man-translate-references manipulates
the arguments such a way that no shell quoting is neded.

> Once again, interactive invocations should let the user type whatever
> she wants, and if that fails in strange ways, it's on the user, not on
> us.

Yes, if the user types nonsense it shall fail. The point is where to
fail. I believe it shall fail already in Man-translate-references, and
not from the man invocation with a shell.

The docstring of man explains already, which kind of arguments are
expected. Whe should simply follow with the
implementation. "File:\\:UserDirs(3pm)" is not a valid argument, and
shall be rejected on Lisp level.

Best regards, Michael.

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Michael Albinus <michael.albinus <at> gmx.de>
Cc: manikulin <at> gmail.com, 66390 <at> debbugs.gnu.org
Subject: Re: bug#66390: `man' allows to inject arbitrary shell code
Date: Sat, 07 Oct 2023 20:24:54 +0300

> From: Michael Albinus <michael.albinus <at> gmx.de>
> Cc: manikulin <at> gmail.com,  66390 <at> debbugs.gnu.org
> Date: Sat, 07 Oct 2023 18:55:01 +0200
> 
> Eli Zaretskii <eliz <at> gnu.org> writes:
> 
> Hi Eli,
> 
> >> The function `Man-translate-references' tries to do it. For example, it
> >> translates the argument "cat(1)" into "1 cat", which doesn't pose a
> >> problem. The function should check stronger, and it should reject
> >> arguments like "File:\\:UserDirs(3pm)".
> >
> > Based on what would we reject such arguments?
> 
> On argument syntax for man. It is documented.

For what versions of 'man'?  There are a lot of different versions; I
myself wrote a clone, for example.

> > And what kind of shell would we assume when rejecting that?
> 
> It isn't a problem of the shell. Man-translate-references manipulates
> the arguments such a way that no shell quoting is neded.

Then there's no problem to begin with, since the OP claims the problem
is with the shell?

> > Once again, interactive invocations should let the user type whatever
> > she wants, and if that fails in strange ways, it's on the user, not on
> > us.
> 
> Yes, if the user types nonsense it shall fail. The point is where to
> fail. I believe it shall fail already in Man-translate-references, and
> not from the man invocation with a shell.

We cannot do that, unless we implement the entire behavior of 'man' in
Emacs.

> The docstring of man explains already, which kind of arguments are
> expected.

Yes, and we update that all the time, given how the systems stretch
these specs.

There's only madness down that road.

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

From: Michael Albinus <michael.albinus <at> gmx.de>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: manikulin <at> gmail.com, 66390 <at> debbugs.gnu.org
Subject: Re: bug#66390: `man' allows to inject arbitrary shell code
Date: Sat, 07 Oct 2023 19:45:18 +0200

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

Hi Eli,

>> On argument syntax for man. It is documented.
>
> For what versions of 'man'?  There are a lot of different versions; I
> myself wrote a clone, for example.

I haven't written such a thing, so you will always beat me. And if you
oppose my proposals, I will happily accept it.

>> > And what kind of shell would we assume when rejecting that?
>>
>> It isn't a problem of the shell. Man-translate-references manipulates
>> the arguments such a way that no shell quoting is neded.
>
> Then there's no problem to begin with, since the OP claims the problem
> is with the shell?

The OP claims that the arguments could be misused, bypassing exotic
strings which would do terrific work in the shell man is using.

>> > Once again, interactive invocations should let the user type whatever
>> > she wants, and if that fails in strange ways, it's on the user, not on
>> > us.
>>
>> Yes, if the user types nonsense it shall fail. The point is where to
>> fail. I believe it shall fail already in Man-translate-references, and
>> not from the man invocation with a shell.
>
> We cannot do that, unless we implement the entire behavior of 'man' in
> Emacs.
>
>> The docstring of man explains already, which kind of arguments are
>> expected.
>
> Yes, and we update that all the time, given how the systems stretch
> these specs.

No, the docstring speaks about -a, -k and -l. That's what we shall do.

> There's only madness down that road.

Well, if you still believe there's nothing to do for us I will be quiet.

Best regards, Michael.

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Michael Albinus <michael.albinus <at> gmx.de>
Cc: manikulin <at> gmail.com, 66390 <at> debbugs.gnu.org
Subject: Re: bug#66390: `man' allows to inject arbitrary shell code
Date: Sat, 07 Oct 2023 21:26:40 +0300

> From: Michael Albinus <michael.albinus <at> gmx.de>
> Cc: manikulin <at> gmail.com,  66390 <at> debbugs.gnu.org
> Date: Sat, 07 Oct 2023 19:45:18 +0200
> 
> Eli Zaretskii <eliz <at> gnu.org> writes:
> 
> >> > And what kind of shell would we assume when rejecting that?
> >>
> >> It isn't a problem of the shell. Man-translate-references manipulates
> >> the arguments such a way that no shell quoting is neded.
> >
> > Then there's no problem to begin with, since the OP claims the problem
> > is with the shell?
> 
> The OP claims that the arguments could be misused, bypassing exotic
> strings which would do terrific work in the shell man is using.

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.

> > There's only madness down that road.
> 
> Well, if you still believe there's nothing to do for us I will be quiet.

We can do something, just not the way it was suggested: avoid using
the shell.

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

From: Max Nikulin <manikulin <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>, Michael Albinus <michael.albinus <at> gmx.de>
Cc: 66390 <at> debbugs.gnu.org
Subject: Re: bug#66390: `man' allows to inject arbitrary shell code
Date: Sun, 8 Oct 2023 10:37:33 +0700

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.

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

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.

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.

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

I was really surprised when I noticed that various Linux distributions 
patched and updated emacs even in stable releases in response to 
https://security-tracker.debian.org/tracker/CVE-2023-28617 Formally the 
score of this CVE was high.

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Maxim Nikulin <manikulin+gzh <at> gmail.com>
Cc: 66390 <at> debbugs.gnu.org, michael.albinus <at> gmx.de
Subject: Re: bug#66390: `man' allows to inject arbitrary shell code
Date: Sun, 08 Oct 2023 08:20:09 +0300

> Date: Sun, 8 Oct 2023 10:42:03 +0700
> Cc: 66390 <at> debbugs.gnu.org
> From: Maxim Nikulin <manikulin+gzh <at> gmail.com>
> 
> On 08/10/2023 00:24, Eli Zaretskii wrote:
> >> From: Michael Albinus Date: Sat, 07 Oct 2023 18:55:01 +0200
> > 
> >> The docstring of man explains already, which kind of arguments are
> >> expected.
> > 
> > Yes, and we update that all the time, given how the systems stretch
> > these specs.
> 
> I see some discrepancy with the declaration of stable API in "Re: 
> Completion of links to man pages"

IMO, you see something that doesn't exist.  The quoted message was
talking about Lisp API for completing names of 'man' pages, not about
the spec of 'man' arguments.

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Max Nikulin <manikulin <at> gmail.com>
Cc: 66390 <at> debbugs.gnu.org, michael.albinus <at> gmx.de
Subject: Re: bug#66390: `man' allows to inject arbitrary shell code
Date: Sun, 08 Oct 2023 08:28:00 +0300

> Date: Sun, 8 Oct 2023 10:37:33 +0700
> Cc: 66390 <at> debbugs.gnu.org
> From: Max Nikulin <manikulin <at> 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'.

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

From: Maxim Nikulin <manikulin+gzh <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>, Michael Albinus <michael.albinus <at> gmx.de>
Cc: 66390 <at> debbugs.gnu.org
Subject: Re: bug#66390: `man' allows to inject arbitrary shell code
Date: Sun, 8 Oct 2023 10:42:03 +0700

On 08/10/2023 00:24, Eli Zaretskii wrote:
>> From: Michael Albinus Date: Sat, 07 Oct 2023 18:55:01 +0200
> 
>> The docstring of man explains already, which kind of arguments are
>> expected.
> 
> Yes, and we update that all the time, given how the systems stretch
> these specs.

I see some discrepancy with the declaration of stable API in "Re: 
Completion of links to man pages"

On 06/10/2023 00:11, Eli Zaretskii wrote:
> From: Ihor Radchenko <yantar92 <at> posteo.net>
> Cc: emacs-orgmode <at> gnu.org, emacs-devel <at> gnu.org
> Date: Thu, 05 Oct 2023 16:53:57 +0000
>> What I am asking here is to provide a stable Elisp API for the above use
>> case. Currently, we have to rely on implementation details.
> 
> From where I stand, we have already a stable API tested by years of
> use.  What is maybe missing is some documentation to allow its easier
> use, that's all.

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

From: Richard Stallman <rms <at> gnu.org>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: manikulin <at> gmail.com, 66390 <at> debbugs.gnu.org, michael.albinus <at> gmx.de
Subject: Re: bug#66390: `man' allows to inject arbitrary shell code
Date: Sun, 08 Oct 2023 22:36:39 -0400

[[[ To any NSA and FBI agents reading my email: please consider    ]]]
[[[ whether defending the US Constitution against all enemies,     ]]]
[[[ foreign or domestic, requires you to follow Snowden's example. ]]]

  > We can do something, just not the way it was suggested: avoid using
  > the shell.

I wonder: do we need to backport this fix to old Emacs versions that we
do not normally maintainn at all, because of the insecurity?

-- 
Dr Richard Stallman (https://stallman.org)
Chief GNUisance of the GNU Project (https://gnu.org)
Founder, Free Software Foundation (https://fsf.org)
Internet Hall-of-Famer (https://internethalloffame.org)

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: rms <at> gnu.org
Cc: manikulin <at> gmail.com, 66390 <at> debbugs.gnu.org, michael.albinus <at> gmx.de
Subject: Re: bug#66390: `man' allows to inject arbitrary shell code
Date: Mon, 09 Oct 2023 14:04:37 +0300

> From: Richard Stallman <rms <at> gnu.org>
> Cc: michael.albinus <at> gmx.de, manikulin <at> gmail.com, 66390 <at> debbugs.gnu.org
> Date: Sun, 08 Oct 2023 22:36:39 -0400
> 
>   > We can do something, just not the way it was suggested: avoid using
>   > the shell.
> 
> I wonder: do we need to backport this fix to old Emacs versions that we
> do not normally maintainn at all, because of the insecurity?

We don't retrofit fixes into old branches of Emacs that are no longer
developed; we leave that to the distros (who maintain old Emacs
versions for many more years than we do).  At this time, this means
only Emacs 29.x and newer can get such fixes, but not older versions.

(Btw, there's no fix yet, just discussions about what would be the
most appropriate fix.)

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

From: Max Nikulin <manikulin <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 66390 <at> debbugs.gnu.org, michael.albinus <at> gmx.de
Subject: Re: bug#66390: `man' allows to inject arbitrary shell code
Date: Mon, 9 Oct 2023 22:12:34 +0700

On 08/10/2023 12:28, Eli Zaretskii wrote:
>> Date: Sun, 8 Oct 2023 10:37:33 +0700 From: Max Nikulin
> 
>> 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.

I see usage of `shell-quote-argument' for completion where shell is not 
involved. During formatting there is parsing of references with some 
regular expressions to get (X) section suffix, but I have not noticed 
quoting. Certainly the code relies on spaces passed literally and 
substituted into shell command directly. If there were page names with 
spaces it would be a problem.

I mean passing through `shell-quote-argument' each word returned by 
`Man-translate-references'

P.S.

(defun Man-translate-cleanup (string)
  "Strip leading, trailing and middle spaces."
   ^^^^^^^^^^^^^

(Man-translate-cleanup " w")
" w"

?

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Max Nikulin <manikulin <at> gmail.com>
Cc: 66390 <at> debbugs.gnu.org, michael.albinus <at> gmx.de
Subject: Re: bug#66390: `man' allows to inject arbitrary shell code
Date: Mon, 09 Oct 2023 18:52:52 +0300

> Date: Mon, 9 Oct 2023 22:12:34 +0700
> Cc: michael.albinus <at> gmx.de, 66390 <at> debbugs.gnu.org
> From: Max Nikulin <manikulin <at> gmail.com>
> 
> On 08/10/2023 12:28, Eli Zaretskii wrote:
> >> Date: Sun, 8 Oct 2023 10:37:33 +0700 From: Max Nikulin
> > 
> >> 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.
> 
> I see usage of `shell-quote-argument' for completion where shell is not 
> involved. During formatting there is parsing of references with some 
> regular expressions to get (X) section suffix, but I have not noticed 
> quoting. Certainly the code relies on spaces passed literally and 
> substituted into shell command directly. If there were page names with 
> spaces it would be a problem.
> 
> I mean passing through `shell-quote-argument' each word returned by 
> `Man-translate-references'

What will this do with a man page called [.1 ?

> (defun Man-translate-cleanup (string)
>    "Strip leading, trailing and middle spaces."
>     ^^^^^^^^^^^^^
> 
> (Man-translate-cleanup " w")
> " w"

But

  (Man-translate-cleanup " ww")
    => "ww"

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

From: lux <lx <at> shellcodes.org>
To: Eli Zaretskii <eliz <at> gnu.org>, Max Nikulin <manikulin <at> gmail.com>
Cc: 66390 <at> debbugs.gnu.org, michael.albinus <at> gmx.de
Subject: Re: bug#66390: `man' allows to inject arbitrary shell code
Date: Tue, 10 Oct 2023 00:30:06 +0800

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

On Sun, 2023-10-08 at 08:28 +0300, Eli Zaretskii wrote:
> > Date: Sun, 8 Oct 2023 10:37:33 +0700
> > Cc: 66390 <at> debbugs.gnu.org
> > From: Max Nikulin <manikulin <at> 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 (text/x-patch, attachment)]

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: lux <lx <at> shellcodes.org>
Cc: manikulin <at> gmail.com, 66390 <at> debbugs.gnu.org, michael.albinus <at> gmx.de
Subject: Re: bug#66390: `man' allows to inject arbitrary shell code
Date: Mon, 09 Oct 2023 19:48:21 +0300

> From: lux <lx <at> shellcodes.org>
> Cc: 66390 <at> debbugs.gnu.org, michael.albinus <at> gmx.de
> Date: Tue, 10 Oct 2023 00:30:06 +0800
> 
> There is indeed an code injection vulnerability issue here, for example:
> 
>   (man ";ls")    <-- The `ls' command will be executed.

So does this:

  (shell-command "ls")

Does it mean we will disallow shell-command? or forcibly quote every
shell command?  We cannot do that.

> Here's my patch and the test cases.

And I ask again: what happens with command (man "[") in this case?

Please believe me: this is not simple.  There's more here than meets
the eye.  In addition to all kinds of weird characters in man-page
names, you also need to consider SEE ALSO links from one man page to
another, which can cross lines and include dashes and whitespace.
Etc. etc...  I had my share of messing with this code, and one thing I
know is that nothing is ever as simple as quoting here.

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

From: Ihor Radchenko <yantar92 <at> posteo.net>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: lux <lx <at> shellcodes.org>, manikulin <at> gmail.com, 66390 <at> debbugs.gnu.org,
 michael.albinus <at> gmx.de
Subject: Re: bug#66390: `man' allows to inject arbitrary shell code
Date: Mon, 09 Oct 2023 17:07:36 +0000

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

>> There is indeed an code injection vulnerability issue here, for example:
>> 
>>   (man ";ls")    <-- The `ls' command will be executed.
>
> So does this:
>
>   (shell-command "ls")
>
> Does it mean we will disallow shell-command? or forcibly quote every
> shell command?  We cannot do that.

You seem to have an idea what MAN-ARGS argument in `man' does. But it is
not described in the docstring. I think it would help if docstring were
more clear about the command argument.

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

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

From: Andreas Schwab <schwab <at> linux-m68k.org>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: lux <lx <at> shellcodes.org>, manikulin <at> gmail.com, 66390 <at> debbugs.gnu.org,
 michael.albinus <at> gmx.de
Subject: Re: bug#66390: `man' allows to inject arbitrary shell code
Date: Mon, 09 Oct 2023 19:20:03 +0200

On Okt 09 2023, Eli Zaretskii wrote:

>> From: lux <lx <at> shellcodes.org>
>> Cc: 66390 <at> debbugs.gnu.org, michael.albinus <at> gmx.de
>> Date: Tue, 10 Oct 2023 00:30:06 +0800
>> 
>> There is indeed an code injection vulnerability issue here, for example:
>> 
>>   (man ";ls")    <-- The `ls' command will be executed.
>
> So does this:
>
>   (shell-command "ls")

shell-command does what it is supposed to do.  man, on the other hand,
is supposed to display a manpage, _not_ execute an arbitrary command
line.  While the doc string of the man command says that it runs a
command to do its work, it does not explain how man-args is related to
that command.

-- 
Andreas Schwab, schwab <at> linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

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

From: lux <lx <at> shellcodes.org>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: manikulin <at> gmail.com, 66390 <at> debbugs.gnu.org, michael.albinus <at> gmx.de
Subject: Re: bug#66390: `man' allows to inject arbitrary shell code
Date: Tue, 10 Oct 2023 10:47:17 +0800

On Mon, 2023-10-09 at 19:48 +0300, Eli Zaretskii wrote:
> > From: lux <lx <at> shellcodes.org>
> > Cc: 66390 <at> debbugs.gnu.org, michael.albinus <at> gmx.de
> > Date: Tue, 10 Oct 2023 00:30:06 +0800
> > 
> > There is indeed an code injection vulnerability issue here, for example:
> > 
> >   (man ";ls")    <-- The `ls' command will be executed.
> 
> So does this:
> 
>   (shell-command "ls")
> 
> Does it mean we will disallow shell-command? or forcibly quote every
> shell command?  We cannot do that.
> 
> 

The responsibilities of the `shell-command' are clear, execute string COMMAND in
inferior shell, But `man' not is, we cannot describe `man' as being "Get a Un*x
manual page and put it in a buffer. But sometime can by the way execute shell
code."

For filenames, the "(", ")", and ";" characters all work. I think we should be
able to handle them correctly, or described in the docstring.

Message #80 received at 66390 <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: manikulin <at> gmail.com, 66390 <at> debbugs.gnu.org, michael.albinus <at> gmx.de
Subject: Re: bug#66390: `man' allows to inject arbitrary shell code
Date: Tue, 10 Oct 2023 07:43:00 +0000

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

> what happens with command (man "[") in this case?

It works fine here with that patch.  IOW, I get the expected man page

     test, [ – condition evaluation utility

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

From: Max Nikulin <manikulin <at> gmail.com>
To: lux <lx <at> shellcodes.org>, Eli Zaretskii <eliz <at> gnu.org>
Cc: 66390 <at> debbugs.gnu.org, michael.albinus <at> gmx.de
Subject: Re: bug#66390: `man' allows to inject arbitrary shell code
Date: Tue, 10 Oct 2023 17:54:59 +0700

On 09/10/2023 23:30, lux wrote:
> 
> Here's my patch and the test cases.

Thank you for your attempt to fix the issue. Unfortunately the proposed 
patch breaks the following case

   M-x man RET -k man RET

That is why I wrote that each word should escaped independently.

I am unsure if (man "-k man") should be supported as call with argument.

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

From: Max Nikulin <manikulin <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>, lux <lx <at> shellcodes.org>
Cc: 66390 <at> debbugs.gnu.org, michael.albinus <at> gmx.de
Subject: Re: bug#66390: `man' allows to inject arbitrary shell code
Date: Tue, 10 Oct 2023 18:09:43 +0700

On 09/10/2023 23:48, Eli Zaretskii wrote:
> And I ask again: what happens with command (man "[") in this case?

"sh" "-c" "man  [ 2>/dev/null | sed  -e '/^[\1-\32][\1-\32]*$/d' #...

so the code in man.el relies on "[" not interpreted as a special 
character when it is alone. It is not escaped!

Perhaps you are confused by the following commit

4ef9cc5a5de 2023-07-26 17:30:21 +0300 Eli Zaretskii: Fix "M-x man RET [ RET"

It affects completion, but not M-x man RET [ RET. (And I am surprised 
that "@" is treated specially for some reason.)

> Please believe me: this is not simple.  There's more here than meets
> the eye.  In addition to all kinds of weird characters in man-page
> names, you also need to consider SEE ALSO links from one man page to
> another, which can cross lines and include dashes and whitespace.
> Etc. etc...  I had my share of messing with this code, and one thing I
> know is that nothing is ever as simple as quoting here.

References split across lines should be handled by the code that 
creates/opens references, not by `man'. `man' should receive cleaned up 
references. (Cross-references is a case when properly implemented roff 
parser has advantages over dealing with text formatted for tty.)

If you believe that other packages must not call `man' then this 
function should not have an argument since it is a part of public interface.

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

From: Richard Stallman <rms <at> gnu.org>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: manikulin <at> gmail.com, 66390 <at> debbugs.gnu.org, michael.albinus <at> gmx.de
Subject: Re: bug#66390: `man' allows to inject arbitrary shell code
Date: Tue, 10 Oct 2023 07:56:32 -0400

[[[ To any NSA and FBI agents reading my email: please consider    ]]]
[[[ whether defending the US Constitution against all enemies,     ]]]
[[[ foreign or domestic, requires you to follow Snowden's example. ]]]

  > We don't retrofit fixes into old branches of Emacs that are no longer
  > developed; 

In general, that is a reasonable policy -- but maybe a serious
security problem, which this eesms to be, calls for special treatment.

               we leave that to the distros (who maintain old Emacs
  > versions for many more years than we do).

That might be sufficient for the problem, but we should think
carefully about whether it _is_ sufficient.

-- 
Dr Richard Stallman (https://stallman.org)
Chief GNUisance of the GNU Project (https://gnu.org)
Founder, Free Software Foundation (https://fsf.org)
Internet Hall-of-Famer (https://internethalloffame.org)

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Stefan Kangas <stefankangas <at> gmail.com>
Cc: lx <at> shellcodes.org, manikulin <at> gmail.com, 66390 <at> debbugs.gnu.org,
 michael.albinus <at> gmx.de
Subject: Re: bug#66390: `man' allows to inject arbitrary shell code
Date: Tue, 10 Oct 2023 15:11:25 +0300

> From: Stefan Kangas <stefankangas <at> gmail.com>
> Date: Tue, 10 Oct 2023 07:43:00 +0000
> Cc: manikulin <at> gmail.com, 66390 <at> debbugs.gnu.org, michael.albinus <at> gmx.de
> 
> Eli Zaretskii <eliz <at> gnu.org> writes:
> 
> > what happens with command (man "[") in this case?
> 
> It works fine here with that patch.  IOW, I get the expected man page
> 
>      test, [ – condition evaluation utility

Does it also work correctly in all the scenarios described in
bug#64795, including completion?

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

From: Stefan Kangas <stefankangas <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: lx <at> shellcodes.org, manikulin <at> gmail.com, 66390 <at> debbugs.gnu.org,
 michael.albinus <at> gmx.de
Subject: Re: bug#66390: `man' allows to inject arbitrary shell code
Date: Tue, 10 Oct 2023 12:25:42 +0000

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

> Does it also work correctly in all the scenarios described in
> bug#64795, including completion?

No, trying to complete there gives the prompt:

    Manual entry: [ [No match]

On the other hand this already seems broken in a different way in Emacs
29 on this macOS machine.  Trying to complete with:

    M-x man RET [ TAB TAB

leads to

    Manual entry: [: [Sole completion]

and RET at this point gives

    Can't find the [: manpage

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

From: lux <lx <at> shellcodes.org>
To: Max Nikulin <manikulin <at> gmail.com>, Eli Zaretskii <eliz <at> gnu.org>
Cc: 66390 <at> debbugs.gnu.org, michael.albinus <at> gmx.de
Subject: Re: bug#66390: `man' allows to inject arbitrary shell code
Date: Tue, 10 Oct 2023 22:30:03 +0800

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

On Tue, 2023-10-10 at 17:54 +0700, Max Nikulin wrote:
> On 09/10/2023 23:30, lux wrote:
> > 
> > Here's my patch and the test cases.
> 
> Thank you for your attempt to fix the issue. Unfortunately the proposed 
> patch breaks the following case
> 
>     M-x man RET -k man RET
> 
> That is why I wrote that each word should escaped independently.
> 
> I am unsure if (man "-k man") should be supported as call with argument.
> 
> 
> 

Thanks for the correction :-)

I am fix my patch, and test on Emacs 30.0.50 it's ok.

Stefan, Max, can you test it again?

[0001-Fix-man.el-code-injection-vulnerability.patch (text/x-patch, attachment)]

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

From: Andreas Schwab <schwab <at> linux-m68k.org>
To: lux <lx <at> shellcodes.org>
Cc: Max Nikulin <manikulin <at> gmail.com>, 66390 <at> debbugs.gnu.org,
 michael.albinus <at> gmx.de, Eli Zaretskii <eliz <at> gnu.org>
Subject: Re: bug#66390: `man' allows to inject arbitrary shell code
Date: Tue, 10 Oct 2023 18:21:34 +0200

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.

-- 
Andreas Schwab, schwab <at> linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

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

From: lux <lx <at> shellcodes.org>
To: Andreas Schwab <schwab <at> linux-m68k.org>
Cc: Max Nikulin <manikulin <at> gmail.com>, 66390 <at> debbugs.gnu.org,
 michael.albinus <at> gmx.de, Eli Zaretskii <eliz <at> gnu.org>
Subject: Re: bug#66390: `man' allows to inject arbitrary shell code
Date: Wed, 11 Oct 2023 11:08:34 +0800

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

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-+").

[0001-Fix-man.el-code-injection-vulnerability.patch (text/x-patch, attachment)]

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

From: Max Nikulin <manikulin <at> gmail.com>
To: lux <lx <at> shellcodes.org>, Andreas Schwab <schwab <at> linux-m68k.org>
Cc: Eli Zaretskii <eliz <at> gnu.org>, 66390 <at> debbugs.gnu.org, michael.albinus <at> gmx.de
Subject: Re: bug#66390: `man' allows to inject arbitrary shell code
Date: Wed, 11 Oct 2023 17:46:20 +0700

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.

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

From: Max Nikulin <manikulin <at> gmail.com>
To: rms <at> gnu.org, Eli Zaretskii <eliz <at> gnu.org>
Cc: 66390 <at> debbugs.gnu.org, michael.albinus <at> gmx.de
Subject: Re: bug#66390: `man' allows to inject arbitrary shell code
Date: Wed, 11 Oct 2023 17:56:11 +0700

On 10/10/2023 18:56, Richard Stallman wrote:
> In general, that is a reasonable policy -- but maybe a serious security 
> problem, which this eesms to be, calls for special treatment.

I would not consider this particular issue as a serious security problem 
despite if reported as a CVE it may get high score. However, I believe, 
it should be addressed.

ol-man is not loaded by default.

Enough features for Org mode are convenient in case of trusted files, 
but close to dangerous when a user walks through a malicious file. There 
are some issues that requires significant amount of efforts to fix 
without ruining usability.

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

From: Stefan Kangas <stefankangas <at> gmail.com>
To: lux <lx <at> shellcodes.org>, Andreas Schwab <schwab <at> linux-m68k.org>
Cc: Max Nikulin <manikulin <at> gmail.com>, 66390 <at> debbugs.gnu.org,
 michael.albinus <at> gmx.de, Eli Zaretskii <eliz <at> gnu.org>
Subject: Re: bug#66390: `man' allows to inject arbitrary shell code
Date: Fri, 20 Oct 2023 14:00:50 -0700

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

> 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-+").

I lost track of this discussion a little bit, but I think we should
try to have this fixed in Emacs 29.2.

Is the below patch acceptable?

> From faa49ba78a203d47740280e5c6fd0e075628b507 Mon Sep 17 00:00:00 2001
> From: Xi Lu <lx <at> shellcodes.org>
> Date: Tue, 10 Oct 2023 22:20:05 +0800
> Subject: [PATCH] Fix man.el code injection vulnerability.
>
> * lisp/man.el (Man-translate-references): Fix code injection.
> * test/lisp/man-tests.el (man-tests-Man-translate-references): New.
> ---
>  lisp/man.el            |  6 +++++-
>  test/lisp/man-tests.el | 12 ++++++++++++
>  2 files changed, 17 insertions(+), 1 deletion(-)
>
> diff --git a/lisp/man.el b/lisp/man.el
> index 506d6060269..a95435c7ea0 100644
> --- a/lisp/man.el
> +++ b/lisp/man.el
> @@ -692,7 +692,11 @@ Man-translate-references
>        (setq name (match-string 2 ref)
>  	    section (match-string 1 ref))))
>      (if (string= name "")
> -	ref				; Return the reference as is
> +        ;; see Bug#66390
> +	(mapconcat 'identity
> +                   (mapcar #'shell-quote-argument
> +                           (split-string ref "\\s-+"))
> +                   " ")                 ; Return the reference as is
>        (if Man-downcase-section-letters-flag
>  	  (setq section (downcase section)))
>        (while slist
> diff --git a/test/lisp/man-tests.el b/test/lisp/man-tests.el
> index e3657d7df8a..1c6dcb63a5c 100644
> --- a/test/lisp/man-tests.el
> +++ b/test/lisp/man-tests.el
> @@ -161,6 +161,18 @@ man-bgproc-filter-buttonize-includes
>            (let ((button (button-at (match-beginning 0))))
>              (should (and button (eq 'Man-xref-header-file (button-type button))))))))))
>
> +(ert-deftest man-tests-Man-translate-references ()
> +  (should (equal (Man-translate-references "basename")
> +                 "basename"))
> +  (should (equal (Man-translate-references "basename(3)")
> +                 "3 basename"))
> +  (should (equal (Man-translate-references "basename(3v)")
> +                 "3v basename"))
> +  (should (equal (Man-translate-references ";id")
> +                 "\\;id"))
> +  (should (equal (Man-translate-references "-k basename")
> +                 "-k basename")))
> +
>  (provide 'man-tests)
>
>  ;;; man-tests.el ends here
> --
> 2.42.0

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Stefan Kangas <stefankangas <at> gmail.com>
Cc: lx <at> shellcodes.org, manikulin <at> gmail.com, 66390 <at> debbugs.gnu.org,
 schwab <at> linux-m68k.org, michael.albinus <at> gmx.de
Subject: Re: bug#66390: `man' allows to inject arbitrary shell code
Date: Sat, 21 Oct 2023 10:19:58 +0300

> From: Stefan Kangas <stefankangas <at> gmail.com>
> Date: Fri, 20 Oct 2023 14:00:50 -0700
> Cc: Max Nikulin <manikulin <at> gmail.com>, 66390 <at> debbugs.gnu.org, michael.albinus <at> gmx.de, 
> 	Eli Zaretskii <eliz <at> gnu.org>
> 
> lux <lx <at> shellcodes.org> writes:
> 
> > 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-+").
> 
> I lost track of this discussion a little bit, but I think we should
> try to have this fixed in Emacs 29.2.

If we have a reliable solution (a hard-to-satisfy condition, see
below), yes.

> Is the below patch acceptable?

I'm not sure it is reliable enough.  man.el is an extremely tricky
package wrt the weird file names it must support (because many man
pages have weird names and include characters that are not normally
found in file names).  In particular, who can guarantee that ';' will
not be part of some man page some day? it's a valid file-name
character on Posix hosts, isn't it?

So I would be happier with installing this on master instead.
Distros which consider this a serious vulnerability can always
cherry-pick the change in their Emacs 29 distributions.

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

From: Andreas Schwab <schwab <at> linux-m68k.org>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: lx <at> shellcodes.org, manikulin <at> gmail.com, 66390 <at> debbugs.gnu.org,
 michael.albinus <at> gmx.de, Stefan Kangas <stefankangas <at> gmail.com>
Subject: Re: bug#66390: `man' allows to inject arbitrary shell code
Date: Sat, 21 Oct 2023 09:35:38 +0200

On Okt 21 2023, Eli Zaretskii wrote:

> found in file names).  In particular, who can guarantee that ';' will
> not be part of some man page some day? it's a valid file-name
> character on Posix hosts, isn't it?

It's not part of the Portable Filename Character Set.

-- 
Andreas Schwab, schwab <at> linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Andreas Schwab <schwab <at> linux-m68k.org>
Cc: lx <at> shellcodes.org, manikulin <at> gmail.com, 66390 <at> debbugs.gnu.org,
 michael.albinus <at> gmx.de, stefankangas <at> gmail.com
Subject: Re: bug#66390: `man' allows to inject arbitrary shell code
Date: Sat, 21 Oct 2023 10:45:06 +0300

> From: Andreas Schwab <schwab <at> linux-m68k.org>
> Cc: Stefan Kangas <stefankangas <at> gmail.com>,  lx <at> shellcodes.org,
>   manikulin <at> gmail.com,  66390 <at> debbugs.gnu.org,  michael.albinus <at> gmx.de
> Date: Sat, 21 Oct 2023 09:35:38 +0200
> 
> On Okt 21 2023, Eli Zaretskii wrote:
> 
> > found in file names).  In particular, who can guarantee that ';' will
> > not be part of some man page some day? it's a valid file-name
> > character on Posix hosts, isn't it?
> 
> It's not part of the Portable Filename Character Set.

That's true, but neither are ':' or '[', and AFAIK we already have
man-page file names which use those two.

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

From: Stefan Kangas <stefankangas <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>, Andreas Schwab <schwab <at> linux-m68k.org>
Cc: lx <at> shellcodes.org, manikulin <at> gmail.com, 66390 <at> debbugs.gnu.org,
 michael.albinus <at> gmx.de
Subject: Re: bug#66390: `man' allows to inject arbitrary shell code
Date: Sat, 21 Oct 2023 02:19:07 -0700

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

> That's true, but neither are ':' or '[', and AFAIK we already have
> man-page file names which use those two.

Perhaps we should add tests for man pages with such characters.

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

From: Stefan Kangas <stefankangas <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: lx <at> shellcodes.org, Ihor Radchenko <yantar92 <at> posteo.net>,
 66390 <at> debbugs.gnu.org, schwab <at> linux-m68k.org, michael.albinus <at> gmx.de,
 manikulin <at> gmail.com
Subject: Re: bug#66390: `man' allows to inject arbitrary shell code
Date: Wed, 10 Jan 2024 13:21:09 -0800

tags 66390 + security
close 66390 30.1
thanks

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

>> From: Stefan Kangas <stefankangas <at> gmail.com>
>>
>> I lost track of this discussion a little bit, but I think we should
>> try to have this fixed in Emacs 29.2.
>
> If we have a reliable solution (a hard-to-satisfy condition, see
> below), yes.
>
>> Is the below patch acceptable?
>
> I'm not sure it is reliable enough.  man.el is an extremely tricky
> package wrt the weird file names it must support (because many man
> pages have weird names and include characters that are not normally
> found in file names).  In particular, who can guarantee that ';' will
> not be part of some man page some day? it's a valid file-name
> character on Posix hosts, isn't it?
>
> So I would be happier with installing this on master instead.
> Distros which consider this a serious vulnerability can always
> cherry-pick the change in their Emacs 29 distributions.

OK, I've now installed the change on master (820f0793f0b).  I'm tagging
the bug "security" to make it easier to find for distro maintainers.

Ihor, I'm copying in you as well, in case you want to add a workaround
for this security-relevant bug to Org mode as well.  AFAIU, org mode
man:// links are vulnerable to a shell injection vulnerability in all
released versions of Emacs, and will continue to be so for users until
they upgrade to 30.1.  See this bug for details.  (Bug#66390)

I'm closing the bug with this message.

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

From: Ihor Radchenko <yantar92 <at> posteo.net>
To: Stefan Kangas <stefankangas <at> gmail.com>
Cc: lx <at> shellcodes.org, 66390 <at> debbugs.gnu.org, schwab <at> linux-m68k.org,
 michael.albinus <at> gmx.de, Eli Zaretskii <eliz <at> gnu.org>, manikulin <at> gmail.com
Subject: Re: bug#66390: `man' allows to inject arbitrary shell code
Date: Thu, 11 Jan 2024 12:07:39 +0000

Stefan Kangas <stefankangas <at> gmail.com> writes:

> OK, I've now installed the change on master (820f0793f0b).  I'm tagging
> the bug "security" to make it easier to find for distro maintainers.
>
> Ihor, I'm copying in you as well, in case you want to add a workaround
> for this security-relevant bug to Org mode as well.  AFAIU, org mode
> man:// links are vulnerable to a shell injection vulnerability in all
> released versions of Emacs, and will continue to be so for users until
> they upgrade to 30.1.  See this bug for details.  (Bug#66390)

Fixed, on bugfix (for the next Org bugfix release).
https://git.savannah.gnu.org/cgit/emacs/org-mode.git/commit/?id=bc3caa8f9

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

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

From: Max Nikulin <manikulin <at> gmail.com>
To: Ihor Radchenko <yantar92 <at> posteo.net>,
 Stefan Kangas <stefankangas <at> gmail.com>
Cc: lx <at> shellcodes.org, Eli Zaretskii <eliz <at> gnu.org>, 66390 <at> debbugs.gnu.org,
 schwab <at> linux-m68k.org, michael.albinus <at> gmx.de
Subject: Re: bug#66390: `man' allows to inject arbitrary shell code
Date: Thu, 11 Jan 2024 21:34:43 +0700

On 11/01/2024 19:07, Ihor Radchenko wrote:
> Stefan Kangas writes:
> 
>> OK, I've now installed the change on master (820f0793f0b).  I'm tagging
>> the bug "security" to make it easier to find for distro maintainers.
>>
>> Ihor, I'm copying in you as well, in case you want to add a workaround
>> for this security-relevant bug to Org mode as well.
[...]
> Fixed, on bugfix (for the next Org bugfix release).
> https://git.savannah.gnu.org/cgit/emacs/org-mode.git/commit/?id=bc3caa8f9

Ihor, I am confused by `org-man-store-link' call in

(if (org-man-store-link (equal (Man-translate-references ";id") "\\;id"))

Is it intentional? I hope, the following is not an issue:

(let ((system-type 'ms-dos))
  (shell-quote-argument ";id"))
"\";id\""

no "\\;id"

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

From: Ihor Radchenko <yantar92 <at> posteo.net>
To: Max Nikulin <manikulin <at> gmail.com>
Cc: lx <at> shellcodes.org, 66390 <at> debbugs.gnu.org, schwab <at> linux-m68k.org,
 Stefan Kangas <stefankangas <at> gmail.com>, michael.albinus <at> gmx.de,
 Eli Zaretskii <eliz <at> gnu.org>
Subject: Re: bug#66390: `man' allows to inject arbitrary shell code
Date: Thu, 11 Jan 2024 15:07:30 +0000

Max Nikulin <manikulin <at> gmail.com> writes:

>> Fixed, on bugfix (for the next Org bugfix release).
>> https://git.savannah.gnu.org/cgit/emacs/org-mode.git/commit/?id=bc3caa8f9
>
> Ihor, I am confused by `org-man-store-link' call in
>
> (if (org-man-store-link (equal (Man-translate-references ";id") "\\;id"))
>
> Is it intentional? I hope, the following is not an issue:

That was a typo.
I fixed it in
https://git.savannah.gnu.org/cgit/emacs/org-mode.git/commit/?id=6b60b5ac12814f0b54209cb6417f9ad1709dce20

> (let ((system-type 'ms-dos))
>    (shell-quote-argument ";id"))
> "\";id\""
>
> no "\\;id"

I took this test from the tests introduced in
https://git.savannah.gnu.org/cgit/emacs.git/commit/?id=820f0793f0b

Stefan, do I miss something or will the tests fail in Dos/Windows builds?

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Ihor Radchenko <yantar92 <at> posteo.net>
Cc: lx <at> shellcodes.org, 66390 <at> debbugs.gnu.org, schwab <at> linux-m68k.org,
 stefankangas <at> gmail.com, michael.albinus <at> gmx.de, manikulin <at> gmail.com
Subject: Re: bug#66390: `man' allows to inject arbitrary shell code
Date: Thu, 11 Jan 2024 17:28:42 +0200

> From: Ihor Radchenko <yantar92 <at> posteo.net>
> Cc: Stefan Kangas <stefankangas <at> gmail.com>, Eli Zaretskii <eliz <at> gnu.org>,
>  lx <at> shellcodes.org, 66390 <at> debbugs.gnu.org, schwab <at> linux-m68k.org,
>  michael.albinus <at> gmx.de
> Date: Thu, 11 Jan 2024 15:07:30 +0000
> 
> Max Nikulin <manikulin <at> gmail.com> writes:
> 
> > (let ((system-type 'ms-dos))
> >    (shell-quote-argument ";id"))
> > "\";id\""
> >
> > no "\\;id"
> 
> I took this test from the tests introduced in
> https://git.savannah.gnu.org/cgit/emacs.git/commit/?id=820f0793f0b
> 
> Stefan, do I miss something or will the tests fail in Dos/Windows builds?

Yes, it failed.  I fixed it now.

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

From: Ihor Radchenko <yantar92 <at> posteo.net>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: lx <at> shellcodes.org, 66390 <at> debbugs.gnu.org, schwab <at> linux-m68k.org,
 stefankangas <at> gmail.com, michael.albinus <at> gmx.de, manikulin <at> gmail.com
Subject: Re: bug#66390: `man' allows to inject arbitrary shell code
Date: Thu, 11 Jan 2024 15:37:14 +0000

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

>> Stefan, do I miss something or will the tests fail in Dos/Windows builds?
>
> Yes, it failed.  I fixed it now.

I now also adjusted the Org mode's workaround.
https://git.savannah.gnu.org/cgit/emacs/org-mode.git/commit/?id=4145ee421

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

Previous Next

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