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