From owner-svn-src-all@FreeBSD.ORG Sun Aug 4 10:03:21 2013 Return-Path: Delivered-To: svn-src-all@FreeBSD.org Received: from mx1.freebsd.org (mx1.freebsd.org [8.8.178.115]) (using TLSv1 with cipher ADH-AES256-SHA (256/256 bits)) (No client certificate requested) by hub.freebsd.org (Postfix) with ESMTP id 59EF1B67; Sun, 4 Aug 2013 10:03:21 +0000 (UTC) (envelope-from jilles@stack.nl) Received: from mx1.stack.nl (unknown [IPv6:2001:610:1108:5012::107]) (using TLSv1 with cipher ADH-CAMELLIA256-SHA (256/256 bits)) (No client certificate requested) by mx1.freebsd.org (Postfix) with ESMTPS id 1B2622431; Sun, 4 Aug 2013 10:03:21 +0000 (UTC) Received: from snail.stack.nl (snail.stack.nl [IPv6:2001:610:1108:5010::131]) by mx1.stack.nl (Postfix) with ESMTP id 70C661203C7; Sun, 4 Aug 2013 12:03:04 +0200 (CEST) Received: by snail.stack.nl (Postfix, from userid 1677) id 55EDC28494; Sun, 4 Aug 2013 12:03:04 +0200 (CEST) Date: Sun, 4 Aug 2013 12:03:04 +0200 From: Jilles Tjoelker To: Hiroki Sato Subject: Re: svn commit: r253887 - head/sys/dev/filemon Message-ID: <20130804100304.GB35080@stack.nl> References: <201308021444.r72EiBk2059771@svn.freebsd.org> <20130802152204.GA1880@stack.nl> <20130804.121523.488481502477873993.hrs@allbsd.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20130804.121523.488481502477873993.hrs@allbsd.org> User-Agent: Mutt/1.5.21 (2010-09-15) Cc: svn-src-head@FreeBSD.org, sjg@FreeBSD.org, svn-src-all@FreeBSD.org, src-committers@FreeBSD.org X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sun, 04 Aug 2013 10:03:21 -0000 On Sun, Aug 04, 2013 at 12:15:23PM +0900, Hiroki Sato wrote: > Jilles Tjoelker 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