pw keeps setting /etc/group to 0600

[Ryan Stone]

/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...@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to "freebsd-curre...@freebsd.org"

[Ryan Stone]

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/


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. I
have no idea what's happening in my example above, though. Baptiste, I
have to ask: how much testing did r242349 receive before it was committed?

[Mateusz Guzik]

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/
>

This is not a bug and I think it always was this way. The process you used
to su/sudo/whatever to root was not in testing group and didn't
magically enter it after you added rstone user to that group. You have
to log in again or do stuff like exec su - rstone.

--
Mateusz Guzik <mjguzik gmail.com>

[Mateusz Guzik]

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

I think descriptos should be hidden. Also I think that passing around
struct gr_something (sorry, never had talent for names) that would
contain all necessary data would be nice.

--
Mateusz Guzik <mjguzik gmail.com>

[Baptiste Daroussin]

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

[Baptiste Daroussin]

Should be fixed now,

regards,
Bapt

[Jaakko Heinonen]

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?

> 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

--
Jaakko

[Mateusz Guzik]

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?
>

Yes, this would fix the race.

But the problem of pw exiting due to other process holding the lock
remains. And I think that fixing it will require holding a lock over
whole time pw is running so that we have stable snapshot of user base at
least in regard of local files.

One could create one lock, say /etc/.pw.lock, that would be used to
synchronize any changes to /etc/master.passwd, /etc/group and whatnot.

And then there is this API issue (but maybe this is just me
nitpicking).

--
Mateusz Guzik <mjguzik gmail.com>

[matt]

Interesting, I noticed my pw segfaulted twice on 'pw groupdel' twice out
of three groups deleted.

Not sure if related. I'm at r243502.

Matt