Bug#855334: /usr/bin/thunderbird: $THUNDERBIRD_OPTIONS does not pass multiple args correctly
On 14.3.2017 01:02, Jiri Palecek wrote:
On 13.3.2017 22:57, Simon McVittie wrote:
On Mon, 13 Mar 2017 at 21:58:17 +0100, Carsten Schoenert wrote:
I had modified the warpper script in the between time a little bit
different. I've done some more effort to catch some special arguments
and get them savely prepared to the binary call.
There are for sure more than one way to get the argument passing done.
+if [[ "${ARG}" =~ ([[:space:]]|[(,|=)]) ]]; then
+TB_ARGS="${TB_ARGS} \"${ARG}\""
+else
+# No special handling needed.
+TB_ARGS="${TB_ARGS} ${ARG}"
...
+eval "${MOZ_LIBDIR}"/"${MOZ_APP_NAME}" "${TB_ARGS}"
No, that is not general and could be a security vulnerability. Consider
what would happen with an argument containing $ or ` or backslashes.
If a quoting approach is to be preferred (possibly to make the script
POSIX-compliant without bashisms), then the easiest (general) way is
to quote it with apostrophes:
TB_ARGS="${TB_ARGS} '$(echo "$ARG" | sed "s/'/'\\\''/")'"
Bummer. sed expression must have /g flag:
TB_ARGS="${TB_ARGS} '$(echo "$ARG" | sed "s/'/'\\\''/g")'"
Sorry
Jiri Palecek
Bug#855334: /usr/bin/thunderbird: $THUNDERBIRD_OPTIONS does not pass multiple args correctly
On Mon 2017-03-13 20:02:12 -0400, Jiri Palecek wrote: > If a quoting approach is to be preferred (possibly to make the script > POSIX-compliant without bashisms), then the easiest (general) way is to > quote it with apostrophes: 0 dkg@alice:~$ head -n1 $(which thunderbird) #!/bin/bash 0 dkg@alice:~$ please let's not get into eval and lowest-common-denominator stuff if we don't need to. if we've got bash in the shebang line, we should make use of bash features :) --dkg
Bug#855334: /usr/bin/thunderbird: $THUNDERBIRD_OPTIONS does not pass multiple args correctly
On 13.3.2017 22:57, Simon McVittie wrote:
On Mon, 13 Mar 2017 at 21:58:17 +0100, Carsten Schoenert wrote:
I had modified the warpper script in the between time a little bit
different. I've done some more effort to catch some special arguments
and get them savely prepared to the binary call.
There are for sure more than one way to get the argument passing done.
+if [[ "${ARG}" =~ ([[:space:]]|[(,|=)]) ]]; then
+TB_ARGS="${TB_ARGS} \"${ARG}\""
+else
+# No special handling needed.
+TB_ARGS="${TB_ARGS} ${ARG}"
...
+eval "${MOZ_LIBDIR}"/"${MOZ_APP_NAME}" "${TB_ARGS}"
No, that is not general and could be a security vulnerability. Consider
what would happen with an argument containing $ or ` or backslashes.
If a quoting approach is to be preferred (possibly to make the script
POSIX-compliant without bashisms), then the easiest (general) way is to
quote it with apostrophes:
TB_ARGS="${TB_ARGS} '$(echo "$ARG" | sed "s/'/'\\\''/")'"
use it:
eval something "$TB_ARGS"
Of course, arrays are much more convenient when you can put up with
bashisms.
Regards
Jiri Palecek
Bug#855334: /usr/bin/thunderbird: $THUNDERBIRD_OPTIONS does not pass multiple args correctly
On Mon, 13 Mar 2017 at 21:58:17 +0100, Carsten Schoenert wrote:
> I had modified the warpper script in the between time a little bit
> different. I've done some more effort to catch some special arguments
> and get them savely prepared to the binary call.
> There are for sure more than one way to get the argument passing done.
+if [[ "${ARG}" =~ ([[:space:]]|[(,|=)]) ]]; then
+TB_ARGS="${TB_ARGS} \"${ARG}\""
+else
+# No special handling needed.
+TB_ARGS="${TB_ARGS} ${ARG}"
...
+eval "${MOZ_LIBDIR}"/"${MOZ_APP_NAME}" "${TB_ARGS}"
No, that is not general and could be a security vulnerability. Consider
what would happen with an argument containing $ or ` or backslashes.
The attached script is a simplified version of that change. The goal is that
the input parses the same as the output.
$ ./t.sh hello
in: argv[1]=«hello»
out: argv[1]=«hello»
$ ./t.sh foo bar
in: argv[1]=«foo»
in: argv[1]=«bar»
out: argv[1]=«foo»
out: argv[1]=«bar»
So far so good, but quote marks and backslashes get lost:
$ ./t.sh "'foo bar'"
in: argv[1]=«'foo bar'»
out: argv[1]=«foo bar»
$ ./t.sh '\a'
in: argv[1]=«\a»
out: argv[1]=«a»
it's easy to get a syntax error:
$ ./t.sh "\""
in: argv[1]=«"»
./t.sh: eval: line 32: unexpected EOF while looking for matching `"'
./t.sh: eval: line 33: syntax error: unexpected end of file
and a maliciously supplied filename or argument (think invoking
thunderbird as a file or URL handler) can cause code execution
(imagine expr was a malicious command here):
$ ./t.sh '$(expr 2 + 2)'
in: argv[1]=«$(expr 2 + 2)»
out: argv[1]=«4»
$ ./t.sh '`expr 2 + 2`'
in: argv[1]=«`expr 2 + 2`»
out: argv[1]=«4»
Please use bash arrays as Daniel suggested: that is almost certainly
the simplest way to make this correct.
S
t.sh
Description: Bourne shell script
Bug#855334: /usr/bin/thunderbird: $THUNDERBIRD_OPTIONS does not pass multiple args correctly
Hello Daniel, On Mon, Mar 13, 2017 at 04:26:43PM -0400, Daniel Kahn Gillmor wrote: > Control: found 855334 1:45.8.0-1 > Control: tags 855334 + patch > > Passing multiple arguments to Thunderbird is still a problem in 45.8.0-1 > except that the failures are silent now :/ I don't wanted to wait on this issue as the otherwise broken script was a more conflicting thing and make the upload really grave. > On Sun 2017-03-12 12:00:53 -0400, Guido Günther wrote: > > > FWIW git-pbuilder has code to mangle options to pass them on to > > pbuilder (which is a very similar problem) and uses a shell array. You > > can rip out the code from there. > > cool, thanks for the pointer! > > I note that git-pbuilder is also a bash script, and it has a > shell_quote() function which looks much more complex than just using the > builtin printf's %q expansion, and git-pbuilder is also trying to mix > explicitly-set external variables with internally-generated variables, > which is more complex than what /usr/bin/thunderbird needs. > > I think the simplest thing is to just replace the declaration, > accumulation, and use entirely with array usage, and leave it at that. > > I've included a patch here that appears to do the right thing for me. Thanks for looking again into the a script. I had modified the warpper script in the between time a little bit different. I've done some more effort to catch some special arguments and get them savely prepared to the binary call. There are for sure more than one way to get the argument passing done. https://anonscm.debian.org/cgit/pkg-mozilla/icedove.git/commit/?h=debian/sid=c2a1d77dd811ffd758f8306a71629e28490bc628 I have tested the script with these changes a lot and every argument is now going correctly to the starting call of the TB binary, also given arguments that are unknown for TB. But that's correct in my eyes as we only want to filter out the argument for controling the wrapper. I'm of course open for improvements. Given Christoph is solving #856185 he is planning to upload a version -2 probably within the next hours. https://bugs.debian.org/856185 Regards Carsten
Bug#855334: /usr/bin/thunderbird: $THUNDERBIRD_OPTIONS does not pass multiple args correctly
Control: found 855334 1:45.8.0-1
Control: tags 855334 + patch
Passing multiple arguments to Thunderbird is still a problem in 45.8.0-1
except that the failures are silent now :/
On Sun 2017-03-12 12:00:53 -0400, Guido Günther wrote:
> FWIW git-pbuilder has code to mangle options to pass them on to
> pbuilder (which is a very similar problem) and uses a shell array. You
> can rip out the code from there.
cool, thanks for the pointer!
I note that git-pbuilder is also a bash script, and it has a
shell_quote() function which looks much more complex than just using the
builtin printf's %q expansion, and git-pbuilder is also trying to mix
explicitly-set external variables with internally-generated variables,
which is more complex than what /usr/bin/thunderbird needs.
I think the simplest thing is to just replace the declaration,
accumulation, and use entirely with array usage, and leave it at that.
I've included a patch here that appears to do the right thing for me.
Regards,
--dkg
diff --git a/thunderbird b/thunderbird
index 28983ca..8d57976 100755
--- a/thunderbird
+++ b/thunderbird
@@ -45,7 +45,7 @@ export VERBOSE=0
# set MOZ_APP_LAUNCHER for gnome-session
export MOZ_APP_LAUNCHER
-TB_ARGS=""
+declare -a TB_ARGS=()
while [ $# -gt 0 ]; do
ARG="$1"
case ${ARG} in
@@ -76,7 +76,7 @@ while [ $# -gt 0 ]; do
exit 1
;;
# every other argument is needed to get down to the TB starting call
-*) TB_ARGS="${TB_ARGS} ${ARG}"
+*) TB_ARGS+=("${ARG}")
;;
esac
shift
@@ -228,8 +228,8 @@ fi
# If we are here we going simply further by starting Thunderbird.
if [ "${DEBUG}" = "" ]; then
-output_debug "call '$MOZ_LIBDIR/$MOZ_APP_NAME ${TB_ARGS}'"
-$MOZ_LIBDIR/$MOZ_APP_NAME "${TB_ARGS}"
+output_debug "call '$MOZ_LIBDIR/$MOZ_APP_NAME ${TB_ARGS[*]}'"
+$MOZ_LIBDIR/$MOZ_APP_NAME "${TB_ARGS[@]}"
else
# User has selected GDB?
if [ "$DEBUGGER" = "1" ]; then
@@ -237,7 +237,7 @@ else
if [ -f /usr/bin/gdb ]; then
if [ -f /usr/lib/debug/usr/lib/thunderbird/thunderbird ]; then
output_info "Starting Thunderbird with GDB ..."
-LANG='' /usr/lib/thunderbird/run-mozilla.sh -g /usr/lib/thunderbird/thunderbird-bin "${TB_ARGS}"
+LANG='' /usr/lib/thunderbird/run-mozilla.sh -g /usr/lib/thunderbird/thunderbird-bin "${TB_ARGS[@]}"
else
output_info "No package 'thunderbird-dbg' installed! Please install first and restart."
exit 1
signature.asc
Description: PGP signature
Bug#855334: /usr/bin/thunderbird: $THUNDERBIRD_OPTIONS does not pass multiple args correctly
On Tue, Mar 07, 2017 at 10:18:25AM +, Simon McVittie wrote:
> Control: reopen -1
> Control: found -1 1:45.7.1-2
> Control: retitle -1 /usr/bin/thunderbird: $TB_ARGS does not pass multiple
> args correctly
>
> On Thu, 16 Feb 2017 at 16:36:03 -0500, Daniel Kahn Gillmor wrote:
> > the special "$@" is getting mangled into the env
> > var THUNDERBIRD_OPTIONS which then gets replaced as a single string,
> > rather than being passed separately.
>
> This is still the case, except the variable is named TB_ARGS now.
>
> # every other argument is needed to get down to the TB starting call
> *) TB_ARGS="${TB_ARGS} ${ARG}"
> ...
> output_debug "call '$MOZ_LIBDIR/$MOZ_APP_NAME ${TB_ARGS}'"
> $MOZ_LIBDIR/$MOZ_APP_NAME "${TB_ARGS}"
>
> Because the thunderbird wrapper is a #!/bin/bash script, you could use
> a bash array. $@ is the only variable in a POSIX shell that has special
> behaviour similar to bash arrays.
FWIW git-pbuilder has code to mangle options to pass them on to pbuilder
(which is a very similar problem) and uses a shell array. You can rip
out the code from there.
Cheers,
-- Guido
>
> > For example (note the whitespace that should be a _ in the child process
> > below):
> >
> > 0 dkg@alice:~$ ps -eFH | grep 'thun[d]er'
> > dkg 18921 3185 0 1072 1588 2 16:32 pts/100:00:00
> > /bin/sh /usr/bin/thunderbird -- -P foo
> > dkg 18932 18921 17 242031 278836 2 16:32 pts/100:00:12
> > /usr/lib/thunderbird/thunderbird -P foo
> > 0 dkg@alice:~$ tr '\0' '_' < /proc/18921/cmdline ; echo
> > /bin/sh_/usr/bin/thunderbird_--_-P_foo_
> > 0 dkg@alice:~$ tr '\0' '_' < /proc/18932/cmdline ; echo
> > /usr/lib/thunderbird/thunderbird_-P foo_
>
> Please test something like this before closing this bug again.
> Typing
>
> /usr/bin/thunderbird -P 'foo bar'
>
> at a shell prompt should result in *two* arguments being passed to
> /usr/lib/thunderbird/thunderbird, with content "-P" and "foo bar". One
> argument is wrong, three arguments are also wrong.
Bug#855334: /usr/bin/thunderbird: $THUNDERBIRD_OPTIONS does not pass multiple args correctly
Control: reopen -1
Control: found -1 1:45.7.1-2
Control: retitle -1 /usr/bin/thunderbird: $TB_ARGS does not pass multiple args
correctly
On Thu, 16 Feb 2017 at 16:36:03 -0500, Daniel Kahn Gillmor wrote:
> the special "$@" is getting mangled into the env
> var THUNDERBIRD_OPTIONS which then gets replaced as a single string,
> rather than being passed separately.
This is still the case, except the variable is named TB_ARGS now.
# every other argument is needed to get down to the TB starting call
*) TB_ARGS="${TB_ARGS} ${ARG}"
...
output_debug "call '$MOZ_LIBDIR/$MOZ_APP_NAME ${TB_ARGS}'"
$MOZ_LIBDIR/$MOZ_APP_NAME "${TB_ARGS}"
Because the thunderbird wrapper is a #!/bin/bash script, you could use
a bash array. $@ is the only variable in a POSIX shell that has special
behaviour similar to bash arrays.
> For example (note the whitespace that should be a _ in the child process
> below):
>
> 0 dkg@alice:~$ ps -eFH | grep 'thun[d]er'
> dkg 18921 3185 0 1072 1588 2 16:32 pts/100:00:00
> /bin/sh /usr/bin/thunderbird -- -P foo
> dkg 18932 18921 17 242031 278836 2 16:32 pts/100:00:12
> /usr/lib/thunderbird/thunderbird -P foo
> 0 dkg@alice:~$ tr '\0' '_' < /proc/18921/cmdline ; echo
> /bin/sh_/usr/bin/thunderbird_--_-P_foo_
> 0 dkg@alice:~$ tr '\0' '_' < /proc/18932/cmdline ; echo
> /usr/lib/thunderbird/thunderbird_-P foo_
Please test something like this before closing this bug again.
Typing
/usr/bin/thunderbird -P 'foo bar'
at a shell prompt should result in *two* arguments being passed to
/usr/lib/thunderbird/thunderbird, with content "-P" and "foo bar". One
argument is wrong, three arguments are also wrong.
Regards,
S
Bug#855334: /usr/bin/thunderbird: $THUNDERBIRD_OPTIONS does not pass multiple args correctly
Package: thunderbird
Version: 1:45.7.1-1
Severity: important
Tags: patch
Consider trying to invoke:
thunderbird -- -P foo
this currently produces:
Warning: unrecognized command line flag -P foo
it does this because the special "$@" is getting mangled into the env
var THUNDERBIRD_OPTIONS which then gets replaced as a single string,
rather than being passed separately.
For example (note the whitespace that should be a _ in the child process below):
0 dkg@alice:~$ ps -eFH | grep 'thun[d]er'
dkg 18921 3185 0 1072 1588 2 16:32 pts/100:00:00
/bin/sh /usr/bin/thunderbird -- -P foo
dkg 18932 18921 17 242031 278836 2 16:32 pts/100:00:12
/usr/lib/thunderbird/thunderbird -P foo
0 dkg@alice:~$ tr '\0' '_' < /proc/18921/cmdline ; echo
/bin/sh_/usr/bin/thunderbird_--_-P_foo_
0 dkg@alice:~$ tr '\0' '_' < /proc/18932/cmdline ; echo
/usr/lib/thunderbird/thunderbird_-P foo_
0 dkg@alice:~$
I believe the patch below fixes the problem:
--- /usr/bin/thunderbird.orig 2017-02-14 18:46:23.0 -0500
+++ /usr/bin/thunderbird2017-02-16 16:28:39.874612108 -0500
@@ -246,8 +246,6 @@
# shift found options
shift $(( OPTIND - 1 ))
-THUNDERBIRD_OPTIONS="$@"
-
# sanity check
if [ "$DEBUGGER" != "" ] && [ "$USER_DEBUGGER" != "" ]; then
echo "You can't use option '-g and '-d' at the same time!"
@@ -353,8 +351,8 @@
# migrate, going further by starting Thunderbird.
if [ "${DEBUG}" = "" ]; then
-debug "call $MOZ_LIBDIR/$MOZ_APP_NAME '${THUNDERBIRD_OPTIONS}'"
-$MOZ_LIBDIR/$MOZ_APP_NAME "${THUNDERBIRD_OPTIONS}"
+debug "call $MOZ_LIBDIR/$MOZ_APP_NAME $@"
+$MOZ_LIBDIR/$MOZ_APP_NAME "$@"
else
# User has selected GDB?
if [ "$DEBUGGER" = "1" ]; then
@@ -362,7 +360,7 @@
if [ -f /usr/bin/gdb ]; then
if [ -f /usr/lib/debug/usr/lib/thunderbird/thunderbird ]; then
echo "Starting Thunderbird with GDB ..."
-LANG= /usr/lib/thunderbird/run-mozilla.sh -g
/usr/lib/thunderbird/thunderbird-bin "${THUNDERBIRD_OPTIONS}"
+LANG= /usr/lib/thunderbird/run-mozilla.sh -g
/usr/lib/thunderbird/thunderbird-bin "$@"
else
echo "No package 'thunderbird-dbg' installed! Please install
first and restart."
exit 1
-- System Information:
Debian Release: 9.0
APT prefers testing-debug
APT policy: (500, 'testing-debug'), (500, 'testing'), (200,
'unstable-debug'), (200, 'unstable'), (1, 'experimental-debug'), (1,
'experimental')
Architecture: amd64 (x86_64)
Foreign Architectures: i386
Kernel: Linux 4.9.0-1-amd64 (SMP w/4 CPU cores)
Locale: LANG=en_US.UTF-8, LC_CTYPE=en_US.UTF-8 (charmap=UTF-8)
Shell: /bin/sh linked to /bin/dash
Init: systemd (via /run/systemd/system)
Versions of packages thunderbird depends on:
ii debianutils 4.8.1
ii fontconfig2.11.0-6.7
ii libasound21.1.3-4
ii libatk1.0-0 2.22.0-1
ii libc6 2.24-9
ii libcairo2 1.14.8-1
ii libdbus-1-3 1.10.14-1
ii libdbus-glib-1-2 0.108-2
ii libevent-2.0-52.0.21-stable-2.1
ii libffi6 3.2.1-6
ii libfontconfig12.11.0-6.7
ii libfreetype6 2.6.3-3+b1
ii libgcc1 1:6.3.0-6
ii libgdk-pixbuf2.0-02.36.4-1
ii libglib2.0-0 2.50.2-2
ii libgtk2.0-0 2.24.31-2
ii libhunspell-1.4-0 1.4.1-2+b1
ii libicu57 57.1-5
ii libnspr4 2:4.12-6
ii libnss3 2:3.26.2-1
ii libpango-1.0-01.40.3-3
ii libpangocairo-1.0-0 1.40.3-3
ii libpangoft2-1.0-0 1.40.3-3
ii libpixman-1-0 0.34.0-1
ii libsqlite3-0 3.16.2-2
ii libstartup-notification0 0.12-4
ii libstdc++66.3.0-6
ii libvpx4 1.6.1-2
ii libx11-6 2:1.6.4-3
ii libxcomposite11:0.4.4-2
ii libxdamage1 1:1.1.4-2+b1
ii libxext6 2:1.3.3-1
ii libxfixes31:5.0.3-1
ii libxrender1 1:0.9.10-1
ii libxt61:1.1.5-1
ii psmisc22.21-2.1+b1
ii zlib1g1:1.2.8.dfsg-5
Versions of packages thunderbird recommends:
ii lightning 1:45.7.1-1
pn myspell-en-us | hunspell-dictionary | myspell-dictionary
Versions of packages thunderbird suggests:
pn apparmor
ii fonts-lyx 2.2.2-1
ii libgssapi-krb5-2 1.15-1
-- no debconf information
