Comment 11 for bug 12030

Message-ID: <email address hidden>
Date: Sun, 9 Jan 2005 21:05:26 +0100
From: Javier =?iso-8859-1?Q?Fern=E1ndez-Sanguino_Pe=F1a?= <email address hidden>
To: <email address hidden>
Cc: Bram Moolenaar <email address hidden>
Subject: vim: Race conditions and symlink attacks in vim (tcltags and vimspell)

--2B/JsCI69OhZNC5r
Content-Type: multipart/mixed; boundary="AhhlLboLdkugWU4S"
Content-Disposition: inline

--AhhlLboLdkugWU4S
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

Package: vim
Version: 1:6.3-046+1
Severity: minor
Tags: patch security sid woody sarge

Hi there,

Reviewing vim as part of the security audit the Audit team [1] is=20
conducting I've found what I believe are some race conditions and symlink=
=20
attacks through temporary files in vim. They appear in two scripts which=20
are not installed in Debian in binary locations (they are installed under
/usr/share/doc/vim/tools/) but are provided with execute permissions.

That's mainly why I'm opening this bug up in Debian's BTS and not=20
contacting the security team directly although the code is present in all=
=20
vim releases in Debian.

These appear in:

1.- the tcltags script (runtime/tools/tcltags):
    (...)
    11 tmp_tagfile=3D/tmp/${program_name}.$$
    (...)
    130 sed -e "/^!_TAG_FILE_SORTED/s/ [01] / $sorted /"=
=20
\
    131 -e "/^!_TAG_FILE_FORMAT/s/ 1 / $format /"=
=20
\
    132 $tagfile > $tmp_tagfile

2.- the vimspell script (runtime/tools/vimspell.sh)

     16 OUTFILE=3D/tmp/vimspell.$$
     17 # if you have "tempfile", use the following line
     18 #OUTFILE=3D`tempfile`
(...)
     30 spell $SPELL_ARGS $INFILE | sort -u |
     31 awk '
     32 {
     33 printf "syntax match SpellErrors \"\\<%s\\>\"\n", $0 ;
     34 }
     35
     36 END {
     37 printf "highlight link SpellErrors ErrorMsg\n\n" ;
     38 }
     39 ' > $OUTFILE
     40 echo "!rm $OUTFILE" >> $OUTFILE
     41 echo $OUTFILE

Since these are tools that are run from vim, an attacker can get a=20
good-enough approximation of the PIDs that will be used in these temporary=
=20
files and can conduct a symlink attack if these tools are used.

The attached patch should fix both of these issues, I've taken the=20
approach implemented in vimtutor, but modified it slightly for vimspell as=
=20
the temporary file cannot be removed by the script (vim removes it) when=20
mktemp and tempfile are not avilable, there will still be a race condition=
=20
in the script. Since most GNU/Linux and UNIX operating systems seem to=20
have either one I don't think it's a big issue, however.

Best regards

Javier

--AhhlLboLdkugWU4S
Content-Type: text/plain; charset=us-ascii
Content-Disposition: attachment; filename="vim-6.3.diff"
Content-Transfer-Encoding: quoted-printable

diff -Nru vim-6.3.old/vim63/runtime/tools/tcltags vim-6.3/vim63/runtime/too=
ls/tcltags
--- vim-6.3.old/vim63/runtime/tools/tcltags 1999-08-01 14:01:46.000000000 +=
0200
+++ vim-6.3/vim63/runtime/tools/tcltags 2005-01-09 20:41:41.000000000 +0100
@@ -8,7 +8,31 @@
 program_version=3D"0.3"
 program_author=3D"Darren Hiebert"
 <email address hidden>"
-tmp_tagfile=3D/tmp/${program_name}.$$
+tmp=3D"${TMPDIR-/tmp}"
+tmp_tagfile=3D`mktemp -t $tmp/tcltagXXXXXX || tempfile -p tclag || echo no=
ne`
+
+# If the standard commands failed then create a directory to put the copy =
in.
+# That is a secure way to make a temp file.
+if test "$tmp_tagfile" =3D none; then
+ tmpdir=3D$tmp/tcltag$$
+ OLD_UMASK=3D`umask`
+ umask 077
+ getout=3Dno
+ mkdir $tmpdir || getout=3Dyes
+ umask $OLD_UMASK
+ if test $getout =3D yes; then
+ echo "Could not create directory for tcltag, exiting."
+ exit 1
+ fi
+ tmp_tagfile=3D$tmpdir/tcltag
+ touch $tmp_tagfile
+ TODELETE=3D$tmpdir
+else
+ TODELETE=3D$tmp_tagfile
+fi
+# remove the copy of the tcltag file on exit
+trap "rm -rf $TODELETE" 0 1 2 3 9 11 13 15
+
=20
 usage=3D"\
 Usage: $program_name [-au] [-{f|o} tagfile] [--format=3Dn] file(s)
@@ -154,6 +178,5 @@
 else
     cp $tmp_tagfile $tagfile
 fi
-rm $tmp_tagfile
=20
 exit 0
diff -Nru vim-6.3.old/vim63/runtime/tools/vimspell.sh vim-6.3/vim63/runtime=
/tools/vimspell.sh
--- vim-6.3.old/vim63/runtime/tools/vimspell.sh 1999-08-01 14:01:46.0000000=
00 +0200
+++ vim-6.3/vim63/runtime/tools/vimspell.sh 2005-01-09 20:51:18.000000000 +=
0100
@@ -13,9 +13,20 @@
 # March 1999
=20
 INFILE=3D$1
-OUTFILE=3D/tmp/vimspell.$$
-# if you have "tempfile", use the following line
-#OUTFILE=3D`tempfile`
+tmp=3D"${TMPDIR-/tmp}"
+OUTFILE=3D`mktemp -t vimspellXXXXXX || tempfile -p vimspell || echo none`
+# If the standard commands failed then create the file
+# since we cannot create a directory (we cannot remove it on exit)
+# create a file in the safest way possible.
+if test "$OUTFILE" =3D none; then
+ OUTFILE=3D$tmp/vimspell$$
+ [ -e $OUTFILE ] && { echo "Cannot use temporary file $OUTFILE, it already=
 exists!; exit 1 ; }=20
+ (umask 077; touch $OUTFILE)
+fi
+# Note the copy of vimspell cannot be deleted on exit since it is
+# used by vim, otherwise it should do this:
+# trap "rm -f $OUTFILE" 0 1 2 3 9 11 13 15
+
=20
 #
 # local spellings

--AhhlLboLdkugWU4S--

--2B/JsCI69OhZNC5r
Content-Type: application/pgp-signature; name="signature.asc"
Content-Description: Digital signature
Content-Disposition: inline

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.2.4 (GNU/Linux)

iD8DBQFB4Y6Gi4sehJTrj0oRAj6UAJ0aSUf4pjG3D/5O/X62tJ1gtzGX0gCgwNqo
FZIKf6HleDHHBtxzRqs3oW0=
=0KeP
-----END PGP SIGNATURE-----

--2B/JsCI69OhZNC5r--