linux-kernel - Re: [PATCH 1/2] proc: restrict access to /proc/PID/io

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <4E08324D.9040605@jp.fujitsu.com>
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).


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/