debmany: CVE-2023-27635: shell injection

Debian Bug report logs - #1031267
debmany: CVE-2023-27635: shell injection

Reply or subscribe to this bug.

Toggle useless messages

Message #3 received at submit@bugs.debian.org (full text, mbox, reply):

From: Jakub Wilk <jwilk@jwilk.net>

To: submit@bugs.debian.org

Subject: debmany: shell injection

Date: Tue, 14 Feb 2023 10:53:02 +0100

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

Package: debian-goodies
Version: 0.88.1
Tags: security

debmany passes filenames from the .deb (which should be considered 
untrusted input) to eval.

I've attached proof-of-concept exploit.

-- 
Jakub Wilk

[injection.deb (application/vnd.debian.binary-package, attachment)]

Message #6 received at 1031267@bugs.debian.org (full text, mbox, reply):

From: Jakub Wilk <jwilk@jwilk.net>

To: 1031267@bugs.debian.org

Subject: Re: Bug#1031267: debmany: shell injection

Date: Tue, 14 Feb 2023 11:02:19 +0100

* Jakub Wilk <jwilk@jwilk.net>, 2023-02-14 10:53:
>attached proof-of-concept exploit.

The code that generated the crafted .deb is here:
https://github.com/jwilk/crafted.deb/blob/master/gen-deb1031267-debmany

-- 
Jakub Wilk

Message #11 received at 1031267@bugs.debian.org (full text, mbox, reply):

From: Axel Beckert <abe@debian.org>

To: Jakub Wilk <jwilk@jwilk.net>, 1031267@bugs.debian.org

Cc: security@debian.org

Subject: Re: Bug#1031267: debmany: shell injection

Date: Tue, 14 Feb 2023 15:53:30 +0100

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

Control: tag -1 + confirmed

Hi Jakub,

thanks for the bug report.

Jakub Wilk wrote:
> debmany passes filenames from the .deb (which should be considered untrusted
> input) to eval.
>
> I've attached proof-of-concept exploit.

Thanks. Can reproduce it.

Given that the full path including the exploit code is always shown to
the user before the exploit actually runs, I consider the impact
rather low:

┌────────────────────────┤ Select a file (file:./injection.deb) ├────────────────────────┐
│                                                                                        │
│  usr/share/doc/$(cowsay${IFS}pwned>/dev/tty;sleep${IFS}inf)/changelog.gz changelog.gz  │
│                                                                                        │
│                      <Ok>                                 <Cancel>                     │
│                                                                                        │
└────────────────────────────────────────────────────────────────────────────────────────┘

Accordingly a severity of only "normal" seems fitting.

Should be fixed nevertheless. Will look into it.

		Regards, Axel
-- 
 ,''`.  |  Axel Beckert <abe@debian.org>, https://people.debian.org/~abe/
: :' :  |  Debian Developer, ftp.ch.debian.org Admin
`. `'   |  4096R: 2517 B724 C5F6 CA99 5329  6E61 2FF9 CD59 6126 16B5
  `-    |  1024D: F067 EA27 26B9 C3FC 1486  202E C09E 1D89 9593 0EDE

[signature.asc (application/pgp-signature, inline)]

Message #18 received at 1031267@bugs.debian.org (full text, mbox, reply):

From: Axel Beckert <abe@debian.org>

To: Jakub Wilk <jwilk@jwilk.net>, 1031267@bugs.debian.org

Subject: Re: Bug#1031267: debmany: shell injection

Date: Sun, 19 Feb 2023 05:47:20 +0100

Control: tag -1 + patch pending

Hi Jakub,

found time to analyse this closer.

Axel Beckert wrote:
> Given that the full path including the exploit code is always shown to
> the user before the exploit actually runs, I consider the impact
> rather low:
> 
> ┌────────────────────────┤ Select a file (file:./injection.deb) ├────────────────────────┐
> │                                                                                        │
> │  usr/share/doc/$(cowsay${IFS}pwned>/dev/tty;sleep${IFS}inf)/changelog.gz changelog.gz  │
> │                                                                                        │
> │                      <Ok>                                 <Cancel>                     │
> │                                                                                        │
> └────────────────────────────────────────────────────────────────────────────────────────┘

And this even though $manpages (which holds a space separated list of
all files to offer) is — on purpose — unquoted when passing to the
selection program.

But the real culprit is the use of eval in lines 415, 422 and 424:

  414       debug "Opening manpage file: "`printf "$mancmdline" "$PWD/$file"` # comment
  415       eval $(printf "$mancmdline" "$PWD/$file")
  416       cd - >/dev/null
  417     else
  418       # other file (usr/share/doc)
  419       debug "Opening other file: "`printf "$othercmdline" "$PWD/$return"` # comment
  420       if [[ "$return" =~ \.gz$ ]]
  421       then
  422               eval $(printf "gzip -dc $PWD/$return | $othercmdline")
  423       else
  424               eval $(printf "$othercmdline" "$PWD/$return")
  425       fi

Eval is evil. Once again.

My first thought was to just exclude all files with shell special
characters, especially '$(' and friends. But

  apt-file search -n \$\( | fgrep /usr/share/doc/

shows that these are actually used (and fail with debmany due to the
issue reported by Jakub):

  sdlbasic: /usr/share/doc/sdlbasic/english/sections/commands/Maths/hex$().html
  sdlbasic: /usr/share/doc/sdlbasic/english/sections/commands/Strings/chr$().html
  sdlbasic: /usr/share/doc/sdlbasic/english/sections/commands/Strings/insert$().html
  sdlbasic: /usr/share/doc/sdlbasic/english/sections/commands/Strings/lCase$().html
  sdlbasic: /usr/share/doc/sdlbasic/english/sections/commands/Strings/lTrim$().html
  sdlbasic: /usr/share/doc/sdlbasic/english/sections/commands/Strings/left$().html
  sdlbasic: /usr/share/doc/sdlbasic/english/sections/commands/Strings/mid$().html
  sdlbasic: /usr/share/doc/sdlbasic/english/sections/commands/Strings/replace$().html
  sdlbasic: /usr/share/doc/sdlbasic/english/sections/commands/Strings/replaceSubstr$().html
  sdlbasic: /usr/share/doc/sdlbasic/english/sections/commands/Strings/reverse$().html
  sdlbasic: /usr/share/doc/sdlbasic/english/sections/commands/Strings/right$().html
  sdlbasic: /usr/share/doc/sdlbasic/english/sections/commands/Strings/space$().html
  sdlbasic: /usr/share/doc/sdlbasic/english/sections/commands/Strings/str$().html
  sdlbasic: /usr/share/doc/sdlbasic/english/sections/commands/Strings/string$().html
  sdlbasic: /usr/share/doc/sdlbasic/english/sections/commands/Strings/trim$().html
  sdlbasic: /usr/share/doc/sdlbasic/english/sections/commands/Strings/typeOf$().html
  sdlbasic: /usr/share/doc/sdlbasic/english/sections/commands/Strings/uCase$().html

Additionally there are tons of files with just $ or any kind of brackets.

Just quoting them "properly" inside the eval and printf won't help as
you can always escape from there by just ending the according
quotation by adding a closing quote character into your file path.

In case of the ".gz" case in your example, it's easier to fix as you
can simply move "gzip -dc $PWD/$return |" out of the eval/printf.

But in the other cases it's less simple.

So I came up with the following fix which uses command instead of
eval, and bash pattern substitution to replace %s with the file name
instead letting printf inside an $() doing the substitution:

diff --git a/debmany/debmany b/debmany/debmany
index 7eeb68b..dfea6e9 100755
--- a/debmany/debmany
+++ b/debmany/debmany
@@ -96,6 +96,19 @@ Examples: debmany foo.deb  show manpages from a local package file foo.deb
   fi
 }
 
+replace_percent_s_and_execute() {
+  replacement="$1"
+  shift
+  declare -a cmdarr
+  cmdarr=($@)
+  debug "cmdarr before; ${cmdarr[@]}"
+  for i in ${!cmdarr[@]}; do
+    cmdarr[$i]="${cmdarr[$i]/\%s/$replacement}"
+  done
+  debug "cmdarr after; ${cmdarr[@]}"
+  command "${cmdarr[@]}"
+}
+
 while [ $# -gt 0 ]
 do
   case $1 in
@@ -377,6 +390,7 @@ else
   dpkg --fsys-tarfile "$file" | tar --wildcards -xf - $mandirs 2>/dev/null
   # find all manpage files
   manpages=`find usr -type f 2>/dev/null|sort|sed -e 's|\([^/]*\)$|\1 \1|'`
+  # | egrep -v '[\`\\${}*?;<>|]'
 fi
 
 while true
@@ -412,16 +426,16 @@ do
         cd "$path"
       fi
       debug "Opening manpage file: "`printf "$mancmdline" "$PWD/$file"` # comment
-      eval $(printf "$mancmdline" "$PWD/$file")
+      replace_percent_s_and_execute "$PWD/$file" "$mancmdline"
       cd - >/dev/null
     else
       # other file (usr/share/doc)
       debug "Opening other file: "`printf "$othercmdline" "$PWD/$return"` # comment
       if [[ "$return" =~ \.gz$ ]]
       then
-              eval $(printf "gzip -dc $PWD/$return | $othercmdline")
+              gzip -dc "$PWD/$return" | replace_percent_s_and_execute '-' "$othercmdline"
       else
-              eval $(printf "$othercmdline" "$PWD/$return")
+              replace_percent_s_and_execute "$PWD/$return" "$othercmdline"
       fi
     fi
   else

This is now committed as
https://salsa.debian.org/debian/debian-goodies/-/commit/495eaafa94d2b4cbee4eed01cd78238e311d096b
but I'd be happy about a review before I upload.

I will also do some more testing on my side.

		Regards, Axel
-- 
 ,''`.  |  Axel Beckert <abe@debian.org>, https://people.debian.org/~abe/
: :' :  |  Debian Developer, ftp.ch.debian.org Admin
`. `'   |  4096R: 2517 B724 C5F6 CA99 5329  6E61 2FF9 CD59 6126 16B5
  `-    |  1024D: F067 EA27 26B9 C3FC 1486  202E C09E 1D89 9593 0EDE

Message #25 received at 1031267@bugs.debian.org (full text, mbox, reply):

From: Salvatore Bonaccorso <carnil@debian.org>

To: Axel Beckert <abe@debian.org>, 1031267@bugs.debian.org

Cc: Jakub Wilk <jwilk@jwilk.net>, team@security.debian.org

Subject: Re: Bug#1031267: debmany: shell injection

Date: Mon, 6 Mar 2023 05:55:46 +0100

Control: retitle -1 debmany: CVE-2023-27635: shell injection

On Sun, Feb 19, 2023 at 05:47:20AM +0100, Axel Beckert wrote:
> Control: tag -1 + patch pending
> 
> Hi Jakub,
> 
> found time to analyse this closer.
> 
> Axel Beckert wrote:
> > Given that the full path including the exploit code is always shown to
> > the user before the exploit actually runs, I consider the impact
> > rather low:
> > 
> > ┌────────────────────────┤ Select a file (file:./injection.deb) ├────────────────────────┐
> > │                                                                                        │
> > │  usr/share/doc/$(cowsay${IFS}pwned>/dev/tty;sleep${IFS}inf)/changelog.gz changelog.gz  │
> > │                                                                                        │
> > │                      <Ok>                                 <Cancel>                     │
> > │                                                                                        │
> > └────────────────────────────────────────────────────────────────────────────────────────┘
> 
> And this even though $manpages (which holds a space separated list of
> all files to offer) is — on purpose — unquoted when passing to the
> selection program.
> 
> But the real culprit is the use of eval in lines 415, 422 and 424:
> 
>   414       debug "Opening manpage file: "`printf "$mancmdline" "$PWD/$file"` # comment
>   415       eval $(printf "$mancmdline" "$PWD/$file")
>   416       cd - >/dev/null
>   417     else
>   418       # other file (usr/share/doc)
>   419       debug "Opening other file: "`printf "$othercmdline" "$PWD/$return"` # comment
>   420       if [[ "$return" =~ \.gz$ ]]
>   421       then
>   422               eval $(printf "gzip -dc $PWD/$return | $othercmdline")
>   423       else
>   424               eval $(printf "$othercmdline" "$PWD/$return")
>   425       fi
> 
> Eval is evil. Once again.
> 
> My first thought was to just exclude all files with shell special
> characters, especially '$(' and friends. But
> 
>   apt-file search -n \$\( | fgrep /usr/share/doc/
> 
> shows that these are actually used (and fail with debmany due to the
> issue reported by Jakub):
> 
>   sdlbasic: /usr/share/doc/sdlbasic/english/sections/commands/Maths/hex$().html
>   sdlbasic: /usr/share/doc/sdlbasic/english/sections/commands/Strings/chr$().html
>   sdlbasic: /usr/share/doc/sdlbasic/english/sections/commands/Strings/insert$().html
>   sdlbasic: /usr/share/doc/sdlbasic/english/sections/commands/Strings/lCase$().html
>   sdlbasic: /usr/share/doc/sdlbasic/english/sections/commands/Strings/lTrim$().html
>   sdlbasic: /usr/share/doc/sdlbasic/english/sections/commands/Strings/left$().html
>   sdlbasic: /usr/share/doc/sdlbasic/english/sections/commands/Strings/mid$().html
>   sdlbasic: /usr/share/doc/sdlbasic/english/sections/commands/Strings/replace$().html
>   sdlbasic: /usr/share/doc/sdlbasic/english/sections/commands/Strings/replaceSubstr$().html
>   sdlbasic: /usr/share/doc/sdlbasic/english/sections/commands/Strings/reverse$().html
>   sdlbasic: /usr/share/doc/sdlbasic/english/sections/commands/Strings/right$().html
>   sdlbasic: /usr/share/doc/sdlbasic/english/sections/commands/Strings/space$().html
>   sdlbasic: /usr/share/doc/sdlbasic/english/sections/commands/Strings/str$().html
>   sdlbasic: /usr/share/doc/sdlbasic/english/sections/commands/Strings/string$().html
>   sdlbasic: /usr/share/doc/sdlbasic/english/sections/commands/Strings/trim$().html
>   sdlbasic: /usr/share/doc/sdlbasic/english/sections/commands/Strings/typeOf$().html
>   sdlbasic: /usr/share/doc/sdlbasic/english/sections/commands/Strings/uCase$().html
> 
> Additionally there are tons of files with just $ or any kind of brackets.
> 
> Just quoting them "properly" inside the eval and printf won't help as
> you can always escape from there by just ending the according
> quotation by adding a closing quote character into your file path.
> 
> In case of the ".gz" case in your example, it's easier to fix as you
> can simply move "gzip -dc $PWD/$return |" out of the eval/printf.
> 
> But in the other cases it's less simple.
> 
> So I came up with the following fix which uses command instead of
> eval, and bash pattern substitution to replace %s with the file name
> instead letting printf inside an $() doing the substitution:
> 
> diff --git a/debmany/debmany b/debmany/debmany
> index 7eeb68b..dfea6e9 100755
> --- a/debmany/debmany
> +++ b/debmany/debmany
> @@ -96,6 +96,19 @@ Examples: debmany foo.deb  show manpages from a local package file foo.deb
>    fi
>  }
>  
> +replace_percent_s_and_execute() {
> +  replacement="$1"
> +  shift
> +  declare -a cmdarr
> +  cmdarr=($@)
> +  debug "cmdarr before; ${cmdarr[@]}"
> +  for i in ${!cmdarr[@]}; do
> +    cmdarr[$i]="${cmdarr[$i]/\%s/$replacement}"
> +  done
> +  debug "cmdarr after; ${cmdarr[@]}"
> +  command "${cmdarr[@]}"
> +}
> +
>  while [ $# -gt 0 ]
>  do
>    case $1 in
> @@ -377,6 +390,7 @@ else
>    dpkg --fsys-tarfile "$file" | tar --wildcards -xf - $mandirs 2>/dev/null
>    # find all manpage files
>    manpages=`find usr -type f 2>/dev/null|sort|sed -e 's|\([^/]*\)$|\1 \1|'`
> +  # | egrep -v '[\`\\${}*?;<>|]'
>  fi
>  
>  while true
> @@ -412,16 +426,16 @@ do
>          cd "$path"
>        fi
>        debug "Opening manpage file: "`printf "$mancmdline" "$PWD/$file"` # comment
> -      eval $(printf "$mancmdline" "$PWD/$file")
> +      replace_percent_s_and_execute "$PWD/$file" "$mancmdline"
>        cd - >/dev/null
>      else
>        # other file (usr/share/doc)
>        debug "Opening other file: "`printf "$othercmdline" "$PWD/$return"` # comment
>        if [[ "$return" =~ \.gz$ ]]
>        then
> -              eval $(printf "gzip -dc $PWD/$return | $othercmdline")
> +              gzip -dc "$PWD/$return" | replace_percent_s_and_execute '-' "$othercmdline"
>        else
> -              eval $(printf "$othercmdline" "$PWD/$return")
> +              replace_percent_s_and_execute "$PWD/$return" "$othercmdline"
>        fi
>      fi
>    else
> 
> This is now committed as
> https://salsa.debian.org/debian/debian-goodies/-/commit/495eaafa94d2b4cbee4eed01cd78238e311d096b
> but I'd be happy about a review before I upload.
> 
> I will also do some more testing on my side.

The issue itself has CVE-2023-27635 now.

Jakub, were you able to peer review Axel's approach?

Axel, on the severity, I do not believe this will warrant a DSA. It is
sufficiently guarded already as the user will be prompted before
execution with the path.

Regards,
Salvatore

Send a report that this bug log contains spam.