On Sat, Nov 17, 2012 at 11:20:21AM -0500, Ryan Stone wrote:
> Wow.  So apparently things are even more broken than I though.  Let's play,
> "What group am I in?"

> root@group-testing:/usr/home/rstone # cd /tmp
> root@group-testing:/tmp # pw groupadd testing
> root@group-testing:/tmp # mkdir testdir
> root@group-testing:/tmp # chown root:testing testdir/
> root@group-testing:/tmp # chmod g+rwx testdir/
> root@group-testing:/tmp # pw usermod
> root@group-testing:/tmp # pw groupmod testing -m rstone
> root@group-testing:/tmp # id rstone
> uid=1001(rstone) gid=1001(rstone) groups=1001(rstone),0(wheel),1002(testing)
> root@group-testing:/tmp # exit
> $ id
> uid=1001(rstone) gid=1001 groups=1001,0
> $ id rstone
> uid=1001(rstone) gid=1001 groups=1001
> $ touch /tmp/testdir/testfile
> touch: /tmp/testdir/testfile: Permission denied
> $ ls -ld /tmp/testdir/
> drwxrwxr-x  2 root  1002  512 Nov 17 11:07 /tmp/testdir/

On Sat, Nov 17, 2012 at 11:20:21AM -0500, Ryan Stone wrote:
> My original complaint that /etc/group gets permissions of 0600 is a result
> of a bug in libutil, which bapt@ ported pw to use in r242349.  The new
> group manipulation API using mktemp to create a temporary file, writes the
> new group database to the temp file and then renames the temp file to
> /etc/group.  The problem here is that mktemp creates a file with a mode of
> 600, and libutil never chmods it.  That should be pretty trivial to fix.

On Mon, Nov 19, 2012 at 11:28:43PM +0100, Mateusz Guzik wrote:
> On Sat, Nov 17, 2012 at 11:20:21AM -0500, Ryan Stone wrote:
> > My original complaint that /etc/group gets permissions of 0600 is a result
> > of a bug in libutil, which bapt@ ported pw to use in r242349.  The new
> > group manipulation API using mktemp to create a temporary file, writes the
> > new group database to the temp file and then renames the temp file to
> > /etc/group.  The problem here is that mktemp creates a file with a mode of
> > 600, and libutil never chmods it.  That should be pretty trivial to fix.

> My additional 0,03$:

> I took closer look to this and I think that problems are much broader
> than this. I don't know if similar problems were present before.

> First, pw should not fail if other instance is running, it should wait
> instead (think of parallel batch scripts adding some users/groups).

> Second, current code has a race:
> lockfd = open(group_file, O_RDONLY, 0);
> if (lockfd < 0 || fcntl(lockfd, F_SETFD, 1) == -1)
>    err(1, "%s", group_file);
> if (flock(lockfd, LOCK_EX|LOCK_NB) == -1) {
> [..]
> gr_copy(pfd, tfd, gr, old_gr); /* copy from groupfile to tempfile */
> [..]
> rename(tempfile,groupfile);

> Now let's consider threads A and B:

> A: open()
> A: lock();
> A: gr_copy
> B: open()

> Now B has file descriptor to /etc/group that is about to be removed.

> A: rename()
> A: unlock()
> B: lock()

> Now B has a lock on unlinked file.

> B: gr_copy()
> B: rename()

> ... and stores new content losing modifications done by A

> Third, I don't like current api.
> gr_lock and gr_tmp have no arguments (that matter anyway)
> gr_copy operates on two descriptors given as arguments
> gr_mkdb takes nothing and is expected to do The Right Thing

On Mon, Nov 19, 2012 at 11:37:45PM +0100, Baptiste Daroussin wrote:
> On Mon, Nov 19, 2012 at 11:28:43PM +0100, Mateusz Guzik wrote:
> > On Sat, Nov 17, 2012 at 11:20:21AM -0500, Ryan Stone wrote:
> > > My original complaint that /etc/group gets permissions of 0600 is a result
> > > of a bug in libutil, which bapt@ ported pw to use in r242349.  The new
> > > group manipulation API using mktemp to create a temporary file, writes the
> > > new group database to the temp file and then renames the temp file to
> > > /etc/group.  The problem here is that mktemp creates a file with a mode of
> > > 600, and libutil never chmods it.  That should be pretty trivial to fix.

> > My additional 0,03$:

> > I took closer look to this and I think that problems are much broader
> > than this. I don't know if similar problems were present before.

> > First, pw should not fail if other instance is running, it should wait
> > instead (think of parallel batch scripts adding some users/groups).

> > Second, current code has a race:
> > lockfd = open(group_file, O_RDONLY, 0);
> > if (lockfd < 0 || fcntl(lockfd, F_SETFD, 1) == -1)
> >       err(1, "%s", group_file);
> > if (flock(lockfd, LOCK_EX|LOCK_NB) == -1) {
> > [..]
> > gr_copy(pfd, tfd, gr, old_gr); /* copy from groupfile to tempfile */
> > [..]
> > rename(tempfile,groupfile);

> > Now let's consider threads A and B:

> > A: open()
> > A: lock();
> > A: gr_copy
> > B: open()

> > Now B has file descriptor to /etc/group that is about to be removed.

> > A: rename()
> > A: unlock()
> > B: lock()

> > Now B has a lock on unlinked file.

> > B: gr_copy()
> > B: rename()

> > ... and stores new content losing modifications done by A

> > Third, I don't like current api.
> > gr_lock and gr_tmp have no arguments (that matter anyway)
> > gr_copy operates on two descriptors given as arguments
> > gr_mkdb takes nothing and is expected to do The Right Thing

> gr_mkdb should chmod 0644 after renaming if rename worked.

> I should work on this soon.

> The API has been design to match the exact same api of pw_utils, I don't like it
> either but at least this is consistent.

> regards,
> Bapt

> First, pw should not fail if other instance is running, it should wait
> instead (think of parallel batch scripts adding some users/groups).

> Second, current code has a race:
> lockfd = open(group_file, O_RDONLY, 0);
> if (lockfd < 0 || fcntl(lockfd, F_SETFD, 1) == -1)
>    err(1, "%s", group_file);
> if (flock(lockfd, LOCK_EX|LOCK_NB) == -1) {
> [..]
> gr_copy(pfd, tfd, gr, old_gr); /* copy from groupfile to tempfile */
> [..]
> rename(tempfile,groupfile);

> Now let's consider threads A and B:

> A: open()
> A: lock();
> A: gr_copy
> B: open()

> Now B has file descriptor to /etc/group that is about to be removed.

> A: rename()
> A: unlock()
> B: lock()

> Now B has a lock on unlinked file.

> B: gr_copy()
> B: rename()

> ... and stores new content losing modifications done by A

On Wed, Nov 21, 2012 at 05:45:43PM +0200, Jaakko Heinonen wrote:
> On 2012-11-19, Mateusz Guzik wrote:
> > First, pw should not fail if other instance is running, it should wait
> > instead (think of parallel batch scripts adding some users/groups).

> > Second, current code has a race:
> > lockfd = open(group_file, O_RDONLY, 0);
> > if (lockfd < 0 || fcntl(lockfd, F_SETFD, 1) == -1)
> >       err(1, "%s", group_file);
> > if (flock(lockfd, LOCK_EX|LOCK_NB) == -1) {
> > [..]
> > gr_copy(pfd, tfd, gr, old_gr); /* copy from groupfile to tempfile */
> > [..]
> > rename(tempfile,groupfile);

> Hmm, could using the O_EXLOCK flag for open() instead of flock() help here?

> /etc/group is supposed to be world-reable, right?  Tools like groups or pw
> groupshow certainly seem to think so:

> [rstone@rstone-server ~]groups
> 1001 920
> [rstone@rstone-server ~]ls -l /etc/group
> -rw-------  1 root  0  482 Nov 14 21:02 /etc/group
> [rstone@rstone-server ~]sudo chmod a+r /etc/group
> Password:
> [rstone@rstone-server ~]groups
> rstone vboxusers
> [rstone@rstone-server ~]sudo pw groupadd foo
> [rstone@rstone-server ~]ls -l /etc/group
> -rw-------  1 root  0  494 Nov 17 10:19 /etc/group
> [rstone@rstone-server ~]

> I'm not sure what caused the regression.  I've been seeing the problem
> since I first installed -CURRENT on the machine a couple of weeks ago.
> _______________________________________________
> freebsd-curr...@freebsd.org mailing list
> http://lists.freebsd.org/mailman/listinfo/freebsd-current
> To unsubscribe, send any mail to "freebsd-current-unsubscr...@freebsd.org"