* [PATCH v2 1/2] procfs: make /proc/*/{stack,syscall,personality} 0400
2014-03-22 13:37 [PATCH resend - v2 0/2] procfs: make /proc/*/{stack,syscall,pagemap} 0400 Djalal Harouni
@ 2014-03-22 13:37 ` Djalal Harouni
2014-03-28 22:32 ` Andrew Morton
0 siblings, 1 reply; 6+ messages in thread
From: Djalal Harouni @ 2014-03-22 13:37 UTC (permalink / raw)
To: linux-kernel, Andrew Morton, Al Viro, Eric W. Biederman,
Kees Cook, Andy Lutomirski, Oleg Nesterov, Linus Torvalds,
Ingo Molnar
Cc: Djalal Harouni
These procfs files contain sensitive information and currently their
mode is 0444. Change this to 0400, so the VFS will be able to block
unprivileged processes from getting file descriptors on arbitrary
privileged /proc/*/{stack,syscall,personality} files.
This reduces the scope of ASLR leaking and bypasses by protecting
already running processes.
Cc: Eric W. Biederman <ebiederm@xmission.com>
Acked-by: Kees Cook <keescook@chromium.org>
Acked-by: Andy Lutomirski <luto@amacapital.net>
Signed-off-by: Djalal Harouni <tixxdz@opendz.org>
---
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 5150706..e69df4b 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -2587,7 +2587,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),
@@ -2597,7 +2597,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),
@@ -2625,7 +2625,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),
@@ -2926,14 +2926,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),
@@ -2963,7 +2963,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),
--
1.7.11.7
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v2 1/2] procfs: make /proc/*/{stack,syscall,personality} 0400
@ 2014-03-24 13:41 Alexey Dobriyan
2014-03-25 10:15 ` Djalal Harouni
0 siblings, 1 reply; 6+ messages in thread
From: Alexey Dobriyan @ 2014-03-24 13:41 UTC (permalink / raw)
To: tixxdz; +Cc: Linux Kernel
> - ONE("stack", S_IRUGO, proc_pid_stack),
> + ONE("stack", S_IRUSR, proc_pid_stack),
no love for /proc/*/wchan?
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2 1/2] procfs: make /proc/*/{stack,syscall,personality} 0400
2014-03-24 13:41 [PATCH v2 1/2] procfs: make /proc/*/{stack,syscall,personality} 0400 Alexey Dobriyan
@ 2014-03-25 10:15 ` Djalal Harouni
0 siblings, 0 replies; 6+ messages in thread
From: Djalal Harouni @ 2014-03-25 10:15 UTC (permalink / raw)
To: Alexey Dobriyan; +Cc: Linux Kernel
Hi Alexey,
On Mon, Mar 24, 2014 at 04:41:43PM +0300, Alexey Dobriyan wrote:
> > - ONE("stack", S_IRUGO, proc_pid_stack),
> > + ONE("stack", S_IRUSR, proc_pid_stack),
>
> no love for /proc/*/wchan?
Yes you are right, didn't want to modify lot of things, and I missed that one.
So there is the ptrace check that will just block cases where users do
_not_ play tricks with the fd. I'll send another v3 with that included.
Thanks!
--
Djalal Harouni
http://opendz.org
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2 1/2] procfs: make /proc/*/{stack,syscall,personality} 0400
2014-03-22 13:37 ` [PATCH v2 1/2] procfs: make /proc/*/{stack,syscall,personality} 0400 Djalal Harouni
@ 2014-03-28 22:32 ` Andrew Morton
2014-04-02 17:34 ` Oleg Nesterov
2014-04-15 12:09 ` Djalal Harouni
0 siblings, 2 replies; 6+ messages in thread
From: Andrew Morton @ 2014-03-28 22:32 UTC (permalink / raw)
To: Djalal Harouni
Cc: linux-kernel, Al Viro, Eric W. Biederman, Kees Cook,
Andy Lutomirski, Oleg Nesterov, Linus Torvalds, Ingo Molnar
On Sat, 22 Mar 2014 14:37:39 +0100 Djalal Harouni <tixxdz@opendz.org> wrote:
> These procfs files contain sensitive information and currently their
> mode is 0444. Change this to 0400, so the VFS will be able to block
> unprivileged processes from getting file descriptors on arbitrary
> privileged /proc/*/{stack,syscall,personality} files.
>
> This reduces the scope of ASLR leaking and bypasses by protecting
> already running processes.
Sigh. Why on earth did we make these 444 in the first place.
There is risk of breakage here. Probably anyone who is using these
files and who observes such breakage is a sophisticated user who
understands the reasons and knows how to make the tools work again.
There's only one way to find out :(
I can't remember why we even added /proc/pid/syscall - the changelog
doesn't give a rationale (this is regrettably common).
Now for a six-year-late code review:
- How the heck can target==current in task_current_syscall()?
- Less talk, more action:
From: Andrew Morton <akpm@linux-foundation.org>
Subject: lib/syscall.c: unexport task_current_syscall()
It is only used by procfs and procfs cannot be a module.
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---
lib/syscall.c | 1 -
1 file changed, 1 deletion(-)
diff -puN lib/syscall.c~a lib/syscall.c
--- a/lib/syscall.c~a
+++ a/lib/syscall.c
@@ -72,4 +72,3 @@ int task_current_syscall(struct task_str
return 0;
}
-EXPORT_SYMBOL_GPL(task_current_syscall);
_
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2 1/2] procfs: make /proc/*/{stack,syscall,personality} 0400
2014-03-28 22:32 ` Andrew Morton
@ 2014-04-02 17:34 ` Oleg Nesterov
2014-04-15 12:09 ` Djalal Harouni
1 sibling, 0 replies; 6+ messages in thread
From: Oleg Nesterov @ 2014-04-02 17:34 UTC (permalink / raw)
To: Andrew Morton
Cc: Djalal Harouni, linux-kernel, Al Viro, Eric W. Biederman,
Kees Cook, Andy Lutomirski, Linus Torvalds, Ingo Molnar,
Frank Ch. Eigler
On 03/28, Andrew Morton wrote:
>
> Now for a six-year-late code review:
>
> - How the heck can target==current in task_current_syscall()?
>
> - Less talk, more action:
>
> From: Andrew Morton <akpm@linux-foundation.org>
> Subject: lib/syscall.c: unexport task_current_syscall()
>
> It is only used by procfs and procfs cannot be a module.
>
> Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
> ---
>
> lib/syscall.c | 1 -
> 1 file changed, 1 deletion(-)
>
> diff -puN lib/syscall.c~a lib/syscall.c
> --- a/lib/syscall.c~a
> +++ a/lib/syscall.c
> @@ -72,4 +72,3 @@ int task_current_syscall(struct task_str
>
> return 0;
> }
> -EXPORT_SYMBOL_GPL(task_current_syscall);
Well, I guess it was added for external tracing modules like systemtap.
And in this case target==current is very likely. And for utrace modules,
but it is dead.
I do not see any usage of task_current_syscall() in
git://sourceware.org/git/systemtap.git, so this change should not upset
systemtap at least.
And if we unexport task_current_syscall(), perhaps we should actually
lib/syscall.c. proc_pid_syscall() can use syscall_get_*() instead.
Or at least we can simplify it, I don't think /proc/ actually needs
wait_task_inactive().
Oleg.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2 1/2] procfs: make /proc/*/{stack,syscall,personality} 0400
2014-03-28 22:32 ` Andrew Morton
2014-04-02 17:34 ` Oleg Nesterov
@ 2014-04-15 12:09 ` Djalal Harouni
1 sibling, 0 replies; 6+ messages in thread
From: Djalal Harouni @ 2014-04-15 12:09 UTC (permalink / raw)
To: Andrew Morton
Cc: linux-kernel, Al Viro, Eric W. Biederman, Kees Cook,
Andy Lutomirski, Oleg Nesterov, Linus Torvalds, Ingo Molnar,
Alexey Dobriyan
On Fri, Mar 28, 2014 at 03:32:31PM -0700, Andrew Morton wrote:
> On Sat, 22 Mar 2014 14:37:39 +0100 Djalal Harouni <tixxdz@opendz.org> wrote:
>
> > These procfs files contain sensitive information and currently their
> > mode is 0444. Change this to 0400, so the VFS will be able to block
> > unprivileged processes from getting file descriptors on arbitrary
> > privileged /proc/*/{stack,syscall,personality} files.
> >
> > This reduces the scope of ASLR leaking and bypasses by protecting
> > already running processes.
>
> Sigh. Why on earth did we make these 444 in the first place.
>
> There is risk of breakage here. Probably anyone who is using these
> files and who observes such breakage is a sophisticated user who
> understands the reasons and knows how to make the tools work again.
> There's only one way to find out :(
>
> I can't remember why we even added /proc/pid/syscall - the changelog
> doesn't give a rationale (this is regrettably common).
Yep :-/
Ok, thank you for taking the patches!
Just to update the tread, Alexey Dobriyan suggested that we also improve
/proc/*/wchan
I'll do it by caching the ptrace check during open time, and re-check it
in proc_pid_wchan() only in case we return an address, this way we do not
break tools. Will add it to the patches I've and send it soon (didn't
want to disturb you during the merge window).
Thanks!
--
Djalal Harouni
http://opendz.org
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2014-04-15 12:09 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-03-24 13:41 [PATCH v2 1/2] procfs: make /proc/*/{stack,syscall,personality} 0400 Alexey Dobriyan
2014-03-25 10:15 ` Djalal Harouni
-- strict thread matches above, loose matches on Subject: below --
2014-03-22 13:37 [PATCH resend - v2 0/2] procfs: make /proc/*/{stack,syscall,pagemap} 0400 Djalal Harouni
2014-03-22 13:37 ` [PATCH v2 1/2] procfs: make /proc/*/{stack,syscall,personality} 0400 Djalal Harouni
2014-03-28 22:32 ` Andrew Morton
2014-04-02 17:34 ` Oleg Nesterov
2014-04-15 12:09 ` Djalal Harouni
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox