From owner-freebsd-arch Mon Feb 5 7:45:16 2001 Delivered-To: freebsd-arch@freebsd.org Received: from fledge.watson.org (fledge.watson.org [204.156.12.50]) by hub.freebsd.org (Postfix) with ESMTP id 52AC737B491 for ; Mon, 5 Feb 2001 07:44:55 -0800 (PST) Received: from fledge.watson.org (robert@fledge.pr.watson.org [192.0.2.3]) by fledge.watson.org (8.11.1/8.11.1) with SMTP id f15FiWh83452; Mon, 5 Feb 2001 10:44:32 -0500 (EST) (envelope-from robert@fledge.watson.org) Date: Mon, 5 Feb 2001 10:44:32 -0500 (EST) From: Robert Watson X-Sender: robert@fledge.watson.org To: Nathan Gould Cc: freebsd-arch@FreeBSD.ORG Subject: Re: Tests for NULL p_ucred under p_cred -- are they needed? In-Reply-To: <3A7E767B.6AADB3B5@zoo.co.uk> Message-ID: MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII Sender: owner-freebsd-arch@FreeBSD.ORG Precedence: bulk X-Loop: FreeBSD.ORG On Mon, 5 Feb 2001, Nathan Gould wrote: > Robert Watson wrote: > > > I've noticed that at various points in the kernel code, there are tests to > > check that the ucred structure in a proc is non-NULL before using it. > > Under what circumstances do we believe it is possible for the ucred > > pointer to be non-NULL? It seems that, in normal usage, it should always > > be defined--the only points where it might be NULL would be during process > > creation and process exit. Are these windows long enough for it to be a > > concern? Are appropriate process locks held, under SMPng, such that it's > > never possible to grab a ucred structure for a process while it is NULL? > > > > It seems that there are other components of the code that assume that if > > (p) is non-NULL, then a ucred must be defined for the process, which seems > > like a consistent assumption assuming appropriate protections are in > > place. > > Surely, if for no other reason, we should be checking for abnormalities > such as non-Null for security reasons i.e. security breaches tend to be > based on non-corformance to publicised identified usage. Well, in the event that the credential was NULL, a number of chunks of code currently present would simply panic; my question was about whether or not those chunks of code are incorrect, or whether we can trim out all the conditionals that test (p_cred) (et al); here are a few samples where there is a conditional: kern_proc.c:392 fill_kinfo_proc(p, kp) struct proc *p; struct kinfo_proc *kp; { ... if (p->p_cred) { kp->ki_uid = p->p_cred->pc_ucred->cr_uid; kp->ki_ruid = p->p_cred->p_ruid; kp->ki_svuid = p->p_cred->p_svuid; ... kern_proc.c:600, 606 static int sysctl_kern_proc(SYSCTL_HANDLER_ARGS) { ... case KERN_PROC_UID: if (p->p_ucred == NULL || p->p_ucred->cr_uid != (uid_t)name[0]) continue; break; ... case KERN_PROC_RUID: if (p->p_ucred == NULL || p->p_cred->p_ruid != (uid_t)name[0]) continue; break; } It appears to me that a struct proc should always have a defined p_cred, although there does appear to be a small window in fork1() where it has been added to the global process list and the struct proc is not yet fully initialized. However, the p_cred pointer in that case is the parent's value; and all processes appear to inherit their credential from proc0 which has one hard-coded in init_main.c. kern_exit.c appears to hold the process lock while releasing both the ucred and cred structures; it's possible there is a window there also because the process isn't removed from some of it's inter-process relationships (pgrp, zombproc, p_sibling) until after the credential has been freed, and the process lock has been released. However, there is a fair amount of code that seems to assume the credential is always defined; largely, that appears to be the case for code that acts on behalf of the process: maybe the key here is that a process's credentials must always be defined between the end of fork1() and the beginning of exit(), meaning that when a process itself requests a service, it will be defined and can be relied on, but during process creation/teardown, the credential may be NULL and therefore code acting on the process cannot assume that the credential exists. Not that procfs chooses to ignore processes without credentials: procfs_vnops.c: 407 static int procfs_getattr(ap) struct vop_getattr_args /* { struct vnode *a_vp; struct vattr *a_vap; struct ucred *a_cred; struct proc *a_p; } */ *ap; { ... default: procp = PFIND(pfs->pfs_pid); if (procp == 0 || procp->p_cred == NULL || procp->p_ucred == NULL) return (ENOENT); The code snippets above came from sysctl() code where a process is retrieving information on other processes, similarly. An exception to this would be in Poul-Henning's p_trespass() from RELENG_4 and early RELENG_5, where p_trespass() is invoked on processes that may receive signals, but without a credential==NULL check that I can find (this is from RELENG_4_2_0_RELEASE): kern_prot.c: 966 int p_trespass(struct proc *p1, struct proc *p2) { ... if (p1->p_cred->p_ruid == p2->p_cred->p_ruid) return (0); As invoked from kern_sig.c kern_sig.c: 100, 876 #define CANSIGNAL(p, q, sig) \ (!p_trespass(p, q) || \ ((sig) == SIGCONT && (q)->p_session == (p)->p_session)) ... int kill(cp, uap) register struct proc *cp; register struct kill_args *uap; { ... /* kill single process */ if ((p = pfind(uap->pid)) == NULL) return (ESRCH); if (!CANSIGNAL(cp, p, uap->signum)) return (EPERM); In any case, there seems to be some inconsistency. It would seem that either (a) it is an invariant that p_cred is non-NULL for all reachable processes via various process lists (except unused processes), (b) it's an invariant that p_cred is non-NULL between the end of fork1() and the beginning of exit(), and that p_cred is therefore always defined if you're acting on behalf of the process, but not necessarily if you're acting on the process. Clearly, (1) would make life easier, and mean we could remove a fair number of checks. However, it may be that (b) is the case, in which case the signal code might require fixing, or the invariants it depends on at least require documenting. This relevant also as I overhaul the process access control routines, because I need to know if it's possible to have processes without credentials, and if so, what it means. Robert N M Watson FreeBSD Core Team, TrustedBSD Project robert@fledge.watson.org NAI Labs, Safeport Network Services To Unsubscribe: send mail to majordomo@FreeBSD.org with "unsubscribe freebsd-arch" in the body of the message