Debian bug report logs - #1720
adduser: races, and chmod/chown - patch provided

Message received at debian-bugs-done:

From irz301.inf.tu-dresden.de!sr1 Mon Oct 23 10:39:02 1995
Return-Path: <sr1@irz301.inf.tu-dresden.de>
Received: from pixar.com by mongo.pixar.com with smtp
	(Smail3.1.28.1 #15) id m0t7Qpx-0005zPC; Mon, 23 Oct 95 10:39 PDT
Received: from irz101.inf.tu-dresden.de by pixar.com with SMTP id AA06709
  (5.67b/IDA-1.5 for debian-bugs-done-pipe@mongo.pixar.com); Mon, 23 Oct 1995 10:32:58 -0700
Received: by irz101.inf.tu-dresden.de (8.6.12/8.6.12-s1) id RAA16150; Mon, 23 Oct 1995 17:43:19 +0100
Date: Mon, 23 Oct 1995 17:43:19 +0100
From: sr1@irz301.inf.tu-dresden.de (Sven Rudolph)
Message-Id: <199510231643.RAA16150@irz101.inf.tu-dresden.de>
To: and1000@cam.ac.uk, debian-bugs-done@pixar.com
Subject:  Bug#1720: adduser: races, and chmod/chown - patch provided

> 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).
> 

Thanks for the bug report

> As always, patch included...

... and especially for the patch.
I applied your patch, your fixes weren't covered by Ian Jackson's one.
I will upload the new release this evening.

FYI: There was a bug in the patch, chmod requires the mode first. 

@@ -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);

        ##


	Sven
-- 
Sven Rudolph (sr1@inf.tu-dresden.de); WWW : http://www.sax.de/~sr1/

Message received at debian-bugs:

From chiark.chu.cam.ac.uk!ian Sat Oct 21 07:16:10 1995
Return-Path: <ian@chiark.chu.cam.ac.uk>
Received: from pixar.com by mongo.pixar.com with smtp
	(Smail3.1.28.1 #15) id m0t6eiX-0005zHC; Sat, 21 Oct 95 07:16 PDT
Received: from artemis.chu.cam.ac.uk by pixar.com with SMTP id AA11140
  (5.67b/IDA-1.5 for debian-bugs-pipe@mongo.pixar.com); Sat, 21 Oct 1995 07:15:39 -0700
Received: from chiark.chu.cam.ac.uk by artemis.chu.cam.ac.uk with smtp
	(Smail3.1.29.1 #33) id m0t6ehx-0007uOC; Sat, 21 Oct 95 15:15 BST
Received: by chiark.chu.cam.ac.uk
	id m0t6ehq-0002bAC
	(Debian /\oo/\ Smail3.1.29.1 #29.33); Sat, 21 Oct 95 15:15 BST
Message-Id: <m0t6ehq-0002bAC@chiark.chu.cam.ac.uk>
Date: Sat, 21 Oct 95 15:15 BST
From: Ian Jackson <ian@chiark.chu.cam.ac.uk>
To: Austin Donnelly <and1000@cam.ac.uk>, debian-bugs@Pixar.com
Subject: Re: Bug#1720: adduser: races, and chmod/chown - patch provided

Austin Donnelly writes ("Bug#1720: adduser: races, and chmod/chown - patch provided"):
> 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...

Please see also my bug reports, #1544 and #1500.  #1544 contains a
patch that fixes all the problems I've encountered with adduser, and
which will probably overlap with Austin's.

I remember seeing a message on debian-* saying that we have a new
maintainer for adduser - would they please step forward so that we can
dump this lot on them ? :-)

If they don't I suppose I could make an interim release, which might
stop any more people submitting patches for overlapping subsets of
bugs.

Ian.

Message received at debian-bugs:

From cam.ac.uk!and1000 Fri Oct 20 18:38:06 1995
Return-Path: <and1000@cam.ac.uk>
Received: from pixar.com by mongo.pixar.com with smtp
	(Smail3.1.28.1 #15) id m0t6Ssv-000BiLC; Fri, 20 Oct 95 18:38 PDT
Received: from black.csi.cam.ac.uk by pixar.com with SMTP id AA10338
  (5.67b/IDA-1.5 for debian-bugs-pipe@mongo.pixar.com); Fri, 20 Oct 1995 18:33:33 -0700
Received: from valour.pem.cam.ac.uk [131.111.200.47] (ident = root) 
	by black.csi.cam.ac.uk with smtp 
	(Smail-3.1.29.0 #36) id m0t6Sop-000CBnC; Sat, 21 Oct 95 02:33 BST
Received: by valour.pem.cam.ac.uk
	id m0t6Sow-000z5NC
	(Debian /\oo/\ Smail3.1.29.1 #29.33); Sat, 21 Oct 95 02:33 BST
Date: Sat, 21 Oct 1995 02:33:58 +0100 (BST)
From: Austin Donnelly <and1000@cam.ac.uk>
X-Sender: and1000@valour.pem.cam.ac.uk
To: debian-bugs@pixar.com
Subject: adduser: races, and chmod/chown - patch provided
Message-Id: <Pine.LNX.3.91.951021022604.15283A-100000@valour.pem.cam.ac.uk>
Mime-Version: 1.0
Content-Type: TEXT/PLAIN; charset=US-ASCII


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;
 }