[RT#69439] Secure temp files handling in bin/dave

Author: cosimo

.gitignore

@@ -2,6 +2,7 @@
 pm_to_blib
 Makefile
 Makefile.old
+MANIFEST.bak
 blib
 pod2htm*
 

MANIFEST

@@ -1,13 +1,6 @@
 bin/dave
 bin/dist
 Changes
-lib/HTTP/DAV.pm
-lib/HTTP/DAV/Comms.pm
-lib/HTTP/DAV/Lock.pm
-lib/HTTP/DAV/Resource.pm
-lib/HTTP/DAV/ResourceList.pm
-lib/HTTP/DAV/Response.pm
-lib/HTTP/DAV/Utils.pm
 doc/Changes.pod
 doc/html/Changes.html
 doc/html/dave.html
@@ -16,8 +9,17 @@ doc/html/index.html
 doc/html/TODO.html
 doc/README.pod
 doc/TODO.pod
+lib/HTTP/DAV.pm
+lib/HTTP/DAV/Changes.pod
+lib/HTTP/DAV/Comms.pm
+lib/HTTP/DAV/Lock.pm
+lib/HTTP/DAV/Resource.pm
+lib/HTTP/DAV/ResourceList.pm
+lib/HTTP/DAV/Response.pm
+lib/HTTP/DAV/Utils.pm
 Makefile.PL
 MANIFEST
+MANIFEST.SKIP
 META.yml			Module meta-data (added by MakeMaker)
 README
 t/1_loadme.t
@@ -42,6 +44,7 @@ t/9_RT_52665.t
 t/9_RT_59674.t
 t/9_RT_60457.t
 t/9_RT_68936.t
+t/9_RT_69439.t
 t/multistatus.xml
 t/test_data/file1
 t/test_data/file2

MANIFEST.SKIP

@@ -0,0 +1,8 @@
+patches/.*
+pod2htm.*
+pm_to_blib$
+blib.*
+\.git.*
+Makefile$
+\.bak$
+\.old$

Makefile.PL

@@ -15,6 +15,7 @@ WriteMakefile(
     'EXE_FILES' => [ map {"bin/$_"} @programs_to_install ],
     'PREREQ_PM' => { 
         'Cwd'          => 0,
+        'File::Temp'   => 0,
         'LWP'          => 5.48,
         'Scalar::Util' => 0,
         'Time::Local'  => 0,
@@ -23,7 +24,6 @@ WriteMakefile(
         'XML::DOM'     => 0,
 
         # bin/dave specific dependencies
-        'File::Temp'       => 0,
         'Getopt::Long'     => 0,
         'Term::ReadLine'   => 0,
         'Text::ParseWords' => 0,

bin/dave

@@ -55,10 +55,10 @@ GetOptions(
    'u|username=s'   => \$user,
    'p|password=s'   => \$pass,
    'tmpdir:s'       => \$TMP_DIR,
-) or pod2usage(2);
+) or Pod::Usage::pod2usage(2);
 
 # Basic sanity check for /tmp dir
-if (! -d $TMP_DIR or ! -w $TMP_DIR) {
+if (! $TMP_DIR or ! -d $TMP_DIR or ! -w $TMP_DIR) {
     die "Can't write into temp dir '$TMP_DIR': $!\n";
 }
 
@@ -238,9 +238,9 @@ sub command_cat {
 
 sub command_edit { 
    my $remote_file = shift;
-   $TMP_DIR||="/tmp";
-   my $EDITOR= $ENV{DAV_EDITOR} || $ENV{EDITOR} || "vi";
-   my $local_file = "$TMP_DIR/dave.perldav." . $$ . time;
+
+   my $EDITOR= $ENV{DAV_EDITOR} || $ENV{EDITOR} || 'vi';
+   my $local_file = HTTP::DAV::_tempfile('dave', $TMP_DIR);
 
    my $resource = $gdc->propfind($remote_file);
    if ( $resource && $resource->is_collection() ) {

lib/HTTP/DAV.pm

@@ -25,6 +25,7 @@ use URI::file;
 use URI::Escape;
 use FileHandle;
 use File::Glob;
+use File::Temp ();
 
 sub new {
     my $class = shift;
@@ -71,6 +72,26 @@ sub DebugLevel {
     $DEBUG = $level;
 }
 
+sub _tempfile {
+    my ($prefix, $tempdir) = @_;
+
+    $prefix ||= 'dav';
+    $tempdir ||= '/tmp';
+
+    my $template = $prefix . 'XXXXXXXXXXXXX';
+
+    my $old_umask = umask 0077;
+    my ($fh, $filename) = File::Temp::tempfile($template,
+        DIR => $tempdir,
+        SUFFIX => '.tmp'
+    );
+    umask $old_umask;
+
+    return wantarray
+        ? ($fh, $filename)
+        : $filename;
+}
+
 ######################################################################
 # new_resource acts as a resource factory.
 # It will create a new one for you each time you ask.

t/9_RT_69439.t

@@ -0,0 +1,41 @@
+#!/usr/bin/env perl
+#
+# RT #69439, insecure /tmp file handling
+#
+
+use strict;
+use warnings;
+use Test::More tests => 11;
+
+use File::Path ();
+use HTTP::DAV;
+
+# Dave uses HTTP::DAV::_tempfile() every time
+# it has to open a new temporary file
+
+my $tmpdir = ".http-dav-test-tmpdir.$$";
+
+ok(File::Path::mkpath($tmpdir), "Created temp dir");
+
+# Generate two temp files one immediately after the other
+my ($fh1, $filename1) = HTTP::DAV::_tempfile('dave', $tmpdir);
+my ($fh2, $filename2) = HTTP::DAV::_tempfile('dave', $tmpdir);
+
+ok($fh1);
+ok($fh2);
+ok($filename1);
+ok($filename2);
+
+# They have to have different filenames
+isnt($filename1, $filename2, "Different filenames should be generated");
+isnt($fh1, $fh2, "They should be different filehandles too, just in case");
+
+#diag("Generated temp file: $filename1");
+#diag("Generated temp file: $filename2");
+
+ok(index($filename1, "$tmpdir/dave") > -1);
+ok(index($filename2, "$tmpdir/dave") > -1);
+
+is(unlink($filename1, $filename2), 2, "Removed temp files");
+
+ok(File::Path::rmtree($tmpdir), "Cleaned up temp dir");