[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Mon, 27 Jun 2011 16:33:33 +0900
From: KOSAKI Motohiro <kosaki.motohiro@...fujitsu.com>
To: segoon@...nwall.com
CC: linux-kernel@...r.kernel.org, balbir@...ux.vnet.ibm.com,
akpm@...ux-foundation.org, viro@...iv.linux.org.uk, rientjes@...gle.com,
wilsons@...rt.ca, security@...nel.org, eparis@...hat.com,
kernel-hardening@...ts.openwall.com
Subject: Re: [PATCH 1/2] proc: restrict access to /proc/PID/io
(2011/06/27 16:03), Vasiliy Kulikov wrote:
> 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 <segoon@...nwall.com>
>>
>> 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.
Just idea. (perhaps it's too dumb).
If a user want to know throughput, usually they only need KB/s granularity.
If a user want to know password hints, they need to know strict bytes granularity.
So, adding some random bytes to this statistics may help to obscure key data,
or just "stat = ROUND_UP(stat, 1024)".
But, I hope to wait another experts response. they may know better approach. :)
> BTW, what to do with sched and status? It stores some sensitive
> information too (execution times and vm space, respectively).
Dunno. I'm not security expert.
>>> ---
>>> 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.
Can you please explain more? I'm feeling "reset at credential change" is
reasonable idea. How broken is there?
>> 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).
Umm. reasonable. task->signal->cred_guard_mutex can be used for preventing
exec() race, I think. (see check_mem_permission() and et al).
Powered by blists - more mailing lists
Confused about mailing lists and their use?
Read about mailing lists on Wikipedia
and check out these
guidelines on proper formatting of your messages.
