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

[lux <lx@shellcodes.org>]

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