Subject: Re: procfs/ptrace/systrace/ktrace diff
To: None <elad@NetBSD.org>
From: YAMAMOTO Takashi <yamt@mwd.biglobe.ne.jp>
List: tech-kern
Date: 11/25/2006 22:10:40
why this patch doesn't have amd64 part?
> 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 22 Nov 2006 18:15:41 -0000
> @@ -472,6 +473,17 @@ ptrace_machdep_dorequest(
> struct iovec iov;
> int write = 0;
>
> + if (ISSET(lt->l_proc->p_flag, P_INEXEC))
> + return (EAGAIN);
> +
> + if (kauth_authorize_process(l->l_cred, KAUTH_PROCESS_CANTRACE,
> + lt->l_proc, (void *)KAUTH_REQ_PROCESS_CANTRACE_PTRACE, NULL,
> + NULL) != 0)
> + return (EPERM);
> +
> + if (!proc_isunder(lt->l_proc, l))
> + return (EPERM);
> +
i think it's better to put this kind of things in callers, rather than
MD code. same for procfs_machdep_rw.
> 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 21 Nov 2006 17:55:11 -0000
> @@ -314,7 +315,8 @@ procfs_rw(v)
> * 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)
> + if (kauth_authorize_process(kauth_cred_get(), KAUTH_PROCESS_CANTRACE,
> + p, (void *)KAUTH_REQ_PROCESS_CANTRACE_PROCFS, pfs, NULL) != 0)
> return EPERM;
>
> curl = curlwp;
please return the return value of kauth_foo() rather than hardcoding EPERM.
same for the rest of the patch.
> Index: miscfs/procfs/procfs_vnops.c
> ===================================================================
> RCS file: /usr/cvs/src/sys/miscfs/procfs/procfs_vnops.c,v
> retrieving revision 1.138
> diff -u -p -r1.138 procfs_vnops.c
> --- miscfs/procfs/procfs_vnops.c 16 Nov 2006 01:33:38 -0000 1.138
> +++ miscfs/procfs/procfs_vnops.c 22 Nov 2006 13:28:24 -0000
> @@ -288,15 +287,19 @@ procfs_open(v)
> if (p2 == NULL)
> return (ENOENT); /* was ESRCH, jsp */
>
> + if (kauth_authorize_process(kauth_cred_get(), KAUTH_PROCESS_CANTRACE,
> + p2, (void *)KAUTH_REQ_PROCESS_CANTRACE_PROCFS, pfs, NULL) != 0)
> + return (EPERM);
> +
> + if (!proc_isunder(p2, l1))
> + return (EPERM);
> +
> 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);
does it mean to prohibit even reading of init's status if securelevel >= 0?
> 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 22 Nov 2006 20:23:07 -0000
> @@ -184,19 +184,16 @@ 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)
> +
> + if (kauth_authorize_process(l->l_cred, KAUTH_PROCESS_CANTRACE,
> + t, (void *)KAUTH_REQ_PROCESS_CANTRACE_PTRACE, NULL,
> + NULL) != 0)
> return (EPERM);
will you fill comments?
> @@ -329,6 +326,12 @@ 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);
> +
> + if (kauth_authorize_process(l->l_cred, KAUTH_PROCESS_CANTRACE,
> + t, (void *)KAUTH_REQ_PROCESS_CANTRACE_PTRACE, NULL,
> + NULL) != 0)
> + return (EPERM);
> +
> error = process_domem(l, lt, &uio);
> if (!write)
> *retval = tmp;
> @@ -361,6 +364,12 @@ sys_ptrace(struct lwp *l, void *v, regis
> default:
> return (EINVAL);
> }
> +
> + if (kauth_authorize_process(l->l_cred, KAUTH_PROCESS_CANTRACE,
> + t, (void *)KAUTH_REQ_PROCESS_CANTRACE_PTRACE, NULL,
> + NULL) != 0)
> + return (EPERM);
> +
> error = process_domem(l, lt, &uio);
> piod.piod_len -= uio.uio_resid;
> (void) copyout(&piod, SCARG(uap, addr), sizeof(piod));
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.
> 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 21 Nov 2006 16:06:17 -0000
> @@ -211,6 +211,74 @@ secmodel_bsd44_suser_process_cb(kauth_cr
> result = KAUTH_RESULT_ALLOW;
> break;
>
> + case KAUTH_PROCESS_CANTRACE:
> + switch ((u_long)arg1) {
> + case KAUTH_REQ_PROCESS_CANTRACE_KTRACE:
> + if (isroot) {
> + result = KAUTH_RESULT_ALLOW;
> + break;
> + }
why to have the identical "isroot" handling for each cases?
> + case KAUTH_REQ_PROCESS_CANTRACE_PTRACE:
> + case KAUTH_REQ_PROCESS_CANTRACE_SYSTRACE:
these seem identical. why don't just fallthrough?
> 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 22 Nov 2006 14:54:19 -0000
> switch (action) {
> + case KAUTH_PROCESS_CANTRACE:
> + switch ((u_long)arg1) {
> + case KAUTH_REQ_PROCESS_CANTRACE_PROCFS:
> + case KAUTH_REQ_PROCESS_CANTRACE_PTRACE:
> + case KAUTH_REQ_PROCESS_CANTRACE_SYSTRACE:
again, these three seems identical.
YAMAMOTO Takashi