[lkml]   [2011]   [Jun]   [27]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
Messages in this thread
    SubjectRe: [PATCH 1/2] proc: restrict access to /proc/PID/io
    On Mon, Jun 27, 2011 at 11:58 +0900, KOSAKI Motohiro wrote:
    > (2011/06/24 21:08), Vasiliy Kulikov wrote:
    > > /proc/PID/io may be used for gathering private information. E.g. for
    > > openssh and vsftpd daemons wchars/rchars may be used to learn the
    > > precise password length. Restrict it to processes being able to ptrace
    > > the target process.
    > >
    > > ptrace_may_access() is needed to prevent keeping open file descriptor of
    > > "io" file, executing setuid binary and gathering io information of the
    > > setuid'ed process.
    > >
    > > Signed-off-by: Vasiliy Kulikov <>
    > This description seems makes sense to me. But Vasilly, I have one question.
    > Doesn't this change break iotop command or other userland tools?

    I don't use iotop, but after reading the sources it looks like it uses
    taskstats for information gathering, which will be broken for sure by
    the second patch. All other userland tools using alien io files will be
    broken too.

    I'd say the whole approach of world readable debugging/statistics
    information was broken from the beginning, now we are stuck with these
    interfaces because of acient mistakes.

    BTW, what to do with sched and status? It stores some sensitive
    information too (execution times and vm space, respectively).

    > > ---
    > > fs/proc/base.c | 7 +++++--
    > > 1 files changed, 5 insertions(+), 2 deletions(-)
    > >
    > > diff --git a/fs/proc/base.c b/fs/proc/base.c
    > > index 14def99..5ae25d1 100644
    > > --- a/fs/proc/base.c
    > > +++ b/fs/proc/base.c
    > > @@ -2712,6 +2712,9 @@ static int do_io_accounting(struct task_struct *task, char *buffer, int whole)
    > > struct task_io_accounting acct = task->ioac;
    > > unsigned long flags;
    > >
    > > + if (!ptrace_may_access(task, PTRACE_MODE_READ))
    > > + return -EACCES;
    > > +
    > I think this check need a comment. Usually procfs don't use ptrace_may_access() directly
    > (see mm_for_maps) because it's racy against exec().

    This makes sense. Reading /proc/self/io and exec'ing setuid program
    would cause the race. What lock should I use to block execve()?

    Also I'm worried about these statistics after dropping the privileges.
    After setuid() and similar things not changing pid unprivileged user
    gets some information about the previous io activity of this task being
    privileged. In some situations it doesn't reveal any sensitive
    information, in some it might. Clearing taskstats on credential
    changing functions would totally break taskstats' interfaces; and should
    be temporary changing fsuid/euid followed by reverting it considered
    harmfull? I don't know.

    > However I think your code is ok.
    > because a few bytes io accounting leak has no big matter.

    Please don't do any assumptions about the significance of these few
    bytes. It can be not "few" bytes if either the scheduler's granularity
    is significant or the scheduler does wrong assumptions about CPU speeds.
    Also if someone gets CAP_SYS_NICE he may totally break these assumptions.

    My ssh example is just a proof that io stat is harmfull *sometimes*.
    I didn't investigate in what cases it is harmless for sure (if it's
    possible at all).


    Vasiliy Kulikov - bringing security into open computing environments

     \ /
      Last update: 2011-06-27 09:17    [W:0.025 / U:27.548 seconds]
    ©2003-2017 Jasper Spaans. hosted at Digital OceanAdvertise on this site