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
