unofficial mirror of bug-gnu-emacs@gnu.org 
 help / color / mirror / code / Atom feed
* bug#66390: `man' allows to inject arbitrary shell code
@ 2023-10-07 12:47 Maxim Nikulin
  2023-10-07 13:04 ` Eli Zaretskii
  0 siblings, 1 reply; 47+ messages in thread
From: Maxim Nikulin @ 2023-10-07 12:47 UTC (permalink / raw)
  To: 66390

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.





^ permalink raw reply	[flat|nested] 47+ messages in thread

* bug#66390: `man' allows to inject arbitrary shell code
  2023-10-07 12:47 bug#66390: `man' allows to inject arbitrary shell code Maxim Nikulin
@ 2023-10-07 13:04 ` Eli Zaretskii
  2023-10-07 14:12   ` Max Nikulin
  0 siblings, 1 reply; 47+ messages in thread
From: Eli Zaretskii @ 2023-10-07 13:04 UTC (permalink / raw)
  To: Maxim Nikulin; +Cc: 66390

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





^ permalink raw reply	[flat|nested] 47+ messages in thread

* bug#66390: `man' allows to inject arbitrary shell code
  2023-10-07 13:04 ` Eli Zaretskii
@ 2023-10-07 14:12   ` Max Nikulin
  2023-10-07 14:19     ` Eli Zaretskii
  0 siblings, 1 reply; 47+ messages in thread
From: Max Nikulin @ 2023-10-07 14:12 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: 66390

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.





^ permalink raw reply	[flat|nested] 47+ messages in thread

* bug#66390: `man' allows to inject arbitrary shell code
  2023-10-07 14:12   ` Max Nikulin
@ 2023-10-07 14:19     ` Eli Zaretskii
  2023-10-07 14:29       ` Max Nikulin
  0 siblings, 1 reply; 47+ messages in thread
From: Eli Zaretskii @ 2023-10-07 14:19 UTC (permalink / raw)
  To: Max Nikulin; +Cc: 66390

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





^ permalink raw reply	[flat|nested] 47+ messages in thread

* bug#66390: `man' allows to inject arbitrary shell code
  2023-10-07 14:19     ` Eli Zaretskii
@ 2023-10-07 14:29       ` Max Nikulin
  2023-10-07 15:10         ` Eli Zaretskii
  2023-10-07 15:37         ` Michael Albinus
  0 siblings, 2 replies; 47+ messages in thread
From: Max Nikulin @ 2023-10-07 14:29 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: 66390

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.





^ permalink raw reply	[flat|nested] 47+ messages in thread

* bug#66390: `man' allows to inject arbitrary shell code
  2023-10-07 14:29       ` Max Nikulin
@ 2023-10-07 15:10         ` Eli Zaretskii
  2023-10-07 15:37         ` Michael Albinus
  1 sibling, 0 replies; 47+ messages in thread
From: Eli Zaretskii @ 2023-10-07 15:10 UTC (permalink / raw)
  To: Max Nikulin; +Cc: 66390

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





^ permalink raw reply	[flat|nested] 47+ messages in thread

* bug#66390: `man' allows to inject arbitrary shell code
  2023-10-07 14:29       ` Max Nikulin
  2023-10-07 15:10         ` Eli Zaretskii
@ 2023-10-07 15:37         ` Michael Albinus
  2023-10-07 15:58           ` Eli Zaretskii
  1 sibling, 1 reply; 47+ messages in thread
From: Michael Albinus @ 2023-10-07 15:37 UTC (permalink / raw)
  To: Max Nikulin; +Cc: Eli Zaretskii, 66390

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.





^ permalink raw reply	[flat|nested] 47+ messages in thread

* bug#66390: `man' allows to inject arbitrary shell code
  2023-10-07 15:37         ` Michael Albinus
@ 2023-10-07 15:58           ` Eli Zaretskii
  2023-10-07 16:55             ` Michael Albinus
  0 siblings, 1 reply; 47+ messages in thread
From: Eli Zaretskii @ 2023-10-07 15:58 UTC (permalink / raw)
  To: Michael Albinus; +Cc: manikulin, 66390

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





^ permalink raw reply	[flat|nested] 47+ messages in thread

* bug#66390: `man' allows to inject arbitrary shell code
  2023-10-07 15:58           ` Eli Zaretskii
@ 2023-10-07 16:55             ` Michael Albinus
  2023-10-07 17:24               ` Eli Zaretskii
  0 siblings, 1 reply; 47+ messages in thread
From: Michael Albinus @ 2023-10-07 16:55 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: manikulin, 66390

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.





^ permalink raw reply	[flat|nested] 47+ messages in thread

* bug#66390: `man' allows to inject arbitrary shell code
  2023-10-07 16:55             ` Michael Albinus
@ 2023-10-07 17:24               ` Eli Zaretskii
  2023-10-07 17:45                 ` Michael Albinus
  2023-10-08  3:42                 ` Maxim Nikulin
  0 siblings, 2 replies; 47+ messages in thread
From: Eli Zaretskii @ 2023-10-07 17:24 UTC (permalink / raw)
  To: Michael Albinus; +Cc: manikulin, 66390

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





^ permalink raw reply	[flat|nested] 47+ messages in thread

* bug#66390: `man' allows to inject arbitrary shell code
  2023-10-07 17:24               ` Eli Zaretskii
@ 2023-10-07 17:45                 ` Michael Albinus
  2023-10-07 18:26                   ` Eli Zaretskii
  2023-10-08  3:42                 ` Maxim Nikulin
  1 sibling, 1 reply; 47+ messages in thread
From: Michael Albinus @ 2023-10-07 17:45 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: manikulin, 66390

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.





^ permalink raw reply	[flat|nested] 47+ messages in thread

* bug#66390: `man' allows to inject arbitrary shell code
  2023-10-07 17:45                 ` Michael Albinus
@ 2023-10-07 18:26                   ` Eli Zaretskii
  2023-10-08  3:37                     ` Max Nikulin
  2023-10-09  2:36                     ` Richard Stallman
  0 siblings, 2 replies; 47+ messages in thread
From: Eli Zaretskii @ 2023-10-07 18:26 UTC (permalink / raw)
  To: Michael Albinus; +Cc: manikulin, 66390

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





^ permalink raw reply	[flat|nested] 47+ messages in thread

* bug#66390: `man' allows to inject arbitrary shell code
  2023-10-07 18:26                   ` Eli Zaretskii
@ 2023-10-08  3:37                     ` Max Nikulin
  2023-10-08  5:28                       ` Eli Zaretskii
  2023-10-09  2:36                     ` Richard Stallman
  1 sibling, 1 reply; 47+ messages in thread
From: Max Nikulin @ 2023-10-08  3:37 UTC (permalink / raw)
  To: Eli Zaretskii, Michael Albinus; +Cc: 66390

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.





^ permalink raw reply	[flat|nested] 47+ messages in thread

* bug#66390: `man' allows to inject arbitrary shell code
  2023-10-07 17:24               ` Eli Zaretskii
  2023-10-07 17:45                 ` Michael Albinus
@ 2023-10-08  3:42                 ` Maxim Nikulin
  2023-10-08  5:20                   ` Eli Zaretskii
  1 sibling, 1 reply; 47+ messages in thread
From: Maxim Nikulin @ 2023-10-08  3:42 UTC (permalink / raw)
  To: Eli Zaretskii, Michael Albinus; +Cc: 66390

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.





^ permalink raw reply	[flat|nested] 47+ messages in thread

* bug#66390: `man' allows to inject arbitrary shell code
  2023-10-08  3:42                 ` Maxim Nikulin
@ 2023-10-08  5:20                   ` Eli Zaretskii
  0 siblings, 0 replies; 47+ messages in thread
From: Eli Zaretskii @ 2023-10-08  5:20 UTC (permalink / raw)
  To: Maxim Nikulin; +Cc: 66390, michael.albinus

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





^ permalink raw reply	[flat|nested] 47+ messages in thread

* bug#66390: `man' allows to inject arbitrary shell code
  2023-10-08  3:37                     ` Max Nikulin
@ 2023-10-08  5:28                       ` Eli Zaretskii
  2023-10-09 15:12                         ` Max Nikulin
  2023-10-09 16:30                         ` lux
  0 siblings, 2 replies; 47+ messages in thread
From: Eli Zaretskii @ 2023-10-08  5:28 UTC (permalink / raw)
  To: Max Nikulin; +Cc: 66390, michael.albinus

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





^ permalink raw reply	[flat|nested] 47+ messages in thread

* bug#66390: `man' allows to inject arbitrary shell code
  2023-10-07 18:26                   ` Eli Zaretskii
  2023-10-08  3:37                     ` Max Nikulin
@ 2023-10-09  2:36                     ` Richard Stallman
  2023-10-09 11:04                       ` Eli Zaretskii
  1 sibling, 1 reply; 47+ messages in thread
From: Richard Stallman @ 2023-10-09  2:36 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: manikulin, 66390, michael.albinus

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







^ permalink raw reply	[flat|nested] 47+ messages in thread

* bug#66390: `man' allows to inject arbitrary shell code
  2023-10-09  2:36                     ` Richard Stallman
@ 2023-10-09 11:04                       ` Eli Zaretskii
  2023-10-10 11:56                         ` Richard Stallman
  0 siblings, 1 reply; 47+ messages in thread
From: Eli Zaretskii @ 2023-10-09 11:04 UTC (permalink / raw)
  To: rms; +Cc: manikulin, 66390, michael.albinus

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





^ permalink raw reply	[flat|nested] 47+ messages in thread

* bug#66390: `man' allows to inject arbitrary shell code
  2023-10-08  5:28                       ` Eli Zaretskii
@ 2023-10-09 15:12                         ` Max Nikulin
  2023-10-09 15:52                           ` Eli Zaretskii
  2023-10-09 16:30                         ` lux
  1 sibling, 1 reply; 47+ messages in thread
From: Max Nikulin @ 2023-10-09 15:12 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: 66390, michael.albinus

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"

?





^ permalink raw reply	[flat|nested] 47+ messages in thread

* bug#66390: `man' allows to inject arbitrary shell code
  2023-10-09 15:12                         ` Max Nikulin
@ 2023-10-09 15:52                           ` Eli Zaretskii
  0 siblings, 0 replies; 47+ messages in thread
From: Eli Zaretskii @ 2023-10-09 15:52 UTC (permalink / raw)
  To: Max Nikulin; +Cc: 66390, michael.albinus

> 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"





^ permalink raw reply	[flat|nested] 47+ messages in thread

* bug#66390: `man' allows to inject arbitrary shell code
  2023-10-08  5:28                       ` Eli Zaretskii
  2023-10-09 15:12                         ` Max Nikulin
@ 2023-10-09 16:30                         ` lux
  2023-10-09 16:48                           ` Eli Zaretskii
  2023-10-10 10:54                           ` Max Nikulin
  1 sibling, 2 replies; 47+ messages in thread
From: lux @ 2023-10-09 16:30 UTC (permalink / raw)
  To: Eli Zaretskii, Max Nikulin; +Cc: 66390, michael.albinus

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


^ permalink raw reply related	[flat|nested] 47+ messages in thread

* bug#66390: `man' allows to inject arbitrary shell code
  2023-10-09 16:30                         ` lux
@ 2023-10-09 16:48                           ` Eli Zaretskii
  2023-10-09 17:07                             ` Ihor Radchenko
                                               ` (4 more replies)
  2023-10-10 10:54                           ` Max Nikulin
  1 sibling, 5 replies; 47+ messages in thread
From: Eli Zaretskii @ 2023-10-09 16:48 UTC (permalink / raw)
  To: lux; +Cc: manikulin, 66390, michael.albinus

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





^ permalink raw reply	[flat|nested] 47+ messages in thread

* bug#66390: `man' allows to inject arbitrary shell code
  2023-10-09 16:48                           ` Eli Zaretskii
@ 2023-10-09 17:07                             ` Ihor Radchenko
  2023-10-09 17:20                             ` Andreas Schwab
                                               ` (3 subsequent siblings)
  4 siblings, 0 replies; 47+ messages in thread
From: Ihor Radchenko @ 2023-10-09 17:07 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: lux, manikulin, 66390, michael.albinus

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>





^ permalink raw reply	[flat|nested] 47+ messages in thread

* bug#66390: `man' allows to inject arbitrary shell code
  2023-10-09 16:48                           ` Eli Zaretskii
  2023-10-09 17:07                             ` Ihor Radchenko
@ 2023-10-09 17:20                             ` Andreas Schwab
  2023-10-10  2:47                             ` lux
                                               ` (2 subsequent siblings)
  4 siblings, 0 replies; 47+ messages in thread
From: Andreas Schwab @ 2023-10-09 17:20 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: lux, manikulin, 66390, michael.albinus

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





^ permalink raw reply	[flat|nested] 47+ messages in thread

* bug#66390: `man' allows to inject arbitrary shell code
  2023-10-09 16:48                           ` Eli Zaretskii
  2023-10-09 17:07                             ` Ihor Radchenko
  2023-10-09 17:20                             ` Andreas Schwab
@ 2023-10-10  2:47                             ` lux
  2023-10-10  7:43                             ` Stefan Kangas
  2023-10-10 11:09                             ` Max Nikulin
  4 siblings, 0 replies; 47+ messages in thread
From: lux @ 2023-10-10  2:47 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: manikulin, 66390, michael.albinus

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.






^ permalink raw reply	[flat|nested] 47+ messages in thread

* bug#66390: `man' allows to inject arbitrary shell code
  2023-10-09 16:48                           ` Eli Zaretskii
                                               ` (2 preceding siblings ...)
  2023-10-10  2:47                             ` lux
@ 2023-10-10  7:43                             ` Stefan Kangas
  2023-10-10 12:11                               ` Eli Zaretskii
  2023-10-10 11:09                             ` Max Nikulin
  4 siblings, 1 reply; 47+ messages in thread
From: Stefan Kangas @ 2023-10-10  7:43 UTC (permalink / raw)
  To: Eli Zaretskii, lux; +Cc: manikulin, 66390, michael.albinus

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





^ permalink raw reply	[flat|nested] 47+ messages in thread

* bug#66390: `man' allows to inject arbitrary shell code
  2023-10-09 16:30                         ` lux
  2023-10-09 16:48                           ` Eli Zaretskii
@ 2023-10-10 10:54                           ` Max Nikulin
  2023-10-10 14:30                             ` lux
  1 sibling, 1 reply; 47+ messages in thread
From: Max Nikulin @ 2023-10-10 10:54 UTC (permalink / raw)
  To: lux, Eli Zaretskii; +Cc: 66390, michael.albinus

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.





^ permalink raw reply	[flat|nested] 47+ messages in thread

* bug#66390: `man' allows to inject arbitrary shell code
  2023-10-09 16:48                           ` Eli Zaretskii
                                               ` (3 preceding siblings ...)
  2023-10-10  7:43                             ` Stefan Kangas
@ 2023-10-10 11:09                             ` Max Nikulin
  4 siblings, 0 replies; 47+ messages in thread
From: Max Nikulin @ 2023-10-10 11:09 UTC (permalink / raw)
  To: Eli Zaretskii, lux; +Cc: 66390, michael.albinus

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.





^ permalink raw reply	[flat|nested] 47+ messages in thread

* bug#66390: `man' allows to inject arbitrary shell code
  2023-10-09 11:04                       ` Eli Zaretskii
@ 2023-10-10 11:56                         ` Richard Stallman
  2023-10-11 10:56                           ` Max Nikulin
  0 siblings, 1 reply; 47+ messages in thread
From: Richard Stallman @ 2023-10-10 11:56 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: manikulin, 66390, michael.albinus

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







^ permalink raw reply	[flat|nested] 47+ messages in thread

* bug#66390: `man' allows to inject arbitrary shell code
  2023-10-10  7:43                             ` Stefan Kangas
@ 2023-10-10 12:11                               ` Eli Zaretskii
  2023-10-10 12:25                                 ` Stefan Kangas
  0 siblings, 1 reply; 47+ messages in thread
From: Eli Zaretskii @ 2023-10-10 12:11 UTC (permalink / raw)
  To: Stefan Kangas; +Cc: lx, manikulin, 66390, michael.albinus

> 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?





^ permalink raw reply	[flat|nested] 47+ messages in thread

* bug#66390: `man' allows to inject arbitrary shell code
  2023-10-10 12:11                               ` Eli Zaretskii
@ 2023-10-10 12:25                                 ` Stefan Kangas
  0 siblings, 0 replies; 47+ messages in thread
From: Stefan Kangas @ 2023-10-10 12:25 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: lx, manikulin, 66390, michael.albinus

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





^ permalink raw reply	[flat|nested] 47+ messages in thread

* bug#66390: `man' allows to inject arbitrary shell code
  2023-10-10 10:54                           ` Max Nikulin
@ 2023-10-10 14:30                             ` lux
  2023-10-10 16:21                               ` Andreas Schwab
  0 siblings, 1 reply; 47+ messages in thread
From: lux @ 2023-10-10 14:30 UTC (permalink / raw)
  To: Max Nikulin, Eli Zaretskii; +Cc: 66390, michael.albinus

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


^ permalink raw reply related	[flat|nested] 47+ messages in thread

* bug#66390: `man' allows to inject arbitrary shell code
  2023-10-10 14:30                             ` lux
@ 2023-10-10 16:21                               ` Andreas Schwab
  2023-10-11  3:08                                 ` lux
  0 siblings, 1 reply; 47+ messages in thread
From: Andreas Schwab @ 2023-10-10 16:21 UTC (permalink / raw)
  To: lux; +Cc: Max Nikulin, 66390, michael.albinus, Eli Zaretskii

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





^ permalink raw reply	[flat|nested] 47+ messages in thread

* bug#66390: `man' allows to inject arbitrary shell code
  2023-10-10 16:21                               ` Andreas Schwab
@ 2023-10-11  3:08                                 ` lux
  2023-10-11 10:46                                   ` Max Nikulin
  2023-10-20 21:00                                   ` Stefan Kangas
  0 siblings, 2 replies; 47+ messages in thread
From: lux @ 2023-10-11  3:08 UTC (permalink / raw)
  To: Andreas Schwab; +Cc: Max Nikulin, 66390, michael.albinus, Eli Zaretskii

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


^ permalink raw reply related	[flat|nested] 47+ messages in thread

* bug#66390: `man' allows to inject arbitrary shell code
  2023-10-11  3:08                                 ` lux
@ 2023-10-11 10:46                                   ` Max Nikulin
  2023-10-20 21:00                                   ` Stefan Kangas
  1 sibling, 0 replies; 47+ messages in thread
From: Max Nikulin @ 2023-10-11 10:46 UTC (permalink / raw)
  To: lux, Andreas Schwab; +Cc: Eli Zaretskii, 66390, michael.albinus

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.





^ permalink raw reply	[flat|nested] 47+ messages in thread

* bug#66390: `man' allows to inject arbitrary shell code
  2023-10-10 11:56                         ` Richard Stallman
@ 2023-10-11 10:56                           ` Max Nikulin
  0 siblings, 0 replies; 47+ messages in thread
From: Max Nikulin @ 2023-10-11 10:56 UTC (permalink / raw)
  To: rms, Eli Zaretskii; +Cc: 66390, michael.albinus

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.





^ permalink raw reply	[flat|nested] 47+ messages in thread

* bug#66390: `man' allows to inject arbitrary shell code
  2023-10-11  3:08                                 ` lux
  2023-10-11 10:46                                   ` Max Nikulin
@ 2023-10-20 21:00                                   ` Stefan Kangas
  2023-10-21  7:19                                     ` Eli Zaretskii
  1 sibling, 1 reply; 47+ messages in thread
From: Stefan Kangas @ 2023-10-20 21:00 UTC (permalink / raw)
  To: lux, Andreas Schwab; +Cc: Max Nikulin, 66390, michael.albinus, Eli Zaretskii

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





^ permalink raw reply	[flat|nested] 47+ messages in thread

* bug#66390: `man' allows to inject arbitrary shell code
  2023-10-20 21:00                                   ` Stefan Kangas
@ 2023-10-21  7:19                                     ` Eli Zaretskii
  2023-10-21  7:35                                       ` Andreas Schwab
  2024-01-10 21:21                                       ` Stefan Kangas
  0 siblings, 2 replies; 47+ messages in thread
From: Eli Zaretskii @ 2023-10-21  7:19 UTC (permalink / raw)
  To: Stefan Kangas; +Cc: lx, manikulin, 66390, schwab, michael.albinus

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





^ permalink raw reply	[flat|nested] 47+ messages in thread

* bug#66390: `man' allows to inject arbitrary shell code
  2023-10-21  7:19                                     ` Eli Zaretskii
@ 2023-10-21  7:35                                       ` Andreas Schwab
  2023-10-21  7:45                                         ` Eli Zaretskii
  2024-01-10 21:21                                       ` Stefan Kangas
  1 sibling, 1 reply; 47+ messages in thread
From: Andreas Schwab @ 2023-10-21  7:35 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: lx, manikulin, 66390, michael.albinus, Stefan Kangas

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





^ permalink raw reply	[flat|nested] 47+ messages in thread

* bug#66390: `man' allows to inject arbitrary shell code
  2023-10-21  7:35                                       ` Andreas Schwab
@ 2023-10-21  7:45                                         ` Eli Zaretskii
  2023-10-21  9:19                                           ` Stefan Kangas
  0 siblings, 1 reply; 47+ messages in thread
From: Eli Zaretskii @ 2023-10-21  7:45 UTC (permalink / raw)
  To: Andreas Schwab; +Cc: lx, manikulin, 66390, michael.albinus, stefankangas

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





^ permalink raw reply	[flat|nested] 47+ messages in thread

* bug#66390: `man' allows to inject arbitrary shell code
  2023-10-21  7:45                                         ` Eli Zaretskii
@ 2023-10-21  9:19                                           ` Stefan Kangas
  0 siblings, 0 replies; 47+ messages in thread
From: Stefan Kangas @ 2023-10-21  9:19 UTC (permalink / raw)
  To: Eli Zaretskii, Andreas Schwab; +Cc: lx, manikulin, 66390, michael.albinus

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.





^ permalink raw reply	[flat|nested] 47+ messages in thread

* bug#66390: `man' allows to inject arbitrary shell code
  2023-10-21  7:19                                     ` Eli Zaretskii
  2023-10-21  7:35                                       ` Andreas Schwab
@ 2024-01-10 21:21                                       ` Stefan Kangas
  2024-01-11 12:07                                         ` Ihor Radchenko
  1 sibling, 1 reply; 47+ messages in thread
From: Stefan Kangas @ 2024-01-10 21:21 UTC (permalink / raw)
  To: Eli Zaretskii
  Cc: lx, Ihor Radchenko, 66390, schwab, michael.albinus, manikulin

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.





^ permalink raw reply	[flat|nested] 47+ messages in thread

* bug#66390: `man' allows to inject arbitrary shell code
  2024-01-10 21:21                                       ` Stefan Kangas
@ 2024-01-11 12:07                                         ` Ihor Radchenko
  2024-01-11 14:34                                           ` Max Nikulin
  0 siblings, 1 reply; 47+ messages in thread
From: Ihor Radchenko @ 2024-01-11 12:07 UTC (permalink / raw)
  To: Stefan Kangas
  Cc: lx, 66390, schwab, michael.albinus, Eli Zaretskii, manikulin

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>





^ permalink raw reply	[flat|nested] 47+ messages in thread

* bug#66390: `man' allows to inject arbitrary shell code
  2024-01-11 12:07                                         ` Ihor Radchenko
@ 2024-01-11 14:34                                           ` Max Nikulin
  2024-01-11 15:07                                             ` Ihor Radchenko
  0 siblings, 1 reply; 47+ messages in thread
From: Max Nikulin @ 2024-01-11 14:34 UTC (permalink / raw)
  To: Ihor Radchenko, Stefan Kangas
  Cc: lx, Eli Zaretskii, 66390, schwab, michael.albinus

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"





^ permalink raw reply	[flat|nested] 47+ messages in thread

* bug#66390: `man' allows to inject arbitrary shell code
  2024-01-11 14:34                                           ` Max Nikulin
@ 2024-01-11 15:07                                             ` Ihor Radchenko
  2024-01-11 15:28                                               ` Eli Zaretskii
  0 siblings, 1 reply; 47+ messages in thread
From: Ihor Radchenko @ 2024-01-11 15:07 UTC (permalink / raw)
  To: Max Nikulin
  Cc: lx, 66390, schwab, Stefan Kangas, michael.albinus, Eli Zaretskii

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>





^ permalink raw reply	[flat|nested] 47+ messages in thread

* bug#66390: `man' allows to inject arbitrary shell code
  2024-01-11 15:07                                             ` Ihor Radchenko
@ 2024-01-11 15:28                                               ` Eli Zaretskii
  2024-01-11 15:37                                                 ` Ihor Radchenko
  0 siblings, 1 reply; 47+ messages in thread
From: Eli Zaretskii @ 2024-01-11 15:28 UTC (permalink / raw)
  To: Ihor Radchenko
  Cc: lx, 66390, schwab, stefankangas, michael.albinus, manikulin

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





^ permalink raw reply	[flat|nested] 47+ messages in thread

* bug#66390: `man' allows to inject arbitrary shell code
  2024-01-11 15:28                                               ` Eli Zaretskii
@ 2024-01-11 15:37                                                 ` Ihor Radchenko
  0 siblings, 0 replies; 47+ messages in thread
From: Ihor Radchenko @ 2024-01-11 15:37 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: lx, 66390, schwab, stefankangas, michael.albinus, manikulin

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>





^ permalink raw reply	[flat|nested] 47+ messages in thread

end of thread, other threads:[~2024-01-11 15:37 UTC | newest]

Thread overview: 47+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-10-07 12:47 bug#66390: `man' allows to inject arbitrary shell code Maxim Nikulin
2023-10-07 13:04 ` Eli Zaretskii
2023-10-07 14:12   ` Max Nikulin
2023-10-07 14:19     ` Eli Zaretskii
2023-10-07 14:29       ` Max Nikulin
2023-10-07 15:10         ` Eli Zaretskii
2023-10-07 15:37         ` Michael Albinus
2023-10-07 15:58           ` Eli Zaretskii
2023-10-07 16:55             ` Michael Albinus
2023-10-07 17:24               ` Eli Zaretskii
2023-10-07 17:45                 ` Michael Albinus
2023-10-07 18:26                   ` Eli Zaretskii
2023-10-08  3:37                     ` Max Nikulin
2023-10-08  5:28                       ` Eli Zaretskii
2023-10-09 15:12                         ` Max Nikulin
2023-10-09 15:52                           ` Eli Zaretskii
2023-10-09 16:30                         ` lux
2023-10-09 16:48                           ` Eli Zaretskii
2023-10-09 17:07                             ` Ihor Radchenko
2023-10-09 17:20                             ` Andreas Schwab
2023-10-10  2:47                             ` lux
2023-10-10  7:43                             ` Stefan Kangas
2023-10-10 12:11                               ` Eli Zaretskii
2023-10-10 12:25                                 ` Stefan Kangas
2023-10-10 11:09                             ` Max Nikulin
2023-10-10 10:54                           ` Max Nikulin
2023-10-10 14:30                             ` lux
2023-10-10 16:21                               ` Andreas Schwab
2023-10-11  3:08                                 ` lux
2023-10-11 10:46                                   ` Max Nikulin
2023-10-20 21:00                                   ` Stefan Kangas
2023-10-21  7:19                                     ` Eli Zaretskii
2023-10-21  7:35                                       ` Andreas Schwab
2023-10-21  7:45                                         ` Eli Zaretskii
2023-10-21  9:19                                           ` Stefan Kangas
2024-01-10 21:21                                       ` Stefan Kangas
2024-01-11 12:07                                         ` Ihor Radchenko
2024-01-11 14:34                                           ` Max Nikulin
2024-01-11 15:07                                             ` Ihor Radchenko
2024-01-11 15:28                                               ` Eli Zaretskii
2024-01-11 15:37                                                 ` Ihor Radchenko
2023-10-09  2:36                     ` Richard Stallman
2023-10-09 11:04                       ` Eli Zaretskii
2023-10-10 11:56                         ` Richard Stallman
2023-10-11 10:56                           ` Max Nikulin
2023-10-08  3:42                 ` Maxim Nikulin
2023-10-08  5:20                   ` Eli Zaretskii

Code repositories for project(s) associated with this public inbox

	https://git.savannah.gnu.org/cgit/emacs.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).