The safe_setuid code

[CJNyfalt]

The safe_setuid_drop and safe_setuid_grab functions have a ridiculous number of ifdefs.

I suggests that at least SAFE_SETUID and SAFE_SETUID_POSIX is turned on permanently. For the first one, I see no reason to turn it off and for the second one, we assume in all other places that all Unix systems are Posix compatible by now.

Finally there is setegid vs setgid. Reading the manpages I can't figure out the difference. Anyone got a clue?

[takkaria]

Originally Posted by CJNyfalt

The safe_setuid_drop and safe_setuid_grab functions have a ridiculous number of ifdefs.

I suggests that at least SAFE_SETUID and SAFE_SETUID_POSIX is turned on permanently. For the first one, I see no reason to turn it off and for the second one, we assume in all other places that all Unix systems are Posix compatible by now.

Finally there is setegid vs setgid. Reading the manpages I can't figure out the difference. Anyone got a clue?

I'll have a look into these; for now, I googled up the following which look useful:

BSD man page for setegid
Linux man page for setegid

The Rationale section of the Open Group's setuid man page seems to explain it, but I haven't got the time right now to read it properly

This slide references a paper (PDF) which explains everything in a much lengthier fashion, which I might print out tonight and read.

[takkaria]

Originally Posted by takkaria

This slide references a paper (PDF) which explains everything in a much lengthier fashion, which I might print out tonight and read.

After reading the above paper and coming out a little confused still:

SAFE_SETUID_POSIX means "comply with the POSIX standard", but it's actually less secure than not having SAFE_SETUID_POSIX defined -- setregid has the better semantics over setgid, but it's not POSIX. This is therefore a better (I think) formulation of safe_setuid_grab():

EDIT: simplify code

void safe_setuid_grab(void)
{
#ifdef SET_UID
# if defined(HAVE_SETREGID)

	if (setregid(getegid(), getgid()) != 0)
		quit("setregid(): cannot set permissions correctly!");

# elif defined(HAVE_SETEGID)

	if (setegid(player_egid) != 0)
		quit("setegid(): cannot set permissions correctly!");

# else

	if (setgid(player_egid) != 0)
		quit("setgid(): cannot set permissions correctly!");

# endif /* HAVE_SETEGID */
#endif /* SET_UID */
}

[CJNyfalt]

Originally Posted by takkaria

This slide references a paper (PDF) which explains everything in a much lengthier fashion, which I might print out tonight and read.

You have to take a second look. The paper recommends using setresuid, which is not setreuid. See section 4.4.
In order of preference, see the slide and section 8.1.1 of the pdf:
1. setresgid (Which we doesn't have code for. Note the s.)
2. setegid (Recommends setregid for cases that we doesn't use.)
3. setgid

"Since setresuid has a clear semantics and is able to set
each user ID individually, it should always be used if
available. Otherwise, to set only the effective uid, se-
teuid(new euid) should be used; ....

setuid should be avoided ...."

[CJNyfalt]

The code should look like this, note the changes in the first part in bold:

void safe_setuid_grab(void)
{
#ifdef SET_UID
# if defined(HAVE_SETRESGID)

	if (setresgid(-1, player_egid, -1) != 0)
		quit("setresgid(): cannot set permissions correctly!");

# elif defined(HAVE_SETEGID)

	if (setegid(player_egid) != 0)
		quit("setegid(): cannot set permissions correctly!");

# else

	if (setgid(player_egid) != 0)
		quit("setgid(): cannot set permissions correctly!");

# endif /* HAVE_SETEGID */
#endif /* SET_UID */
}

[takkaria]

Originally Posted by CJNyfalt

The code should look like this, note the changes in the first part in bold:

(snip)

Got it, thanks. As I said, I came out confused.

[Neil Stevens]

Beware what you do with the setuid stuff if you intend on supporting multiuser Angband on Darwin. Apple's apparently does something odd/buggy/whatever. I'm sorry that I can't tell you exactly what's wrong; we don't use setuid on ToME anymore so the issue became moot.

[CJNyfalt]

Originally Posted by Neil Stevens

Beware what you do with the setuid stuff if you intend on supporting multiuser Angband on Darwin. Apple's apparently does something odd/buggy/whatever. I'm sorry that I can't tell you exactly what's wrong; we don't use setuid on ToME anymore so the issue became moot.

What do you then do with high-scores (and player ghosts if you have them)? Private for each user or none at all?

[takkaria]

Originally Posted by Neil Stevens

Beware what you do with the setuid stuff if you intend on supporting multiuser Angband on Darwin. Apple's apparently does something odd/buggy/whatever. I'm sorry that I can't tell you exactly what's wrong; we don't use setuid on ToME anymore so the issue became moot.

I don't intend to support multiuser Angband on Darwin, so it's all good.

[Neil Stevens]

Originally Posted by takkaria

I don't intend to support multiuser Angband on Darwin, so it's all good.

People with X servers will be sad, heh.