Date: Sun, 4 Aug 2013 12:03:04 +0200 From: Jilles Tjoelker <jilles@stack.nl> To: Hiroki Sato <hrs@FreeBSD.org> Cc: svn-src-head@FreeBSD.org, sjg@FreeBSD.org, svn-src-all@FreeBSD.org, src-committers@FreeBSD.org Subject: Re: svn commit: r253887 - head/sys/dev/filemon Message-ID: <20130804100304.GB35080@stack.nl> In-Reply-To: <20130804.121523.488481502477873993.hrs@allbsd.org> References: <201308021444.r72EiBk2059771@svn.freebsd.org> <20130802152204.GA1880@stack.nl> <20130804.121523.488481502477873993.hrs@allbsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Sun, Aug 04, 2013 at 12:15:23PM +0900, Hiroki Sato wrote:
> Jilles Tjoelker <jilles@stack.nl> wrote
> in <20130802152204.GA1880@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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20130804100304.GB35080>