* [PATCH] signal: always clear sa_restorer on execve @ 2013-03-11 19:11 Kees Cook 2013-03-11 19:28 ` Oleg Nesterov 0 siblings, 1 reply; 6+ messages in thread From: Kees Cook @ 2013-03-11 19:11 UTC (permalink / raw) To: linux-kernel Cc: Al Viro, Oleg Nesterov, Andrew Morton, Eric W. Biederman, Serge Hallyn, Emese Revfy, PaX Team When the new signal handlers are set up for a fork, the location of sa_restorer is not cleared, leaking a parent process's address space location to children. This allows for a potential bypass of the parent's ASLR by examining the sa_restorer value returned when calling sigaction(). $ cat /proc/$$/maps ... 7fb9f3083000-7fb9f3238000 r-xp 00000000 fd:01 404469 .../libc-2.15.so ... $ ./leak ... 7f278bc74000-7f278be29000 r-xp 00000000 fd:01 404469 .../libc-2.15.so ... 1 0 (nil) 0x7fb9f30b94a0 2 4000000 (nil) 0x7f278bcaa4a0 3 4000000 (nil) 0x7f278bcaa4a0 4 0 (nil) 0x7fb9f30b94a0 ... Signed-off-by: Kees Cook <keescook@chromium.org> Reported-by: Emese Revfy <re.emese@gmail.com> Cc: Emese Revfy <re.emese@gmail.com> Cc: PaX Team <pageexec@freemail.hu> Cc: stable@vger.kernel.org --- kernel/signal.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/kernel/signal.c b/kernel/signal.c index 2ec870a..8c8e3ca 100644 --- a/kernel/signal.c +++ b/kernel/signal.c @@ -485,6 +485,9 @@ flush_signal_handlers(struct task_struct *t, int force_default) if (force_default || ka->sa.sa_handler != SIG_IGN) ka->sa.sa_handler = SIG_DFL; ka->sa.sa_flags = 0; +#ifdef __ARCH_HAS_SA_RESTORER + ka->sa.sa_restorer = NULL; +#endif sigemptyset(&ka->sa.sa_mask); ka++; } -- 1.7.9.5 -- Kees Cook Chrome OS Security ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] signal: always clear sa_restorer on execve 2013-03-11 19:11 [PATCH] signal: always clear sa_restorer on execve Kees Cook @ 2013-03-11 19:28 ` Oleg Nesterov 2013-03-11 19:34 ` Kees Cook 0 siblings, 1 reply; 6+ messages in thread From: Oleg Nesterov @ 2013-03-11 19:28 UTC (permalink / raw) To: Kees Cook Cc: linux-kernel, Al Viro, Andrew Morton, Eric W. Biederman, Serge Hallyn, Emese Revfy, PaX Team On 03/11, Kees Cook wrote: > > When the new signal handlers are set up for a fork, the location of > sa_restorer is not cleared, leaking a parent process's address space > location to children. This allows for a potential bypass of the parent's > ASLR by examining the sa_restorer value returned when calling sigaction(). I don't understand. fork() should not change restorer/etc, and the child has the same address space anyway. There is no any leak and the patch can't make any difference in this case because flush_signal_handlers() is not called by fork(). > @@ -485,6 +485,9 @@ flush_signal_handlers(struct task_struct *t, int force_default) > if (force_default || ka->sa.sa_handler != SIG_IGN) > ka->sa.sa_handler = SIG_DFL; > ka->sa.sa_flags = 0; > +#ifdef __ARCH_HAS_SA_RESTORER > + ka->sa.sa_restorer = NULL; > +#endif However, exec sets SIG_DFL but keeps ->sa_restorer, so probably this patch makes sense anyway. Oleg. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] signal: always clear sa_restorer on execve 2013-03-11 19:28 ` Oleg Nesterov @ 2013-03-11 19:34 ` Kees Cook 2013-03-11 19:38 ` Oleg Nesterov 2013-03-11 19:42 ` Eric W. Biederman 0 siblings, 2 replies; 6+ messages in thread From: Kees Cook @ 2013-03-11 19:34 UTC (permalink / raw) To: Oleg Nesterov Cc: LKML, Al Viro, Andrew Morton, Eric W. Biederman, Serge Hallyn, Emese Revfy, PaX Team On Mon, Mar 11, 2013 at 12:28 PM, Oleg Nesterov <oleg@redhat.com> wrote: > On 03/11, Kees Cook wrote: >> >> When the new signal handlers are set up for a fork, the location of >> sa_restorer is not cleared, leaking a parent process's address space >> location to children. This allows for a potential bypass of the parent's >> ASLR by examining the sa_restorer value returned when calling sigaction(). > > I don't understand. > > fork() should not change restorer/etc, and the child has the same address > space anyway. There is no any leak and the patch can't make any difference > in this case because flush_signal_handlers() is not called by fork(). I probably failed to explain this correctly. From the perspective of what should be considered "secret", it only matters across the exec, not the fork (since the VMAs haven't changed until the exec). But the info leak is easy to see, and this patch fixes it. As you say, since other things were reset, so should sa_restorer. > >> @@ -485,6 +485,9 @@ flush_signal_handlers(struct task_struct *t, int force_default) >> if (force_default || ka->sa.sa_handler != SIG_IGN) >> ka->sa.sa_handler = SIG_DFL; >> ka->sa.sa_flags = 0; >> +#ifdef __ARCH_HAS_SA_RESTORER >> + ka->sa.sa_restorer = NULL; >> +#endif > > However, exec sets SIG_DFL but keeps ->sa_restorer, so probably this > patch makes sense anyway. > > Oleg. > -Kees -- Kees Cook Chrome OS Security ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] signal: always clear sa_restorer on execve 2013-03-11 19:34 ` Kees Cook @ 2013-03-11 19:38 ` Oleg Nesterov 2013-03-11 19:42 ` Eric W. Biederman 1 sibling, 0 replies; 6+ messages in thread From: Oleg Nesterov @ 2013-03-11 19:38 UTC (permalink / raw) To: Kees Cook Cc: LKML, Al Viro, Andrew Morton, Eric W. Biederman, Serge Hallyn, Emese Revfy, PaX Team On 03/11, Kees Cook wrote: > On Mon, Mar 11, 2013 at 12:28 PM, Oleg Nesterov <oleg@redhat.com> wrote: > > On 03/11, Kees Cook wrote: > >> > >> When the new signal handlers are set up for a fork, the location of > >> sa_restorer is not cleared, leaking a parent process's address space > >> location to children. This allows for a potential bypass of the parent's > >> ASLR by examining the sa_restorer value returned when calling sigaction(). > > > > I don't understand. > > > > fork() should not change restorer/etc, and the child has the same address > > space anyway. There is no any leak and the patch can't make any difference > > in this case because flush_signal_handlers() is not called by fork(). > > I probably failed to explain this correctly. From the perspective of > what should be considered "secret", it only matters across the exec, > not the fork (since the VMAs haven't changed until the exec). Exactly. So may be you should update the changelog? It really looks as if the parent should "hide" something from the forked child. > But the > info leak is easy to see, and this patch fixes it. As you say, since > other things were reset, so should sa_restorer. Yes, I didn't argue with the patch itself. Oleg. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] signal: always clear sa_restorer on execve 2013-03-11 19:34 ` Kees Cook 2013-03-11 19:38 ` Oleg Nesterov @ 2013-03-11 19:42 ` Eric W. Biederman 2013-03-11 20:02 ` Kees Cook 1 sibling, 1 reply; 6+ messages in thread From: Eric W. Biederman @ 2013-03-11 19:42 UTC (permalink / raw) To: Kees Cook Cc: Oleg Nesterov, LKML, Al Viro, Andrew Morton, Serge Hallyn, Emese Revfy, PaX Team Kees Cook <keescook@chromium.org> writes: > On Mon, Mar 11, 2013 at 12:28 PM, Oleg Nesterov <oleg@redhat.com> wrote: >> On 03/11, Kees Cook wrote: >>> >>> When the new signal handlers are set up for a fork, the location of >>> sa_restorer is not cleared, leaking a parent process's address space >>> location to children. This allows for a potential bypass of the parent's >>> ASLR by examining the sa_restorer value returned when calling sigaction(). >> >> I don't understand. >> >> fork() should not change restorer/etc, and the child has the same address >> space anyway. There is no any leak and the patch can't make any difference >> in this case because flush_signal_handlers() is not called by fork(). > > I probably failed to explain this correctly. From the perspective of > what should be considered "secret", it only matters across the exec, > not the fork (since the VMAs haven't changed until the exec). But the > info leak is easy to see, and this patch fixes it. As you say, since > other things were reset, so should sa_restorer. At the very least please correct the explanation in your patch description. Too often I have had seen a confused patch description, indicate confusion elsewhere in the patch. Let's make it easy for reviewers and future bisectors to understand what is intended. >>> @@ -485,6 +485,9 @@ flush_signal_handlers(struct task_struct *t, int force_default) >>> if (force_default || ka->sa.sa_handler != SIG_IGN) >>> ka->sa.sa_handler = SIG_DFL; >>> ka->sa.sa_flags = 0; >>> +#ifdef __ARCH_HAS_SA_RESTORER >>> + ka->sa.sa_restorer = NULL; >>> +#endif Also I am inclined to suggest this should be an inline function in a header. clear_sa_restorer(ka); Just so we don't litter the code with #ifdefs. Eric ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] signal: always clear sa_restorer on execve 2013-03-11 19:42 ` Eric W. Biederman @ 2013-03-11 20:02 ` Kees Cook 0 siblings, 0 replies; 6+ messages in thread From: Kees Cook @ 2013-03-11 20:02 UTC (permalink / raw) To: Eric W. Biederman Cc: Oleg Nesterov, LKML, Al Viro, Andrew Morton, Serge Hallyn, Emese Revfy, PaX Team On Mon, Mar 11, 2013 at 12:42 PM, Eric W. Biederman <ebiederm@xmission.com> wrote: > Kees Cook <keescook@chromium.org> writes: > >> On Mon, Mar 11, 2013 at 12:28 PM, Oleg Nesterov <oleg@redhat.com> wrote: >>> On 03/11, Kees Cook wrote: >>>> >>>> When the new signal handlers are set up for a fork, the location of >>>> sa_restorer is not cleared, leaking a parent process's address space >>>> location to children. This allows for a potential bypass of the parent's >>>> ASLR by examining the sa_restorer value returned when calling sigaction(). >>> >>> I don't understand. >>> >>> fork() should not change restorer/etc, and the child has the same address >>> space anyway. There is no any leak and the patch can't make any difference >>> in this case because flush_signal_handlers() is not called by fork(). >> >> I probably failed to explain this correctly. From the perspective of >> what should be considered "secret", it only matters across the exec, >> not the fork (since the VMAs haven't changed until the exec). But the >> info leak is easy to see, and this patch fixes it. As you say, since >> other things were reset, so should sa_restorer. > > At the very least please correct the explanation in your patch > description. > > Too often I have had seen a confused patch description, indicate > confusion elsewhere in the patch. Let's make it easy for reviewers > and future bisectors to understand what is intended. Sounds good. I'll re-attempt and send a v2. >>>> @@ -485,6 +485,9 @@ flush_signal_handlers(struct task_struct *t, int force_default) >>>> if (force_default || ka->sa.sa_handler != SIG_IGN) >>>> ka->sa.sa_handler = SIG_DFL; >>>> ka->sa.sa_flags = 0; >>>> +#ifdef __ARCH_HAS_SA_RESTORER >>>> + ka->sa.sa_restorer = NULL; >>>> +#endif > > Also I am inclined to suggest this should be an inline function in a > header. > clear_sa_restorer(ka); > > Just so we don't litter the code with #ifdefs. Yeah, I took a look, and the code was already using the __ARCH_HAS... ifdef, so I went with that. I can respin with some kind of set_sa_restorer() for the other places sa_restorer is used. -Kees -- Kees Cook Chrome OS Security ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2013-03-11 20:02 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2013-03-11 19:11 [PATCH] signal: always clear sa_restorer on execve Kees Cook 2013-03-11 19:28 ` Oleg Nesterov 2013-03-11 19:34 ` Kees Cook 2013-03-11 19:38 ` Oleg Nesterov 2013-03-11 19:42 ` Eric W. Biederman 2013-03-11 20:02 ` Kees Cook
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.