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

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

[Maxim Nikulin]

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.

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

[Eli Zaretskii]

> From: Maxim Nikulin <m.a.nikulin@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.

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

[Max Nikulin]

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.

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

[Eli Zaretskii]

> Date: Sat, 7 Oct 2023 21:12:54 +0700
> Cc: 66390@debbugs.gnu.org
> From: Max Nikulin <manikulin@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.

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

[Max Nikulin]

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.

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

[Eli Zaretskii]

> Date: Sat, 7 Oct 2023 21:29:12 +0700
> Cc: 66390@debbugs.gnu.org
> From: Max Nikulin <manikulin@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.

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

[Michael Albinus]

Max Nikulin <manikulin@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.

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

[Eli Zaretskii]

> From: Michael Albinus <michael.albinus@gmx.de>
> Cc: Eli Zaretskii <eliz@gnu.org>,  66390@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.

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

[Michael Albinus]

Eli Zaretskii <eliz@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.

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

[Eli Zaretskii]

> From: Michael Albinus <michael.albinus@gmx.de>
> Cc: manikulin@gmail.com,  66390@debbugs.gnu.org
> Date: Sat, 07 Oct 2023 18:55:01 +0200
> 
> Eli Zaretskii <eliz@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.

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

[Michael Albinus]

Eli Zaretskii <eliz@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.

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

[Eli Zaretskii]

> From: Michael Albinus <michael.albinus@gmx.de>
> Cc: manikulin@gmail.com,  66390@debbugs.gnu.org
> Date: Sat, 07 Oct 2023 19:45:18 +0200
> 
> Eli Zaretskii <eliz@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.

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

[Max Nikulin]

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.

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

[Maxim Nikulin]

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@posteo.net>
> Cc: emacs-orgmode@gnu.org, emacs-devel@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.

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

[Eli Zaretskii]

> Date: Sun, 8 Oct 2023 10:42:03 +0700
> Cc: 66390@debbugs.gnu.org
> From: Maxim Nikulin <manikulin+gzh@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.

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'.

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

[Richard Stallman]

[[[ 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)

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

[Eli Zaretskii]

> From: Richard Stallman <rms@gnu.org>
> Cc: michael.albinus@gmx.de, manikulin@gmail.com, 66390@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.)

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

[Max Nikulin]

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"

?

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

[Eli Zaretskii]

> Date: Mon, 9 Oct 2023 22:12:34 +0700
> Cc: michael.albinus@gmx.de, 66390@debbugs.gnu.org
> From: Max Nikulin <manikulin@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"

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

[lux]

[-- Attachment #1: Type: text/plain, Size: 2722 bytes --]

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.

[-- Attachment #2: 0001-Fix-man.el-code-injection-vulnerability.patch --]
[-- Type: text/x-patch, Size: 1760 bytes --]

From 1c2905d93d3cba966a7d244f4c278c720ceff378 Mon Sep 17 00:00:00 2001
From: Xi Lu <lx@shellcodes.org>
Date: Tue, 10 Oct 2023 00:21:31 +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            |  2 +-
 test/lisp/man-tests.el | 10 ++++++++++
 2 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/lisp/man.el b/lisp/man.el
index 286edf9314e..43839959622 100644
--- a/lisp/man.el
+++ b/lisp/man.el
@@ -684,7 +684,7 @@ Man-translate-references
       (setq name (match-string 2 ref)
 	    section (match-string 1 ref))))
     (if (string= name "")
-	ref				; Return the reference as is
+	(shell-quote-argument ref)      ; 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..48570967a09 100644
--- a/test/lisp/man-tests.el
+++ b/test/lisp/man-tests.el
@@ -161,6 +161,16 @@ 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")))
+
 (provide 'man-tests)
 
 ;;; man-tests.el ends here
-- 
2.42.0

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

[Eli Zaretskii]

> From: lux <lx@shellcodes.org>
> Cc: 66390@debbugs.gnu.org, michael.albinus@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.

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

[Ihor Radchenko]

Eli Zaretskii <eliz@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>

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

[Andreas Schwab]

On Okt 09 2023, Eli Zaretskii wrote:

>> From: lux <lx@shellcodes.org>
>> Cc: 66390@debbugs.gnu.org, michael.albinus@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@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

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

[lux]

On Mon, 2023-10-09 at 19:48 +0300, Eli Zaretskii wrote:
> > From: lux <lx@shellcodes.org>
> > Cc: 66390@debbugs.gnu.org, michael.albinus@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.

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

[Stefan Kangas]

Eli Zaretskii <eliz@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

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

[Max Nikulin]

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.

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

[Max Nikulin]

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.

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

[Richard Stallman]

[[[ 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)

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

[Eli Zaretskii]

> From: Stefan Kangas <stefankangas@gmail.com>
> Date: Tue, 10 Oct 2023 07:43:00 +0000
> Cc: manikulin@gmail.com, 66390@debbugs.gnu.org, michael.albinus@gmx.de
> 
> Eli Zaretskii <eliz@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?

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

[Stefan Kangas]

Eli Zaretskii <eliz@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

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

[lux]

[-- Attachment #1: Type: text/plain, Size: 582 bytes --]

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?

[-- Attachment #2: 0001-Fix-man.el-code-injection-vulnerability.patch --]
[-- Type: text/x-patch, Size: 2019 bytes --]

From c1989c15171a4a40dcf6f9bfbf2975c0b7895dd2 Mon Sep 17 00:00:00 2001
From: Xi Lu <lx@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..9d8b3a6cf2d 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 " "))
+                   " ")                 ; 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

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

[Andreas Schwab]

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@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

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

[lux]

[-- Attachment #1: Type: text/plain, Size: 475 bytes --]

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



[-- Attachment #2: 0001-Fix-man.el-code-injection-vulnerability.patch --]
[-- Type: text/x-patch, Size: 2023 bytes --]

From faa49ba78a203d47740280e5c6fd0e075628b507 Mon Sep 17 00:00:00 2001
From: Xi Lu <lx@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

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.

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

[Max Nikulin]

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.

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

[Stefan Kangas]

lux <lx@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@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

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

[Eli Zaretskii]

> From: Stefan Kangas <stefankangas@gmail.com>
> Date: Fri, 20 Oct 2023 14:00:50 -0700
> Cc: Max Nikulin <manikulin@gmail.com>, 66390@debbugs.gnu.org, michael.albinus@gmx.de, 
> 	Eli Zaretskii <eliz@gnu.org>
> 
> lux <lx@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.

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

[Andreas Schwab]

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@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

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

[Eli Zaretskii]

> From: Andreas Schwab <schwab@linux-m68k.org>
> Cc: Stefan Kangas <stefankangas@gmail.com>,  lx@shellcodes.org,
>   manikulin@gmail.com,  66390@debbugs.gnu.org,  michael.albinus@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.

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

[Stefan Kangas]

Eli Zaretskii <eliz@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.

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

[Stefan Kangas]

tags 66390 + security
close 66390 30.1
thanks

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Stefan Kangas <stefankangas@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.

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

[Ihor Radchenko]

Stefan Kangas <stefankangas@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>

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

[Max Nikulin]

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"

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

[Ihor Radchenko]

Max Nikulin <manikulin@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>

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

[Eli Zaretskii]

> From: Ihor Radchenko <yantar92@posteo.net>
> Cc: Stefan Kangas <stefankangas@gmail.com>, Eli Zaretskii <eliz@gnu.org>,
>  lx@shellcodes.org, 66390@debbugs.gnu.org, schwab@linux-m68k.org,
>  michael.albinus@gmx.de
> Date: Thu, 11 Jan 2024 15:07:30 +0000
> 
> Max Nikulin <manikulin@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.

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

[Ihor Radchenko]

Eli Zaretskii <eliz@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>