Date: Mon, 5 Feb 2001 10:44:32 -0500 (EST) From: Robert Watson <rwatson@FreeBSD.ORG> To: Nathan Gould <ngould@zoo.co.uk> Cc: freebsd-arch@FreeBSD.ORG Subject: Re: Tests for NULL p_ucred under p_cred -- are they needed? Message-ID: <Pine.NEB.3.96L.1010205102219.74962L-100000@fledge.watson.org> In-Reply-To: <3A7E767B.6AADB3B5@zoo.co.uk>
next in thread | previous in thread | raw e-mail | index | archive | help
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?Pine.NEB.3.96L.1010205102219.74962L-100000>