[PATCH 1/2] proc: restrict access to /proc/PID/io

[PATCH 1/2] proc: restrict access to /proc/PID/io

[Vasiliy Kulikov]

/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@openwall.com>
---
 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;
+
 	if (whole && lock_task_sighand(task, &flags)) {
 		struct task_struct *t = task;
 
@@ -2843,7 +2846,7 @@ static const struct pid_entry tgid_base_stuff[] = {
 	REG("coredump_filter", S_IRUGO|S_IWUSR, proc_coredump_filter_operations),
 #endif
 #ifdef CONFIG_TASK_IO_ACCOUNTING
-	INF("io",	S_IRUGO, proc_tgid_io_accounting),
+	INF("io",	S_IRUSR, proc_tgid_io_accounting),
 #endif
 #ifdef CONFIG_HARDWALL
 	INF("hardwall",   S_IRUGO, proc_pid_hardwall),
@@ -3185,7 +3188,7 @@ static const struct pid_entry tid_base_stuff[] = {
 	REG("make-it-fail", S_IRUGO|S_IWUSR, proc_fault_inject_operations),
 #endif
 #ifdef CONFIG_TASK_IO_ACCOUNTING
-	INF("io",	S_IRUGO, proc_tid_io_accounting),
+	INF("io",	S_IRUSR, proc_tid_io_accounting),
 #endif
 #ifdef CONFIG_HARDWALL
 	INF("hardwall",   S_IRUGO, proc_pid_hardwall),
-- 
1.7.0.4

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

[KOSAKI Motohiro]

(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@openwall.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?


> ---
>  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(). However I think your code is ok.
because a few bytes io accounting leak has no big matter.

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

[Vasiliy Kulikov]

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@openwall.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.

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).


Thanks,

-- 
Vasiliy Kulikov
http://www.openwall.com - bringing security into open computing environments

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

[KOSAKI Motohiro]

(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@openwall.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).

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

[Vasiliy Kulikov]

(cc'ed Linus)

On Mon, Jun 27, 2011 at 16:33 +0900, KOSAKI Motohiro wrote:
> (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@openwall.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. :)

Yep, Linus has already suggested a similar thing:
http://www.openwall.com/lists/oss-security/2011/06/27/4

As to random bytes - if it is very predictable (e.g. rand() % 10000) one
may restore the original value.  But it would still do much harm to good
programs (io stats going up and down!).  Adding really random bytes
seems somewhat too complicated for these needs to me.

As to rounding - this is a workaround, not a fix.  What if some program
reads one byte from tty and then do some io activity exactly of 1kb-1?
Then you just measure kbs and get original tty activity.  (just a crazy
example to show that it is not a full solution.)


Without any proof/estimate, just an idea: it's possible to get not only
password length, which needs as much granularity as possible, but
another information, which doesn't need any strict value.  E.g.
statistics changing, what should indicate that some significant event
has just happened.


> >>> ---
> >>>  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?

In the code I see taskstats statistics is kept untouched, so it would
break userspace assumptions about the statistics.


> >> 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).

(the same should go into the taskstats fix) 

Looks sane, thank you!


-- 
Vasiliy Kulikov
http://www.openwall.com - bringing security into open computing environments

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

[KOSAKI Motohiro]

(2011/06/27 17:52), Vasiliy Kulikov wrote:
> (cc'ed Linus)
> 
> On Mon, Jun 27, 2011 at 16:33 +0900, KOSAKI Motohiro wrote:
>> (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@openwall.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. :)
> 
> Yep, Linus has already suggested a similar thing:
> http://www.openwall.com/lists/oss-security/2011/06/27/4
> 
> As to random bytes - if it is very predictable (e.g. rand() % 10000) one
> may restore the original value.  But it would still do much harm to good
> programs (io stats going up and down!).  Adding really random bytes
> seems somewhat too complicated for these needs to me.
> 
> As to rounding - this is a workaround, not a fix.  What if some program
> reads one byte from tty and then do some io activity exactly of 1kb-1?
> Then you just measure kbs and get original tty activity.  (just a crazy
> example to show that it is not a full solution.)
> 
> 
> Without any proof/estimate, just an idea: it's possible to get not only
> password length, which needs as much granularity as possible, but
> another information, which doesn't need any strict value.  E.g.
> statistics changing, what should indicate that some significant event
> has just happened.

I'm ok any alternative way if you have. I only want to don't break iotop.
It's used very widely and frequently from performance tuning engineers.

I'm sorry. I can't answer your story is real threat or not. I'm not security
expert. I don't have enough knowledge.

Thanks.

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

[Vasiliy Kulikov]

On Mon, Jun 27, 2011 at 19:07 +0900, KOSAKI Motohiro wrote:
> I'm ok any alternative way if you have. I only want to don't break iotop.
> It's used very widely and frequently from performance tuning engineers.

It doesn't break listening for own processes statistics.  It disables
spying for alien processes.  Or is iotop does something useful using
other users' processes information?  The only thing I can think about is
watching for the related processes running under the separate account
(e.g. separate DB and web user accounts).

For performance things IMO the precise io activity is needed anyway.  If
you have to watch for other processes, you probably have to be root
(which is overkill, though).


Don't know whether it can help - I've suggested the way to define a
group allowed to read somewhat private procfs files:

http://www.openwall.com/lists/kernel-hardening/2011/06/15/19

It is incomplete (in sense of taskstats missing) and it is a bit dumb/not
configurable.  Here I suggest a bit more configurable scheme:

http://www.openwall.com/lists/kernel-hardening/2011/06/22/1


Thanks,

-- 
Vasiliy Kulikov
http://www.openwall.com - bringing security into open computing environments

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

[Solar Designer]

On Mon, Jun 27, 2011 at 12:52:42PM +0400, Vasiliy Kulikov wrote:
> As to random bytes - if it is very predictable (e.g. rand() % 10000) one
> may restore the original value.  But it would still do much harm to good
> programs (io stats going up and down!).  Adding really random bytes
> seems somewhat too complicated for these needs to me.

Random noise doesn't help very much even if it's totally unpredictable
and even if it's much louder than the signal.  It will only increase the
number of samples needed to see the signal through the noise.

More effective ways to deal with side-channel attacks are to make things
appear constant or, better yet, to remove the side-channel altogether
if possible.  I'd happily break iotop for non-admins on many of my
systems, so please give me a way to do it.

http://en.wikipedia.org/wiki/Side_channel_attack#Countermeasures

Alexander

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

[Balbir Singh]

On Mon, Jun 27, 2011 at 2:22 PM, Vasiliy Kulikov <segoon@openwall.com> wrote:
> (cc'ed Linus)
>
> On Mon, Jun 27, 2011 at 16:33 +0900, KOSAKI Motohiro wrote:
>> (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@openwall.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. :)
>
> Yep, Linus has already suggested a similar thing:
> http://www.openwall.com/lists/oss-security/2011/06/27/4
>

I think this is a good idea

> As to random bytes - if it is very predictable (e.g. rand() % 10000) one
> may restore the original value.  But it would still do much harm to good
> programs (io stats going up and down!).  Adding really random bytes
> seems somewhat too complicated for these needs to me.
>
> As to rounding - this is a workaround, not a fix.  What if some program
> reads one byte from tty and then do some io activity exactly of 1kb-1?
> Then you just measure kbs and get original tty activity.  (just a crazy
> example to show that it is not a full solution.)
>

That would happen with a probability of 1/1024

>
> Without any proof/estimate, just an idea: it's possible to get not only
> password length, which needs as much granularity as possible, but
> another information, which doesn't need any strict value.  E.g.
> statistics changing, what should indicate that some significant event
> has just happened.
>

Statistics would change a KB granularity, so yes if large KB's of IO
happen, you can figure things are changing (even top can give that
information), but that also means you have larger area to look at,
with small changes they might go unnoticed unless the KB boundary is
hit.

>
>> >>> ---
>> >>>  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?
>
> In the code I see taskstats statistics is kept untouched, so it would
> break userspace assumptions about the statistics.
>

Could you please elaborate on this point?

>
>> >> 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).
>
> (the same should go into the taskstats fix)
>
> Looks sane, thank you!

Balbir Singh

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

[Vasiliy Kulikov]

On Tue, Jun 28, 2011 at 06:54 +0530, Balbir Singh wrote:
> > As to rounding - this is a workaround, not a fix.  What if some program
> > reads one byte from tty and then do some io activity exactly of 1kb-1?
> > Then you just measure kbs and get original tty activity.  (just a crazy
> > example to show that it is not a full solution.)
> >
> 
> That would happen with a probability of 1/1024

I'd not claim about probability here, but anyway rounding would be not
a fix, just a workaround.  Also note that the random value is program
dependent, it is not chosen at the program start time or anything
similar.  IOW, if the program is vulnerable, it is vulnerable with 100%
probability.


> >> > 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?
> >
> > In the code I see taskstats statistics is kept untouched, so it would
> > break userspace assumptions about the statistics.
> >
> 
> Could you please elaborate on this point?

It's better to consider my phrase as a question :)  Like this: don't
userspace programs rely on the fact that all statistic information
wouldn't be cleared after execve()/set*id()/prctl() ?  I'm not familiar
with taskstats at all, so I don't know whether this assumption makes
sense to userspace application.


Thanks,

-- 
Vasiliy Kulikov
http://www.openwall.com - bringing security into open computing environments

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

[Balbir Singh]

On Tue, Jun 28, 2011 at 1:20 PM, Vasiliy Kulikov <segoon@openwall.com> wrote:
> On Tue, Jun 28, 2011 at 06:54 +0530, Balbir Singh wrote:
>> > As to rounding - this is a workaround, not a fix.  What if some program
>> > reads one byte from tty and then do some io activity exactly of 1kb-1?
>> > Then you just measure kbs and get original tty activity.  (just a crazy
>> > example to show that it is not a full solution.)
>> >
>>
>> That would happen with a probability of 1/1024
>
> I'd not claim about probability here, but anyway rounding would be not
> a fix, just a workaround.  Also note that the random value is program
> dependent, it is not chosen at the program start time or anything
> similar.  IOW, if the program is vulnerable, it is vulnerable with 100%
> probability.
>

I was thinking along ASLR lines, ASLR reduces the probability of
malware finding specific address in code, but does not eliminate it
completely. The other possibility is that we do both ROUND_UP and
ROUND_DOWN as long as we keep the statistics as progressively
increasing. I am trying to find a way to make the statistics available
without compromising the system. In the worst case as you suggest may
be the statistics would be available only to root, but that is the
final drop down scenario.

>
>> >> > 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?
>> >
>> > In the code I see taskstats statistics is kept untouched, so it would
>> > break userspace assumptions about the statistics.
>> >
>>
>> Could you please elaborate on this point?
>
> It's better to consider my phrase as a question :)  Like this: don't
> userspace programs rely on the fact that all statistic information
> wouldn't be cleared after execve()/set*id()/prctl() ?  I'm not familiar
> with taskstats at all, so I don't know whether this assumption makes
> sense to userspace application.

No we don't clear taskstats info on credential changes.

Balbir Singh

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

[Vasiliy Kulikov]

On Wed, Jun 29, 2011 at 06:46 +0530, Balbir Singh wrote:
> On Tue, Jun 28, 2011 at 1:20 PM, Vasiliy Kulikov <segoon@openwall.com> wrote:
> > On Tue, Jun 28, 2011 at 06:54 +0530, Balbir Singh wrote:
> >> > As to rounding - this is a workaround, not a fix.  What if some program
> >> > reads one byte from tty and then do some io activity exactly of 1kb-1?
> >> > Then you just measure kbs and get original tty activity.  (just a crazy
> >> > example to show that it is not a full solution.)
> >> >
> >>
> >> That would happen with a probability of 1/1024
> >
> > I'd not claim about probability here, but anyway rounding would be not
> > a fix, just a workaround.  Also note that the random value is program
> > dependent, it is not chosen at the program start time or anything
> > similar.  IOW, if the program is vulnerable, it is vulnerable with 100%
> > probability.
> >
> 
> I was thinking along ASLR lines, ASLR reduces the probability of
> malware finding specific address in code, but does not eliminate it
> completely.

You confuse a bug fix with a prevention of an exploitation technique
here.  ASLR doesn't fix anything, it tries to break exploits that use
bugs like arbitrary writes/reads.  If there is arbitrary write bug,
almost always the game is over; that's why such probabilistic measure is
acceptable.  On the contrary, /proc/*/io leak is a bug, which is fairly
fixable by restricting an access (breaking programs, though).  So, from
the security point of view these cases are not comparable.


> In the worst case as you suggest may
> be the statistics would be available only to root, but that is the
> final drop down scenario.

Yes, it breaks iotop, but it is a full solution.


> No we don't clear taskstats info on credential changes.

If taskstats info is allowed to travel through credential changes, it
exposes the similar private information.


Thanks,

-- 
Vasiliy Kulikov
http://www.openwall.com - bringing security into open computing environments

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

[Balbir Singh]

On Fri, Jun 24, 2011 at 5:38 PM, Vasiliy Kulikov <segoon@openwall.com> 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.
>

Hmm.. How do I reproduce this, won't they be enough randomness around
rchar/wchar by the time the attacker reads it?

> 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.

Balbir Singh

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

[Balbir Singh]

On Tue, Jun 28, 2011 at 6:43 AM, Balbir Singh <balbir@linux.vnet.ibm.com> wrote:
> On Fri, Jun 24, 2011 at 5:38 PM, Vasiliy Kulikov <segoon@openwall.com> 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.
>>
>
> Hmm.. How do I reproduce this, won't they be enough randomness around
> rchar/wchar by the time the attacker reads it?
>
>> 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.

Please NOTE my email address has changed to bsingharora@gmail.com, the
last email has an invalid from header.
I apologize for the inconvenience

Balbir Singh

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

[Vasiliy Kulikov]

On Tue, Jun 28, 2011 at 06:43 +0530, Balbir Singh wrote:
> On Fri, Jun 24, 2011 at 5:38 PM, Vasiliy Kulikov <segoon@openwall.com> 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.
> >
> 
> Hmm.. How do I reproduce this,

Just register taskstats listener and wait for "vsftpd" process.
read_characters = strlen("USER username\r\n") + strlen("PASSWD
pass\r\n") + 1.

> won't they be enough randomness around
> rchar/wchar by the time the attacker reads it?

No, if you set "UsePrivilegeSeparation yes" in /etc/sshd/sshd_config or
similar setting in vsftpd config (set by default), one process will
have very small io activity, which is 100% related to the io in
question.

http://www.openwall.com/lists/oss-security/2011/06/24/6

So, the total io = io with the network + io with privileged parent.


Thanks,

-- 
Vasiliy Kulikov
http://www.openwall.com - bringing security into open computing environments