[ On Tuesday, March 4, 2003 at 18:52:08 (+0900), Noriyuki Soda wrote: ]
> Subject: Re: commoning up code that changes uids and gids
>
> >>>>> On Tue, 4 Mar 2003 09:20:31 +0000, David Laight <david@l8s.co.uk> said:
> 
> >> please don't add these unless you really really really really need to.
> 
> > On what basis?
> 
> Because using setreuid(2) is just wrong.

Indeed.  Just plain wrong.

> Such code should be rewritten by using saved UID feature.

I'm not so sure about that.  The only safe thing to do with root if
privileges must be retained is to fork a child process and only lower
privs in that child.  In principle the same rule should apply no matter
what the starting privilege "level", though of course the risk is much
lower if the starting "level" is not root.

I've made the following patches to my kernel (from the 1-6 branch), and
I have never defined ALLOW_SETRE_ID in any kernel I've built since:

Index: kern_prot.c
===================================================================
RCS file: /cvs/master/m-NetBSD/main/src/sys/kern/kern_prot.c,v
retrieving revision 1.68
diff -c -u -r1.68 kern_prot.c
--- kern_prot.c	6 Dec 2001 23:11:59 -0000	1.68
+++ kern_prot.c	20 Sep 2002 19:32:50 -0000
@@ -51,6 +51,7 @@
 
 #include <sys/param.h>
 #include <sys/acct.h>
+#include <sys/syslog.h>
 #include <sys/systm.h>
 #include <sys/ucred.h>
 #include <sys/proc.h>
@@ -422,7 +423,10 @@
 	} */ *uap = v;
 	struct pcred *pc = p->p_cred;
 	uid_t ruid, euid;
-	int error, changed = 0;
+	int error;
+#ifdef ALLOW_SETRE_ID
+	int changed = 0;
+#endif
 
 	ruid = SCARG(uap, ruid);
 	euid = SCARG(uap, euid);
@@ -438,6 +442,7 @@
 	    (error = suser(pc->pc_ucred, &p->p_acflag)))
 		return (error);
 
+#ifdef ALLOW_SETRE_ID
 	if (euid != (uid_t)-1 && euid != pc->pc_ucred->cr_uid) {
 		pc->pc_ucred = crcopy(pc->pc_ucred);
 		pc->pc_ucred->cr_uid = euid;
@@ -455,6 +460,18 @@
 
 	if (changed)
 		p_sugid(p);
+#else
+	log(LOG_ERR, "%s: pid %d [eid %d:%d, rid %d:%d] called setreuid(%d, %d)\n",
+	    p->p_comm,
+	    p->p_pid,
+	    p->p_ucred->cr_uid,
+	    p->p_ucred->cr_gid,
+	    p->p_cred->p_ruid == p->p_ucred->cr_uid ? p->p_cred->p_svuid : p->p_cred->p_ruid,
+	    p->p_cred->p_rgid == p->p_ucred->cr_gid ? p->p_cred->p_svgid : p->p_cred->p_rgid,
+	    ruid, euid);
+	uprintf("%s: [pid %d] sorry, setreuid() is not supported\n", p->p_comm, p->p_pid);
+#endif
+
 	return (0);
 }
 
@@ -533,7 +550,10 @@
 	} */ *uap = v;
 	struct pcred *pc = p->p_cred;
 	gid_t rgid, egid;
-	int error, changed = 0;
+	int error;
+#ifdef ALLOW_SETRE_ID
+	int changed = 0;
+#endif
 
 	rgid = SCARG(uap, rgid);
 	egid = SCARG(uap, egid);
@@ -549,6 +569,7 @@
 	    (error = suser(pc->pc_ucred, &p->p_acflag)))
 		return (error);
 
+#ifdef ALLOW_SETRE_ID
 	if (egid != (gid_t)-1 && pc->pc_ucred->cr_gid != egid) {
 		pc->pc_ucred = crcopy(pc->pc_ucred);
 		pc->pc_ucred->cr_gid = egid;
@@ -564,6 +585,18 @@
 
 	if (changed)
 		p_sugid(p);
+#else
+	log(LOG_ERR, "%s: pid %d [eid %d:%d, rid %d:%d] called setregid(%d, %d)\n",
+	    p->p_comm,
+	    p->p_pid,
+	    p->p_ucred->cr_uid,
+	    p->p_ucred->cr_gid,
+	    p->p_cred->p_ruid == p->p_ucred->cr_uid ? p->p_cred->p_svuid : p->p_cred->p_ruid,
+	    p->p_cred->p_rgid == p->p_ucred->cr_gid ? p->p_cred->p_svgid : p->p_cred->p_rgid,
+	    rgid, egid);
+	uprintf("%s: [pid %d] sorry, setregid() is not supported\n", p->p_comm, p->p_pid);
+#endif
+
 	return (0);
 }
 


-- 
								Greg A. Woods

+1 416 218-0098;            <g.a.woods@ieee.org>;           <woods@robohack.ca>
Planix, Inc. <woods@planix.com>; VE3TCP; Secrets of the Weird <woods@weird.com>