Subject: Re: procfs/ptrace/systrace/ktrace diff
To: YAMAMOTO Takashi <yamt@mwd.biglobe.ne.jp>
From: Elad Efrat <elad@NetBSD.org>
List: tech-security
Date: 11/26/2006 15:46:44
This is a multi-part message in MIME format.
--Boundary_(ID_o62MmSkV3iKvaQAGFvYhOQ)
Content-type: text/plain; charset=ISO-8859-1
Content-transfer-encoding: 7BIT
if there are no objections, I'll commit the attached diff. changes from
previous diff posted:
- subsystem (procfs/ptrace/systrace/ktrace) moved to action itself
- proper context passed to authorization wrappers
- proc_isunder() moved to secmodel code
-e.
Elad Efrat wrote:
> YAMAMOTO Takashi wrote:
>
>>>> why this patch doesn't have amd64 part?
>>> what should we change for amd64?
>>>
>>> phyre:arch {31} egrep 'HAVE_(PTRACE|PROCFS)_MACHDEP' i386/include/*
>>> i386/include/ptrace.h:#define __HAVE_PTRACE_MACHDEP
>>> i386/include/ptrace.h:#define __HAVE_PROCFS_MACHDEP
>>> phyre:arch {32} egrep 'HAVE_(PTRACE|PROCFS)_MACHDEP' amd64/include/*
>>> phyre:arch {33}
>>>
>>> (I think whoever did amd64 did some heavy copy/paste ;)
>> if procfs_machdep_rw is merely dead code, it's fine.
>
> it's dead code. I'll remove it.
>
>>> proc_isunder() should be in the secmodel.
>> do you mean chroot(8) should be a part of secmodel?
>
> it already kinda is. we don't provide any context (yet) but there is
> a chroot action. I would like to move proc_isunder() to the secmodel
> code, yes.
>
>>>> does it mean to prohibit even reading of init's status if securelevel >= 0?
>>> yeah. can change, but again, we need to pass more context.
>> why don't you pass the necessary context?
>
> we have two args to play with. assuming they all need the tracer too,
> that leaves us with one argument free. that's not enough for at least
> procfs. if I can shift the subsystem to the action itself, as I
> suggested, I can pass more context.
>
>>>> i'm not sure if it's a good idea to make every callers of process_doXXX
>>>> use kauth_foo() directly. maybe it depends how much/kind of contexts you
>>>> will pass to listeners, tho.
>>> see above wrt/context. "<lwp> wants to use procfs to <r/w> on <node>",
>>> "<lwp> wants to use ptrace to <req> on <proc>", etc.
>> i wonder whether process_doXXX or its callers is a better place to
>> call kauth_foo().
>
> issue is that process_doXXX don't know who called them, ptrace or procfs
> or whatever subsystem that we may introduce in the future.
>
> -e.
>
--
Elad Efrat
--Boundary_(ID_o62MmSkV3iKvaQAGFvYhOQ)
Content-type: text/plain; name=proc.diff
Content-transfer-encoding: 7BIT
Content-disposition: inline; filename=proc.diff
Index: arch/i386/i386/process_machdep.c
===================================================================
RCS file: /usr/cvs/src/sys/arch/i386/i386/process_machdep.c,v
retrieving revision 1.59
diff -u -p -r1.59 process_machdep.c
--- arch/i386/i386/process_machdep.c 16 Nov 2006 01:32:38 -0000 1.59
+++ arch/i386/i386/process_machdep.c 24 Nov 2006 13:25:00 -0000
@@ -524,9 +524,6 @@ process_machdep_doxmmregs(curl, l, uio)
char *kv;
int kl;
- if ((error = process_checkioperm(curl, l->l_proc)) != 0)
- return (error);
-
kl = sizeof(r);
kv = (char *) &r;
Index: arch/powerpc/powerpc/process_machdep.c
===================================================================
RCS file: /usr/cvs/src/sys/arch/powerpc/powerpc/process_machdep.c,v
retrieving revision 1.21
diff -u -p -r1.21 process_machdep.c
--- arch/powerpc/powerpc/process_machdep.c 1 Mar 2006 12:38:12 -0000 1.21
+++ arch/powerpc/powerpc/process_machdep.c 24 Nov 2006 13:25:40 -0000
@@ -225,9 +225,6 @@ process_machdep_dovecregs(struct lwp *cu
char *kv;
int kl;
- if ((error = process_checkioperm(curl, l->l_proc)) != 0)
- return (error);
-
kl = sizeof(r);
kv = (char *) &r;
Index: miscfs/procfs/procfs_subr.c
===================================================================
RCS file: /usr/cvs/src/sys/miscfs/procfs/procfs_subr.c,v
retrieving revision 1.72
diff -u -p -r1.72 procfs_subr.c
--- miscfs/procfs/procfs_subr.c 16 Nov 2006 01:33:38 -0000 1.72
+++ miscfs/procfs/procfs_subr.c 24 Nov 2006 20:43:39 -0000
@@ -85,6 +85,7 @@ __KERNEL_RCSID(0, "$NetBSD: procfs_subr.
#include <sys/stat.h>
#include <sys/file.h>
#include <sys/filedesc.h>
+#include <sys/kauth.h>
#include <miscfs/procfs/procfs.h>
@@ -304,20 +305,27 @@ procfs_rw(v)
struct lwp *l;
struct pfsnode *pfs = VTOPFS(vp);
struct proc *p;
+ int error;
if (uio->uio_offset < 0)
return EINVAL;
p = PFIND(pfs->pfs_pid);
if (p == 0)
return ESRCH;
+
+ if (ISSET(p->p_flag, P_INEXEC))
+ return (EAGAIN);
+
+ curl = curlwp;
+
/*
* Do not allow init to be modified while in secure mode; it
* could be duped into changing the security level.
*/
- if (uio->uio_rw == UIO_WRITE && p == initproc && securelevel > -1)
- return EPERM;
-
- curl = curlwp;
+ error = kauth_authorize_process(curl->l_cred, KAUTH_PROCESS_CANPROCFS,
+ p, curl, pfs, KAUTH_ARG(uio->uio_rw == UIO_READ ? FREAD : FWRITE));
+ if (error)
+ return (error);
/* XXX NJWLWP
* The entire procfs interface needs work to be useful to
Index: miscfs/procfs/procfs_vnops.c
===================================================================
RCS file: /usr/cvs/src/sys/miscfs/procfs/procfs_vnops.c,v
retrieving revision 1.139
diff -u -p -r1.139 procfs_vnops.c
--- miscfs/procfs/procfs_vnops.c 25 Nov 2006 09:39:34 -0000 1.139
+++ miscfs/procfs/procfs_vnops.c 24 Nov 2006 20:43:48 -0000
@@ -288,15 +288,20 @@ procfs_open(v)
if (p2 == NULL)
return (ENOENT); /* was ESRCH, jsp */
+ if (ISSET(p2->p_flag, P_INEXEC))
+ return (EAGAIN);
+
+ error = kauth_authorize_process(l1->l_cred, KAUTH_PROCESS_CANPROCFS,
+ p2, l1, pfs, KAUTH_ARG(ap->a_mode));
+ if (error)
+ return (error);
+
switch (pfs->pfs_type) {
case PFSmem:
if (((pfs->pfs_flags & FWRITE) && (ap->a_mode & O_EXCL)) ||
((pfs->pfs_flags & O_EXCL) && (ap->a_mode & FWRITE)))
return (EBUSY);
- if ((error = process_checkioperm(l1, p2)) != 0)
- return (error);
-
if (ap->a_mode & FWRITE)
pfs->pfs_flags = ap->a_mode & (FWRITE|O_EXCL);
Index: kern/sys_process.c
===================================================================
RCS file: /usr/cvs/src/sys/kern/sys_process.c,v
retrieving revision 1.114
diff -u -p -r1.114 sys_process.c
--- kern/sys_process.c 13 Nov 2006 02:52:08 -0000 1.114
+++ kern/sys_process.c 25 Nov 2006 12:25:11 -0000
@@ -184,28 +184,24 @@ sys_ptrace(struct lwp *l, void *v, regis
* (4) it's not owned by you, or is set-id on exec
* (unless you're root), or...
*/
- if ((kauth_cred_getuid(t->p_cred) !=
- kauth_cred_getuid(l->l_cred) ||
- ISSET(t->p_flag, P_SUGID)) &&
- (error = kauth_authorize_generic(l->l_cred,
- KAUTH_GENERIC_ISSUSER, &l->l_acflag)) != 0)
- return (error);
/*
* (5) ...it's init, which controls the security level
* of the entire system, and the system was not
* compiled with permanently insecure mode turned on
*/
- if (t == initproc && securelevel > -1)
- return (EPERM);
+
+ error = kauth_authorize_process(l->l_cred,
+ KAUTH_PROCESS_CANPTRACE, t, l, KAUTH_ARG(SCARG(uap, req)),
+ NULL);
+ if (error)
+ return (error);
/*
* (6) the tracer is chrooted, and its root directory is
* not at or above the root directory of the tracee
*/
- if (!proc_isunder(t, l))
- return EPERM;
break;
case PT_READ_I:
@@ -329,6 +325,13 @@ sys_ptrace(struct lwp *l, void *v, regis
uio.uio_resid = sizeof(tmp);
uio.uio_rw = write ? UIO_WRITE : UIO_READ;
UIO_SETUP_SYSSPACE(&uio);
+
+ error = kauth_authorize_process(l->l_cred,
+ KAUTH_PROCESS_CANPTRACE, t, l, KAUTH_ARG(SCARG(uap, req)),
+ NULL);
+ if (error)
+ return (error);
+
error = process_domem(l, lt, &uio);
if (!write)
*retval = tmp;
@@ -361,6 +364,13 @@ sys_ptrace(struct lwp *l, void *v, regis
default:
return (EINVAL);
}
+
+ error = kauth_authorize_process(l->l_cred,
+ KAUTH_PROCESS_CANPTRACE, t, l, KAUTH_ARG(SCARG(uap, req)),
+ NULL);
+ if (error)
+ return (error);
+
error = process_domem(l, lt, &uio);
piod.piod_len -= uio.uio_resid;
(void) copyout(&piod, SCARG(uap, addr), sizeof(piod));
@@ -573,6 +583,13 @@ sys_ptrace(struct lwp *l, void *v, regis
uio.uio_resid = sizeof(struct reg);
uio.uio_rw = write ? UIO_WRITE : UIO_READ;
uio.uio_vmspace = vm;
+
+ error = kauth_authorize_process(l->l_cred,
+ KAUTH_PROCESS_CANPTRACE, t, l,
+ KAUTH_ARG(SCARG(uap, req)), NULL);
+ if (error)
+ return (error);
+
error = process_doregs(l, lt, &uio);
uvmspace_free(vm);
return error;
@@ -611,6 +628,13 @@ sys_ptrace(struct lwp *l, void *v, regis
uio.uio_resid = sizeof(struct fpreg);
uio.uio_rw = write ? UIO_WRITE : UIO_READ;
uio.uio_vmspace = vm;
+
+ error = kauth_authorize_process(l->l_cred,
+ KAUTH_PROCESS_CANPTRACE, t, l,
+ KAUTH_ARG(SCARG(uap, req)), NULL);
+ if (error)
+ return (error);
+
error = process_dofpregs(l, lt, &uio);
uvmspace_free(vm);
return error;
@@ -619,6 +643,12 @@ sys_ptrace(struct lwp *l, void *v, regis
#ifdef __HAVE_PTRACE_MACHDEP
PTRACE_MACHDEP_REQUEST_CASES
+ error = kauth_authorize_process(l->l_cred,
+ KAUTH_PROCESS_CANPTRACE, t, l, KAUTH_ARG(SCARG(uap, req)),
+ NULL);
+ if (error)
+ return (error);
+
return (ptrace_machdep_dorequest(l, lt,
SCARG(uap, req), SCARG(uap, addr),
SCARG(uap, data)));
@@ -637,7 +667,6 @@ process_doregs(struct lwp *curl /*tracer
struct uio *uio)
{
#if defined(PT_GETREGS) || defined(PT_SETREGS)
- struct proc *p = l->l_proc;
int error;
struct reg r;
char *kv;
@@ -646,8 +675,8 @@ process_doregs(struct lwp *curl /*tracer
if (uio->uio_offset < 0 || uio->uio_offset > (off_t)sizeof(r))
return EINVAL;
- if ((error = process_checkioperm(curl, p)) != 0)
- return error;
+ if (ISSET(l->l_proc->p_flag, P_INEXEC))
+ return (EAGAIN);
kl = sizeof(r);
kv = (char *)&r;
@@ -695,7 +724,6 @@ process_dofpregs(struct lwp *curl /*trac
struct uio *uio)
{
#if defined(PT_GETFPREGS) || defined(PT_SETFPREGS)
- struct proc *p = l->l_proc;
int error;
struct fpreg r;
char *kv;
@@ -704,8 +732,8 @@ process_dofpregs(struct lwp *curl /*trac
if (uio->uio_offset < 0 || uio->uio_offset > (off_t)sizeof(r))
return EINVAL;
- if ((error = process_checkioperm(curl, p)) != 0)
- return (error);
+ if (ISSET(l->l_proc->p_flag, P_INEXEC))
+ return (EAGAIN);
kl = sizeof(r);
kv = (char *)&r;
@@ -763,6 +791,7 @@ process_domem(struct lwp *curl /*tracer*
vaddr_t addr;
#endif
+ error = 0;
len = uio->uio_resid;
if (len == 0)
@@ -772,8 +801,8 @@ process_domem(struct lwp *curl /*tracer*
addr = uio->uio_offset;
#endif
- if ((error = process_checkioperm(curl, p)) != 0)
- return (error);
+ if (ISSET(p->p_flag, P_INEXEC))
+ return (EAGAIN);
vm = p->p_vmspace;
Index: kern/kern_systrace.c
===================================================================
RCS file: /usr/cvs/src/sys/kern/kern_systrace.c,v
retrieving revision 1.61
diff -u -p -r1.61 kern_systrace.c
--- kern/kern_systrace.c 1 Nov 2006 10:17:58 -0000 1.61
+++ kern/kern_systrace.c 24 Nov 2006 19:16:39 -0000
@@ -1204,6 +1204,11 @@ systrace_io(struct str_process *strp, st
uio.uio_resid = io->strio_len;
uio.uio_vmspace = l->l_proc->p_vmspace;
+ error = kauth_authorize_process(l->l_cred, KAUTH_PROCESS_CANSYSTRACE,
+ t, NULL, NULL, NULL);
+ if (error)
+ return (error);
+
#ifdef __NetBSD__
error = process_domem(l, proc_representative_lwp(t), &uio);
#else
@@ -1267,11 +1272,6 @@ systrace_attach(struct fsystrace *fst, p
* special privileges using setuid() from being
* traced. This is good security.]
*/
- if ((kauth_cred_getuid(proc->p_cred) != kauth_cred_getuid(p->p_cred) ||
- ISSET(proc->p_flag, P_SUGID)) &&
- (error = kauth_authorize_generic(p->p_cred, KAUTH_GENERIC_ISSUSER,
- &p->p_acflag)) != 0)
- goto out;
/*
* (5) ...it's init, which controls the security level
@@ -1279,10 +1279,11 @@ systrace_attach(struct fsystrace *fst, p
* compiled with permanently insecure mode turned
* on.
*/
- if ((proc->p_pid == 1) && (securelevel > -1)) {
- error = EPERM;
+
+ error = kauth_authorize_process(kauth_cred_get(),
+ KAUTH_PROCESS_CANSYSTRACE, proc, NULL, NULL, NULL);
+ if (error)
goto out;
- }
error = systrace_insert_process(fst, proc, NULL);
Index: kern/kern_ktrace.c
===================================================================
RCS file: /usr/cvs/src/sys/kern/kern_ktrace.c,v
retrieving revision 1.111
diff -u -p -r1.111 kern_ktrace.c
--- kern/kern_ktrace.c 1 Nov 2006 10:17:58 -0000 1.111
+++ kern/kern_ktrace.c 24 Nov 2006 19:15:12 -0000
@@ -1292,16 +1292,8 @@ ktrace_thread(void *arg)
int
ktrcanset(struct lwp *calll, struct proc *targetp)
{
- kauth_cred_t caller = calll->l_cred;
- kauth_cred_t target = targetp->p_cred;
-
- if ((kauth_cred_geteuid(caller) == kauth_cred_getuid(target) &&
- kauth_cred_getuid(target) == kauth_cred_getsvuid(target) &&
- kauth_cred_getgid(caller) == kauth_cred_getgid(target) && /* XXX */
- kauth_cred_getgid(target) == kauth_cred_getsvgid(target) &&
- (targetp->p_traceflag & KTRFAC_ROOT) == 0 &&
- (targetp->p_flag & P_SUGID) == 0) ||
- kauth_cred_geteuid(caller) == 0)
+ if (kauth_authorize_process(calll->l_cred, KAUTH_PROCESS_CANKTRACE,
+ targetp, NULL, NULL, NULL) == 0)
return (1);
return (0);
Index: sys/kauth.h
===================================================================
RCS file: /usr/cvs/src/sys/sys/kauth.h,v
retrieving revision 1.23
diff -u -p -r1.23 kauth.h
--- sys/kauth.h 25 Nov 2006 20:50:20 -0000 1.23
+++ sys/kauth.h 24 Nov 2006 20:35:42 -0000
@@ -118,6 +118,10 @@ enum kauth_system_req {
enum {
KAUTH_PROCESS_CANSEE=1,
KAUTH_PROCESS_CANSIGNAL,
+ KAUTH_PROCESS_CANKTRACE,
+ KAUTH_PROCESS_CANPROCFS,
+ KAUTH_PROCESS_CANPTRACE,
+ KAUTH_PROCESS_CANSYSTRACE,
KAUTH_PROCESS_CORENAME,
KAUTH_PROCESS_RESOURCE,
KAUTH_PROCESS_SETID
Index: secmodel/bsd44/secmodel_bsd44_suser.c
===================================================================
RCS file: /usr/cvs/src/sys/secmodel/bsd44/secmodel_bsd44_suser.c,v
retrieving revision 1.16
diff -u -p -r1.16 secmodel_bsd44_suser.c
--- secmodel/bsd44/secmodel_bsd44_suser.c 16 Nov 2006 01:33:51 -0000 1.16
+++ secmodel/bsd44/secmodel_bsd44_suser.c 24 Nov 2006 20:48:38 -0000
@@ -211,6 +211,45 @@ secmodel_bsd44_suser_process_cb(kauth_cr
result = KAUTH_RESULT_ALLOW;
break;
+ case KAUTH_PROCESS_CANKTRACE:
+ if ((p->p_traceflag & KTRFAC_ROOT) || (p->p_flag & P_SUGID)) {
+ result = KAUTH_RESULT_DENY;
+ break;
+ }
+
+ if (kauth_cred_geteuid(cred) == kauth_cred_getuid(p->p_cred) &&
+ kauth_cred_getuid(cred) == kauth_cred_getsvuid(p->p_cred) &&
+ kauth_cred_getgid(cred) == kauth_cred_getgid(p->p_cred) &&
+ kauth_cred_getgid(cred) == kauth_cred_getsvgid(p->p_cred)) {
+ result = KAUTH_RESULT_ALLOW;
+ break;
+ }
+
+ result = KAUTH_RESULT_DENY;
+ break;
+
+ case KAUTH_PROCESS_CANPROCFS:
+ case KAUTH_PROCESS_CANPTRACE:
+ if (!proc_isunder(p, (struct lwp *)arg1)) {
+ result = KAUTH_RESULT_DENY;
+ break;
+ }
+ /*FALLTHROUGH*/
+ case KAUTH_PROCESS_CANSYSTRACE:
+ if (isroot) {
+ result = KAUTH_RESULT_ALLOW;
+ break;
+ }
+
+ if (kauth_cred_getuid(cred) != kauth_cred_getuid(p->p_cred) ||
+ ISSET(p->p_flag, P_SUGID)) {
+ result = KAUTH_RESULT_DENY;
+ break;
+ }
+
+ result = KAUTH_RESULT_ALLOW;
+ break;
+
case KAUTH_PROCESS_RESOURCE:
switch ((u_long)arg1) {
case KAUTH_REQ_PROCESS_RESOURCE_NICE:
Index: secmodel/bsd44/secmodel_bsd44_securelevel.c
===================================================================
RCS file: /usr/cvs/src/sys/secmodel/bsd44/secmodel_bsd44_securelevel.c,v
retrieving revision 1.16
diff -u -p -r1.16 secmodel_bsd44_securelevel.c
--- secmodel/bsd44/secmodel_bsd44_securelevel.c 22 Nov 2006 20:57:52 -0000 1.16
+++ secmodel/bsd44/secmodel_bsd44_securelevel.c 24 Nov 2006 19:29:56 -0000
@@ -227,11 +227,25 @@ secmodel_bsd44_securelevel_process_cb(ka
kauth_action_t action, void *cookie, void *arg0,
void *arg1, void *arg2, void *arg3)
{
+ struct proc *p;
int result;
result = KAUTH_RESULT_DENY;
+ p = arg0;
switch (action) {
+ case KAUTH_PROCESS_CANPROCFS:
+ case KAUTH_PROCESS_CANPTRACE:
+ case KAUTH_PROCESS_CANSYSTRACE:
+ if ((p == initproc) && (securelevel >= 0)) {
+ result = KAUTH_RESULT_DENY;
+ break;
+ }
+
+ result = KAUTH_RESULT_ALLOW;
+
+ break;
+
case KAUTH_PROCESS_CORENAME:
if (securelevel < 2)
result = KAUTH_RESULT_ALLOW;
--Boundary_(ID_o62MmSkV3iKvaQAGFvYhOQ)--