Bug#337492: Note: Unsafe usage of temporary files

[Javier Fernández-Sanguino Peña]

Package: note
Version: 1.3.1-2
Priority: important
Tag: security

The 'note' program sets up temporary files in an unsafe way which leads to
race conditions since it first generates a temporary filename (but does not
create the file), removes it (twice, first race condition) and then tries to
output to the tempotary filename (second race condition). The filename
randomness is generate ad hoc and an attacker could have a hard time getting
it through but it really makes no sense to do it this way when File::Temp is
already available.

Also, in the sources and provided as an example, the 'strestest' script has a
temporary symlink vulnerabity and a small bug (the temporary file is not
removed on program exit). Instead of writting to /tmp/xxx.$$ is should use 
tempfile.

The attached patch fixes both issues.

Regards

Javier


diff -Nru note-1.3.1/bin/note note-1.3.1.new/bin/note
--- note-1.3.1/bin/note 2005-01-12 22:03:03.000000000 +0100
+++ note-1.3.1.new/bin/note     2005-11-04 18:26:21.000000000 +0100
@@ -35,6 +35,8 @@
 use Getopt::Long;
 use FileHandle;
 use File::Spec;
+use File::Temp qw/tempfile/;
+
 #use Data::Dumper;
 
 
@@ -673,8 +675,6 @@
     return if $db->lock();
     if ($conf{alwayseditor}) {
       $TEMP = &gettemp;
-      # security!
-      unlink $TEMP;
       # let the user edit it...
       $editor = &find_editor;
       if ($editor) {
@@ -1448,18 +1448,10 @@
 
 
 sub gettemp {
-    my($random, @range);
-    @range=('0'..'9','a'..'z','A'..'Z');
-    srand(time||$$);
-    for (0..10) {
-      $random .= $range[rand(int($#range)+1)];
-    }
-    my $tempfile = File::Spec->catfile($conf{tempdirectory}, $USER . $random);
-    if (-e $tempfile) {
-      # avoid race conditions!
-      unlink $tempfile;
-    }
-    return $tempfile;
+    my ($fh, $filename) = tempfile("note.XXXXXX", DIR => $conf{tempdirectory}, 
UNLINK => 1)
+           or  die "Cannot create temporary file: $!" ;
+    close $fh;
+    return $filename;
   }
 
 
diff -Nru note-1.3.1/bin/stresstest.sh note-1.3.1.new/bin/stresstest.sh
--- note-1.3.1/bin/stresstest.sh        2005-01-12 22:03:03.000000000 +0100
+++ note-1.3.1.new/bin/stresstest.sh    2005-11-04 18:22:38.000000000 +0100
@@ -28,25 +28,27 @@
 stress ()
 {
     FILES=""
+    tmpfile=`tempfile` || { echo "$0: Cannot create temporary file" >&2; exit 
1;  }
+    trap " [ -f \"$tmpfile\" ] && /bin/rm -f -- \"$tmpfile\"" 0 1 2 3 13 15
     for file in `ls $1|sort`
     do
        echo "$1/$file"
        if [ -d "$1/$file" ] ; then
            stress "$1/$file"
        else
-           #echo "$1/" > /tmp/$$
-           #echo $file >> /tmp/$$
-           #`cat /tmp/$$ | note -`
+           #echo "$1/" > $tmpfile
+           #echo $file >> $tmpfile
+           #`cat $tmpfile | note -`
            FILES="$FILES $file"
        fi
     done       
-    echo "$1/" > /tmp/$$
-    echo "$FILES" >> /tmp/$$
+    echo "$1/" > $tmpfile
+    echo "$FILES" >> $tmpfile
     case $FILES in
        "")
                ;;
        *)      
-               RES=`cat /tmp/$$ | note -`
+               RES=`cat $tmpfile | note -`
                ;;
     esac               
     FILES=""

signature.asc
Description: Digital signature