Subject: Re: procfs/ptrace/systrace/ktrace diff
To: None <elad@NetBSD.org>
From: YAMAMOTO Takashi <yamt@mwd.biglobe.ne.jp>
List: tech-security
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