From owner-freebsd-arch Mon May 7 13: 8:14 2001 Delivered-To: freebsd-arch@freebsd.org Received: from meow.osd.bsdi.com (meow.osd.bsdi.com [204.216.28.88]) by hub.freebsd.org (Postfix) with ESMTP id 90F3B37B422; Mon, 7 May 2001 13:08:05 -0700 (PDT) (envelope-from jhb@FreeBSD.org) Received: from laptop.baldwin.cx (john@jhb-laptop.osd.bsdi.com [204.216.28.241]) by meow.osd.bsdi.com (8.11.2/8.11.2) with ESMTP id f47K7uG88251; Mon, 7 May 2001 13:07:57 -0700 (PDT) (envelope-from jhb@FreeBSD.org) Message-ID: X-Mailer: XFMail 1.4.0 on FreeBSD X-Priority: 3 (Normal) Content-Type: text/plain; charset=us-ascii Content-Transfer-Encoding: 8bit MIME-Version: 1.0 In-Reply-To: Date: Mon, 07 May 2001 13:03:40 -0700 (PDT) From: John Baldwin To: Robert Watson Subject: RE: Patch to eliminate struct pcred Cc: arch@FreeBSD.org Sender: owner-freebsd-arch@FreeBSD.ORG Precedence: bulk X-Loop: FreeBSD.ORG On 07-May-01 Robert Watson wrote: > Index: compat/svr4/svr4_misc.c > =================================================================== > RCS file: /home/ncvs/src/sys/compat/svr4/svr4_misc.c,v > retrieving revision 1.30 > diff -u -r1.30 svr4_misc.c > --- compat/svr4/svr4_misc.c 2001/05/01 08:11:52 1.30 > +++ compat/svr4/svr4_misc.c 2001/05/06 00:43:54 > @@ -1294,13 +1294,8 @@ > /* > * Free up credentials. > */ > - PROC_LOCK(q); > - if (--q->p_cred->p_refcnt == 0) { > - crfree(q->p_ucred); > - uifree(q->p_cred->p_uidinfo); > - FREE(q->p_cred, M_SUBPROC); > - q->p_cred = NULL; > - } > + crfree(q->p_ucred); > + q->p_ucred = NULL; Removing the proc lock here looks suspicious, but I think it might mirror a change I just made to kern_exit.c in wait1(), in which case it is ok. > Index: kern/kern_exec.c > =================================================================== > RCS file: /home/ncvs/src/sys/kern/kern_exec.c,v > retrieving revision 1.126 > diff -u -r1.126 kern_exec.c > --- kern/kern_exec.c 2001/05/01 08:12:56 1.126 > +++ kern/kern_exec.c 2001/05/06 16:25:06 > @@ -104,8 +104,9 @@ > register struct execve_args *uap; > { > struct nameidata nd, *ndp; > + struct ucred *oldcred = p->p_ucred, *newcred; > register_t *stack_base; > - int error, len, i; > + int error, len, i, intrace; > struct image_params image_params, *imgp; > struct vattr attr; > int (*img_first) __P((struct image_params *)); > @@ -272,23 +273,31 @@ > p->p_flag &= ~P_PPWAIT; > wakeup((caddr_t)p->p_pptr); > } > + intrace = p->p_flag & P_TRACED; > + PROC_UNLOCK(p); This unlock is busted since we then try to unlock this lock again later on since you didn't remove the other unlocks. Also, this whole caching of the intrace flag is bogus too. If you read a value and release the lock, then you have now lost the ability to safely make decisions on the value you just read. You have to hold the lock over both reading the value and deciding what to do based on that value so that the entire thing is an "atomic" operation. For now, I would just revert the intrace changes to check the flag directly like the code does now and not add in this proc unlock. > /* > + * XXX: Note, the whole execve() is incredibly racey right now > + * with regards to debugging and privilege/credential management. > + * This MUST be fixed prior to any release. > + */ > + > + /* > * Implement image setuid/setgid. > * > * Don't honor setuid/setgid if the filesystem prohibits it or if > * the process is being traced. > */ > - if ((((attr.va_mode & VSUID) && p->p_ucred->cr_uid != attr.va_uid) || > - ((attr.va_mode & VSGID) && p->p_ucred->cr_gid != attr.va_gid)) && > - (imgp->vp->v_mount->mnt_flag & MNT_NOSUID) == 0 && > - (p->p_flag & P_TRACED) == 0) { > + newcred = NULL; > + if ((((attr.va_mode & VSUID) && oldcred->cr_uid != attr.va_uid) || > + ((attr.va_mode & VSGID) && oldcred->cr_gid != attr.va_gid)) && > + (imgp->vp->v_mount->mnt_flag & MNT_NOSUID) == 0 && intrace == 0) { > PROC_UNLOCK(p); > /* > * Turn off syscall tracing for set-id programs, except for > * root. > */ > - if (p->p_tracep && suser(p)) { > + if (p->p_tracep && suser_xxx(oldcred, NULL, PRISON_ROOT)) { > p->p_traceflag = 0; > vrele(p->p_tracep); > p->p_tracep = NULL; > @@ -296,25 +305,42 @@ > /* > * Set the new credentials. > */ > - p->p_ucred = crcopy(p->p_ucred); > + newcred = crdup(p->p_ucred); > if (attr.va_mode & VSUID) > - change_euid(p, attr.va_uid); > + change_euid(newcred, attr.va_uid); > if (attr.va_mode & VSGID) > - p->p_ucred->cr_gid = attr.va_gid; > + change_egid(newcred, attr.va_gid); > setsugid(p); > setugidsafety(p); > } else { > - if (p->p_ucred->cr_uid == p->p_cred->p_ruid && > - p->p_ucred->cr_gid == p->p_cred->p_rgid) > - p->p_flag &= ~P_SUGID; > + if (oldcred->cr_uid == oldcred->cr_ruid && > + oldcred->cr_gid == oldcred->cr_rgid) > + p->p_flag &= ~P_SUGID; /* XXX locking */ > PROC_UNLOCK(p); > } > > /* > * Implement correct POSIX saved-id behavior. > + * > + * XXX: determine whether tests and sets should occur on old or > + * new credentials. > */ > - p->p_cred->p_svuid = p->p_ucred->cr_uid; > - p->p_cred->p_svgid = p->p_ucred->cr_gid; > + if (p->p_ucred->cr_svuid != p->p_ucred->cr_uid || > + p->p_ucred->cr_svgid != p->p_ucred->cr_gid) { > + if (newcred != NULL) > + newcred = crdup(p->p_ucred); > + > + change_svuid(newcred, p->p_ucred->cr_uid); > + change_svgid(newcred, p->p_ucred->cr_gid); > + } > + > + if (newcred != NULL) { > + struct ucred *oldcred; > + > + oldcred = p->p_ucred; > + p->p_ucred = newcred; > + crfree(oldcred); > + } > > /* > * Store the vp for use in procfs -- John Baldwin -- http://www.FreeBSD.org/~jhb/ PGP Key: http://www.baldwin.cx/~john/pgpkey.asc "Power Users Use the Power to Serve!" - http://www.FreeBSD.org/ To Unsubscribe: send mail to majordomo@FreeBSD.org with "unsubscribe freebsd-arch" in the body of the message