Bug#1720: adduser: races, and chmod/chown - patch provided

[Austin Donnelly <and1000@cam.ac.uk>]

Package: adduser
Version: 1.94-1

Three different bugs fixed here:

 (1) There were a few race conditions in locking the password and
     group files.  A badly timed ^C could result in the lockfile
     not being cleared.

 (2) chown()/chmod() persistantly used in the wrong order throughout.
     Could people please take note: chown()ing a file removes the
     setuid and setgid bits on it!  It's no use chmod()ing a file to
     be setgid, then chown()ing it to someone else.

 (3) The copy_to_file() routine doesn't preserve permissions.  This
     means that giving user's a default .xsession (which must be rwx)
     isn't possible. I've modified copy_to_file() to now copy the
     permissions with the file - but the files are chown()ed later, so
     the setuid/setgid bit will be lost. (This is probably the right
     thing to happen, in this instance).


As always, patch included...


Austin


--- /usr/sbin/adduser	Thu Oct  5 20:02:18 1995
+++ adduser	Sat Oct 21 02:24:19 1995
@@ -328,8 +328,8 @@
 	exit 1;
     }
     # lock the password file
-    link $PASSWD, $PLOCK;
     $passwd_state = "locked";
+    link $PASSWD, $PLOCK;
 }
 ##
 ## check if the group file is locked, if necessary:
@@ -344,8 +344,8 @@
 	exit 1;
     }
     # lock the group file
-    link $GROUP, $GLOCK;
     $group_state = "locked";
+    link $GROUP, $GLOCK;
 }


@@ -508,11 +508,11 @@
     }

     if ($make_group_also) {
-	chmod (02775, $home_dir);
 	chown ($new_uid, $new_gid, $home_dir);
+	chmod (02775, $home_dir);
     } else {
-	chmod (0755, $home_dir);
 	chown ($new_uid, 0, $home_dir);
+	chmod (0755, $home_dir);
     }

     &clean_up();
@@ -651,6 +651,7 @@
 	}
 	mkdir ($home_dir, $dir_mode);
 	chown ($new_uid, $new_gid, $home_dir);
+	chmod ($home_dir, $dir_mode); # since chown() resets set{gu}id bit
 	print "done.\n" if ($verbose);

 	##
@@ -857,12 +858,20 @@
 ##
 sub copy_to_dir {
     local ($file, $dir) = @_;
+    local ($dev, $ino, $mode, $nlink, $uid, $gid, $rdev, $size,
+	   $atime, $mtime, $ctime, $blksize, $blocks);

     if ((! -f $file) || (! -d $dir)) {
 	return -1;
     }

     open (FILE, $file) || die "open: $!";
+
+    # grab access permissions for later
+    ($dev,$ino,$mode,$nlink,$uid,$gid,$rdev,$size,
+     $atime,$mtime,$ctime,$blksize,$blocks)
+	= stat($file);
+
     $file =~ s+.*/++;		# get the basename
     open (NEWFILE, ">$dir/$file") || die "open: $!";

@@ -872,6 +881,10 @@

     close FILE;
     close NEWFILE;
+
+    # now copy permissions saved earlier. Note setuid/setgid bit destroyed
+    # by a later chown call - maybe thats a good thing!
+    chmod($mode, "$dir/$file");

     return 1;
 }