Skip site navigation (1)Skip section navigation (2)
Date:      Fri, 2 Jan 2015 09:16:42 +0000 (GMT)
From:      Robert Watson <rwatson@FreeBSD.org>
To:        Konstantin Belousov <kostikbel@gmail.com>
Cc:        arch@freebsd.org
Subject:   Re: Disabling ptrace
Message-ID:  <alpine.BSF.2.11.1501020906300.69379@fledge.watson.org>
In-Reply-To: <20141230111941.GE42409@kib.kiev.ua>
References:  <20141230111941.GE42409@kib.kiev.ua>

next in thread | previous in thread | raw e-mail | index | archive | help
On Tue, 30 Dec 2014, Konstantin Belousov wrote:

> The question about a facility to disable introspection functionality (ptrace 
> etc) for a process was asked several times. The latest query made me 
> actually code the feature. Note that other systems, e.g. Linux and OSX, do 
> have similar facilities.
>
> Patch is below, it provides two new procctl(2) requests.  PROC_TRACE_ENABLE 
> enables or disables tracing.  It includes core dumping, ptrace, ktrace, 
> debugging sysctls and hwpmc.  PROC_TRACE_STATUS allows to get the tracing 
> state.
>
> Most interesting question is how should disabling of trace behave with 
> regard of fork and exec. IMO, the right model is to protect access to the 
> _program_ address space, which translates to inheritance of the attribute 
> for fork, and reenabling the tracing on exec. On the other hand, I 
> understand that some users want to inherit the tracing disable on exec, so 
> there are PROC_TRACE_SET_DISABLED and PROC_TRACE_SET_DISABLED_EXEC, the 
> later makes disable to be kept after exec.
>
> Note that it is trivial for root on the host to circumvent the feature.

I admit some curiosity about the use case as well -- if this is a security 
feature, possibly this should be a property of the credential rather than the 
process?  Among other things, this could help implement trace limitations in 
asynchronous contexts -- not that I'm aware of too many currently, but if 
you don't want the I/O of a program traced in the synchronous context, I 
guess it might follow that you wouldn't want that to happen asynchronously 
either (etc).

On the other hand, if this is a security feature, it doesn't really 'stand 
alone' very well, as in absence of a more complete policy, simply disabling 
introspection features doesn't provide guarantees, just annoyance until 
someone writes a suitable script to work around it.  This has been one of my 
objections to securelevel, FWIW: while the various bits building it are useful 
for admins (e.g., file flages), securelevel isn't a 'coherent' policy that 
gives you strong integrity properties, rather, a set of manual frobs and 
inconsistent design choices that mean it's very hard to imagine an admin ever 
getting it to be useful without running with read-only filesystems, etc (and 
even then).

In MAC policies such as Biba and MLS, we mediate the ability to debug target 
processes based on additional labels, but there framing integrity and 
confidentiality policies give the restriction a security context.  In the Mac 
OS X version of the MAC Framework, support for disabling debugging following 
execve() is slightly more mature in FreeBSD: in FreeBSD it's done based on 
label change (a la credential change), but in Mac OS X it tracks a policy 
entry point that decides whether the change has access-control significance 
(which means you can have information-flow tracking policies that manipulate 
labels but don't change functional aspects of the system, which isn't possible 
in the FreeBSD version currently).  There's a good argument we should pick up 
the Mac OS X design choice there at some point.

The reason I wonder about the use case is actually one to do with accurate 
performance debugging of systems: ssh, ssh-agent, and friends can already 
request that they not be core dumped to avoid leaking keying material to 
filesystems, but having them be exempt from hwpmc, DTrace, etc, will make 
understanding system and application behaviour harder -- and if they 'fail 
silently' then users could suffer potentially confusing (and misleading) 
results.  Is the general concern here that people are turning on 
tracing/profiling/etc tools and not understanding that they may be leaking 
keying material?

(Note that we do have a global safety frob to disable ptrace and friends 
systemically, security.bsd.unprivileged_proc_debug, which is intended as an 
easily available workaround for security vulnerabilities discovered in 
process-debugging facilities, which have historically proven a bit 
vulnerability-prone across all operating systems I'm aware of.  We should 
remember this next time we cut an advisory on a ptrace/ktrace/ec 
vulnerability.)

Robert

>
> diff --git a/lib/libc/sys/procctl.2 b/lib/libc/sys/procctl.2
> index 649e0ad..4e7e5b8 100644
> --- a/lib/libc/sys/procctl.2
> +++ b/lib/libc/sys/procctl.2
> @@ -29,7 +29,7 @@
> .\"
> .\" $FreeBSD$
> .\"
> -.Dd December 16, 2014
> +.Dd December 29, 2014
> .Dt PROCCTL 2
> .Os
> .Sh NAME
> @@ -275,6 +275,51 @@ delivery failed, e.g. due to the permission problems.
> If no such process exist, the
> .Fa rk_fpid
> field is set to -1.
> +.It Dv PROC_TRACE_ENABLE
> +Enable or disable tracing of the specified process(es), according to the
> +value of the integer argument.
> +Tracing includes attachment to the process using
> +.Xr ptrace 2
> +and
> +.Xr ktrace 2 ,
> +debugging sysctls,
> +.Xr hwpmc 4
> +and core dumping.
> +Possible values for the
> +.Fa data
> +argument are:
> +.Bl -tag -width "Dv PROC_TRACE_SET_DISABLED_EXEC"
> +.It Dv PROC_TRACE_SET_ENABLED
> +Enable the tracing, after it was disabled by
> +.Dv PROC_TRACE_SET_DISABLED .
> +Only allowed for self.
> +.It Dv PROC_TRACE_SET_DISABLED
> +Disable tracing for the specified process.
> +The tracing is re-enabled when the process changes the executing
> +program with
> +.Xr execve 2
> +syscall.
> +A child inherits the trace settings from the parent.
> +.It Dv PROC_TRACE_SET_DISABLED_EXEC
> +Same as
> +.Dv PROC_TRACE_SET_DISABLED ,
> +but setting persist for the process even after
> +.Xr execve 2 .
> +.It Dv PROC_TRACE_STATUS
> +Returns the current status of tracing for the specified process into
> +the integer variable pointed to by
> +.Fa data .
> +If tracing is disabled,
> +.Fa data
> +is set to -1.
> +If tracing is enabled, but no debugger is attached by
> +.Xr ptrace 2
> +syscall, the
> +.Fa data
> +is set to 0.
> +If a debugger is attached,
> +.Fa data
> +is set to the pid of the debugger process.
> .El
> .Sh RETURN VALUES
> If an error occurs, a value of -1 is returned and
> @@ -343,9 +388,28 @@ The
> .Dv PROC_REAP_ACQUIRE
> request was issued by a process that had already acquired reaper status
> and has not yet released it.
> +.It Bq Er EBUSY
> +The
> +.Dv PROC_TRACE_ENABLE
> +request was issued for the process already traced.
> +.It Bq Er EPERM
> +The
> +.Dv PROC_TRACE_ENABLE
> +request to re-enable tracing of the process (
> +.Dv PROC_TRACE_SET_ENABLED ) ,
> +or to disable persistence of the
> +.Dv PROC_TRACE_SET_DISABLED
> +on
> +.Xr execve 2
> +was issued for the non-current process.
> +.It Bq Er EINVAL
> +The value of integer parameter for the
> +.Dv PROC_TRACE_ENABLE
> +request is invalid.
> .El
> .Sh SEE ALSO
> .Xr kill 2 ,
> +.Xr ktrace 2 ,
> .Xr ptrace 2 ,
> .Xr wait 2 ,
> .Xr init 8
> diff --git a/sys/compat/freebsd32/freebsd32_misc.c b/sys/compat/freebsd32/freebsd32_misc.c
> index 1457f57..f96e78e 100644
> --- a/sys/compat/freebsd32/freebsd32_misc.c
> +++ b/sys/compat/freebsd32/freebsd32_misc.c
> @@ -2969,6 +2969,7 @@ freebsd32_procctl(struct thread *td, struct freebsd32_procctl_args *uap)
>
> 	switch (uap->com) {
> 	case PROC_SPROTECT:
> +	case PROC_TRACE_ENABLE:
> 		error = copyin(PTRIN(uap->data), &flags, sizeof(flags));
> 		if (error != 0)
> 			return (error);
> @@ -2997,6 +2998,9 @@ freebsd32_procctl(struct thread *td, struct freebsd32_procctl_args *uap)
> 			return (error);
> 		data = &x.rk;
> 		break;
> +	case PROT_TRACE_STATUS:
> +		data = &flags;
> +		break;
> 	default:
> 		return (EINVAL);
> 	}
> @@ -3012,6 +3016,10 @@ freebsd32_procctl(struct thread *td, struct freebsd32_procctl_args *uap)
> 		if (error == 0)
> 			error = error1;
> 		break;
> +	case PROC_TRACE_STATUS:
> +		if (error == 0)
> +			error = copyout(&flags, uap->data, sizeof(flags));
> +		break;
> 	}
> 	return (error);
> }
> diff --git a/sys/kern/kern_exec.c b/sys/kern/kern_exec.c
> index 19c33b6..5b80f5c 100644
> --- a/sys/kern/kern_exec.c
> +++ b/sys/kern/kern_exec.c
> @@ -634,6 +634,8 @@ interpret:
> 	 * it that it now has its own resources back
> 	 */
> 	p->p_flag |= P_EXEC;
> +	if ((p->p_flag2 & P2_NOTRACE_EXEC) == 0)
> +		p->p_flag2 &= ~P2_NOTRACE;
> 	if (p->p_flag & P_PPWAIT) {
> 		p->p_flag &= ~(P_PPWAIT | P_PPTRACE);
> 		cv_broadcast(&p->p_pwait);
> diff --git a/sys/kern/kern_fork.c b/sys/kern/kern_fork.c
> index f469db6..2c83422 100644
> --- a/sys/kern/kern_fork.c
> +++ b/sys/kern/kern_fork.c
> @@ -494,7 +494,7 @@ do_fork(struct thread *td, int flags, struct proc *p2, struct thread *td2,
> 	 * Increase reference counts on shared objects.
> 	 */
> 	p2->p_flag = P_INMEM;
> -	p2->p_flag2 = 0;
> +	p2->p_flag2 = p1->p_flag2 & (P2_NOTRACE | P2_NOTRACE_EXEC);
> 	p2->p_swtick = ticks;
> 	if (p1->p_flag & P_PROFIL)
> 		startprofclock(p2);
> diff --git a/sys/kern/kern_procctl.c b/sys/kern/kern_procctl.c
> index 9b0d14a..e265ed5 100644
> --- a/sys/kern/kern_procctl.c
> +++ b/sys/kern/kern_procctl.c
> @@ -280,6 +280,62 @@ reap_kill(struct thread *td, struct proc *p, struct procctl_reaper_kill *rk)
> 	return (error);
> }
>
> +static int
> +trace_enable(struct thread *td, struct proc *p, int state)
> +{
> +
> +	PROC_LOCK_ASSERT(p, MA_OWNED);
> +
> +	/*
> +	 * Ktrace changes p_traceflag from or to zero under the
> +	 * process lock, so the test does not need to acquire ktrace
> +	 * mutex.
> +	 */
> +	if ((p->p_flag & P_TRACED) != 0 || p->p_traceflag != 0)
> +		return (EBUSY);
> +
> +	switch (state) {
> +	case PROC_TRACE_SET_ENABLED:
> +		if (td->td_proc != p)
> +			return (EPERM);
> +		p->p_flag2 &= ~(P2_NOTRACE | P2_NOTRACE_EXEC);
> +		break;
> +	case PROC_TRACE_SET_DISABLED_EXEC:
> +		p->p_flag2 |= P2_NOTRACE_EXEC | P2_NOTRACE;
> +		break;
> +	case PROC_TRACE_SET_DISABLED:
> +		if ((p->p_flag2 & P2_NOTRACE_EXEC) != 0) {
> +			KASSERT((p->p_flag2 & P2_NOTRACE) != 0,
> +			    ("dandling P2_NOTRACE_EXEC"));
> +			if (td->td_proc != p)
> +				return (EPERM);
> +			p->p_flag2 &= ~P2_NOTRACE_EXEC;
> +		} else {
> +			p->p_flag2 |= P2_NOTRACE;
> +		}
> +		break;
> +	default:
> +		return (EINVAL);
> +	}
> +	return (0);
> +}
> +
> +static int
> +trace_status(struct thread *td, struct proc *p, int *data)
> +{
> +
> +	if ((p->p_flag2 & P2_NOTRACE) != 0) {
> +		KASSERT((p->p_flag & P_TRACED) == 0,
> +		    ("%d traced but tracing disabled", p->p_pid));
> +		*data = -1;
> +	} else if ((p->p_flag & P_TRACED) != 0) {
> +		*data = p->p_pptr->p_pid;
> +	} else {
> +		*data = 0;
> +	}
> +	return (0);
> +}
> +
> #ifndef _SYS_SYSPROTO_H_
> struct procctl_args {
> 	idtype_t idtype;
> @@ -302,6 +358,7 @@ sys_procctl(struct thread *td, struct procctl_args *uap)
>
> 	switch (uap->com) {
> 	case PROC_SPROTECT:
> +	case PROC_TRACE_ENABLE:
> 		error = copyin(uap->data, &flags, sizeof(flags));
> 		if (error != 0)
> 			return (error);
> @@ -328,6 +385,9 @@ sys_procctl(struct thread *td, struct procctl_args *uap)
> 			return (error);
> 		data = &x.rk;
> 		break;
> +	case PROC_TRACE_STATUS:
> +		data = &flags;
> +		break;
> 	default:
> 		return (EINVAL);
> 	}
> @@ -342,6 +402,10 @@ sys_procctl(struct thread *td, struct procctl_args *uap)
> 		if (error == 0)
> 			error = error1;
> 		break;
> +	case PROC_TRACE_STATUS:
> +		if (error == 0)
> +			error = copyout(&flags, uap->data, sizeof(flags));
> +		break;
> 	}
> 	return (error);
> }
> @@ -364,6 +428,10 @@ kern_procctl_single(struct thread *td, struct proc *p, int com, void *data)
> 		return (reap_getpids(td, p, data));
> 	case PROC_REAP_KILL:
> 		return (reap_kill(td, p, data));
> +	case PROC_TRACE_ENABLE:
> +		return (trace_enable(td, p, *(int *)data));
> +	case PROC_TRACE_STATUS:
> +		return (trace_status(td, p, data));
> 	default:
> 		return (EINVAL);
> 	}
> @@ -375,6 +443,7 @@ kern_procctl(struct thread *td, idtype_t idtype, id_t id, int com, void *data)
> 	struct pgrp *pg;
> 	struct proc *p;
> 	int error, first_error, ok;
> +	bool tree_locked;
>
> 	switch (com) {
> 	case PROC_REAP_ACQUIRE:
> @@ -382,6 +451,7 @@ kern_procctl(struct thread *td, idtype_t idtype, id_t id, int com, void *data)
> 	case PROC_REAP_STATUS:
> 	case PROC_REAP_GETPIDS:
> 	case PROC_REAP_KILL:
> +	case PROC_TRACE_STATUS:
> 		if (idtype != P_PID)
> 			return (EINVAL);
> 	}
> @@ -391,11 +461,17 @@ kern_procctl(struct thread *td, idtype_t idtype, id_t id, int com, void *data)
> 	case PROC_REAP_STATUS:
> 	case PROC_REAP_GETPIDS:
> 	case PROC_REAP_KILL:
> +	case PROC_TRACE_ENABLE:
> 		sx_slock(&proctree_lock);
> +		tree_locked = true;
> 		break;
> 	case PROC_REAP_ACQUIRE:
> 	case PROC_REAP_RELEASE:
> 		sx_xlock(&proctree_lock);
> +		tree_locked = true;
> +		break;
> +	case PROC_TRACE_STATUS:
> +		tree_locked = false;
> 		break;
> 	default:
> 		return (EINVAL);
> @@ -456,6 +532,7 @@ kern_procctl(struct thread *td, idtype_t idtype, id_t id, int com, void *data)
> 		error = EINVAL;
> 		break;
> 	}
> -	sx_unlock(&proctree_lock);
> +	if (tree_locked)
> +		sx_unlock(&proctree_lock);
> 	return (error);
> }
> diff --git a/sys/kern/kern_prot.c b/sys/kern/kern_prot.c
> index 2a667b5..a5d9596 100644
> --- a/sys/kern/kern_prot.c
> +++ b/sys/kern/kern_prot.c
> @@ -1714,6 +1714,10 @@ p_candebug(struct thread *td, struct proc *p)
> 	if ((p->p_flag & P_INEXEC) != 0)
> 		return (EBUSY);
>
> +	/* Denied explicitely */
> +	if ((p->p_flag2 & P2_NOTRACE) != 0)
> +		return (EPERM);
> +
> 	return (0);
> }
>
> diff --git a/sys/kern/kern_sig.c b/sys/kern/kern_sig.c
> index a4f0f88..c4025ba 100644
> --- a/sys/kern/kern_sig.c
> +++ b/sys/kern/kern_sig.c
> @@ -3243,7 +3243,8 @@ coredump(struct thread *td)
> 	MPASS((p->p_flag & P_HADTHREADS) == 0 || p->p_singlethread == td);
> 	_STOPEVENT(p, S_CORE, 0);
>
> -	if (!do_coredump || (!sugid_coredump && (p->p_flag & P_SUGID) != 0)) {
> +	if (!do_coredump || (!sugid_coredump && (p->p_flag & P_SUGID) != 0) ||
> +	    (p->p_flag2 & P2_NOTRACE) != 0) {
> 		PROC_UNLOCK(p);
> 		return (EFAULT);
> 	}
> diff --git a/sys/sys/proc.h b/sys/sys/proc.h
> index d7a45e9..4b660d5 100644
> --- a/sys/sys/proc.h
> +++ b/sys/sys/proc.h
> @@ -674,6 +674,8 @@ struct proc {
>
> /* These flags are kept in p_flag2. */
> #define	P2_INHERIT_PROTECTED 0x00000001 /* New children get P_PROTECTED. */
> +#define	P2_NOTRACE	0x00000002	/* No ptrace(2) attach or coredumps. */
> +#define	P2_NOTRACE_EXEC 0x00000004	/* Keep P2_NOPTRACE on exec(2). */
>
> /* Flags protected by proctree_lock, kept in p_treeflags. */
> #define	P_TREE_ORPHANED		0x00000001	/* Reparented, on orphan list */
> diff --git a/sys/sys/procctl.h b/sys/sys/procctl.h
> index d11b2b2..503c09b 100644
> --- a/sys/sys/procctl.h
> +++ b/sys/sys/procctl.h
> @@ -41,6 +41,8 @@
> #define	PROC_REAP_STATUS	4	/* reaping status */
> #define	PROC_REAP_GETPIDS	5	/* get descendants */
> #define	PROC_REAP_KILL		6	/* kill descendants */
> +#define	PROC_TRACE_ENABLE	7	/* en/dis ptrace and coredumps */
> +#define	PROC_TRACE_STATUS	8	/* query tracing status */
>
> /* Operations for PROC_SPROTECT (passed in integer arg). */
> #define	PPROT_OP(x)	((x) & 0xf)
> @@ -96,6 +98,10 @@ struct procctl_reaper_kill {
> #define	REAPER_KILL_CHILDREN	0x00000001
> #define	REAPER_KILL_SUBTREE	0x00000002
>
> +#define	PROC_TRACE_SET_ENABLED		1
> +#define	PROC_TRACE_SET_DISABLED		2
> +#define	PROC_TRACE_SET_DISABLED_EXEC	3
> +
> #ifndef _KERNEL
> __BEGIN_DECLS
> int	procctl(idtype_t, id_t, int, void *);
> _______________________________________________
> freebsd-arch@freebsd.org mailing list
> http://lists.freebsd.org/mailman/listinfo/freebsd-arch
> To unsubscribe, send any mail to "freebsd-arch-unsubscribe@freebsd.org"
>



Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?alpine.BSF.2.11.1501020906300.69379>