Debian bug report logs - #1720 adduser: races, and chmod/chown - patch provided Package: adduser ; Reported by: Austin Donnelly ; Done: sr1@irz301.inf.tu-dresden.de (Sven Rudolph). ----------------------------------------------------------------------- Message received at debian-bugs-done: From irz301.inf.tu-dresden.de!sr1 Mon Oct 23 10:39:02 1995 Return-Path: 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/ ----------------------------------------------------------------------- Notification sent to Austin Donnelly : Bug acknowledged by developer. Full text available. ----------------------------------------------------------------------- Reply sent to sr1@irz301.inf.tu-dresden.de (Sven Rudolph) : You have taken responsibility. Full text available. ----------------------------------------------------------------------- Message received at debian-bugs: From chiark.chu.cam.ac.uk!ian Sat Oct 21 07:16:10 1995 Return-Path: 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: Date: Sat, 21 Oct 95 15:15 BST From: Ian Jackson To: Austin Donnelly , 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. ----------------------------------------------------------------------- Acknowledgement sent to Ian Jackson : Extra info received and forwarded. Full text available. ----------------------------------------------------------------------- Information forwarded to debian-devel@pixar.com : Bug#1720 ; Package adduser . Full text available. ----------------------------------------------------------------------- Message received at debian-bugs: From cam.ac.uk!and1000 Fri Oct 20 18:38:06 1995 Return-Path: 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 X-Sender: and1000@valour.pem.cam.ac.uk To: debian-bugs@pixar.com Subject: adduser: races, and chmod/chown - patch provided Message-Id: 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; } ----------------------------------------------------------------------- Acknowledgement sent to Austin Donnelly : New bug report received and forwarded. Full text available. ----------------------------------------------------------------------- Report forwarded to debian-devel@pixar.com : Bug#1720 ; Package adduser . Full text available. ----------------------------------------------------------------------- Ian Jackson / iwj10@thor.cam.ac.uk , with the debian-bugs tracking mechanism This page last modified 07:43:01 GMT Wed 01 Nov