sysctl: control functionality of /proc/pid/mem

PATCH In review Kees Cook 6 participants 9 messages Jan 23, 2012 – Jan 31, 2012

Subsystems (3)

Documentation Proc filesystem Proc sysctl

lore

Share & tools

lore

Kees Cook

author

2012-01-23 21:32 UTC #0 lore raw

PATCH v3 1/1 sysctl: control functionality of /proc/pid/mem

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 <[email protected]>

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

Documentation/sysctl/kernel.txt +15 -0

index 8c20fbd..9e7bad6 100644
@@ -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

fs/proc/base.c +13 -1

index 9cde9ed..53133c7 100644
@@ -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;

kernel/sysctl.c +10 -0

index f487f25..6fd4bc0 100644
@@ -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,
+	},
 	{ }
 };

Signature · 1 line

1.7.0.4

Vasiliy Kulikov

2012-01-24 11:03 UTC #1 lore raw

(cc'ed kernel-hardening)

Quoted 4 lines · 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).
kernel/sysctl.c +0 -0

...

Quoted 14 lines

index f487f25..6fd4bc0 100644

kernel/sysctl.c +1 -0

@@ -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,
 	},

kernel/sysctl.c static struct ctl_table kern_table[] = { · line 1016

Quoted 10 lines

kernel/sysctl.c +9 -0

 #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,

Signature · 2 lines

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

Alexey Dobriyan

2012-01-24 11:12 UTC #2 lore raw

Quoted 11 lines · Vasiliy Kulikov

kernel/sysctl.c static struct ctl_table kern_table[] = { · line 1016
Quoted 8 lines
kernel/sysctl.c +8 -0
+		.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?

Vasiliy Kulikov

2012-01-24 11:51 UTC #3 lore raw

Quoted 13 lines · Alexey Dobriyan

Quoted 11 lines · Vasiliy Kulikov
kernel/sysctl.c static struct ctl_table kern_table[] = { · line 1016
Quoted 8 lines
kernel/sysctl.c +8 -0
+		.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.*.

Quoted 2 lines

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,

Signature · 2 lines

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

Kees Cook

author

2012-01-31 19:22 UTC #4 lore raw

Quoted 17 lines · Vasiliy Kulikov

Quoted 10 lines · Alexey Dobriyan
Quoted 8 lines · Vasiliy Kulikov
Quoted 8 lines
+ .procname = "proc_pid_mem",
+ .data = &sysctl_proc_pid_mem,
+ .maxlen = sizeof(int),
+ .mode = 0644,
+ .proc_handler = proc_dointvec_minmax,
+ .extra1 = &zero,
+ .extra2 = &two,
+ },
Quoted 5 lines
Quoted 2 lines
E.g. moving all such stuff to some sysctl group, not bloating kernel.*.
Ehh.
How bloat is measured in this case?

Quoted 7 lines

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

Signature · 2 lines

Kees Cook
ChromeOS Security

Colin Walters

2012-01-24 17:34 UTC #5 lore raw

In reply to Kees Cook

Quoted 2 lines · 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).

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?

Andrew Morton

2012-01-25 23:30 UTC #6 lore raw

In reply to Kees Cook

Quoted 9 lines · On Mon, 23 Jan 2012 13:21:15 -0800 Kees Cook <[email protected]> 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 <[email protected]>
---
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?

Quoted 17 lines

Documentation/sysctl/kernel.txt +4 -0

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

Documentation/sysctl/kernel.txt send before ratelimiting kicks in. · line 493

Quoted 10 lines

Documentation/sysctl/kernel.txt +10 -0

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

Eric W. Biederman

2012-01-26 04:20 UTC #7 lore raw

Quoted 7 lines · Andrew Morton

Quoted 4 lines · On Mon, 23 Jan 2012 13:21:15 -0800 Kees Cook <[email protected]> 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).

Quoted 8 lines

Quoted 5 lines
Signed-off-by Kees Cook <[email protected]>
---
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

Quoted 50 lines

Quoted 17 lines
Documentation/sysctl/kernel.txt +4 -0
@@ -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
Documentation/sysctl/kernel.txt send before ratelimiting kicks in. · line 493
Quoted 10 lines
Documentation/sysctl/kernel.txt +10 -0
+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.

Kees Cook

author

2012-01-31 19:26 UTC #8 lore raw

In reply to Andrew Morton

Quoted 12 lines · Andrew Morton

Quoted 2 lines · On Mon, 23 Jan 2012 13:21:15 -0800 Kees Cook <[email protected]> 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.

Quoted 7 lines

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.

Quoted 2 lines

- 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

Signature · 2 lines

Kees Cook
ChromeOS Security