[GRASS5] [bug #2877] (grass) Insecure tempfile creation

[Request Tracker]

this bug's URL: http://intevation.de/rt/webrt?serial_num=2877
-------------------------------------------------------------------------

Subject: Insecure tempfile creation

Platform: GNU/Linux/i386
grass obtained from: Trento Italy site
grass binary for platform: Compiled from Sources
GRASS Version: 5.7.0

This is a showstopper bug for Debian. Packages with security bugs cannot enter the stable distribution. This is Debian Bug #287651:
http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=287651

Received: (at submit) by bugs.debian.org; 29 Dec 2004 11:01:19 +0000
>From jfs at dat.etsit.upm.es Wed Dec 29 03:01:19 2004
Return-path: <jfs at dat.etsit.upm.es>
Received: from tornado.dat.etsit.upm.es (dat.etsit.upm.es) [138.100.17.73] 
	by spohr.debian.org with smtp (Exim 3.35 1 (Debian))
	id 1CjbZu-00028E-00; Wed, 29 Dec 2004 03:01:18 -0800
Received: (qmail 10639 invoked by uid 1013); 29 Dec 2004 11:01:16 -0000
Date: Wed, 29 Dec 2004 12:01:16 +0100
From: Javier =?iso-8859-1?Q?Fern=E1ndez-Sanguino_Pe=F1a?= <jfs at computer.org>
To: submit at bugs.debian.org
Subject: grass: Multiple vulnerabilities (symlink attacks) due to improper temporary files use in scripts and source code
Message-ID: <20041229110116.GB7144 at dat.etsit.upm.es>
Mime-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha1;
	protocol="application/pgp-signature"; boundary="VrqPEDrXMn8OVzN4"
Content-Disposition: inline
User-Agent: Mutt/1.5.6+20040722i
Delivered-To: submit at bugs.debian.org
X-Spam-Checker-Version: SpamAssassin 2.60-bugs.debian.org_2004_03_25 
	(1.212-2003-09-23-exp) on spohr.debian.org
X-Spam-Status: No, hits=-8.0 required=4.0 tests=BAYES_00,HAS_PACKAGE 
	autolearn=no version=2.60-bugs.debian.org_2004_03_25
X-Spam-Level: 

Package: grass
Version: 5.0.3-5.1
Priority: grave
Tags: security sarge sid

A lot of scripts provided withing Grass are vulnerable to race conditions
through symlink attacks in temporary files. Many of these scripts either
create temporary files in an insecure manner (shell scripts do not use 'set
-e' and 'set -C', for example) or/and are easily guessable.

Some examples include:

grass-5.0.3/src/scripts/contrib/i.oif/i.oif:

---------------------------------------------------------
(...)
# save the Stddev for TM bands
echo "Calculation Standarddeviations for all bands:"
$GISBASE/etc/i.oif/r.stddev $1 |tail -1 >/tmp/i.oif.stddev
(...)
---------------------------------------------------------


grass-5.0.3/src/CMD/generic/GISGEN.sh:

--------------------------------------------------------
case $# in
0)
    tmp1=/tmp/GISGEN1.$$
    tmp2=/tmp/GISGEN2.$$
    tmp3=/tmp/GISGEN3.$$
    rm -f $tmp1
        touch $tmp1
(...)
   rm -f $tmp2
    rm -f $tmp3
    echo "a == 1 { print \$0 ; next }" > $tmp3
    echo "\$0 == \"$STEP\" { a = 1; print \$0 }" >> $tmp3
    awk -f $tmp3 $tmp1 > $tmp2
    rm -f $tmp3 $tmp1
----------------------------------------------------------

grass-5.0.3/src/mapdev/v.in.arc.poly/script/v.in.arc.poly

----------------------------------------------------------
bindir=$GISBASE/etc
tempfile=/tmp/temp.ply
(...)
echo 'start eliminating double nodes in ' $1
$bindir/permut $GISDBASE/$LOCATION_NAME/$MAPSET/arc/temp.ply $tempfile
rm $tempfile
(...)
----------------------------------------------------------
[Note: permut just opens this output file without further checks:
./src/mapdev/v.in.arc.poly/permut/permut.c
(...)
       if ((outfile = fopen (out_ply, "w")) == NULL)
        {
          printf ("can't open tempfile %s\n", out_ply);
          exit (1);
        }
(...)
]

./src/paint/Drivers/versatec/3236/DRIVER.sh
----------------------------------------------------------
(...)
TMPDIR1=/tmp/versatec
TMPDIR2=/tmp/versatec
(...)
RASTERFILE=$TMPDIR1/_paint
SPRINT="/bin/sprint >&2"
SPRINT_COMMAND="$SPRINT $RASTERFILE -v -p 3236 -w $TMPDIR2 -x $ZOOM -y 
$ZOOM"

----------------------------------------------------------

grass-5.0.3/src/scripts/contrib/i.spectral/i.spectral
----------------------------------------------------------
.where -1 |r.what input=$RASTERMAPS > /tmp/spectr.dum1
cat /tmp/spectr.dum1 | cut -d'|' -f4,5,6,7,8,9,10| tr '|' '\012' > 
/tmp/spectr.dum2
----------------------------------------------------------

Now those are just exmaples of the "easily guessable" temporary files used. 
But a lot of scripts make use of the $$ construct (either within shell 
scripts or C code using getpid()) that is not directly guessable but can be 
infered in a system where a given user is running grass more or less 
accurately either:

- by looking at the /tmp/ directory and detecting when a given temporary
file is created and symlink the "next one". For example in
./src/scripts/contrib/r.plane/r.plane the following tmp files are created
in succession: /tmp/$$, /tmp/$$dip, /tmp/$$, /tmp/$$ea, /tmp/$$, /tmp/$$no,
/tmp/$$ (removed and reused several times). So an attacker 

- by bulk creating a huge number of temporary files using the current PID 
of the grass program as a base

Just try a 'grep -r "/tmp/"' on the sources and you'll see what I mean.

I cannot determine, as I don't use grass, wether the scripts there are 
actually used regularly by users. I would suggest however to patch those 
either by:

a) Safely creating a per user temporary directory and have all scripts use 
that as a location for all of the temporary files if defined. For example, 
in a common startup script do:

TMPGRASS = `mktemp -dt grass-XXXXXX` || { echo "Cannot create temporary 
directory"; exit 1 ; }
export TMPGRASS

and in auxiliary scripts do:
[ ! -n "TMPGRASS" ] && TMPGRASS=`mktemp -dt grass-XXXXXX` || { echo "Cannot 
create temporary directory"; exit 1 ; }
(...)
tempfile="$TMPGRASS/tempfile"

b) Changing all shell scripts to use mktemp or tempfile (might 
make those Debian-specific) when setting up temporary files.

All the C files, however, need to be modified so that they use mkstemp(). 
So, for example, instead of this:

(in grass-5.0.3/src/imagery/i.ask/popup.c):

   char tempfile1[40], tempfile2[40];
(...)
   sprintf (tempfile1, "/tmp/i.ask1.%d", getpid());
   sprintf (tempfile2, "/tmp/i.ask2.%d", getpid());


it should use this:

    int tempfd1; int tempfd2;

    if ( ( tempfd1 = mkstemp("/tmp/i.ask1.XXXXXX") ) < 0 ) {
	    /* Do something if this breaks! */ 
    }
    if ( ( tempfd2 = mkstemp("/tmp/i.ask2.XXXXXX") ) < 0 ) {
	    /* Do something if this breaks! */ 
    }

and pass the filedescriptor (instead of the filename) to functions later
on. This means that ./src/libes/raster/Panel.c needs to be rewritten (or
extended to use fd instead of names in its call. 

BTW, what does this mean?

grass-5.0.3/src/libes/raster/Panel.c
(...)
  /* make sure this file can be written by anybody */
        num = umask(0);
        close(creat(name,0666));
        umask(num);
(...)

!!!

Doesn't look too safe to me.. What's the panel used for?


Now, I'm not sure I can provide a patch fixing all of those, but I'm 
willing to provide a full patch (at least for the shell scripts) if time 
permits.

However, IMHO this makes this software package unsuitable for release.

Regards


Javier

-------------------------------------------- Managed by Request Tracker