Bug 593392 - setup-ds-admin.pl -k creates world readable file

https://bugzilla.redhat.com/show_bug.cgi?id=593392
Resolves: bug 593392
Bug Description: setup-ds-admin.pl -k creates world readable file
Reviewed by: thoger (Thanks!)
Branch: HEAD
Fix Description: Use umask to make sure we create a .inf file that is
only viewable by the user.  In addition, only create the temp file and
filename when necessary.  In some places, the code was creating a .inf
file on disk when it could just create one in memory.  The code should
check to see if the Inf object has a file associated with it before
attemtping to unlink it.  Make sure we do not unlink a .inf file given
with -f on the command line.  If the user specified -k, always write to
a temp file using __temp__ as the keyword to tell Inf->write to generate
a temp file.
Platforms tested: RHEL5 x86_64
Flag Day: no
Doc impact: no

ldap/admin/src/scripts/DSMigration.pm.in

@@ -1105,7 +1105,7 @@ sub migrateDS {
             $mig->msg($FATAL, 'error_opening_dseldif', "$oldconfigdir/dse.ldif", $!);
             return 0;
         }
-        debug(2, "Using inffile $inf->{filename} created from $oldconfigdir\n");
+        debug(2, "Using inf created from $oldconfigdir\n");
 
         # create servers but do not start them until after databases
         # have been migrated
@@ -1113,7 +1113,9 @@ sub migrateDS {
 
         # create the new instance
         @errs = createDSInstance($inf);
-        unlink($inf->{filename});
+        if ($inf->{filename}) {
+            unlink($inf->{filename});
+        }
         if (@errs) {
             $mig->msg(@errs);
             $mig->msg($FATAL, 'error_creating_dsinstance', $inst);

ldap/admin/src/scripts/DSUtil.pm.in

@@ -769,30 +769,19 @@ sub createInfFromConfig {
         return 0;
     }
 
-    my ($outfh, $inffile) = tempfile(SUFFIX => '.inf');
-    if (!$outfh || !$inffile) {
-        push @{$errs}, "error_opening_tempinf", $fname, $!;
-        if ($outfh) {
-            close $outfh;
-        }
-        $conn->close();
-        return 0;
-    }
-    print $outfh "[General]\n";
-    print $outfh "FullMachineName = ", $ent->getValues('nsslapd-localhost'), "\n";
-    print $outfh "SuiteSpotUserID = ", $ent->getValues('nsslapd-localuser'), "\n";
-    print $outfh "[slapd]\n";
-    print $outfh "RootDN = ", $ent->getValues('nsslapd-rootdn'), "\n";
-    print $outfh "RootDNPwd = ", $ent->getValues('nsslapd-rootpw'), "\n";
-    print $outfh "ServerPort = ", $ent->getValues('nsslapd-port'), "\n";
-    print $outfh "ServerIdentifier = $id\n";
+    my $inf = new Inf();
+    $inf->{General}->{FullMachineName} = $ent->getValues('nsslapd-localhost');
+    $inf->{General}->{SuiteSpotUserID} = $ent->getValues('nsslapd-localuser');
+    $inf->{slapd}->{RootDN} = $ent->getValues('nsslapd-rootdn');
+    $inf->{slapd}->{RootDNPwd} = $ent->getValues('nsslapd-rootpw');
+    $inf->{slapd}->{ServerPort} = $ent->getValues('nsslapd-port');
+    $inf->{slapd}->{ServerIdentifier} = $id;
 
     my $suffix;
     $ent = $conn->search("cn=ldbm database,cn=plugins,cn=config",
                          "one", "(objectclass=*)");
     if (!$ent) {
         push @{$errs}, "error_opening_dseldif", $fname, $!;
-        close $outfh;
         $conn->close();
         return 0;
     }
@@ -807,7 +796,6 @@ sub createInfFromConfig {
     $ent = $conn->search("cn=config", "base", "(objectclass=*)");
     if (!$ent) {
         push @{$errs}, "error_opening_dseldif", $fname, $!;
-        close $outfh;
         $conn->close();
         return 0;
     }
@@ -816,12 +804,9 @@ sub createInfFromConfig {
     $conn->close();
 
     if ($inst_dir) {
-        print $outfh "inst_dir = $inst_dir\n";
+        $inf->{slapd}->{inst_dir} = $inst_dir;
     }
-    print $outfh "Suffix = $suffix\n";
-    close $outfh;
-
-    my $inf = new Inf($inffile);
+    $inf->{slapd}->{Suffix} = $suffix;
 
     return $inf;
 }

ldap/admin/src/scripts/Inf.pm

@@ -41,6 +41,8 @@
 
 package Inf;
 
+use File::Temp qw(tempfile tempdir);
+
 #require    Exporter;
 #@ISA       = qw(Exporter);
 #@EXPORT    = qw();
@@ -50,6 +52,9 @@ sub new {
     my $self = {};
 
     $self->{filename} = shift;
+    $self->{writable} = shift; # do not overwrite user supplied file
+    # if you want to init an Inf with a writable file, use
+    # $inf = new Inf($filename, 1)
 
     $self = bless $self, $type;
 
@@ -162,7 +167,7 @@ sub writeSection {
     my $section = $self->{$name};
     if (ref($section) eq 'HASH') {
         print $fh "[$name]\n";
-        for my $key (keys %{$section}) {
+        for my $key (sort keys %{$section}) {
             if (defined($section->{$key})) {
                 my $val = $section->{$key};
                 $val =~ s/\n/\\\n/g; # make continuation lines
@@ -175,28 +180,50 @@ sub writeSection {
 sub write {
     my $self = shift;
     my $filename = shift;
+    my $fh;
 
-    if ($filename) {
+    return if ($filename and $filename eq "-");
+
+    # see if user wants to force use of a temp file
+    if ($filename and $filename eq '__temp__') {
+        $self->{writable} = 1;
+        $filename = '';
+        delete $self->{filename};
+    }
+
+    if (!$self->{writable}) {
+        return; # do not overwrite read only file
+    }
+
+    if ($filename) { # use user supplied filename
         $self->{filename} = $filename;
-    } else {
+    } elsif ($self->{filename}) { # use existing filename
         $filename = $self->{filename};
+    } else { # create temp filename
+        ($fh, $self->{filename}) = tempfile("setupXXXXXX", UNLINK => 0,
+                                            SUFFIX => ".inf", OPEN => 1,
+                                            DIR => File::Spec->tmpdir);
     }
 
-    return if ($filename eq "-");
-
-    if (!open(INF, ">$filename")) {
-        print STDERR "Error: could not write inf file $filename: $!\n";
-        return;
+    my $savemask = umask(0077);
+    if (!$fh) {
+        if (!open(INF, ">$filename")) {
+            print STDERR "Error: could not write inf file $filename: $!\n";
+            umask($savemask);
+            return;
+        }
+        $fh = *INF;
     }
     # write General section first
-    $self->writeSection('General', \*INF);
-    print INF "\n";
+    $self->writeSection('General', $fh);
+    print $fh "\n";
     for my $key (keys %{$self}) {
         next if ($key eq 'General');
-        $self->writeSection($key, \*INF);
-        print INF "\n";
+        $self->writeSection($key, $fh);
+        print $fh "\n";
     }
-    close INF;
+    close $fh;
+    umask($savemask);
 }
 
 sub updateFromArgs {

ldap/admin/src/scripts/Migration.pm.in

@@ -54,9 +54,6 @@ use Exporter ();
 @EXPORT    = qw();
 @EXPORT_OK = qw();
 
-# tempfiles
-use File::Temp qw(tempfile tempdir);
-
 # hostname
 use Net::Domain qw(hostfqdn);
 
@@ -68,8 +65,6 @@ use Mozilla::LDAP::LDIF;
 
 use Getopt::Long;
 
-use File::Temp qw(tempfile tempdir);
-
 use SetupLog;
 use DSUtil;
 
@@ -210,7 +205,6 @@ sub init {
     $actualsroot =~ s/\/+$//; # trim trailing '/'s, if any
     $self->{actualsroot} = $actualsroot || $self->{oldsroot};
     $self->{silent} = $silent;
-    $self->{inffile} = $inffile;
     $self->{keep} = $keep;
     $self->{preonly} = $preonly;
     $self->{logfile} = $logfile;
@@ -218,18 +212,11 @@ sub init {
     $self->{log} = new SetupLog($self->{logfile}, "migrate");
     $self->{start_servers} = 1; # start servers as soon as they are migrated
     # if user supplied inf file, use that to initialize
-    if (defined($self->{inffile})) {
-        $self->{inf} = new Inf($self->{inffile});
+    if (defined($inffile)) {
+        $self->{inf} = new Inf($inffile);
     } else {
         $self->{inf} = new Inf;
     }
-    my $fh;
-    # create a temp inf file for writing for other processes
-    # never overwrite the user supplied inf file
-    ($fh, $self->{inffile}) = tempfile("migrateXXXXXX", UNLINK => !$keep,
-                                       SUFFIX => ".inf", OPEN => 0,
-                                       DIR => File::Spec->tmpdir);
-    $self->{inf}->{filename} = $self->{inffile};
 
     # see if user passed in default inf values - also, command line
     # arguments override those passed in via an inf file - this

ldap/admin/src/scripts/Setup.pm.in

@@ -52,9 +52,6 @@ use Exporter ();
 @EXPORT    = qw($SILENT $EXPRESS $TYPICAL $CUSTOM);
 @EXPORT_OK = qw($SILENT $EXPRESS $TYPICAL $CUSTOM);
 
-# tempfiles
-use File::Temp qw(tempfile tempdir);
-
 # hostname
 use Net::Domain qw(hostfqdn);
 
@@ -66,8 +63,6 @@ use Mozilla::LDAP::LDIF;
 
 use Getopt::Long;
 
-use File::Temp qw(tempfile tempdir);
-
 use SetupLog;
 use DSUtil;
 use Inf;
@@ -141,7 +136,6 @@ sub init {
                );
 
     $self->{silent} = $silent;
-    $self->{inffile} = $inffile;
     $self->{keep} = $keep;
     $self->{preonly} = $preonly;
     $self->{update} = $update;
@@ -149,18 +143,11 @@ sub init {
     $self->{logfile} = $logfile;
     $self->{log} = new SetupLog($self->{logfile});
     # if user supplied inf file, use that to initialize
-    if (defined($self->{inffile})) {
-        $self->{inf} = new Inf($self->{inffile});
+    if (defined($inffile)) {
+        $self->{inf} = new Inf($inffile);
     } else {
         $self->{inf} = new Inf;
     }
-    my $fh;
-    # create a temp inf file for writing for other processes
-    # never overwrite the user supplied inf file
-    ($fh, $self->{inffile}) = tempfile("setupXXXXXX", UNLINK => !$keep,
-                                       SUFFIX => ".inf", OPEN => 0,
-                                       DIR => File::Spec->tmpdir);
-    $self->{inf}->{filename} = $self->{inffile};
 
     # see if user passed in default inf values - also, command line
     # arguments override those passed in via an inf file - this

ldap/admin/src/scripts/migrate-ds.pl.in

@@ -68,9 +68,14 @@ $mig->msg('end_ds_migration');
 $mig->doExit(0);
 
 END {
-    if ($mig) {
-        if (!$mig->{keep}) {
-            unlink $mig->{inffile};
-        }
+    if ($mig and $mig->{keep}) {
+        $mig->{inf}->write("__temp__");
     }
 }
+
+# emacs settings
+# Local Variables:
+# mode:perl
+# indent-tabs-mode: nil
+# tab-width: 4
+# End:

ldap/admin/src/scripts/setup-ds.pl.in

@@ -74,7 +74,6 @@ if (!$setup->{silent}) {
     if ($rc) {
         $setup->doExit();
     }
-    $setup->{inf}->write();
 }
 
 my @errs;
@@ -105,10 +104,8 @@ if (@errs) {
 $setup->doExit(0);
 
 END {
-    if ($setup) {
-        if (!$setup->{keep}) {
-            unlink $setup->{inffile};
-        }
+    if ($setup and $setup->{keep}) {
+        $setup->{inf}->write("__temp__");
     }
 }