[PATCH v3] sysctl: control functionality of /proc/pid/mem

[PATCH v3] sysctl: control functionality of /proc/pid/mem

[Kees Cook]

Add the "proc_pid_mem" sysctl to control whether or not /proc/pid/mem is
allowed to work: 0: disabled, 1: read only, 2: read/write (default).

Signed-off-by: Kees Cook <keescook@chromium.org>
---
v3:
 - document the default, thanks to Randy Dunlap.
 - remove needless CONFIG_PROC_FS checks, thanks to Eric W. Biederman.
v2:
 - fix memory leak in error path, thanks to Ingo Molnar.
---
 Documentation/sysctl/kernel.txt |   15 +++++++++++++++
 fs/proc/base.c                  |   14 +++++++++++++-
 kernel/sysctl.c                 |   10 ++++++++++
 3 files changed, 38 insertions(+), 1 deletions(-)

diff --git a/Documentation/sysctl/kernel.txt b/Documentation/sysctl/kernel.txt
index 8c20fbd..9e7bad6 100644
--- a/Documentation/sysctl/kernel.txt
+++ b/Documentation/sysctl/kernel.txt
@@ -56,6 +56,7 @@ show up in /proc/sys/kernel:
 - printk_delay
 - printk_ratelimit
 - printk_ratelimit_burst
+- proc_pid_mem
 - randomize_va_space
 - real-root-dev               ==> Documentation/initrd.txt
 - reboot-cmd                  [ SPARC only ]
@@ -477,6 +478,20 @@ send before ratelimiting kicks in.
 
 ==============================================================
 
+proc_pid_mem:
+
+This option can be used to select the level of access given to potential
+ptracers when using the per-process "mem" file in /proc/pid/mem.
+
+0 - Disable entirely.
+
+1 - Allow potential ptracers read access to process memory, but not writes.
+
+2 - Allow potential ptracers read and write access to process memory. This
+    is the default.
+
+==============================================================
+
 randomize_va_space:
 
 This option can be used to select the type of process address
diff --git a/fs/proc/base.c b/fs/proc/base.c
index 9cde9ed..53133c7 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -109,6 +109,8 @@ struct pid_entry {
 	union proc_op op;
 };
 
+int sysctl_proc_pid_mem = 2;
+
 #define NOD(NAME, MODE, IOP, FOP, OP) {			\
 	.name = (NAME),					\
 	.len  = sizeof(NAME) - 1,			\
@@ -699,9 +701,13 @@ static const struct file_operations proc_single_file_operations = {
 
 static int mem_open(struct inode* inode, struct file* file)
 {
-	struct task_struct *task = get_proc_task(file->f_path.dentry->d_inode);
+	struct task_struct *task;
 	struct mm_struct *mm;
 
+	if (sysctl_proc_pid_mem < 1)
+		return -EACCES;
+
+	task = get_proc_task(file->f_path.dentry->d_inode);
 	if (!task)
 		return -ESRCH;
 
@@ -726,6 +732,9 @@ static ssize_t mem_read(struct file * file, char __user * buf,
 	unsigned long src = *ppos;
 	struct mm_struct *mm = file->private_data;
 
+	if (sysctl_proc_pid_mem < 1)
+		return -EACCES;
+
 	if (!mm)
 		return 0;
 
@@ -770,6 +779,9 @@ static ssize_t mem_write(struct file * file, const char __user *buf,
 	unsigned long dst = *ppos;
 	struct mm_struct *mm = file->private_data;
 
+	if (sysctl_proc_pid_mem < 2)
+		return -EACCES;
+
 	if (!mm)
 		return 0;
 
diff --git a/kernel/sysctl.c b/kernel/sysctl.c
index f487f25..6fd4bc0 100644
--- a/kernel/sysctl.c
+++ b/kernel/sysctl.c
@@ -103,6 +103,7 @@ extern int percpu_pagelist_fraction;
 extern int compat_log;
 extern int latencytop_enabled;
 extern int sysctl_nr_open_min, sysctl_nr_open_max;
+extern int sysctl_proc_pid_mem;
 #ifndef CONFIG_MMU
 extern int sysctl_nr_trim_pages;
 #endif
@@ -1004,6 +1005,15 @@ static struct ctl_table kern_table[] = {
 		.proc_handler	= proc_dointvec,
 	},
 #endif
+	{
+		.procname	= "proc_pid_mem",
+		.data		= &sysctl_proc_pid_mem,
+		.maxlen		= sizeof(int),
+		.mode		= 0644,
+		.proc_handler	= proc_dointvec_minmax,
+		.extra1		= &zero,
+		.extra2		= &two,
+	},
 	{ }
 };
 
-- 
1.7.0.4

Re: [PATCH v3] sysctl: control functionality of /proc/pid/mem

[Vasiliy Kulikov]

(cc'ed kernel-hardening)

On Mon, Jan 23, 2012 at 13:21 -0800, Kees Cook wrote:
> 
> Add the "proc_pid_mem" sysctl to control whether or not /proc/pid/mem is
> allowed to work: 0: disabled, 1: read only, 2: read/write (default).
> diff --git a/kernel/sysctl.c b/kernel/sysctl.c
...
> index f487f25..6fd4bc0 100644
> --- a/kernel/sysctl.c
> +++ b/kernel/sysctl.c
> @@ -103,6 +103,7 @@ extern int percpu_pagelist_fraction;
>  extern int compat_log;
>  extern int latencytop_enabled;
>  extern int sysctl_nr_open_min, sysctl_nr_open_max;
> +extern int sysctl_proc_pid_mem;
>  #ifndef CONFIG_MMU
>  extern int sysctl_nr_trim_pages;
>  #endif
> @@ -1004,6 +1005,15 @@ static struct ctl_table kern_table[] = {
>  		.proc_handler	= proc_dointvec,
>  	},
>  #endif
> +	{
> +		.procname	= "proc_pid_mem",
> +		.data		= &sysctl_proc_pid_mem,
> +		.maxlen		= sizeof(int),
> +		.mode		= 0644,
> +		.proc_handler	= proc_dointvec_minmax,
> +		.extra1		= &zero,
> +		.extra2		= &two,
> +	},

While I fully agree that we should step by step opt out all deprecated and
legacy features, which are not widely used nowadays and are probably
dangerous from the security POV, but we should define a standart way
of doing it.  E.g. moving all such stuff to some sysctl group, not bloating
kernel.*.  Probably sysctl dir kernel.legacy.*?  Then this sysctl would be
kernel.legacy.proc_pid_mem.

Thanks,

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

Re: [PATCH v3] sysctl: control functionality of /proc/pid/mem

[Alexey Dobriyan]

On 1/24/12, Vasiliy Kulikov <segoon@openwall.com> wrote:
>> +		.procname	= "proc_pid_mem",
>> +		.data		= &sysctl_proc_pid_mem,
>> +		.maxlen		= sizeof(int),
>> +		.mode		= 0644,
>> +		.proc_handler	= proc_dointvec_minmax,
>> +		.extra1		= &zero,
>> +		.extra2		= &two,
>> +	},

>E.g. moving all such stuff to some sysctl group, not bloating
> kernel.*.

Ehh.
How bloat is measured in this case?

Re: [PATCH v3] sysctl: control functionality of /proc/pid/mem

[Vasiliy Kulikov]

On Tue, Jan 24, 2012 at 13:12 +0200, Alexey Dobriyan wrote:
> On 1/24/12, Vasiliy Kulikov <segoon@openwall.com> wrote:
> >> +		.procname	= "proc_pid_mem",
> >> +		.data		= &sysctl_proc_pid_mem,
> >> +		.maxlen		= sizeof(int),
> >> +		.mode		= 0644,
> >> +		.proc_handler	= proc_dointvec_minmax,
> >> +		.extra1		= &zero,
> >> +		.extra2		= &two,
> >> +	},
> 
> >E.g. moving all such stuff to some sysctl group, not bloating
> > kernel.*.
> 
> Ehh.
> How bloat is measured in this case?

Do we want to add such sort of sysctls "from time to time" when we
consider one or another feature as deprecated?  If yes, I'd group them
somehow, e.g. by introducing subdirectory inside of kernel.

Btw, kernel sysctl dir contains all sort of stuff which goes to "kernel"
as if it is "etc".  It already contains ftrace, perf, printk, scheduler,
ipc.  IMHO plain kernel hierarchy is not profitable in the long term.

Thanks,

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

Re: [PATCH v3] sysctl: control functionality of /proc/pid/mem

[Colin Walters]

On Mon, 2012-01-23 at 13:21 -0800, Kees Cook wrote:
> Add the "proc_pid_mem" sysctl to control whether or not /proc/pid/mem is
> allowed to work: 0: disabled, 1: read only, 2: read/write (default).

Both your git commit and the Documentation/ entry are totally missing a
rationale for this.  Why would an admin want to set this on or off?
What replaces it, if anything?  What are the tradeoffs with its
replacement?  What userspace programs might break when this is toggled?

Re: [PATCH v3] sysctl: control functionality of /proc/pid/mem

[Andrew Morton]

On Mon, 23 Jan 2012 13:21:15 -0800
Kees Cook <keescook@chromium.org> wrote:

> 
> Add the "proc_pid_mem" sysctl to control whether or not /proc/pid/mem is
> allowed to work: 0: disabled, 1: read only, 2: read/write (default).
> 
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
> v3:
>  - document the default, thanks to Randy Dunlap.
>  - remove needless CONFIG_PROC_FS checks, thanks to Eric W. Biederman.

I was wondering about that.  Is CONFIG_PROC_FS=n, CONFIG_SYSCTL=y an
impossible combination?  If so, why?

> --- a/Documentation/sysctl/kernel.txt
> +++ b/Documentation/sysctl/kernel.txt
> @@ -56,6 +56,7 @@ show up in /proc/sys/kernel:
>  - printk_delay
>  - printk_ratelimit
>  - printk_ratelimit_burst
> +- proc_pid_mem
>  - randomize_va_space
>  - real-root-dev               ==> Documentation/initrd.txt
>  - reboot-cmd                  [ SPARC only ]
> @@ -477,6 +478,20 @@ send before ratelimiting kicks in.
>  
>  ==============================================================
>  
> +proc_pid_mem:
> +
> +This option can be used to select the level of access given to potential
> +ptracers when using the per-process "mem" file in /proc/pid/mem.
> +
> +0 - Disable entirely.
> +
> +1 - Allow potential ptracers read access to process memory, but not writes.
> +
> +2 - Allow potential ptracers read and write access to process memory. This
> +    is the default.
> +
> +==============================================================

I agree with Colin on this (he stole my line!).



Overall, the patch looks really hacky and random.  I felt the same way
as Vasily: it's easy to see how a significant number of similar (and
hacky and random) patches could be added, resulting in a regrettable
mess.

Is there some better designed, more organized way of approaching all of
this?  Random ideas:

- A parallel /procfs-perms filesystem.  You write a number into
  /procfs-perms/stat to affect access to /proc/stat (although why the
  heck not just run `chmod /proc/stat'?) It's unclear how to handle
  /proc/pid/.  Perhaps literally have a /procfs-perms/pid/ directory.

- Make tasks inherit their /proc/pid/* permissions across fork, do a
  chmod /proc/1/whatever in initscripts.

- Other and better things ;)   This particular approach makes my toes
  curl.

Re: [PATCH v3] sysctl: control functionality of /proc/pid/mem

[Eric W. Biederman]

Andrew Morton <akpm@linux-foundation.org> writes:

> On Mon, 23 Jan 2012 13:21:15 -0800
> Kees Cook <keescook@chromium.org> wrote:
>
>> 
>> Add the "proc_pid_mem" sysctl to control whether or not /proc/pid/mem is
>> allowed to work: 0: disabled, 1: read only, 2: read/write (default).
>> 
>> Signed-off-by: Kees Cook <keescook@chromium.org>
>> ---
>> v3:
>>  - document the default, thanks to Randy Dunlap.
>>  - remove needless CONFIG_PROC_FS checks, thanks to Eric W. Biederman.
>
> I was wondering about that.  Is CONFIG_PROC_FS=n, CONFIG_SYSCTL=y an
> impossible combination?  If so, why?

Fundamentally because the only way sysctls get to usespace is threw
/proc/sys/.  The binary sysctl emulation layer reads /proc/sys through
the internal kernel mount.

As I recall the symbol define tree:
CONFIG_PROC_SYSCTL select CONFIG_SYSCTL
CONFIG_PROC_SYSCTL depends on CONFIG_PROC_FS.
And the only way CONFIG_SYSCTL gets set is if it is select.

If anyone cares enough we can probably cleanup the Kconfig bits to have
fewer symbols.

At a practical level I think the real reason I objected is that it is
ugly to just dump things into kernel/sysctl.c with #defines everywhere.

Eric


>> --- a/Documentation/sysctl/kernel.txt
>> +++ b/Documentation/sysctl/kernel.txt
>> @@ -56,6 +56,7 @@ show up in /proc/sys/kernel:
>>  - printk_delay
>>  - printk_ratelimit
>>  - printk_ratelimit_burst
>> +- proc_pid_mem
>>  - randomize_va_space
>>  - real-root-dev               ==> Documentation/initrd.txt
>>  - reboot-cmd                  [ SPARC only ]
>> @@ -477,6 +478,20 @@ send before ratelimiting kicks in.
>>  
>>  ==============================================================
>>  
>> +proc_pid_mem:
>> +
>> +This option can be used to select the level of access given to potential
>> +ptracers when using the per-process "mem" file in /proc/pid/mem.
>> +
>> +0 - Disable entirely.
>> +
>> +1 - Allow potential ptracers read access to process memory, but not writes.
>> +
>> +2 - Allow potential ptracers read and write access to process memory. This
>> +    is the default.
>> +
>> +==============================================================
>
> I agree with Colin on this (he stole my line!).
>
>
>
> Overall, the patch looks really hacky and random.  I felt the same way
> as Vasily: it's easy to see how a significant number of similar (and
> hacky and random) patches could be added, resulting in a regrettable
> mess.
>
> Is there some better designed, more organized way of approaching all of
> this?  Random ideas:
>
> - A parallel /procfs-perms filesystem.  You write a number into
>   /procfs-perms/stat to affect access to /proc/stat (although why the
>   heck not just run `chmod /proc/stat'?) It's unclear how to handle
>   /proc/pid/.  Perhaps literally have a /procfs-perms/pid/ directory.
>
> - Make tasks inherit their /proc/pid/* permissions across fork, do a
>   chmod /proc/1/whatever in initscripts.
>
> - Other and better things ;)   This particular approach makes my toes
>   curl.

Re: [PATCH v3] sysctl: control functionality of /proc/pid/mem

[Kees Cook]

On Tue, Jan 24, 2012 at 3:51 AM, Vasiliy Kulikov <segoon@openwall.com> wrote:
> On Tue, Jan 24, 2012 at 13:12 +0200, Alexey Dobriyan wrote:
>> On 1/24/12, Vasiliy Kulikov <segoon@openwall.com> wrote:
>> >> +          .procname       = "proc_pid_mem",
>> >> +          .data           = &sysctl_proc_pid_mem,
>> >> +          .maxlen         = sizeof(int),
>> >> +          .mode           = 0644,
>> >> +          .proc_handler   = proc_dointvec_minmax,
>> >> +          .extra1         = &zero,
>> >> +          .extra2         = &two,
>> >> +  },
>>
>> >E.g. moving all such stuff to some sysctl group, not bloating
>> > kernel.*.
>>
>> Ehh.
>> How bloat is measured in this case?
>
> Do we want to add such sort of sysctls "from time to time" when we
> consider one or another feature as deprecated?  If yes, I'd group them
> somehow, e.g. by introducing subdirectory inside of kernel.
>
> Btw, kernel sysctl dir contains all sort of stuff which goes to "kernel"
> as if it is "etc".  It already contains ftrace, perf, printk, scheduler,
> ipc.  IMHO plain kernel hierarchy is not profitable in the long term.

Yeah, after reconsidering this, this sysctl is not the right approach.

-Kees

-- 
Kees Cook
ChromeOS Security

Re: [PATCH v3] sysctl: control functionality of /proc/pid/mem

[Kees Cook]

On Wed, Jan 25, 2012 at 3:30 PM, Andrew Morton
<akpm@linux-foundation.org> wrote:
> On Mon, 23 Jan 2012 13:21:15 -0800
> Kees Cook <keescook@chromium.org> wrote:
>> Add the "proc_pid_mem" sysctl to control whether or not /proc/pid/mem is
>> allowed to work: 0: disabled, 1: read only, 2: read/write (default).
>
> I agree with Colin on this (he stole my line!).
>
> Overall, the patch looks really hacky and random.  I felt the same way
> as Vasily: it's easy to see how a significant number of similar (and
> hacky and random) patches could be added, resulting in a regrettable
> mess.
>
> Is there some better designed, more organized way of approaching all of
> this?  Random ideas:
>
> - A parallel /procfs-perms filesystem.  You write a number into
>  /procfs-perms/stat to affect access to /proc/stat (although why the
>  heck not just run `chmod /proc/stat'?) It's unclear how to handle
>  /proc/pid/.  Perhaps literally have a /procfs-perms/pid/ directory.

This seems like too much overhead to me.

> - Make tasks inherit their /proc/pid/* permissions across fork, do a
>  chmod /proc/1/whatever in initscripts.

This is actually pretty interesting. I think as long as it only
allowed the _reduction_ of access. I don't want a process to open up
access, just restrict it further. It does make me wonder what the
side-effects would be, though. Dropping user r/w perms to things for
non-DAC_OVERRIDE processes would be ... weird.

-Kees

-- 
Kees Cook
ChromeOS Security