svn commit: r253887 - head/sys/dev/filemon

[Jilles Tjoelker]

On Sun, Aug 04, 2013 at 12:15:23PM +0900, Hiroki Sato wrote:
> Jilles Tjoelker <jilles at stack.nl> wrote
>   in <20130802152204.GA1880 at stack.nl>:

> ji> You can simplify the code using the fairly new pget(). This will also
> ji> fix the incorrect errno when the process does not exist (should be
> ji> [ESRCH]).
> ji>
> ji> This change is a step in the right direction but is incomplete. Although
> ji> the check protects currently running processes, I do not see how it
> ji> prevents tracing a process that gets the same PID again after the
> ji> original target process has exited. This not only leaks sensitive
> ji> information but may also prevent tracing by the legitimate owner of the
> ji> process (because only one filemon will write events for a process). This
> ji> could be fixed by setting filemon->pid = -1 in
> ji> filemon_wrapper_sys_exit() and not allowing P_WEXIT and zombies in
> ji> FILEMON_SET_PID (PGET_NOTWEXIT disallows both). An [EBUSY] when there is
> ji> already a filemon monitoring the process may also be useful (or writing
> ji> copies of the events to all attached filemons).

>  Thank you for your comments.  Can you review the attached patch?  If
>  there is no problem, I will commit this and MFC to stable branches.

> Index: sys/dev/filemon/filemon.c
> ===================================================================
> --- sys/dev/filemon/filemon.c	(revision 253911)
> +++ sys/dev/filemon/filemon.c	(working copy)
> @@ -164,13 +164,12 @@
> 
>  	/* Set the monitored process ID. */
>  	case FILEMON_SET_PID:
> -		p = pfind(*((pid_t *)data));
> -		if (p == NULL)
> -			return (EINVAL);
> -		error = p_candebug(curthread, p);
> -		if (error == 0)
> +		error = pget(*((pid_t *)data), PGET_CANDEBUG | PGET_NOTWEXIT,
> +		    &p);
> +		if (error == 0) {
>  			filemon->pid = p->p_pid;
> -		PROC_UNLOCK(p);
> +			PROC_UNLOCK(p);
> +		}
>  		break;
> 
>  	default:
> Index: sys/dev/filemon/filemon_wrapper.c
> ===================================================================
> --- sys/dev/filemon/filemon_wrapper.c	(revision 253911)
> +++ sys/dev/filemon/filemon_wrapper.c	(working copy)
> @@ -574,6 +574,7 @@
>  			    (uintmax_t)now.tv_sec, (uintmax_t)now.tv_usec);
> 
>  			filemon_output(filemon, filemon->msgbufr, len);
> +			filemon->pid = -1;
>  		}
> 
>  		/* Unlock the found filemon structure. */

This looks OK, but I have not tested it.

I think filemon_ioctl() may need to lock the struct filemon. Replacing
the fp seems particularly unsafe.

I did not fully know about the recursive effect on child processes when
I wrote my earlier mail. Filemon allows tracing setuid programs this
way, which may be a security risk and is not possible with ktrace/truss.
Perhaps it is best to commit this patch, but also add a warning to
filemon(4) that it should not be loaded on systems with untrusted users
or the permissions on /dev/filemon should be restricted (via
/etc/devfs.rules).

-- 
Jilles Tjoelker